Closed
Bug 420439
Opened 17 years ago
Closed 17 years ago
"ASSERTION: Please remove this from the document properly" with mutation event, execCommand
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla1.9
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical] post 1.8-branch)
Attachments
(3 files, 1 obsolete file)
654 bytes,
text/html
|
Details | |
42.49 KB,
text/plain
|
Details | |
8.79 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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]
Reporter | ||
Updated•17 years ago
|
Flags: blocking1.9?
Whiteboard: [sg:critical?]
Reporter | ||
Comment 1•17 years ago
|
||
Reporter | ||
Updated•17 years ago
|
Whiteboard: [sg:critical?] → [sg:critical]
Comment 2•17 years ago
|
||
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 | ||
Updated•17 years ago
|
Assignee: nobody → mats.palmgren
OS: Mac OS X → All
Hardware: PC → All
Assignee | ||
Comment 3•17 years ago
|
||
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()...
Assignee | ||
Comment 4•17 years ago
|
||
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)
Comment 6•17 years ago
|
||
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 7•17 years ago
|
||
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+
Assignee | ||
Comment 8•17 years ago
|
||
(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)
Assignee | ||
Updated•17 years ago
|
Attachment #307061 -
Attachment is obsolete: true
Updated•17 years ago
|
Attachment #313282 -
Flags: superreview?(peterv)
Attachment #313282 -
Flags: superreview+
Attachment #313282 -
Flags: review?(peterv)
Attachment #313282 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Attachment #313282 -
Flags: approval1.9?
Comment 9•17 years ago
|
||
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+
Assignee | ||
Comment 10•17 years ago
|
||
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: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Comment 11•16 years ago
|
||
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+
Updated•16 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•