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

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: 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][[e-mail]]]

Um, this may require fixing nsTransactionItem.cpp and other things in transaction manager. Code not really touched since 2001

Comment 2

[Jesse Ruderman]

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][[e-mail]]]

Sorry, this taking so long. Will try to fix this asap.
The code is bizarre.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

(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]]

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][[e-mail]]]

Attachment: nits fixed

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Checked in to trunk.

Comment 9

[Johnny Stenback (:jst)]

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]]

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 - ex-MoCo]]

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]

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]

likewise 1.8.0

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 352161 [details] [diff] [review]
nits fixed

Approved for 1.8.1.21, a=dveditz for release-drivers.

Comment 15

[Alexander Sack]

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]

Comment on attachment 352161 [details] [diff] [review]
nits fixed

a=asac for 1.8.0

Comment 17

[Aakash Desai [:aakashd]]

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