Summary: | Style change in invisible iframes can cause accelerated content to disappear | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Labour <piman> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Antoine Labour
2011-04-12 22:42:12 PDT
Created attachment 89341 [details]
Test case
Created attachment 89493 [details]
Patch
Comment on attachment 89493 [details]
Patch
Walking up the frame tree for every call to needsToBeComposited() is too slow.
any suggested alternative ? 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). 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? Firefox 3.6.16 also loads display:none iframes. (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. 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... 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. Created attachment 89666 [details]
Patch
New patch up, I'm working on tests, but if you have comments on the general approach, please let me know. 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.
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. Created attachment 89892 [details]
Patch
New patch up. Here I'm preventing renderers to be created in the child document if it will be invisible. 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. Created attachment 90129 [details]
Patch
Added tests. 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? 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). (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. Created attachment 90138 [details]
Patch
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. (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. 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. (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. Doesn't ensureRootPlatformLayer rely on it being false in that case to set up the (top-level) root layer (via chrome client) ? NVM, I get it now. It seems to work, I'll post a patch shortly. Created attachment 90225 [details]
Patch
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 on attachment 90225 [details]
Patch
Looks good and nice tests. R=me.
Comment on attachment 90225 [details] Patch Landed by hand: http://trac.webkit.org/changeset/84307 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 Interesting. The commit-queue barfs now if non-layout tests fail. :( Sorry for the trouble. http://trac.webkit.org/changeset/84307 might have broken Windows 7 Release (Tests) The cq will be made healthy again by bug 58955. (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? > 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.
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). |