Bug 58414 – Style change in invisible iframes can cause accelerated content to disappear

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antoine Labour]

See repro case in attachment (unpack, load main.html). After 2 seconds, the iframe content changes style that triggers a compositing change there. This causes layers in the main page to disappear. 
The problem is that it seems to cause the root layer for the page to be replaced by the invisible iframe's root layer.

Here's the stack trace:

#0  WebCore::LayerRendererChromium::setRootLayer (this=0xd6769a0, layer=...) at third_party/WebKit/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:495
#1  0x09821c59 in WebKit::WebViewImpl::setRootGraphicsLayer (this=0xd676580, layer=0xd813400) at third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:2336
#2  0x0983b5d7 in WebKit::ChromeClientImpl::attachRootGraphicsLayer (this=0xd676598, frame=0xe749000, graphicsLayer=0xd7fdc40) at third_party/WebKit/Source/WebKit/chromium/src/ChromeClientImpl.cpp:811
#3  0x0a26695e in WebCore::RenderLayerCompositor::attachRootPlatformLayer (this=0xd818900, attachment=WebCore::RenderLayerCompositor::RootLayerAttachedViaChromeClient) at third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:1496    <---- wrong
#4  0x0a2666e4 in WebCore::RenderLayerCompositor::ensureRootPlatformLayer (this=0xd818900) at third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:1461
#5  0x0a2622de in WebCore::RenderLayerCompositor::enableCompositingMode (this=0xd818900, enable=true) at third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:135
#6  0x0a262a36 in WebCore::RenderLayerCompositor::updateBacking (this=0xd818900, layer=0xe1e087c, shouldRepaint=WebCore::RenderLayerCompositor::CompositingChangeRepaintNow) at third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:330
#7  0x0a262ce7 in WebCore::RenderLayerCompositor::updateLayerCompositingState (this=0xd818900, layer=0xe1e087c, shouldRepaint=WebCore::RenderLayerCompositor::CompositingChangeRepaintNow) at third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:400
#8  0x0a254fbb in WebCore::RenderLayer::styleChanged (this=0xe1e087c, diff=WebCore::StyleDifferenceLayout, oldStyle=0xe76ce40) at third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3911
#9  0x0a21e5ec in WebCore::RenderBoxModelObject::styleDidChange (this=0xd760a0c, diff=WebCore::StyleDifferenceLayout, oldStyle=0xe76ce40) at third_party/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:363
#10 0x0a20d479 in WebCore::RenderBox::styleDidChange (this=0xd760a0c, diff=WebCore::StyleDifferenceLayout, oldStyle=0xe76ce40) at third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp:289
#11 0x0a1ca58a in WebCore::RenderBlock::styleDidChange (this=0xd760a0c, diff=WebCore::StyleDifferenceLayout, oldStyle=0xe76ce40) at third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:228
#12 0x0a282334 in WebCore::RenderObject::setStyle (this=0xd760a0c, style=...) at third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:1759
#13 0x0a281cc9 in WebCore::RenderObject::setAnimatableStyle (this=0xd760a0c, style=...) at third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:1676
#14 0x09e2ff07 in WebCore::Node::setRenderStyle (this=0xe1c8f00, s=...) at third_party/WebKit/Source/WebCore/dom/Node.cpp:1554
#15 0x09e0e8bb in WebCore::Element::recalcStyle (this=0xe1c8f00, change=WebCore::Node::NoChange) at third_party/WebKit/Source/WebCore/dom/Element.cpp:1111
#16 0x09e0ebc6 in WebCore::Element::recalcStyle (this=0xe7334b0, change=WebCore::Node::NoChange) at third_party/WebKit/Source/WebCore/dom/Element.cpp:1144
#17 0x09e0ebc6 in WebCore::Element::recalcStyle (this=0xe108be0, change=WebCore::Node::NoChange) at third_party/WebKit/Source/WebCore/dom/Element.cpp:1144
#18 0x09dd969a in WebCore::Document::recalcStyle (this=0xdccb000, change=WebCore::Node::NoChange) at third_party/WebKit/Source/WebCore/dom/Document.cpp:1508
#19 0x09dd99fc in WebCore::Document::updateStyleIfNeeded (this=0xdccb000) at third_party/WebKit/Source/WebCore/dom/Document.cpp:1560
#20 0x09dd9ae7 in WebCore::Document::updateStyleForAllDocuments () at third_party/WebKit/Source/WebCore/dom/Document.cpp:1577
#21 0x0a0f6378 in WebCore::ScheduledAction::execute (this=0xd760100, proxy=0xe1cfac0) at third_party/WebKit/Source/WebCore/bindings/v8/ScheduledAction.cpp:120
#22 0x0a0f61bc in WebCore::ScheduledAction::execute (this=0xd760100, context=0xdccb134) at third_party/WebKit/Source/WebCore/bindings/v8/ScheduledAction.cpp:95
#23 0x09fe8ec4 in WebCore::DOMTimer::fired (this=0xe112900) at third_party/WebKit/Source/WebCore/page/DOMTimer.cpp:148


