Reliable way to cause double frame construction for content using XBL

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
P2
normal
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({fixed1.9.0.11, fixed1.9.1})

Trunk
x86
Mac OS X
fixed1.9.0.11, fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
wanted1.9.0.x ?
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments, 1 obsolete attachment)

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?
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.
Flags: blocking1.9.1?
1.9.0 does not have incremental XML parsing.
No I lie, it does. Sorry.
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.
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...
Does this need an sg: rating?
Probably, yes.  Given what will happen if I stick an animated image in as the doubled node, the right rating is sg:critical.
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.
So do you still think we should block on this? Jonas/Jst?
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?
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #369742 - Flags: superreview?(roc)
Attachment #369742 - Flags: review?(roc)
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.
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.
Attachment #369742 - Attachment is obsolete: true
Attachment #369743 - Flags: superreview?(roc)
Attachment #369743 - Flags: review?(roc)
Attachment #369742 - Flags: superreview?(roc)
Attachment #369742 - Flags: review?(roc)
Attachment #369743 - Flags: superreview?(roc)
Attachment #369743 - Flags: superreview+
Attachment #369743 - Flags: review?(roc)
Attachment #369743 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/6a8a88c62925 as usual without test...
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment on attachment 369743 [details] [diff] [review]
Just the frame check

I think we should take this on branches.
Attachment #369743 - Flags: approval1.9.1?
Attachment #369743 - Flags: approval1.9.0.10?
I backed this out to see whether it's causing the Tp regression we're seeing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded as http://hg.mozilla.org/mozilla-central/rev/a45593185b3a
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Attachment #369743 - Flags: approval1.9.1?
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3649feda0c96
Keywords: fixed1.9.1
Flags: wanted1.9.0.x?
Whiteboard: [sg:critical]
Attachment #369743 - Flags: approval1.9.0.10? → approval1.9.0.10+
Comment on attachment 369743 [details] [diff] [review]
Just the frame check

Approved for 1.9.0.10. a=ss
Fixed in CVS.
Keywords: fixed1.9.0
Keywords: fixed1.9.0 → fixed1.9.0.10
(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.
> 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.
Whiteboard: [sg:critical] → [sg:critical?]
Boris, does this affect 1.8.1?
Flags: wanted1.8.1.x?
Flags: blocking1.8.1.next?
Depends on: 495648
> 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

8 years ago
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.
Martin, you want to roll in or also backport bug 495648.

Comment 26

8 years ago
(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.
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next+
Group: core-security
You need to log in before you can comment on or make changes to this bug.