Closed Bug 395340 Opened 17 years ago Closed 16 years ago

Crash [@ nsINode::GetNodeParent] with CSS counters and contentEditable


(Core :: DOM: Core & HTML, defect, P1)






(Reporter: jruderman, Assigned: peterv)


(Depends on 1 open bug)


(4 keywords, Whiteboard: [sg:critical?])

Crash Data


(2 files)

Attached file testcase
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)
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, 
    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.
Component: Style System (CSS) → DOM
Flags: blocking1.9?
Keywords: regression
OS: Mac OS X → All
QA Contact: style-system → general
Hardware: PC → All
Whiteboard: [sg:critical?]
Group: security
Attached patch v1Splinter Review
Like this?
Assignee: nobody → peterv
Attachment #286044 - Flags: superreview?
Attachment #286044 - Flags: review?
Attachment #286044 - Flags: superreview?(bzbarsky)
Attachment #286044 - Flags: superreview?
Attachment #286044 - Flags: review?(bzbarsky)
Attachment #286044 - Flags: review?
Comment on 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?

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+
(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?

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...
Comment on attachment 286044 [details] [diff] [review]

Fixes a crash caused by contenteditable.
Attachment #286044 - Flags: approvalM9?
Comment on attachment 286044 [details] [diff] [review]

a=endgame drivers for M9
Attachment #286044 - Flags: approvalM9?
Attachment #286044 - Flags: approvalM9+
Attachment #286044 - Flags: approval1.9+
Flags: blocking1.9? → blocking1.9+
Closed: 16 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Flags: in-testsuite?
This patch is believed to be the cause of bug 401288, which has made 
plain text composer unusable.
Yes, that was already known.
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.
Blocks: 424238
This doesn't affect the 1.8 branch because contenteditable is new in 1.9.
Group: security
Flags: wanted1.8.1.x-
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Depends on: 434766
No longer blocks: 424238
Depends on: 424238
Crash Signature: [@ nsINode::GetNodeParent]
Depends on: 1248187
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.