742 bytes, application/xhtml+xml
1.45 KB, patch
Samuel Sidler (old account; do not CC): approval188.8.131.52+
|Details | Diff | Splinter Review|
1022 bytes, patch
|Details | Diff | Splinter Review|
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.
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?
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.
Pushed http://hg.mozilla.org/mozilla-central/rev/6a8a88c62925 as usual without test...
Comment on attachment 369743 [details] [diff] [review] Just the frame check I think we should take this on branches.
I backed this out to see whether it's causing the Tp regression we're seeing.
Comment on attachment 369743 [details] [diff] [review] Just the frame check Approved for 184.108.40.206. a=ss
Fixed in CVS.
(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 220.127.116.11.
> 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.
Boris, does this affect 1.8.1?
> 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.
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.
(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.