On frame #4, the RenderLayerCompositor for the iframe calls attachRootPlatformLayer with RootLayerAttachedViaChromeClient. The reason for that is that at the beginning of ensureRootPlatformLayer(), shouldPropagateCompositingToEnclosingFrame() returns false because the iframe doesn't have a renderer (because it's invisible).

[Antoine Labour]

Created attachment 89341 [details]
Test case

[Simon Fraser (smfr)]

<rdar://problem/9279281>

[Antoine Labour]

Created attachment 89493 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 89493 [details]
Patch

Walking up the frame tree for every call to needsToBeComposited() is too slow.

[Antoine Labour]

any suggested alternative ?

[James Robinson]

I think we need to figure out somewhere to cache whether an iframe's ownerElement has a renderer and only propagate updates when that value changes.

This patch will also need a layout test (currently it'll only apply to Chromium and Safari/WebKit2).

[James Robinson]

I'm wondering if the real bug here is that we are loading this iframe at all.  We used to not do that, and I don't think other browser load <iframe>s that are display:none.  When did this behavior change and was this change in behavior intentional?

[Antoine Labour]

Firefox 3.6.16 also loads display:none iframes.

[Chris Marrin]

(In reply to comment #7)
> I'm wondering if the real bug here is that we are loading this iframe at all.  We used to not do that, and I don't think other browser load <iframe>s that are display:none.  When did this behavior change and was this change in behavior intentional?

Even if that is a behavioral change, the fix isn't to avoid the load. There will be other ways to trigger a recomputation of the compositing tree and this bug will show up again.

[Chris Marrin]

Is it true that we are creating renderers for iframes that are not display:none, but that are inside iframes that are display:none? If so, it seems like the solution is not to avoid loading that nested iframe but to avoid creating a renderer for it. Then the inRenderTree() test would simply test if the current iframe has a renderer, without the traversal. Or something like that...

[Antoine Labour]

I did confirm that RenderObject are currently created for iframes inside of invisible iframes.

The approach I was going to take is to somehow cache the inRenderTree result in the HTMLFrameOwnerElement: when creating a renderer, check the conditions, and check the parent. If the bit changes, propagate downwards if needed. Same when destroying the renderer. Propagating downwards is the slightly tricky bit, but I think I can do it by having the HTMLFrameOwnerElement register themselves to their parent HTMLFrameOwnerElement (if any) when they start caring about it, so that the parent doesn't have to go through all the DOM. Working on a patch right now.

I'm very new to all that code, so if avoiding creating RenderObjects in invisible iframes is desirable and won't cause bad side effects, then I'm happy to take that road instead.

[Antoine Labour]

Created attachment 89666 [details]
Patch

[Antoine Labour]

New patch up, I'm working on tests, but if you have comments on the general approach, please let me know.

[Simon Fraser (smfr)]

Comment on attachment 89666 [details]
Patch

It's bad to cache rendering-related state on a DOM object. You should cache in Frame or FrameView.

[James Robinson]

This is tricky.  I think what we want to do is create the iframe and its DOM but somehow avoid constructing an actual render tree for it.  I'm not sure how to go about that - documents definitely expect to have a RenderView and it's not immediately clear how to avoid constructing a render object tree beneath that view in the case where the Frame's ownerRenderer() is NULL, if that's even the right approach, in a way that doesn't violate a bunch of other expectations.

[Antoine Labour]

Created attachment 89892 [details]
Patch

[Antoine Labour]

New patch up. Here I'm preventing renderers to be created in the child document if it will be invisible.

[James Robinson]

Comment on attachment 89892 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This patch definitely needs tests.  I'd suggest to start a pixel test to verify that the compositing regression does not occur and a few tests that add/remove iframes and toggle display: on ancestors to ensure that everything ends up as expected.

[Antoine Labour]

Created attachment 90129 [details]
Patch

[Antoine Labour]

Added tests.

[Simon Fraser (smfr)]

Comment on attachment 90129 [details]
Patch

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

I'm wondering about side effects of calling setShouldCreateRenderers(false) now. Will the behavior of plugins in iframes change?

> LayoutTests/compositing/iframes/invisible-nested-iframe-show.html:1
> +<!DOCTYPE html>

The scrollbar in the pixel result means that this test  result should move to platform-specific directories. Or change the test to avoid the scrollbar.

Can the tests use layerTreeAsText() and be text tests?

[Antoine Labour]

I would like to keep pixel test for invisible-iframe and invisible-nested-iframe, but changing the other 2 to text tests seems fine.

As to plugin behavior in invisible iframes, it will change to be consistent with plugins in invisible divs (i.e. won't be loaded).

[Simon Fraser (smfr)]

(In reply to comment #22)

> As to plugin behavior in invisible iframes, it will change to be consistent with plugins in invisible divs (i.e. won't be loaded).

If there is a behavior change here, it should be tested, and the ramifications for the web should be considered. You may break web sites.

[Antoine Labour]

Created attachment 90138 [details]
Patch

[Antoine Labour]

New patch with text tests for invisible-nested-iframe-{show,hide}.

For the plugin tests, do you mean to test that the behavior is actually broken ? I can add that if you wish, though it seems counter-productive.

The ramifications for the web... well. Plugins are handled awkwardly in WebKit in that they are linked to the render tree instead of the DOM. When that is solved, the (rare) extra failures caused by this change will go away as well.
It's not the intention of this patch to fix plugins - if you need me to fix that as well before you accept this patch, please let me know, though it won't happen any time soon.
I think having correct rendering for visible content outweighs correct behavior in invisible iframes - and several other developers seem to agree that not creating render objects for invisible iframes is the right thing to do.

[Simon Fraser (smfr)]

(In reply to comment #25)

> The ramifications for the web... well. Plugins are handled awkwardly in WebKit in that they are linked to the render tree instead of the DOM. When that is solved, the (rare) extra failures caused by this change will go away as well.
> It's not the intention of this patch to fix plugins - if you need me to fix that as well before you accept this patch, please let me know, though it won't happen any time soon.
> I think having correct rendering for visible content outweighs correct behavior in invisible iframes - and several other developers seem to agree that not creating render objects for invisible iframes is the right thing to do.

All I'm saying is that if you decide to change this behavior, you need to do some testing to make sure no major sites break, and be prepared to deal with the consequences.

There are safer ways to fix this bug: just make sure that the compositing layer state doesn't go bad, or don't create compositing layers for display:none plugins.

[Vangelis Kokkevis]

What if we modify RenderLayerCompositor::shouldPropagateCompositingToEnclosingIFrame() and return true when (ownerElement != NULL) && (renderer == NULL)  (modulo whatever special treatment needs to happen on the mac)? 

That won't disturb the existing compositor's root layer if an enclosed iframe doesn't have a renderer (e.g. hidden) which seems to be the root of the issue here.

The only side-effect appears to be that if the enclosed iframe would have triggered the compositor, we would propagate compositing up to the parent even if the iframe is hidden.  I wouldn't think that would be a big issue though.

A local patch of this change seems to be work fine.

[Simon Fraser (smfr)]

(In reply to comment #27)
> What if we modify RenderLayerCompositor::shouldPropagateCompositingToEnclosingIFrame() and return true when (ownerElement != NULL) && (renderer == NULL)  (modulo whatever special treatment needs to happen on the mac)? 
> 
> That won't disturb the existing compositor's root layer if an enclosed iframe doesn't have a renderer (e.g. hidden) which seems to be the root of the issue here.
> 
> The only side-effect appears to be that if the enclosed iframe would have triggered the compositor, we would propagate compositing up to the parent even if the iframe is hidden.  I wouldn't think that would be a big issue though.
> 
> A local patch of this change seems to be work fine.

I think that's a safer approach.

[Antoine Labour]

Doesn't ensureRootPlatformLayer rely on it being false in that case to set up the (top-level) root layer (via chrome client) ?

[Antoine Labour]

NVM, I get it now. It seems to work, I'll post a patch shortly.

[Antoine Labour]

Created attachment 90225 [details]
Patch

[Antoine Labour]

New patch up. I'm assuming if allowsIndependentlyCompositedFrames is true (on the mac), then returning false from shouldPropagateCompositingToEnclosingFrame will do the right thing in this case.

[James Robinson]

Comment on attachment 90225 [details]
Patch

Looks good and nice tests.  R=me.

[James Robinson]

Comment on attachment 90225 [details]
Patch

Landed by hand: http://trac.webkit.org/changeset/84307

[WebKit Commit Bot]

Comment on attachment 90225 [details]
Patch

Rejecting attachment 90225 [details] from commit-queue.

Failed to run "[u'zip', u'-r', u'../commit-queue-logs/58414-layout-test-results-3.zip', u'/tmp/layout-test-resul..." exit_code: 12

	zip warning: name not matched: /tmp/layout-test-results

zip error: Nothing to do! (try: zip -r ../commit-queue-logs/58414-layout-test-results-3.zip . -i /tmp/layout-test-results)

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

[Eric Seidel (no email)]

Interesting.  The commit-queue barfs now if non-layout tests fail. :(  Sorry for the trouble.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84307 might have broken Windows 7 Release (Tests)

[Eric Seidel (no email)]

The cq will be made healthy again by bug 58955.

[Chris Marrin]

(In reply to comment #38)
> The cq will be made healthy again by bug 58955.

So then this is just noise from the commit-queue, not any issue with the patch?

[Adam Barth]

> So then this is just noise from the commit-queue, not any issue with the patch?

My understanding is that your patch caused a test to fail, but not a LayoutTest.  The commit-queue wasn't smart enough to handle that case and so reported a bug about itself.  That bug will be fixed soon, but your patch might still be causing that non-LayoutTest test to fail.

[Eric Seidel (no email)]

It wasn't actually your patch.  webkitpy tests were failing on the bots.  The commit-queue got sad when it tried to archive away layout test results (after a test run failure) but there were none (cause no layout tests failed, only webkitpy tests).