REGRESSION(87526): ASSERT(!needsLayout()) followed by graphical glitches on google charts (svg loaded in iframe)

[James Robinson]

The URL listed loads up an SVG document inside an iframe.  Since http://trac.webkit.org/changeset/87526 landed, this page fails assertions in debug.  This assertion failure corresponds to some very strange graphical glitches in release builds.

[Nikolas Zimmermann]

Ouch, could you reduce a testcase for me? This sounds pretty important to fix!

[Alexey Proskuryakov]

<rdar://problem/9824256>

[Levi Weintraub]

We don't have a repro we can share yet, but I know the offending line. When RenderSVGRoot::negotiateSizeWithHostDocumentIfNeeded calls ownerRenderer->setNeedsLayoutPrefWidthsRecalc(), it sets us up for this. We get called to paint before that layout has occurred which triggers this bug. Commenting this out makes the assertions go away.

[Nikolas Zimmermann]

(In reply to comment #3)
> We don't have a repro we can share yet, but I know the offending line. When RenderSVGRoot::negotiateSizeWithHostDocumentIfNeeded calls ownerRenderer->setNeedsLayoutPrefWidthsRecalc(), it sets us up for this. We get called to paint before that layout has occurred which triggers this bug. Commenting this out makes the assertions go away.

That's what I guessed as well. I believe this is related to other flakiness in that area, that is guilty for some svg/zoom/page/zoom-object* tests that have recently been disabled. Let's see.

It would be great if we had a simple reduction though!

[Levi Weintraub]

I believe I have an idea as the problem here, but still no solution. Layout is rooted at the frame level, and the code in RenderSVGRoot sets the dirty layout bit on contents of its containing frame. If the containing frame has already layed out at this point, there's no infrastructure to tell the main FrameView to re-layout that frame before painting, which triggers this problem.

Now the question is how can we avoid invalidating the containing frame or triggering re-layout before flushDeferredRepaints is called.

[Levi Weintraub]

Created attachment 101934 [details]
Patch

[James Robinson]

Comment on attachment 101934 [details]
Patch

R=me.  Unfortunately I don't think there is any simple way to salvage this code, it needs to be redone.

[WebKit Review Bot]

Comment on attachment 101934 [details]
Patch

Rejecting attachment 101934 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
nd text mismatch : (2)
  http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml = IMAGE+TEXT
  svg/hixie/intrinsic/002.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/9244255

[WebKit Review Bot]

Comment on attachment 101934 [details]
Patch

Attachment 101934 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9244270

New failing tests:
http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
svg/hixie/intrinsic/002.html

[Nikolas Zimmermann]

Comment on attachment 101934 [details]
Patch

I'm removing the r+.

Do you consider it polite just rolling it out, instead of giving me a chance to have a look? There's no reduction here, nothing I could have done so far.

Please think about what you do.

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 101934 [details])
> R=me.  Unfortunately I don't think there is any simple way to salvage this code, it needs to be redone.

First of all, I think I am the one who's supposed to review this, no?
Second, there's no way to reproduce the problem for me on non-Chromium, atm.
I am waiting for a reduction, and thus can't do any debugging until I have one.

I'm a bit angry right now. You can't remove a vital piece of new code, without even waiting for the original authors opinion.

[James Robinson]

Sorry, but this function is clearly incorrect, even to a casual inspection.  It doesn't look like the author or reviewer were very familiar with how layout works in WebKit, or more likely they missed it, but there's no way this will work without significant other changes.

[Nikolas Zimmermann]

