Closed
Bug 395340
Opened 17 years ago
Closed 17 years ago
Crash [@ nsINode::GetNodeParent] with CSS counters and contentEditable
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla1.9beta1
People
(Reporter: jruderman, Assigned: peterv)
References
(Depends on 1 open bug)
Details
(4 keywords, Whiteboard: [sg:critical?])
Crash Data
Attachments
(2 files)
|
471 bytes,
text/html
|
Details | |
|
2.02 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
beltzner
:
approvalM9+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
Loading the testcase triggers three assertions and a null-dereference crash.
###!!! ASSERTION: identical: 'pseudoType1 != pseudoType2', file /Users/jruderman/trunk/mozilla/layout/base/nsGenConList.cpp, line 124
###!!! ASSERTION: null check on startContent should be sufficient to null check nodeContent as well, since if nodeContent is for the root, startContent (which is before it) must be too: 'nodeContent || !startContent', file /Users/jruderman/trunk/mozilla/layout/base/nsCounterManager.cpp, line 145
###!!! ASSERTION: The possible descendant is null!: 'aPossibleDescendant', file /Users/jruderman/trunk/mozilla/content/base/src/nsContentUtils.cpp, line 1152
Thread 0 Crashed:
0 nsINode::GetNodeParent() const + 9 (nsINode.h:486)
1 nsContentUtils::ContentIsDescendantOf(nsINode*, nsINode*) + 152 (nsContentUtils.cpp:1158)
2 nsCounterList::SetScope(nsCounterNode*) + 348 (nsCounterManager.cpp:148)
3 nsCounterList::RecalcAll() + 61 (nsCounterManager.cpp:176)
...
|
||
Comment 1•17 years ago
|
||
This is a bug in the contentEditable code. The frame tree here is completely broken as a result of this bug; it's lucky that it's crashing with a null deref in counter code. If you took out the counter stuff, it shouldn't be that hard to end up with exploitable crashes, I suspect.
The basic issue is this callstack:
#0 nsCSSFrameConstructor::ContentInserted (this=0x8d6e828, aContainer=0x89cdc98,
aChild=0x8e89060, aIndexInContainer=1, aFrameState=0x8d4d890)
at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:8895
#1 0xb69d37a8 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x8d6e828,
aContent=0x8e89060) at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:11242
#2 0xb69d11b7 in nsCSSFrameConstructor::RestyleElement (this=0x8d6e828,
aContent=0x8e89060, aPrimaryFrame=0x8fedb74, aMinHint=0)
at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:10070
#3 0xb69d6ed4 in nsCSSFrameConstructor::ProcessOneRestyle (this=0x8d6e828,
aContent=0x8e89060, aRestyleHint=eReStyle_Self, aChangeHint=0)
at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:13122
#4 0xb69d710a in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x8d6e828)
at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:13173
#5 0xb6a359c6 in PresShell::DoFlushPendingNotifications (this=0x8f0ed18,
aType=Flush_Style, aInterruptibleReflow=0)
at ../../../mozilla/layout/base/nsPresShell.cpp:4447
#6 0xb6a35893 in PresShell::FlushPendingNotifications (this=0x8f0ed18,
aType=Flush_Style) at ../../../mozilla/layout/base/nsPresShell.cpp:4411
#7 0xb6ca793c in nsDocument::FlushPendingNotifications (this=0x8e96e40,
aType=Flush_Style) at ../../../../mozilla/content/base/src/nsDocument.cpp:4948
#8 0xafed1262 in nsEditingSession::SetupEditorOnWindow (this=0x8c1f860,
aWindow=0x8a62b00)
at ../../../../mozilla/editor/composer/src/nsEditingSession.cpp:342
#9 0xafed0e46 in nsEditingSession::MakeWindowEditable (this=0x8c1f860,
aWindow=0x8a62b00, aEditorType=0xb72b0742 "html", aDoAfterUriLoad=0,
aMakeWholeDocumentEditable=0, aInteractive=1)
at ../../../../mozilla/editor/composer/src/nsEditingSession.cpp:223
#10 0xb6df9724 in nsHTMLDocument::EditingStateChanged (this=0x8e96e40)
at ../../../../../mozilla/content/html/document/src/nsHTMLDocument.cpp:3987
#11 0xb6df833d in nsHTMLDocument::ChangeContentEditableCount (this=0x8e96e40,
aElement=0x8ff4138, aChange=1)
at ../../../../../mozilla/content/html/document/src/nsHTMLDocument.cpp:3759
#12 0xb6d62fc3 in nsGenericHTMLElement::BindToTree (this=0x8ff4138, aDocument=0x8e96e40,
aParent=0x8e89060, aBindingParent=0x0, aCompileEventHandlers=1)
at ../../../../../mozilla/content/html/content/src/nsGenericHTMLElement.cpp:1152
This causes a frame reconstruct on the <body> under the BindToTree of the outer <div>. So we recreate frames for the body, construct frames for both divs... _then_ get a ContentAppended for the outer div from the actual append, and construct a second set of frames for the divs. After that it's all bad.
We shouldn't be synchronously doing MakeWindowEditable() under BindToTree. We need to either do it at a safe spot before returning from AppendChild/InsertBefore (right about where we'd fire mutation events, say, or at the end of the update), or we need to put it off on an event.
Doing it at the end of the update would make a lot of sense to me, frankly.
I think we absolutely need to fix this for 1.9.
Blocks: contenteditable
Component: Style System (CSS) → DOM
Flags: blocking1.9?
Keywords: regression
OS: Mac OS X → All
QA Contact: style-system → general
Hardware: PC → All
|
Reporter | |
Updated•17 years ago
|
Whiteboard: [sg:critical?]
|
||
Updated•17 years ago
|
Group: security
|
Assignee | |
Comment 2•17 years ago
|
||
Like this?
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attachment #286044 -
Flags: superreview?
Attachment #286044 -
Flags: review?
|
|
Assignee | |
Updated•17 years ago
|
Attachment #286044 -
Flags: superreview?(bzbarsky)
Attachment #286044 -
Flags: superreview?
Attachment #286044 -
Flags: review?(bzbarsky)
Attachment #286044 -
Flags: review?
|
|
||
Comment 3•17 years ago
|
||
Comment on attachment 286044 [details] [diff] [review]
v1
>Index: content/html/document/src/nsHTMLDocument.cpp
>+ (mUpdateNestLevel > 0 && EditingShouldBeOn() != IsEditingOn())) {
Why do we need that second check? Take it out if it's not needed?
In general, this looks good. Does it fix the testcase?
Attachment #286044 -
Flags: superreview?(bzbarsky)
Attachment #286044 -
Flags: superreview+
Attachment #286044 -
Flags: review?(bzbarsky)
Attachment #286044 -
Flags: review+
|
|
Assignee | |
Comment 4•17 years ago
|
||
(In reply to comment #3)
> (From update of attachment 286044 [details] [diff] [review])
> >Index: content/html/document/src/nsHTMLDocument.cpp
> >+ (mUpdateNestLevel > 0 && EditingShouldBeOn() != IsEditingOn())) {
>
> Why do we need that second check? Take it out if it's not needed?
If we're just changing the contenteditable state of an element in a document that's already being edited we do want the rest of the code in that function to run, it updates spellchecking of the node. Unless you think we should postpone that too? I'll have to do some refactoring if so.
> In general, this looks good. Does it fix the testcase?
Yes.
|
|
||
Comment 5•17 years ago
|
||
It looks like all the real work of spellchecking is async, so it should be ok, I guess.
Perhaps what we should also do (separate bug) is make the presshell not safe to flush during updates...
|
|
Assignee | |
Comment 7•17 years ago
|
||
Comment on attachment 286044 [details] [diff] [review]
v1
Fixes a crash caused by contenteditable.
Attachment #286044 -
Flags: approvalM9?
|
||
Comment 8•17 years ago
|
||
Comment on attachment 286044 [details] [diff] [review]
v1
a=endgame drivers for M9
Attachment #286044 -
Flags: approvalM9?
Attachment #286044 -
Flags: approvalM9+
Attachment #286044 -
Flags: approval1.9+
|
|
||
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
|
|
Assignee | |
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
|
||
Updated•17 years ago
|
Flags: in-testsuite?
|
||
Comment 9•17 years ago
|
||
This patch is believed to be the cause of bug 401288, which has made
plain text composer unusable.
|
|
||
Comment 10•17 years ago
|
||
Yes, that was already known.
|
||
Comment 11•17 years ago
|
||
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110103 Minefield/3.0a9pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9a9pre) Gecko/2007110104 Minefield/3.0a9pre. No crash and no assertions with the testcase.
Status: RESOLVED → VERIFIED
|
|
Reporter | |
Comment 12•17 years ago
|
||
This doesn't affect the 1.8 branch because contenteditable is new in 1.9.
Group: security
Flags: wanted1.8.1.x-
|
|
||
Updated•16 years ago
|
|
||
Updated•13 years ago
|
Crash Signature: [@ nsINode::GetNodeParent]
|
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•