Closed Bug 449006 Opened 11 years ago Closed 11 years ago

[FIX] Crash [@ PlaceholderTxn::RedoTransaction] with redo, formatblock, DOMCharacterDataModified mutation listener

Categories

(Core :: Editor, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 2 open bugs)

Details

(5 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(2 files, 1 obsolete file)

Loading the testcase makes PlaceholderTxn::RedoTransaction dereference a random address.  With MallocScribble and MallocPreScribble enabled, I've seen it dereference 0xaaaaaaaa, 0x55555555, and 0x0000000c.
Whiteboard: [sg:critical?]
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.
Flags: blocking1.9.1?
Assignee: nobody → Olli.Pettay
Flags: blocking1.9.1? → wanted1.9.1+
Sorry, this taking so long. Will try to fix this asap.
The code is bizarre.
Attached patch Refcounted TransactionItem (obsolete) — Splinter Review
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).
Attachment #347980 - Flags: superreview?(peterv)
Attachment #347980 - Flags: review?(peterv)
(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.
Summary: Crash [@ PlaceholderTxn::RedoTransaction] with redo, formatblock, DOMCharacterDataModified mutation listener → [FIX] Crash [@ PlaceholderTxn::RedoTransaction] with redo, formatblock, DOMCharacterDataModified mutation listener
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));
Attachment #347980 - Flags: superreview?(peterv)
Attachment #347980 - Flags: superreview+
Attachment #347980 - Flags: review?(peterv)
Attachment #347980 - Flags: review+
Attached patch nits fixedSplinter Review
Attachment #347980 - Attachment is obsolete: true
Checked in to trunk.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #352161 - Flags: approval1.9.1?
Attachment #352161 - Flags: approval1.9.0.6?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.6+
Whiteboard: [sg:critical?] → [sg:critical?][needs 1.9.1 approval and landing]
Attachment #352161 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 352161 [details] [diff] [review]
nits fixed

a=jst for 1.9.1. Big patch, but small change, really.
Whiteboard: [sg:critical?][needs 1.9.1 approval and landing] → [sg:critical?][needs 1.9.1 landing]
Keywords: fixed1.9.1
Whiteboard: [sg:critical?][needs 1.9.1 landing] → [sg:critical?]
Comment on attachment 352161 [details] [diff] [review]
nits fixed

Approved for 1.9.0.6, a=dveditz for release-drivers.
Attachment #352161 - Flags: approval1.9.0.6? → approval1.9.0.6+
Keywords: fixed1.9.0.6
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] [diff] [review] applies cleanly on 1.8.1, so i guess we want it too there.
Flags: wanted1.8.1.x+
likewise 1.8.0
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next+
Attachment #352161 - Flags: approval1.8.1.next?
Attachment #352161 - Flags: approval1.8.0.next?
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Comment on attachment 352161 [details] [diff] [review]
nits fixed

Approved for 1.8.1.21, a=dveditz for release-drivers.
Attachment #352161 - Flags: approval1.8.1.next? → approval1.8.1.next+
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
Keywords: fixed1.8.1.21
Group: core-security
Flags: in-testsuite+
Comment on attachment 352161 [details] [diff] [review]
nits fixed

a=asac for 1.8.0
Attachment #352161 - Flags: approval1.8.0.next? → approval1.8.0.next+
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
Status: RESOLVED → VERIFIED
Crash Signature: [@ PlaceholderTxn::RedoTransaction]
You need to log in before you can comment on or make changes to this bug.