(In reply to comment #12)
> Sorry, but this function is clearly incorrect, even to a casual inspection.  It doesn't look like the author or reviewer were very familiar with how layout works in WebKit, or more likely they missed it, but there's no way this will work without significant other changes.

And this is enough justification to roll-out a _piece_ of the patch without further discussing with the original author?

This is not acceptable, in any way.

[Nikolas Zimmermann]

I agree that there's a bug in the code, that's pretty obvious, on a second sight. It only works for a few tests, because the order of paint/layout calls just matches there (though there's still a race).

I'm going to look into a proper solution.

@James: To summarize again, why I felt angry.
The patch landed over 2 months ago. Now a  bug report has been opened, and a fix is attached, that you gave r+, without giving me the time to comment it. It's not that I have ignored this bug report or anything.

I consider this impolite, and others will agree with me.
Also you should rethink whether it's helpful to state that neither the author nor the reviewer has an understanding of the layout code. After all Rob & me have worked with this codebase since a decade now.
That doesn't mean we're producing bug-free code, nor that my code is always regression-free.

[Nikolas Zimmermann]

Created attachment 102004 [details]
New patch

New approach, which hopefully fixes the whole scope of the problem.
Please look carefuly, wheter I forgot something, or if there's still a misunderstanding present.

The bug is fixed, and I see no regressions on Google Charts anymore. It's very hard to create a reduction though :(

[Nikolas Zimmermann]

Created attachment 102010 [details]
New patch - v2

Not sure, why it doesn't apply. I started from scratch with LayoutTests, and did all svn mv's again - maybe it works now.

[WebKit Review Bot]

Comment on attachment 102010 [details]
New patch - v2

Attachment 102010 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9224023

New failing tests:
svg/zoom/page/zoom-svg-through-object-with-override-size.html
svg/zoom/page/zoom-svg-through-object-with-huge-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml

[Levi Weintraub]

(In reply to comment #14)
> @James: To summarize again, why I felt angry.
> The patch landed over 2 months ago. Now a  bug report has been opened, and a fix is attached, that you gave r+, without giving me the time to comment it. It's not that I have ignored this bug report or anything.

Sorry to have angered you Nikolas. This was a nasty bug that we needed a fix for. I appreciate you getting a superior one out the door so fast.

As for rolling out a piece of the patch, the original no longer unapplies cleanly, and both our patches remove the fundamentally broken function.

[Nikolas Zimmermann]

(In reply to comment #18)
> Sorry to have angered you Nikolas. This was a nasty bug that we needed a fix for. I appreciate you getting a superior one out the door so fast.
I wasn't angry about your patch at all, rather that it got r+ before I could comment...
Anyhow, it would still be great to get a reduction for that, but it's hard with the obfuscated js...

[James Robinson]

This patch has exactly the same bug that the old code had, and triggers ASSERT(!needsLayout()) in FrameView::paintContents() in debug, although unreliably.  It's highly sensitive to timing, but the bug itself is pretty clear - our layout code does not work properly if you mark a containing FrameView as needing layout during layout.  In other words, we don't handle dependency cycles.  The latest patch changes the ordering of the cycle around, but it still has it.

Consider the typical painting routine (http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp#L597 is one example, although AFAIK all ports do roughly the same thing).  When this function enters, it expects to be able to call FrameView:: updateLayoutAndStyleIfNeededRecursive() on the main frame, then call paintContents() on the main frame.  FrameView:: updateLayoutAndStyleIfNeededRecursive() (http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L2631) does a preorder traversal and calls layout() on each of these.  It's important that this traversal is preorder, since layout on a frame might changes the dimensions of an <iframe> causing more layout to happen inside the subframe.  It's also critical that during this iteration the results of layout inside a subframe do not effect anything outside of that frame.

With this patch (or the previous code), layout on a subframe marks a parent as needing layout.  If this codepath is triggered by the embedder wanting to paint, then the call to FrameView::paintContents() will hit ASSERT(!needsLayout()) in debug and paint incorrect results in release.  If the layout is triggered by a script query (for, say, offsetWidth) then the code will just return the wrong result.  Introducing cross-frame layout dependencies is something that needs to be done carefully, since it introduces potentially non-terminating cycles.

[James Robinson]

Comment on attachment 102010 [details]
New patch - v2

View in context: https://bugs.webkit.org/attachment.cgi?id=102010&action=review

> Source/WebCore/page/FrameView.cpp:2120
> +    m_frame->ownerRenderer()->setNeedsLayoutAndPrefWidthsRecalc();

This is the exact same bug as before.

[Nikolas Zimmermann]

(In reply to comment #21)
> (From update of attachment 102010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102010&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2120
> > +    m_frame->ownerRenderer()->setNeedsLayoutAndPrefWidthsRecalc();
> 
> This is the exact same bug as before.
Hm, is that true? Before we marked the ownerRenderer as setNeedsLayout, after layout and performPostLayoutTasks were done. Anyhow...

(In reply to comment #20)
> This patch has exactly the same bug that the old code had, and triggers ASSERT(!needsLayout()) in FrameView::paintContents() in debug, although unreliably.  It's highly sensitive to timing, but the bug itself is pretty clear - our layout code does not work properly if you mark a containing FrameView as needing layout during layout.  In other words, we don't handle dependency cycles.  The latest patch changes the ordering of the cycle around, but it still has it.
I changed exactly that - it doesn't trigger layout while the containing FrameView layout is still running. The containing FrameView layout finishes, then peformPostLayoutTasks is called, and at this point, I'm calling setNeedsLayoutAndPrefWidthsRecalc(), which then schedules a layout again for the containing block.
Are you worried that the async scheduling is the problem here? (aka. we could receive paint events, before we re-scheduled layout starts?).

> 
> Consider the typical painting routine (http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp#L597 is one example, although AFAIK all ports do roughly the same thing).  When this function enters, it expects to be able to call FrameView:: updateLayoutAndStyleIfNeededRecursive() on the main frame, then call paintContents() on the main frame.  FrameView:: updateLayoutAndStyleIfNeededRecursive() (http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L2631) does a preorder traversal and calls layout() on each of these.  It's important that this traversal is preorder, since layout on a frame might changes the dimensions of an <iframe> causing more layout to happen inside the subframe. 
Agreed, this is how it works...

> It's also critical that during this iteration the results of layout inside a subframe do not effect anything outside of that frame.
Exactly, that's why it failed obviously before as RenderSVGRoot::layout affected its containing FrameView layout (negotiateSizeWithHostDocument). At least that part is gone now.
 
> With this patch (or the previous code), layout on a subframe marks a parent as needing layout.  If this codepath is triggered by the embedder wanting to paint, then the call to FrameView::paintContents() will hit ASSERT(!needsLayout()) in debug and paint incorrect results in release.  If the layout is triggered by a script query (for, say, offsetWidth) then the code will just return the wrong result. 
If it's possible to trigger paint events and/or scripting after I called setNeedsLayout() on the ownerRenderer, before the scheduled relayout takes place, you're right.

> Introducing cross-frame layout dependencies is something that needs to be done carefully, since it introduces potentially non-terminating cycles.
Well I did just that, I'm tracking the negotiation to break the cycle: if the SVG has done layout once, and we then notified the parent FrameView to relayout, we make sure we won't notify the FrameView again, as long as the SVG document didn't change its size.


Okay I'm rethinking this again. Would following work:

- When FrameView::updateLayoutAndStyleIfNeededRecursive() is called for the top main frame of the page, we're calling layout() and finish the layout for the top main frame, except for its widgets.
- When layout is done, we're diving recusively into the FrameViews associated with the Widgets and lay them out.

A quick solution would be:
Make updateLayoutAndStyleIfNeededRecursive return a bool. If it returns true, just continue diving into the FrameViews, recurisvely. If it returns false, we'll stop immediately, go back to our root FrameView, and restart the recursive layout. At this point repaints are deferred, so we should be safe.

What do you think?

[Nikolas Zimmermann]

Created attachment 102126 [details]
Patch v3

Prototyping the new concept, seems to work fine - and should be much safer.
James, please double-check if I missed something!

[Nikolas Zimmermann]

Created attachment 102128 [details]
Patch v4

Cleanup patch, there were some leftovers.

[James Robinson]

Comment on attachment 102128 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=102128&action=review

Do we really have to do a full 2-pass layout here?  Is there any way to negotiate the dimensions without running the full layout algorithm?  It's very difficult to prove that this always terminates after 2 passes.

I can't find any specification that defines this behavior, would you mind linking to it?

> Source/WebCore/page/FrameView.cpp:2678
> +    // If leaveLayout is true, one of the child FrameViews triggered a relayout of our main frame.

Is there anything special about the main frame?  It seems that a child FrameView could trigger a relayout of any ancestor FrameView in this scheme.  For example:

<body>
  <iframe>
    <iframe src="something.svg">

it seems that after calling updateLayoutAndStyleIfNeededRecursive() the intermediate iframe could be marked as needing layout, but not the main frame.  In that case, is there any special reason to pop the stack all the way back to the rootmost frame before recursing?

> Source/WebCore/page/FrameView.cpp:2693
> +            return result;

Do you need to call flushDeferredRepaints() for any of these early-aborts?  The comment below leads me to think that you do.

> Source/WebCore/page/FrameView.cpp:2700
> +            ASSERT(contentBox->isSVGRoot());

You need some #if ENABLE(SVG) around here.  It's also pretty gross to make FrameView so intimately aware of SVG specifics.

[James Robinson]

Also keep in mind there are other ways to enter layout.  For example, the JS bindings for layout-derived properties such as offsetWidth call Document::updateStyleIgnorePendingStylesheets(), which doesn't hit any of the codepaths you've patched here.  I think that they are likely to just return the wrong result.

[Nikolas Zimmermann]

(In reply to comment #25)
> (From update of attachment 102128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102128&action=review
> 
> Do we really have to do a full 2-pass layout here?  Is there any way to negotiate the dimensions without running the full layout algorithm?  It's very difficult to prove that this always terminates after 2 passes.
A full two pass layout is probably not needed - I only wanted to demonstrate it works, in theory. I'm sure I can do better.

> 
> I can't find any specification that defines this behavior, would you mind linking to it?
Sure, I'll give you all relevant links.

The following parts of CSS 2.1 describe how to compute the width/height for inline replaced elements:
http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-width
http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-height

http://dev.w3.org/SVG/profiles/1.1F2/publish/coords.html#ViewportSpace
The whole paragraph is important, especially "The negotiation process uses any information provided by the containing document and the SVG content itself to choose the viewport location and size.".

As you can see in the CSS 2.1 links, we need to know the intrinsic width/height/ratio of the embedded document, to be able to figure out the right size of the <iframe>. That means we have to re-layout the iframe once the SVG doc appeared.

> 
> > Source/WebCore/page/FrameView.cpp:2678
> > +    // If leaveLayout is true, one of the child FrameViews triggered a relayout of our main frame.
> 
> Is there anything special about the main frame?  It seems that a child FrameView could trigger a relayout of any ancestor FrameView in this scheme.  For example:
> 
> <body>
>   <iframe>
>     <iframe src="something.svg">
> 
> it seems that after calling updateLayoutAndStyleIfNeededRecursive() the intermediate iframe could be marked as needing layout, but not the main frame.  In that case, is there any special reason to pop the stack all the way back to the rootmost frame before recursing?
Nope there's no special reason, and it's actually wrong. I'll make sure to fix it in a next version of the patch.

> 
> > Source/WebCore/page/FrameView.cpp:2693
> > +            return result;
> 
> Do you need to call flushDeferredRepaints() for any of these early-aborts?  The comment below leads me to think that you do.
I'm not sure about that one.

> 
> > Source/WebCore/page/FrameView.cpp:2700
> > +            ASSERT(contentBox->isSVGRoot());
> 
> You need some #if ENABLE(SVG) around here.  It's also pretty gross to make FrameView so intimately aware of SVG specifics.
I agree, I'll look into making it more abstract. Though you should be aware that in theory every content type that has an instrinic size&ratio could participate in this logic (could and should!). Think about embedding PDFs using <iframe>. We do want to support intrinsic size negotiation with PDF documents at some point.

Supporting SVGs is just the start.

[Nikolas Zimmermann]

(In reply to comment #26)
> Also keep in mind there are other ways to enter layout.  For example, the JS bindings for layout-derived properties such as offsetWidth call Document::updateStyleIgnorePendingStylesheets(), which doesn't hit any of the codepaths you've patched here.  I think that they are likely to just return the wrong result.

You mean updateLayoutIgnorePendingStylesheets(), right?

When asking the iframe for it's size (after calling eg. offsetWidth) you'd get 300x150 for a <iframe src="foo.svg"> if foo is not loaded yet. That's actually correct. As soon as it appears, a relayout is triggered, and at that point I'd get the right intrinsic dimensions.

I'm not fully convinced yet, I need to handle the size negotiation stuff right in FrameView::layout.

[Nikolas Zimmermann]

CC'ing Dan & Simon, to maybe get additional input. I'm uploading a revised version soon, that avoids doing two full-pass layouts, as suggested by James.

[Nikolas Zimmermann]

(In reply to comment #28)
> (In reply to comment #26)
> > Also keep in mind there are other ways to enter layout.  For example, the JS bindings for layout-derived properties such as offsetWidth call Document::updateStyleIgnorePendingStylesheets(), which doesn't hit any of the codepaths you've patched here.  I think that they are likely to just return the wrong result.
> 
> You mean updateLayoutIgnorePendingStylesheets(), right?
> 
> When asking the iframe for it's size (after calling eg. offsetWidth) you'd get 300x150 for a <iframe src="foo.svg"> if foo is not loaded yet. That's actually correct. As soon as it appears, a relayout is triggered, and at that point I'd get the right intrinsic dimensions.
> 
> I'm not fully convinced yet, I need to handle the size negotiation stuff right in FrameView::layout.

Self-respond:
I'm going to create a testcase that does:
<body><iframe src="foo.svg"></iframe>

foo.svg then runs a script that changes its intrinsic size after a while - let's see whether this kind of updating already works or not...

I'll write some testcases regarding nesting SVGs and iframes first.

[WebKit Review Bot]

Comment on attachment 102128 [details]
Patch v4

Attachment 102128 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9224043

New failing tests:
css2.1/20110323/replaced-intrinsic-005.htm
css2.1/20110323/replaced-intrinsic-001.htm
css2.1/20110323/replaced-intrinsic-004.htm
svg/zoom/page/zoom-svg-through-object-with-huge-size.xhtml
fast/forms/input-spinbutton-capturing.html
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
fast/forms/implicit-submission.html
http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml
fast/forms/input-number-large-padding.html
fast/forms/input-number-events.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml
svg/zoom/page/zoom-svg-through-object-with-override-size.html
svg/hixie/intrinsic/002.html
http/tests/inspector/resource-tree/resource-tree-reload.html

[Levi Weintraub]

We haven't really gotten where we need to be and this is a blocking issue for Chromium. Seems like we should land my fix in the meantime...

[Nikolas Zimmermann]

I'm still debugging it, got the original testcase working again as dicussed with jamesr on IRC though sync layout triggering via <script> is problematic. I'm working on a proper fix.

Side note:
I don't get why Chromium folks always want to get things reverted, instead of waiting for a proper fix.
If you want to release a new Chrome version, it seems totally fine to apply your patch in that certain release branch, but why in trunk?

We generally roll out patches if they eg. break the mac build or tests, and this is not the case here - it works on mac as-is (even though depending on a hack, but that doesn't matter.)

[James Robinson]

What is currently in trunk definitely does not work on mac, or anywhere else.  It's badly broken and has been for over a month now.  That said, I think it'd be fine for us (chromium) to just revert on our release branch, as we have been.

[Nikolas Zimmermann]

(In reply to comment #34)
> What is currently in trunk definitely does not work on mac, or anywhere else.  It's badly broken and has been for over a month now.  That said, I think it'd be fine for us (chromium) to just revert on our release branch, as we have been.

Please use "new-run-webkit-tests --tolerance 0 -p svg" and see it magically works on mac as-is.
That's what I've been saying since a month? On mac, it's hard to trigger the bug - on chromium it always triggers it.

[Levi Weintraub]

Hard to trigger isn't the same as working :-/

[Nikolas Zimmermann]

(In reply to comment #35)
> (In reply to comment #34)
> > What is currently in trunk definitely does not work on mac, or anywhere else.  It's badly broken and has been for over a month now.  That said, I think it'd be fine for us (chromium) to just revert on our release branch, as we have been.
> 
> Please use "new-run-webkit-tests --tolerance 0 -p svg" and see it magically works on mac as-is.
> That's what I've been saying since a month? On mac, it's hard to trigger the bug - on chromium it always triggers it.

Note: I still agree the code is broken, don't get me wrong, I can create a testcase that even fails with mac trunk. I'm just arguing that the tests work. My both Snow Leopard machines run the SVG pixel tests without a problem with tolerance 0.011 (our default threshold shared with Dirk & Rob).

[Nikolas Zimmermann]

(In reply to comment #36)
> Hard to trigger isn't the same as working :-/

I never claimed that! I'm just working full time to fix the bug atm, so I'd rather like to avoid reverting the patch, as I plan to have it fixed fully in the next days :-)

[Levi Weintraub]

(In reply to comment #38)
> (In reply to comment #36)
> > Hard to trigger isn't the same as working :-/
> 
> I never claimed that! I'm just working full time to fix the bug atm, so I'd rather like to avoid reverting the patch, as I plan to have it fixed fully in the next days :-)

Your effort is appreciated :) I'm going to proceed without reverting your patch, but I'd add that breaking the tests or Mac build isn't the only criteria for rolling out patches, particularly when you can repro the bugs outside of the test harness. Code that breaks like this shouldn't really be allowed to live on in trunk imho.

[James Robinson]

(In reply to comment #37)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > What is currently in trunk definitely does not work on mac, or anywhere else.  It's badly broken and has been for over a month now.  That said, I think it'd be fine for us (chromium) to just revert on our release branch, as we have been.
> > 
> > Please use "new-run-webkit-tests --tolerance 0 -p svg" and see it magically works on mac as-is.
> > That's what I've been saying since a month? On mac, it's hard to trigger the bug - on chromium it always triggers it.
> 
> Note: I still agree the code is broken, don't get me wrong, I can create a testcase that even fails with mac trunk. I'm just arguing that the tests work. My both Snow Leopard machines run the SVG pixel tests without a problem with tolerance 0.011 (our default threshold shared with Dirk & Rob).

And I'm saying that having the tests pass is of zero utility to users.  The pages they actually want to use that use SVG are broken.

[Nikolas Zimmermann]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #36)
> > > Hard to trigger isn't the same as working :-/
> > 
> > I never claimed that! I'm just working full time to fix the bug atm, so I'd rather like to avoid reverting the patch, as I plan to have it fixed fully in the next days :-)
> 
> Your effort is appreciated :) I'm going to proceed without reverting your patch, but I'd add that breaking the tests or Mac build isn't the only criteria for rolling out patches, particularly when you can repro the bugs outside of the test harness. Code that breaks like this shouldn't really be allowed to live on in trunk imho.

Fair enough, but if we decide to roll out I'd highly appreciate if the _whole_ patch is reverted including all follow-up build/expectations fixes etc. not just parts of it. It makes relanding it much easier, and well it makes hardly no sense to have CSS 2.1 intrinsic size negotiation without the "notify the parent frameview about our change" concept.

[Levi Weintraub]

Created attachment 102723 [details]
Patch for Chromium's M14 branch

CHROMIUM ONLY

 James, can you take a look at this and make sure it's sane? I had to do some work extricating this patch given its age. Just respond with "r=me" if it's acceptable since I don't want it being confused as a patch for trunk.

[Levi Weintraub]

Committed r92293: <http://trac.webkit.org/changeset/92293>

[James Robinson]

(In reply to comment #27)
> (In reply to comment #25)
> > (From update of attachment 102128 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=102128&action=review
> > 
> > Do we really have to do a full 2-pass layout here?  Is there any way to negotiate the dimensions without running the full layout algorithm?  It's very difficult to prove that this always terminates after 2 passes.
> A full two pass layout is probably not needed - I only wanted to demonstrate it works, in theory. I'm sure I can do better.
> 
> > 
> > I can't find any specification that defines this behavior, would you mind linking to it?
> Sure, I'll give you all relevant links.
> 
> The following parts of CSS 2.1 describe how to compute the width/height for inline replaced elements:
> http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-width
> http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-height
> 
> http://dev.w3.org/SVG/profiles/1.1F2/publish/coords.html#ViewportSpace
> The whole paragraph is important, especially "The negotiation process uses any information provided by the containing document and the SVG content itself to choose the viewport location and size.".
> 
> As you can see in the CSS 2.1 links, we need to know the intrinsic width/height/ratio of the embedded document, to be able to figure out the right size of the <iframe>. That means we have to re-layout the iframe once the SVG doc appeared.
> 

So backing up a bit, I'm pretty confident that none of this applies to <iframe>, or anything other than image-y things.  So I don't think we should have any code related to size negotiation for FrameView - it should only exist in the various image classes (SVGImage, etc).  Having FrameViews participate in size negotiation is a breaking change for web content in general.

[Nikolas Zimmermann]

(In reply to comment #44)
> > As you can see in the CSS 2.1 links, we need to know the intrinsic width/height/ratio of the embedded document, to be able to figure out the right size of the <iframe>. That means we have to re-layout the iframe once the SVG doc appeared.
> > 
> 
> So backing up a bit, I'm pretty confident that none of this applies to <iframe>, or anything other than image-y things.  So I don't think we should have any code related to size negotiation for FrameView - it should only exist in the various image classes (SVGImage, etc).  Having FrameViews participate in size negotiation is a breaking change for web content in general.

image-y things? As I'm slightly confused why you think this is not needed, I'm retrying to explain:
The links I showed you tell you how to compute the intrinsic size of a replaced element.
So if I use <object> to embed foo.svg, <object> is a replaced element, whose intrinsic size should be determined according to the CSS 2.1 rules. The same applies to <iframe>.
Just grep for iframe in LayoutTests/css2.1/20110323. _All_ of these tests depend on this logic.

To rephrase: It's plain wrong to assume that this is only related to _images_. It's about any document that carries intrinsic size/ratio information that can be embedded.

The size negotiation logic has to be implemented for:
* all CSS image values (background-image, mask-image, border-image, etc.. this is a seperated patch)
* html:img/svg:image
* iframe/object/...

I've discussed the initial concept with hyatt, he was fine with it and reviewed it.
See https://bugs.webkit.org/show_bug.cgi?id=15849.

Note that implementing the size negotiation for CSS image values as well as html:img/svg:image, does NOT need any adjustments to FrameView, it works without these layout dependencies (completly different approach, see the other patch from me in the review queue).

Does this resolve your question/worries?

[Nikolas Zimmermann]

Created attachment 102898 [details]
Patch v5

New approach, seems to fix all regressions and all of my new tests that previously failed badly (with trunk and/or my older patches prior to v4).
I'm uploading early to get cr-linux-ews results.

[WebKit Review Bot]

Comment on attachment 102898 [details]
Patch v5

Attachment 102898 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9315409

New failing tests:
svg/zoom/page/zoom-svg-through-object-with-override-size.html
svg/zoom/page/zoom-svg-through-object-with-huge-size.xhtml
svg/as-object/nested-embedded-svg-size-changes.html
svg/as-object/embedded-svg-immediate-offsetWidth-query.html
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml
svg/as-object/embedded-svg-size-changes.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml

[Zoltan Herczeg]

Comment on attachment 102898 [details]
Patch v5

The patch seems ok, couple of things:

View in context: https://bugs.webkit.org/attachment.cgi?id=102898&action=review

> Source/WebCore/page/FrameView.cpp:1123
> +    ASSERT(frameView);

This code could be moved down.

> Source/WebCore/page/FrameView.cpp:1131
> +    frameView->layout();

Before this line. Its purpose would be clearer.

> Source/WebCore/page/FrameView.cpp:2155
> +RenderBox* FrameView::embeddedContentBox() const
> +{
> +    RenderView* contentRenderer = m_frame->contentRenderer();
> +    if (!contentRenderer)
> +        return 0;
> +
> +    RenderObject* rootChild = contentRenderer->firstChild();
> +    if (!rootChild || !rootChild->isBox())
> +        return 0;
> +
> +#if ENABLE(SVG)
> +    // Curently only embedded SVG documents participate in the size-negotiation logic.
> +    if (rootChild->isSVGRoot())
> +        return toRenderBox(rootChild);
> +#endif
> +
> +    return 0;
> +}
> +

Am I see right that this code have any effect if SVG is enabled? Otherwise, it just returns with 0. Maybe we could put an ENABLE(SVG) around the whole function? (Including the header, call sites, etc.) At least it should be an inline { return 0; } if SVG is disabled.

> Source/WebCore/page/FrameView.h:319
> +    void forceLayoutParentViewIfNeeded();

Make this function inline, please.

> Source/WebCore/rendering/svg/RenderSVGRoot.h:47
> +    void scheduledSizeNegotiationWithHostDocument() { ASSERT(m_needsSizeNegotiationWithHostDocument); m_needsSizeNegotiationWithHostDocument = false; }

WebKit has a one statement in every line policy, strange that the style bot did not capture it. Fix it please.

> LayoutTests/ChangeLog:15
> +        * platform/mac/fast/block/positioning/rtl-fixed-positioning-expected.png:
> +        * platform/mac/fast/block/positioning/vertical-rl/fixed-positioning-expected.png:

Why are these changed?

[Nikolas Zimmermann]

(In reply to comment #48)
> (From update of attachment 102898 [details])
> The patch seems ok, couple of things:
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=102898&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1123
> > +    ASSERT(frameView);
> 
> This code could be moved down.
... 
> Before this line. Its purpose would be clearer.
Fixed.


> > Source/WebCore/page/FrameView.cpp:2155
> > +RenderBox* FrameView::embeddedContentBox() const
> Am I see right that this code have any effect if SVG is enabled? Otherwise, it just returns with 0. Maybe we could put an ENABLE(SVG) around the whole function? (Including the header, call sites, etc.) At least it should be an inline { return 0; } if SVG is disabled.
Fixed - I chose to use an inline return 0, as we do it like this in other places as well in rendering/.

> 
> > Source/WebCore/page/FrameView.h:319
> > +    void forceLayoutParentViewIfNeeded();
> 
> Make this function inline, please.
Fixed.

> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.h:47
> > +    void scheduledSizeNegotiationWithHostDocument() { 
... 
> WebKit has a one statement in every line policy, strange that the style bot did not capture it. Fix it please.
Fixed.

> 
> > LayoutTests/ChangeLog:15
> > +        * platform/mac/fast/block/positioning/rtl-fixed-positioning-expected.png:
> > +        * platform/mac/fast/block/positioning/vertical-rl/fixed-positioning-expected.png:
> 
> Why are these changed?
Somehow NRWT decides to rebaseline them everytime I run new-run-webkit-tests --tolerance 0.011 -p fast/block, without passing --reset-results to them. Reverted again.

Will prepare a new patch soon...

[Nikolas Zimmermann]

Created attachment 103048 [details]
Patch v6

Fixed Zoltans comments, uploading revised version.

[Early Warning System Bot]

Comment on attachment 103048 [details]
Patch v6

Attachment 103048 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9317251

[Gyuyoung Kim]

Comment on attachment 103048 [details]
Patch v6

Attachment 103048 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9302821

[Nikolas Zimmermann]

Created attachment 103049 [details]
Patch v7

Fix build, as discussed on IRC. De-inline embeddedContentBox() again, it's used from other translation units as well. I specifically want to avoid moving embeddedContentBox() inline into the header to avoid having to pull in various includes in FrameView.h.

[Nikolas Zimmermann]

Adam Barth was so kind to provide me the layout test results zip. As expected the new tests need a rebaseline for chromium, but they seem to work just fine! No other regressions are introduced. I think we found a working patch that solves the problem completely w/o troubles.

[WebKit Review Bot]

Comment on attachment 103049 [details]
Patch v7

Attachment 103049 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9304959

New failing tests:
svg/zoom/page/zoom-svg-through-object-with-override-size.html
svg/zoom/page/zoom-svg-through-object-with-huge-size.xhtml
svg/as-object/nested-embedded-svg-size-changes.html
svg/as-object/embedded-svg-immediate-offsetWidth-query.html
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml
svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml
svg/as-object/embedded-svg-size-changes.html
svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml

[Zoltan Herczeg]

Comment on attachment 103049 [details]
Patch v7

r=me with the following comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=103049&action=review

> Source/WebCore/ChangeLog:11
> +        Fix host <-> embedded document size negotiation race. The current implemented solution relied on a specific

typo: currently

> Source/WebCore/ChangeLog:12
> +        order of paint/layout calls, which is broken. Consider rendering a document containing a object/iframe/embed

typo: an

> Source/WebCore/page/FrameView.cpp:1100
> +    forceLayoutParentViewIfNeeded();

Inline function definition should precede the usage.

> Source/WebCore/page/FrameView.cpp:1122
> +inline void FrameView::forceLayoutParentViewIfNeeded()

Please move this function before the usage.

[Nikolas Zimmermann]

(In reply to comment #56)
> (From update of attachment 103049 [details])
> r=me with the following comments:
Thanks Zoltan. To summarize: This patch is tested on WebKit(1)/WebKit2 with my old 32bit mbp and a 64bit mbp, once using the old Safari w/o webkit2 on the 32bit machine, and once using the new Safari with webkit2 on the 64bit machine. Full DRT runs show no regressions. The chromium bots only demand rebaselines for the new tests - I think it's fine.

As it's weekend, I'm going to land the patch soon and give it a try on the bots.
I wished James had commented in the meanwhile - I'd still like to give it a try to see if all bots agree this is stable now.

Fixed Zoltans comments. Landed in r92545.

[James Robinson]

Sorry for not commenting earlier, I've been sick. I am fairly certain this patch will result in infinite loops in layout, I'll post a test case this afternoon health allowing.

[Nikolas Zimmermann]

(In reply to comment #58)
> Sorry for not commenting earlier, I've been sick. I am fairly certain this patch will result in infinite loops in layout, I'll post a test case this afternoon health allowing.
Get well soon!
If you find anything is problematic, I'm happy to work on it this week.

[Levi Weintraub]

Comment on attachment 103049 [details]
Patch v7

Clearing flags. This was landed in r92545