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

Status: VERIFIED FIXED

(Core :: XBL, defect, P2)

Description

[Jesse Ruderman]

Attachment: 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].

Comment 1

[Jesse Ruderman]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure if this should be blocking though, can you think of a way to turn this into an exploitable crash?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Jesse Ruderman]

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.

Comment 9

[Jesse Ruderman]

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.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, say

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.  ;)

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 16

[Jesse Ruderman]

The testcase doesn't trigger any assertions on the 1.8 branch.

Comment 17

[Carsten Book [:Tomcat]]

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