Bug 58414

Summary: Style change in invisible iframes can cause accelerated content to disappear
Product: WebKit Reporter: Antoine Labour <piman>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarrin, commit-queue, eric, jamesr, kbr, simon.fraser, vangelis, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch jamesr: review+, commit-queue: commit-queue-

Description Antoine Labour 2011-04-12 22:42:12 PDT
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).
Comment 1 Antoine Labour 2011-04-12 22:42:48 PDT
Created attachment 89341 [details]
Test case
Comment 2 Simon Fraser (smfr) 2011-04-13 11:21:00 PDT
<rdar://problem/9279281>
Comment 3 Antoine Labour 2011-04-13 17:03:46 PDT
Created attachment 89493 [details]
Patch
Comment 4 Simon Fraser (smfr) 2011-04-13 17:10:55 PDT
Comment on attachment 89493 [details]
Patch

Walking up the frame tree for every call to needsToBeComposited() is too slow.
Comment 5 Antoine Labour 2011-04-13 17:15:44 PDT
any suggested alternative ?
Comment 6 James Robinson 2011-04-13 17:17:31 PDT
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).
Comment 7 James Robinson 2011-04-13 18:05:46 PDT
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?
Comment 8 Antoine Labour 2011-04-13 18:14:24 PDT
Firefox 3.6.16 also loads display:none iframes.
Comment 9 Chris Marrin 2011-04-14 11:10:15 PDT
(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.
Comment 10 Chris Marrin 2011-04-14 11:17:04 PDT
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...
Comment 11 Antoine Labour 2011-04-14 12:20:30 PDT
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.
Comment 12 Antoine Labour 2011-04-14 15:10:40 PDT
Created attachment 89666 [details]
Patch
Comment 13 Antoine Labour 2011-04-14 15:11:34 PDT
New patch up, I'm working on tests, but if you have comments on the general approach, please let me know.
Comment 14 Simon Fraser (smfr) 2011-04-14 15:27:13 PDT
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.
Comment 15 James Robinson 2011-04-14 18:15:53 PDT
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.
Comment 16 Antoine Labour 2011-04-15 17:29:57 PDT
Created attachment 89892 [details]
Patch
Comment 17 Antoine Labour 2011-04-15 17:31:13 PDT
New patch up. Here I'm preventing renderers to be created in the child document if it will be invisible.
Comment 18 James Robinson 2011-04-15 18:08:46 PDT
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.
Comment 19 Antoine Labour 2011-04-18 19:07:15 PDT
Created attachment 90129 [details]
Patch
Comment 20 Antoine Labour 2011-04-18 19:07:32 PDT
Added tests.
Comment 21 Simon Fraser (smfr) 2011-04-18 20:36:59 PDT
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?
Comment 22 Antoine Labour 2011-04-18 20:52:23 PDT
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).
Comment 23 Simon Fraser (smfr) 2011-04-18 20:58:46 PDT
(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.
Comment 24 Antoine Labour 2011-04-18 21:24:06 PDT
Created attachment 90138 [details]
Patch
Comment 25 Antoine Labour 2011-04-18 21:40:51 PDT
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.
Comment 26 Simon Fraser (smfr) 2011-04-18 21:50:21 PDT
(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.
Comment 27 Vangelis Kokkevis 2011-04-19 10:19:50 PDT
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.
Comment 28 Simon Fraser (smfr) 2011-04-19 10:28:32 PDT
(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.
Comment 29 Antoine Labour 2011-04-19 10:39:23 PDT
Doesn't ensureRootPlatformLayer rely on it being false in that case to set up the (top-level) root layer (via chrome client) ?
Comment 30 Antoine Labour 2011-04-19 11:00:40 PDT
NVM, I get it now. It seems to work, I'll post a patch shortly.
Comment 31 Antoine Labour 2011-04-19 11:35:48 PDT
Created attachment 90225 [details]
Patch
Comment 32 Antoine Labour 2011-04-19 11:37:17 PDT
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.
Comment 33 James Robinson 2011-04-19 13:05:53 PDT
Comment on attachment 90225 [details]
Patch

Looks good and nice tests.  R=me.
Comment 34 James Robinson 2011-04-19 16:24:55 PDT
Comment on attachment 90225 [details]
Patch

Landed by hand: http://trac.webkit.org/changeset/84307
Comment 35 WebKit Commit Bot 2011-04-19 16:33:40 PDT
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
Comment 36 Eric Seidel (no email) 2011-04-19 16:36:47 PDT
Interesting.  The commit-queue barfs now if non-layout tests fail. :(  Sorry for the trouble.
Comment 37 WebKit Review Bot 2011-04-19 17:22:50 PDT
http://trac.webkit.org/changeset/84307 might have broken Windows 7 Release (Tests)
Comment 38 Eric Seidel (no email) 2011-04-19 18:22:12 PDT
The cq will be made healthy again by bug 58955.
Comment 39 Chris Marrin 2011-04-19 19:05:44 PDT
(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?
Comment 40 Adam Barth 2011-04-19 19:10:15 PDT
> 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.
Comment 41 Eric Seidel (no email) 2011-04-19 19:17:05 PDT
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).