476245 - Crash [@ nsObjectFrame::Init] with XBL, GC, -moz-column, QuickTime

Status: VERIFIED FIXED

(Core :: XBL, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Temporarily install https://www.squarefree.com/extensions/quitter.xpi to make the testcase's "pleaseGC" function work.

Comment 1

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

Would be great if you could attach a stack

Comment 2

[Jesse Ruderman]

Attachment: stack

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the GetCurrentDoc() of the node passed to nsObjectFrame::Init is null in this case.  It has a non-null GetOwnerDoc().

It looks to me like we're taking the anon content, sticking it into the main DOM, then unbinding it when the gc() tears down the binding.  Or something.

Then we end up with nodes that are in the document but have been unbound.  That's really bad.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Sicking and Shaver made big frowny faces during triage --> blocking and setting security-sensitive.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds like a DOM bug.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although maybe it's a layout anonymous content bug, so I'll take it back.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I suspect it's really an XBL bug at heart...  The fact that you can move the anon content out of the binding like that is just completely broken; we should be throwing when someone tries that.  Imo.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I installed Quitter, but I don't crash on the testcase. I just get

WARNING: *** XBL doc with no root element! Something went horribly wrong! ***: file /Users/roc/mozilla-trunk/content/xbl/src/nsXBLService.cpp, line 475
JavaScript error: https://bug476245.bugzilla.mozilla.org/attachment.cgi?id=359876&t=0bb8sQlfXO, line 20: document.getAnonymousNodes(v) is null
WARNING: *** XBL doc with no root element! Something went horribly wrong! ***: file /Users/roc/mozilla-trunk/content/xbl/src/nsXBLService.cpp, line 475

Breaking at that line, I see that bindingDocument and mBoundDocument are two different documents (the former nsXMLDocument, the latter nsHTMLDocument), which makes no sense. This reminds me of bug 445232, where I can't get XBL to work in testcases that Jesse does...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Odd...  The crash worked just fine for me...

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Can't reproduce this (installed Quitter).
Should some pref be enabled?

Comment 11

[Damon Sicore (:damons)]

Need an owner here.  Takers?

Comment 12

[Jesse Ruderman]

Still crashes for me.  I had to save the file from Bugzilla because of Bugzilla's new token/redirect thing for authenticating access to attachments.  Be sure to save it with an .xhtml extension.

Comment 13

[Johnny Stenback (:jst)]

bz, can you take this one (as you have no other blockers that I know :)? If not, let me know and I'll find someone else.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, so the situation is a bit more complicated than I thought, actually.  Normally we do NOT allow removal of anon content from the DOM via DOM APIs, because we check its index in its parent and then bail.  But in this case, the removal of the bound node from the document triggers an UninstallAnonymousContent from nsXBLBinding::ChangeDocument.  Then ~nsXBLBinding does another UninstallAnonymousContent on mContent.  In between, the script grabbed the now-unbound anon content and stuck it into the main page DOM.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Would it help if we had a flag to tell that UninstallAnonymousContent is
already called, or perhaps UninstallAnonymousContent could clear mContent?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I considered that, but I'd rather enforce our invariants about anon content roots.  To that end, I think I'm going to change things so that NODE_IS_ANONYMOUS is set for all anon content subtree roots, not just native ones, and check for this flag in nsGenericElement::doReplaceOrInsertBefore.

Still compiling with the changes and need to test, but I think that should do the trick.

Comment 17

[Johnny Stenback (:jst)]

Setting NODE_IS_ANONYMOUS will make it impossible for non-chrome code to touch that node once bug 475864 is fixed.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Why, exactly?  I'm keeping the distinction between IsRootOfAnonymousSubtree(), IsRootOfNativeAnonymousSubtree(), IsNativeAnonymous(), etc...

Comment 19

[Johnny Stenback (:jst)]

Sorry, my bad, too many flags. NODE_IS_IN_ANONYMOUS_SUBTREE is the flag that will have that effect.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, perhaps

This makes the testcase throw on the appendChild(anon) call.

I'll try to write a mochitest for this once we settle whether this is the behavior we want.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 363250 [details] [diff] [review]
Like so, perhaps

If this is taken to branch, also Bug 476097 (or the nsIContent change) is needed.

The patch changes svg:use behavior. Perhaps you could have SetFlags(NODE_IS_ANONYMOUS) in BindToTree so that the flag is set whenever parent == bindingparent?
Or put SetFlags to CSSFC to keep the current svg:use behavior.
Either way, r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ah, <use> would actually trigger the assert I added in IsRootOfAnonymousSubtree(), indeed.  

I'll add a SetFlags(NODE_IS_ANONYMOUS) to the CSSFC code.  Good catch there.

I knoew I asked the right folks for review.  ;)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Pushed <http://hg.mozilla.org/mozilla-central/rev/a328b5ae57e0>.  I guess I should hold the test until after we open up this bug?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/08453f810585

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This got backed out of m-c for breaking test_videocontrols.html and 1.9.1 for leaks.  (bz said he'd update the bug, but I'm commenting quickly so it doesn't get lost.)

Comment 26

[Boris Zbarsky [:bzbarsky]]

The 1.9.1 issue was actually not leaks; it was us failing an assert on the leak test (which runs with fatal asserts) because bug 476097 never landed on branch.  So I'll just add the nsIContent change from that to the branch patch for this bug.

Looking into the videocontrols issue.

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK, so this is failing because video.currentTime comes out to 2.066 while the expectedTime is 1.9165 (because videoDuration is 3.833).  The test asserts that the currentTime and expectedTime differ by no more than 0.1.

In a build without this patch, the duration and expected time are identical, but video.currentTime is 1.999, which is within the 0.1 tolerance.

It's not clear to me why this patch would affect the video.currentTime in this testcase.  I guess I'll dig into that, but any ideas would be much appreciated...

Oh, with the patch it looks like the progress bar for the video doesn't show anything either.

Comment 28

[Boris Zbarsky [:bzbarsky]]

OK, so the problem is that if XBL is used to bind anonymous content to a native anonymous node all that anonymous content is marked with NODE_IS_IN_ANONYMOUS_SUBTREE.  Then since we mark the root XBL-bound nodes with NODE_IS_ANONYMOUS they look like native anonymous content roots.  This probably changes the way event retargeting works, or something, thus confusing something in the video controls.

Options seem to be:

1)  Sort out exactly what the video controls issue is and fix it.
2)  Don't mark XBL-bound anon kids of a native anon node with
    NODE_IS_IN_ANONYMOUS_SUBTREE.  This would fail the asserts we now have in
    nsIContent::IsInNativeAnonymousSubtree, so might not be that desirable,
    since it would cause user sheets to apply to this XBL-bound content.  Then
    again, maybe we want them to?  It might also fix bug 470852.
