484031 - Reliable way to cause double frame construction for content using XBL

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase is attached.  You might need to download it locally due to bugzilla's attachment URI thing, but it'd work on any web server.

The basic idea is to have a binding with two insertion points; one of these needs to be an inline and the other needs to be a descendant of the inline's containing block.  The we force a ContentAppended with at least two nodes after the binding has been attached (the testcase uses a script that blocks the parser and spins the event queue using sync XHR until the binding has been attached).  The first node has block display and goes into the inline insertion point; the others go into the other insertion point.

Since there are multiple insertion points, the frame constructor converts the ContentAppended call into a sequence of ContentInserted calls.  The first ContentInserted sticks a block inside the inline insertion point, which reframes the containing block.  After that we already have frames for all the other kids, and their ContentInserted calls end up double-creating the frames.  The testcase just uses a single doubled kid: a textnode that uses the text "Doubled", which shouldn't be crashy.  But it could easily toss in iframes, image frames, etc, and cause various trouble.

Possible solutions I see:

1)  Do a GetPrimaryFrameFor before doing ContentInserted in this case.  That
    would also bite fieldsets, since those claim multiple insertion points, but
    might be ok.
2)  If a node has a binding with multiple insertion points (including nested
    ones, etc), never add more than one child at a time to it; notify after
    each child.

#2 is hard to do with our current parser; not sure about Henri's parser.  And in any case, we'd need it for both XHTML and HTML (the page could be HTML if it just linked to a separate XBL file).

Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This doesn't trigger the doubling with Firefox 3 or my 1.9.0 build for some reason.  I thought that had incremental XML parsing, and it certainly has the relevant frame construction code...

In any case, it does trigger doubling with 1.9.1 builds and trunk.

Comment 2

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

1.9.0 does not have incremental XML parsing.

Comment 3

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

No I lie, it does. Sorry.

Comment 4

[Henri Sivonen (:hsivonen)]

If the condition of #2 (node having a binding with multiple insertion points) expensive to find out on a per-insertion basis?

The relevant methods are the two methods starting here:
http://hg.mozilla.org/users/mrbkap_mozilla.com/html5parsing/file/ad080d04a0e5/content/html/parser/src/nsHtml5TreeBuilderHSupplement.h#l86

If would be a matter of the first calling the second conditionally. Thus, easy to do, but the effect depends on how expensive the check is.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Probably somewhat expensive, yes.  I'm tempted to go with #1, since it's more reliable and guarantees that we only take a perf hit in the multiple insertion point case...

Comment 6

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

Does this need an sg: rating?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Probably, yes.  Given what will happen if I stick an animated image in as the doubled node, the right rating is sg:critical.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er, maybe not.  That won't guarantee that we leak the frame, come to think of it.  So we still need a rating, but we might be closer to ok than I thought, esp. with some of smaug's changes to subdocument frames.

Comment 9

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

So do you still think we should block on this? Jonas/Jst?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Let's just fix this.  I realized that the GetPrimaryFrameFor() check is cheap in the common case (the case when we don't double content), due to the MAY_HAVE_FRAME flag and due to this being ContentAppended for multiple nodes (so it's coming from the parser, basically, hence these nodes definitely should not have MAY_HAVE_FRAME set).

roc, what do you think of this approach?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, and to be clear the layout phase stuff is just to fix warnings that were driving me nuts.  I suppose I could move that part into bug 337957...  Patch coming up without that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Just the frame check

This should work unless the node's frame gets suppresed (e.g. whitespace inside a table row or some such).  Not sure what we can do about that case, but "double-constructing" there should be safe enough in any case.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/6a8a88c62925 as usual without test...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 369743 [details] [diff] [review]
Just the frame check

I think we should take this on branches.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I backed this out to see whether it's causing the Tp regression we're seeing.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Relanded as http://hg.mozilla.org/mozilla-central/rev/a45593185b3a

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3649feda0c96

Comment 18

[Samuel Sidler (old account; do not CC)]

Comment on attachment 369743 [details] [diff] [review]
Just the frame check

Approved for 1.9.0.10. a=ss

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed in CVS.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #1)
> Created an attachment (id=368063) [details]
> Testcase
> 
> This doesn't trigger the doubling with Firefox 3 or my 1.9.0 build for some
> reason.  I thought that had incremental XML parsing, and it certainly has the
> relevant frame construction code...
> 
> In any case, it does trigger doubling with 1.9.1 builds and trunk.

Is there any way to verify this fix for 1.9.0? As mentioned above, the attached testcase doesn't cause any issues with 1.9.0.10.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Is there any way to verify this fix for 1.9.0?

Probably, yes.  Start with the attached testcase and modify it to trigger the relevant code on 1.9.0... That might involve some debugging to figure out why it doesn't right now.  I didn't think that was worth it.

Comment 22

[Samuel Sidler (old account; do not CC)]

Boris, does this affect 1.8.1?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Boris, does this affect 1.8.1?

I'd be very very surprised if not.  That said, I'm not sure nothing else along these lines does, so I'm not sure how worth it it is to fix it there.

Comment 24

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8 patch

Updated patch for 1.8 although I can't confirm it fixes the bug on 1.8. The testcase acts the same way with and without the patch.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Martin, you want to roll in or also backport bug 495648.

Comment 26

[Martin Stránský [:stransky] (ni? me)]

(In reply to comment #25)
> Martin, you want to roll in or also backport bug 495648.

1.8 patch attached to bug 495648, thanks for the info.