Last Comment Bug 484031 - Reliable way to cause double frame construction for content using XBL
: Reliable way to cause double frame construction for content using XBL
Status: RESOLVED FIXED
[sg:critical?]
: fixed1.9.0.11, fixed1.9.1
Product: Core
Classification: Components
Component: Layout: Misc Code (show other bugs)
: Trunk
: x86 Mac OS X
: P2 normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 495648
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-18 11:18 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2013-03-31 16:04 PDT (History)
15 users (show)
roc: blocking1.9.1+
dveditz: wanted1.9.0.x?
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (742 bytes, application/xhtml+xml)
2009-03-18 11:21 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Fix (3.52 KB, patch)
2009-03-27 14:12 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Just the frame check (1.45 KB, patch)
2009-03-27 14:25 PDT, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
roc: superreview+
samuel.sidler+old: approval1.9.0.11+
Details | Diff | Splinter Review
1.8 patch (1022 bytes, patch)
2009-06-14 23:50 PDT, Martin Stránský
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2009-03-18 11:18:12 PDT
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 [:bz] (still a bit busy) 2009-03-18 11:21:50 PDT
Created attachment 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.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-18 13:59:07 PDT
1.9.0 does not have incremental XML parsing.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-18 13:59:23 PDT
No I lie, it does. Sorry.
Comment 4 Henri Sivonen (:hsivonen) 2009-03-20 06:39:08 PDT
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 [:bz] (still a bit busy) 2009-03-20 06:53:30 PDT
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] 2009-03-24 21:46:21 PDT
Does this need an sg: rating?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2009-03-24 21:49:57 PDT
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 [:bz] (still a bit busy) 2009-03-24 21:51:04 PDT
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] 2009-03-27 13:19:21 PDT
So do you still think we should block on this? Jonas/Jst?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2009-03-27 14:12:13 PDT
Created attachment 369742 [details] [diff] [review]
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 [:bz] (still a bit busy) 2009-03-27 14:15:26 PDT
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 [:bz] (still a bit busy) 2009-03-27 14:25:29 PDT
Created attachment 369743 [details] [diff] [review]
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 [:bz] (still a bit busy) 2009-03-30 08:40:43 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/6a8a88c62925 as usual without test...
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2009-03-30 08:41:08 PDT
Comment on attachment 369743 [details] [diff] [review]
Just the frame check

I think we should take this on branches.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2009-03-30 15:05:08 PDT
I backed this out to see whether it's causing the Tp regression we're seeing.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2009-03-30 20:22:07 PDT
Relanded as http://hg.mozilla.org/mozilla-central/rev/a45593185b3a
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2009-03-31 12:01:20 PDT
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3649feda0c96
Comment 18 Samuel Sidler (old account; do not CC) 2009-04-10 16:08:21 PDT
Comment on attachment 369743 [details] [diff] [review]
Just the frame check

Approved for 1.9.0.10. a=ss
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2009-04-17 10:12:13 PDT
Fixed in CVS.
Comment 20 Al Billings [:abillings] 2009-05-11 15:05:06 PDT
(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 [:bz] (still a bit busy) 2009-05-11 18:29:22 PDT
> 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) 2009-05-29 10:26:27 PDT
Boris, does this affect 1.8.1?
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2009-06-01 15:14:42 PDT
> 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ý 2009-06-14 23:50:53 PDT
Created attachment 383226 [details] [diff] [review]
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 [:bz] (still a bit busy) 2009-06-15 15:14:43 PDT
Martin, you want to roll in or also backport bug 495648.
Comment 26 Martin Stránský 2009-06-18 05:43:05 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.