3)  Don't mark XBL-bound anon kids of a native anon node with
    NODE_IS_ANONYMOUS.  That is, don't apply this bug's fix to those nodes;
    just assume that content can't get at them and that the code that can won't
    do bad things.  This is probably the safest fix.  It'll require also
    changing IsRootOfAnonymousSubtree to deal with this special case.

Smaug, thoughts?  In particular, will option 2 be OK in terms of what happens to event targets and originalTargets? 

I've tested that both option 2 and option 3 fix the videocontrols test.

Comment 29

[Justin Dolske [:Dolske]]

The seeking threshold in the test is there because the seek sometimes isn't deterministic. Even seeking to 0.0 sometimes ended up at a non-zero position. Dunno why, filed bug 477434. It sure seems surprising that this patch would affect that, makes me wonder if there's some random uninitialized data getting into the mix (such that this patch pushes something around and causes a different result).

Comment 30

[Boris Zbarsky [:bzbarsky]]

Given comment 28, I suspect it's just a matter of what nodes are event targets.

I suppose there's also an option 4:

4) Change the IsRootOfAnonymousSubtree() check to check whether the parent is
   native anonymous too.  This would mean that native anon roots whose parent is
   native anon wouldn't test to be roots.  I don't think we should do it.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #28)

> This probably
> changes the way event retargeting works, or something, thus confusing 
> something in the video controls.
Retargeting happens when event propagates from native anon root to its parent.
Same thing happens when propagation goes from XBL-anon to binding parent.
But at least mouse event handling changes if something becomes native anon,
because we don't want to propagate mouseover and mouseout events when they
occur within the same native anon subtree.

> Options seem to be:
> 
> 1)  Sort out exactly what the video controls issue is and fix it.
> 2)  Don't mark XBL-bound anon kids of a native anon node with
>     NODE_IS_IN_ANONYMOUS_SUBTREE.  This would fail the asserts we now have in
>     nsIContent::IsInNativeAnonymousSubtree, so might not be that desirable,
>     since it would cause user sheets to apply to this XBL-bound content.
Right. I really don't like this option. Some nodes in native anon wouldn't
be native anon after all...

>  Then
>     again, maybe we want them to?  
Why would we want that?


>It might also fix bug 470852.
> 3)  Don't mark XBL-bound anon kids of a native anon node with
>     NODE_IS_ANONYMOUS.  That is, don't apply this bug's fix to those nodes;
>     just assume that content can't get at them and that the code that can won't
>     do bad things.  This is probably the safest fix.  It'll require also
>     changing IsRootOfAnonymousSubtree to deal with this special case.
This is ugly, but possible.

> 
> Smaug, thoughts?  In particular, will option 2 be OK in terms of what happens
> to event targets and originalTargets? 
In terms of event handling 2 and 3 should be ok.

> 4) Change the IsRootOfAnonymousSubtree() check to check whether the parent is
>    native anonymous too.  This would mean that native anon roots whose parent
> is
>    native anon wouldn't test to be roots.  I don't think we should do it.
I believe this would break mouse event handling on form elements, like <input type="file"> and perhaps also <select size>1 >

Comment 32

[Boris Zbarsky [:bzbarsky]]

I guess there is also:

5) Add a third flag to indicate what I want to indicate here, since two flags are
   not enough to draw all the distinctions we want to draw.

I'm going to go with solution 3, as the safest one of the bunch (barring 5, which eats up an extra flag bit).

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Well, there are still plenty of bits left.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix with the added flag

This includes the an iid rev for nsINode, since I'm changing the values of a bunch of flags.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff against previous patch, for review ease

Comment 36

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/1963ead6e19b

Comment 37

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/53ef73297533

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre ID:20090310044308

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090309 Shiretoko/3.1b4pre (.NET CLR 3.5.30729) ID:20090309034003