420439 - "ASSERTION: Please remove this from the document properly" with mutation event, execCommand

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Reloading the testcase, or leaving it for another page, triggers this assertion once followed by a crash.  Quitting the browser while the page is visible triggers the assertion six times without crashing.  The crash appears to be exploitable.

###!!! ASSERTION: Please remove this from the document properly: '!IsInDoc()', file /Users/jruderman/trunk/mozilla/content/base/src/nsGenericElement.cpp, line 1219

The most common signatures for the crash are:
* [@ nsCSSFrameConstructor::ContentInserted]
* [@ nsPropertyTable::GetPropertyInternal]
* [@ nsCSSFrameConstructor::GetInsertionPoint]

Comment 1

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when you leave)

Comment 2

[Daniel Veditz [:dveditz]]

Couldn't reproduce on branch, appears to be trunk-only.

Comment 3

[Mats Palmgren (inactive)]

Attachment: stack

nsHTMLEditor::ShowResizers() leads to a nested ShowInlineTableEditingUI()
which makes this code bogus since it caches the state up front:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/libeditor/html/nsHTMLAnonymousUtils.cpp&rev=1.17&root=/cvsroot&mark=301-303,307,314,321,335,344,353#296
The first stack shows the nested call to ShowInlineTableEditingUI(),
the second stack shows the second call at the end of
CheckSelectionStateForAnonymousButtons(), now with 'mInlineEditedCell',
'mAddColumnBeforeButton' etc being non-null.

What make this interesting is that we have frames for this anonymous
content in the frame tree; overwriting 'mAddColumnBeforeButton' etc with
new values makes its frame the last object with a strong ref; If there
is a queued restyle event (for example) we do:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1468&root=/cvsroot&mark=11262,11268#11215
when ContentRemoved() deletes the frame the last ref to the content
is removed so it gets deleted too (with the assert in comment 0); then
we call ContentInserted()...

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The fix is in nsHTMLAnonymousUtils.cpp, but I'm adding a bit of extra
safety so we don't overwrite these values again.  I know we normally
just assert and don't handle the error, but the editor code is messy
and broken in so many ways I don't trust it...

(I still see the assertion in bug 336104 with this patch on the
testcase, but I think that assert is bogus.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 307061 [details] [diff] [review]
Patch rev. 1

Really needs to go to peterv

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

I've been working on the resizing code for my work on bug 399046 recently, and I've run into the problem that Mats fixed in this patch, and his patch looks sensible to me.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 307061 [details] [diff] [review]
Patch rev. 1

>Index: editor/libeditor/html/nsHTMLInlineTableEditor.cpp
>===================================================================

>+  if (mInlineEditedCell) {

Should this check for example mAddColumnBeforeButton instead to make sure HideInlineTableEditingUI succeeded completely?

>Index: editor/libeditor/html/nsHTMLObjectResizer.cpp
>===================================================================

>@@ -467,24 +473,26 @@ nsHTMLEditor::HideResizers(void)

>+  mActivatedHandle = nsnull;

I think this needs to remove the _moz_activated attribute?

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

(In reply to comment #7)
> Should this check for example mAddColumnBeforeButton instead to make sure
> HideInlineTableEditingUI succeeded completely?

I think I prefer to keep it as is, I just wanted to catch a missed call,
not that it completed successfully.  If it's expected to never fail then
we should add assertions in HideInlineTableEditingUI() instead.
It should be harmless to continue here even if a preceding 
HideInlineTableEditingUI() call failed.

> I think this needs to remove the _moz_activated attribute?

Yeah, seems reasonable, added that.

Comment 9

[Damon Sicore (:damons)]

Comment on attachment 313282 [details] [diff] [review]
Patch rev. 2

Has a test case, fixes a crash.  a1.9+=damons.

Comment 10

[Mats Palmgren (inactive)]

mozilla/editor/libeditor/html/nsHTMLAbsPosition.cpp 	1.15
mozilla/editor/libeditor/html/nsHTMLAnonymousUtils.cpp 	1.20
mozilla/editor/libeditor/html/nsHTMLInlineTableEditor.cpp 	1.7
mozilla/editor/libeditor/html/nsHTMLObjectResizer.cpp 	1.32
mozilla/editor/libeditor/html/crashtests/420439.html 	1.1
mozilla/editor/libeditor/html/crashtests/crashtests.list 	1.4 

-> FIXED

Comment 11

[Samuel Sidler (old account; do not CC)]

This bug was fixed in April and does not apply to the 1.8 branch. Can we open it up?