Closed Bug 420439 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Editor, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical] post 1.8-branch)

Attachments

(3 files, 1 obsolete file)

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]
Flags: blocking1.9?
Whiteboard: [sg:critical?]
Whiteboard: [sg:critical?] → [sg:critical]
Couldn't reproduce on branch, appears to be trunk-only.
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical] → [sg:critical] post 1.8-branch
Assignee: nobody → mats.palmgren
OS: Mac OS X → All
Hardware: PC → All
Attached file 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()...
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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.)
Attachment #307061 - Flags: superreview?(roc)
Attachment #307061 - Flags: review?(roc)
Comment on attachment 307061 [details] [diff] [review]
Patch rev. 1

Really needs to go to peterv
Attachment #307061 - Flags: superreview?(roc)
Attachment #307061 - Flags: superreview?(peterv)
Attachment #307061 - Flags: review?(roc)
Attachment #307061 - Flags: review?(peterv)
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.
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-
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?
Attachment #307061 - Flags: superreview?(peterv)
Attachment #307061 - Flags: superreview+
Attachment #307061 - Flags: review?(peterv)
Attachment #307061 - Flags: review+
Attached patch Patch rev. 2Splinter Review
(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.
Attachment #313282 - Flags: superreview?(peterv)
Attachment #313282 - Flags: review?(peterv)
Attachment #307061 - Attachment is obsolete: true
Attachment #313282 - Flags: superreview?(peterv)
Attachment #313282 - Flags: superreview+
Attachment #313282 - Flags: review?(peterv)
Attachment #313282 - Flags: review+
Attachment #313282 - Flags: approval1.9?
Comment on attachment 313282 [details] [diff] [review]
Patch rev. 2

Has a test case, fixes a crash.  a1.9+=damons.
Attachment #313282 - Flags: approval1.9? → approval1.9+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
This bug was fixed in April and does not apply to the 1.8 branch. Can we open it up?
Flags: wanted1.9.0.x+
Group: core-security
You need to log in before you can comment on or make changes to this bug.