[FIX]"ASSERTION: This method should never be called on content nodes that are not in a document!" with cloned xul node

VERIFIED FIXED

Status

()

defect
P2
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

(Blocks 3 bugs, 4 keywords)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][dbaron-1.9:Rs])

Attachments

(2 attachments)

Reporter

Description

12 years ago
Posted file testcase
Loading the testcase causes three assertions and a crash:

###!!! ASSERTION: Should be in an update while creating frames: 'mUpdateCount != 0', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8464

###!!! ASSERTION: This method should never be called on content nodes that are not in a document!: 'aContent->IsInDoc()', file /Users/jruderman/trunk/mozilla/content/html/document/src/nsHTMLDocument.cpp, line 1928

The other assertion in nsHTMLDocument::MatchAnchors is also triggered, but it's misleading in this case.  The assertions are followed by a null-dereference crash [@ nsHTMLDocument::MatchAnchors].
Reporter

Comment 1

12 years ago
Why are frames being constructed for an element that's not in the document?

Why is the line
  "" + [window];
needed for the crash?

Is the crash a regression from bug 398108?
I doubt the crash is, no.  I have no idea why the |"" + [window]| thing is required for the crash.

What I do know is that the basic problem is that we determine whether to call BeginUpdate/EndUpdate based on GetCurrentDoc(), but IMPL_MUTATION_NOTIFICATION has:

    if (!node && prev->HasFlag(NODE_FORCE_XBL_BINDINGS)) {        \
      /* For elements that have the NODE_FORCE_XBL_BINDINGS flag  \
         set we need to notify the document */                    \
      node = prev->GetOwnerDoc();                                 \
    }                                                             \

so in this case we notify the append on the document.  Which is just very very wrong.  We added this in bug 380872 with a plan to investigate, but I'm not sure the investigate ever happened.  We should do so now.  Calling this stuff while not in an update is really bad, imo.  If the issue is that we want the binding manager noticing changes to the xbl anon content (which I think is the case), it should explicitly observe nodes with NODE_FORCE_XBL_BINDINGS and not pass on those notifications to anyone.  That seems like a better idea than notifying the document and everything that notifies...
Blocks: 380872
Flags: blocking1.9?
Keywords: regression
I think I recall trying to remove that code and having all hell break loose on me. But it could very well be that making the bindingmanager observe those nodes directly will fix it.
Not sure if this should be blocking though, can you think of a way to turn this into an exploitable crash?
Well.. On Linux it _is_ an exploitable crash (null derefs are exploitable on Linux).

Past that...  I'm not sure.  I'd have to check all our document observers.  But in general, violating invariants like this scares me.  It's really easy to either have or add code that doesn't deal with the violation and explodes spectacularly.
I agree it's bad, but it's a matter of prioritizing. It's definitely wanted, and if someone writes a patch I think we should take it. But if "all" it does is a nullpointer deref for now then IMHO we can fix it for the next release.

Is the status still that nullpointer derefs are exploitable on linux? If so that's an OS bug. It's trivial to make us crash that way, simply allocate enough memory and we're bound to forget an OOM check somewhere.
Linux kills processes on OOM, usually way before they get back a null pointer.

Seriously, though, I'm pretty sure I should be able to come up with an exploit for this.  But it seems like a bad time investment to look for it...  If you really feel that we should, I'll put in that time, though.
Reporter

Updated

12 years ago
Summary: "ASSERTION: This method should never be called on content nodes that are not in a document!" → "ASSERTION: This method should never be called on content nodes that are not in a document!" with cloned xul node
Flags: blocking1.9? → blocking1.9+
Whiteboard: [sg:critical?] → [sg:critical?][dbaron-1.9:Rs]
Group: security
CC list accessible: false
Not accessible to reporter
Reporter

Updated

12 years ago
CC list accessible: true
Accessible to reporter
Reporter

Comment 8

12 years ago
Moving from P3 to P2 because this bug gets in the way of testing for an important assertion ("Should be in an update while creating frames").  The sooner this is fixed, the sooner we will find out whether there are other ways to trigger this assertion.
Priority: P3 → P2
Reporter

Comment 9

12 years ago
bz/sicking, do you think you'll be able to fix this for 1.9?  Sooner would probably be better than later, because once this is fixed, I can determine whether I hit other bugs that trigger the "Should be in an update while creating frames" assertion.
It sort of feels bad yeah, though it definitely won't be for beta2. I'd say this is somewhere between a P2 and P3, so I might punt it to P3 if I'm running low on time.

But it'd definitely be good to have for 1.9
Assignee: nobody → jonas
Assignee: jonas → nobody
Component: XP Toolkit/Widgets: XUL → XBL
QA Contact: xptoolkit.xul → xbl
I was thinking... now that binding manager always updates insertion points before calling its observers, would it be reasonable to make those observers observers on the document again, remove the code for forwarding observers from the binding manager, and explicitly notify the binding manager in nsNodeUtils before notifying anything else, even for disconnected subtrees?
Posted patch Like so, saySplinter Review
This also fixes a problem where we didn't remove the insertion parent mapping on ContentRemoved....  Not a big deal, but it was making the testcase kinda fail.  ;)
Assignee: jonas → bzbarsky
Status: NEW → ASSIGNED
Attachment #290829 - Flags: superreview?(jonas)
Attachment #290829 - Flags: review?(jonas)
OS: Mac OS X → All
Hardware: PC → All
Summary: "ASSERTION: This method should never be called on content nodes that are not in a document!" with cloned xul node → [FIX]"ASSERTION: This method should never be called on content nodes that are not in a document!" with cloned xul node
Blocks: 342954
Comment on attachment 290829 [details] [diff] [review]
Like so, say

Hmm.. I'm not super exited about always notifying the binding manager first, though I can't come up with a good argument for not doing so. And it does mean that we're more consistent since it'll be run before _all_ observers, not just the ones on the document...

Could we maybe only notify on the ownerdoc->bindingmanager for nodes either in the tree, or with the FORCE_XBL flag set?

r/sr=me either way. Not sure if this would be safer to land after beta2 though?
Attachment #290829 - Flags: superreview?(jonas)
Attachment #290829 - Flags: superreview+
Attachment #290829 - Flags: review?(jonas)
Attachment #290829 - Flags: review+
> And it does mean that we're more consistent since it'll be run before _all_
> observers

Yes.  In XBL2 terms, we update the flattened tree stuff first, before notifying observers.  That makes the most sense to me.

> Could we maybe only notify on the ownerdoc->bindingmanager for nodes either in
> the tree, or with the FORCE_XBL flag set?

I don't think so.  In particular, we should notify on mutations of the anonymous subtree of a node with the FORCE_XBL flag set, so that the insertion point info will be updated correctly.  Otherwise when it's inserted in the DOM we'll get the wrong things happening.

I did test the usual culprits (mailnews UI in particular) pretty carefully.  My gut instinct is to land this now, and if there is any sort of issue which comes up (which I think is unlikely) to just back it out and reland after b2 when we'll have more time to sort things out.  Does that make sense?
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Reporter

Comment 16

11 years ago
The testcase doesn't trigger any assertions on the 1.8 branch.
Group: security
Flags: wanted1.8.1.x-
verified fixed using the testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre + also on Mac 10.5 - no assertion on testcase 

--> Verified fixed
Status: RESOLVED → VERIFIED
Depends on: 453308
You need to log in before you can comment on or make changes to this bug.