Bugzilla@Mozilla – Bug 449006
[FIX] Crash [@ PlaceholderTxn::RedoTransaction] with redo, formatblock, DOMCharacterDataModified mutation listener
Last modified: 2009-04-28 13:42:02 PDT
Summon comment box
Created attachment 332179 [details] testcase (crashes Firefox when loaded) Loading the testcase makes PlaceholderTxn::RedoTransaction dereference a random address. With MallocScribble and MallocPreScribble enabled, I've seen it dereference 0xaaaaaaaa, 0x55555555, and 0x0000000c.
Um, this may require fixing nsTransactionItem.cpp and other things in transaction manager. Code not really touched since 2001
Smaug, are you interested in doing that? Since it's code that hasn't been touched in a while, it would be better to do this before Firefox 3.1 is too close.
Sorry, this taking so long. Will try to fix this asap. The code is bizarre.
Created attachment 347980 [details] [review] Refcounted TransactionItem This makes nsTransactionItem refcounted and changes relevant code to use nsRefPtr (or NS_ADDREF in nsTransactionStack). Also fixes nsTransactionItem::GetTransaction to return AddRefed nsITransaction and the code which call that method use now nsCOMPtr. This way we can keep objects alive. Removed nsTransactionReleaseFunctor, because that isn't used for anything (dtor calls Clear anyway). The whole nsTransactionStack should probably go away eventually and nsTArray<nsRefPtr<nsTransactionItem> > or similar could be used there. I searched for all |delete| operators in the directory and removed manual deleting of nsTransactionItems. Fixes the crash. There is a JS exception, but I don't think we can properly handle dom mutation caused by mutation event listeners. Passes chrome/browser-chrome/mochitest without any new leaks. The patch applies without any fuzz to trunk, 1.9.0 and 1.8 (some of the code is from 1998).
(In reply to comment #4) > Fixes the crash. There is a JS exception, but I don't think > we can properly handle dom mutation caused by mutation event listeners. er, I don't think we can make editor to handle properly all the mutations caused by mutation listeners. At least not now.
Comment on attachment 347980 [details] [review] Refcounted TransactionItem >diff --git a/editor/txmgr/src/nsTransactionItem.h b/editor/txmgr/src/nsTransactionItem.h > #define nsTransactionItem_h__ Nit: add a newline. >+#include "nsITransaction.h" >diff --git a/editor/txmgr/src/nsTransactionManager.cpp b/editor/txmgr/src/nsTransactionManager.cpp >@@ -529,107 +529,99 @@ nsTransactionManager::SetMaxTransactionC >+ tx = nsnull; Don't need this. >+ result = mUndoStack.PopBottom(getter_AddRefs(tx)); >+ tx = nsnull; Don't need this. >+ result = mRedoStack.PopBottom(getter_AddRefs(tx)); >diff --git a/editor/txmgr/src/nsTransactionStack.cpp b/editor/txmgr/src/nsTransactionStack.cpp > nsTransactionStack::Clear(void) >+ tx = nsnull; Don't need this. >+ result = Pop(getter_AddRefs(tx)); >@@ -163,39 +163,31 @@ nsTransactionRedoStack::~nsTransactionRe >+ tx = nsnull; Don't need this. >+ result = PopBottom(getter_AddRefs(tx));
Created attachment 352161 [details] [review] nits fixed
Checked in to trunk.
Comment on attachment 352161 [details] [review] nits fixed a=jst for 1.9.1. Big patch, but small change, really.
Comment on attachment 352161 [details] [review] nits fixed Approved for 1.9.0.6, a=dveditz for release-drivers.
Verified fixed for 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009010604 GranParadiso/3.0.6pre.
patch from attachment 352161 [details] [review] applies cleanly on 1.8.1, so i guess we want it too there.
likewise 1.8.0
Comment on attachment 352161 [details] [review] nits fixed Approved for 1.8.1.21, a=dveditz for release-drivers.
Committed to MOZILLA_1_8_BRANCH Checking in editor/txmgr/src/nsTransactionItem.cpp; /cvsroot/mozilla/editor/txmgr/src/nsTransactionItem.cpp,v <-- nsTransactionItem.cpp new revision: 1.20.28.1; previous revision: 1.20 done Checking in editor/txmgr/src/nsTransactionItem.h; /cvsroot/mozilla/editor/txmgr/src/nsTransactionItem.h,v <-- nsTransactionItem.h new revision: 1.11.28.1; previous revision: 1.11 done Checking in editor/txmgr/src/nsTransactionList.cpp; /cvsroot/mozilla/editor/txmgr/src/nsTransactionList.cpp,v <-- nsTransactionList.cpp new revision: 1.8.28.1; previous revision: 1.8 done Checking in editor/txmgr/src/nsTransactionList.h; /cvsroot/mozilla/editor/txmgr/src/nsTransactionList.h,v <-- nsTransactionList.h new revision: 1.6.28.1; previous revision: 1.6 done Checking in editor/txmgr/src/nsTransactionManager.cpp; /cvsroot/mozilla/editor/txmgr/src/nsTransactionManager.cpp,v <-- nsTransactionManager.cpp new revision: 1.43.18.1; previous revision: 1.43 done Checking in editor/txmgr/src/nsTransactionStack.cpp; /cvsroot/mozilla/editor/txmgr/src/nsTransactionStack.cpp,v <-- nsTransactionStack.cpp new revision: 1.20.28.1; previous revision: 1.20 done Checking in editor/txmgr/src/nsTransactionStack.h; /cvsroot/mozilla/editor/txmgr/src/nsTransactionStack.h,v <-- nsTransactionStack.h new revision: 1.11.28.1; previous revision: 1.11 done
Comment on attachment 352161 [details] [review] nits fixed a=asac for 1.8.0
verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090428 Minefield/3.6a1pre ID:20090428031037 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090427 Shiretoko/3.5b5pre ID:20090427031112