Last Comment Bug 449006 - [FIX] Crash [@ PlaceholderTxn::RedoTransaction] with redo, formatblock, DOMCharacterDataModified mutation listener
: [FIX] Crash [@ PlaceholderTxn::RedoTransaction] with redo, formatblock, DOMCh...
Status: VERIFIED FIXED
[sg:critical?]
: crash, fixed1.8.1.21, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 325861 336383
  Show dependency treegraph
 
Reported: 2008-08-04 00:45 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
12 users (show)
roc: wanted1.9.1+
dveditz: blocking1.9.0.6+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
asac: wanted1.8.1.x+
asac: blocking1.8.0.next+
asac: wanted1.8.0.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (686 bytes, text/html)
2008-08-04 00:45 PDT, Jesse Ruderman
no flags Details
Refcounted TransactionItem (28.42 KB, patch)
2008-11-13 09:25 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
peterv: review+
peterv: superreview+
Details | Diff | Review
nits fixed (28.28 KB, patch)
2008-12-09 11:53 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jst: approval1.9.1+
dveditz: approval1.9.0.6+
dveditz: approval1.8.1.next+
asac: approval1.8.0.next+
Details | Diff | Review

Description Jesse Ruderman 2008-08-04 00:45:12 PDT
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.
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-08-04 09:54:48 PDT
Um, this may require fixing nsTransactionItem.cpp and other things in transaction manager. Code not really touched since 2001
Comment 2 Jesse Ruderman 2008-09-04 20:49:18 PDT
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.
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-11-13 07:03:58 PST
Sorry, this taking so long. Will try to fix this asap.
The code is bizarre.
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-11-13 09:25:07 PST
Created attachment 347980 [details] [diff] [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).
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-11-13 09:26:54 PST
(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 6 Peter Van der Beken [:peterv] 2008-12-09 11:29:20 PST
Comment on attachment 347980 [details] [diff] [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));
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-12-09 11:53:18 PST
Created attachment 352161 [details] [diff] [review]
nits fixed
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-12-10 08:49:14 PST
Checked in to trunk.
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2008-12-22 13:37:23 PST
Comment on attachment 352161 [details] [diff] [review]
nits fixed

a=jst for 1.9.1. Big patch, but small change, really.
Comment 10 Daniel Veditz [:dveditz] 2009-01-05 11:34:35 PST
Comment on attachment 352161 [details] [diff] [review]
nits fixed

Approved for 1.9.0.6, a=dveditz for release-drivers.
Comment 11 Al Billings [:abillings] 2009-01-06 11:19:41 PST
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.
Comment 12 Alexander Sack 2009-01-12 02:18:26 PST
patch from attachment 352161 [details] [diff] [review] applies cleanly on 1.8.1, so i guess we want it too there.
Comment 13 Alexander Sack 2009-01-12 02:19:14 PST
likewise 1.8.0
Comment 14 Daniel Veditz [:dveditz] 2009-01-16 11:33:05 PST
Comment on attachment 352161 [details] [diff] [review]
nits fixed

Approved for 1.8.1.21, a=dveditz for release-drivers.
Comment 15 Alexander Sack 2009-01-20 04:09:22 PST
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 16 Alexander Sack 2009-02-09 07:52:24 PST
Comment on attachment 352161 [details] [diff] [review]
nits fixed

a=asac for 1.8.0
Comment 17 Aakash Desai [:aakashd] 2009-04-28 13:42:02 PDT
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

Note You need to log in before you can comment on or make changes to this bug.