Bug 176261 – Async frame scrolling: handle fixed root backgrounds in frames

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

[Simon Fraser (smfr)]

Need to correctly handle background-attachment: fixed; on the html/body in iframes, to avoid repaints etc.

[Simon Fraser (smfr)]

We register a slow-repaint object because we hit RenderElement::styleWillChange() before we've made a backing for the iframe's RenderView so view().compositor().supportsFixedRootBackgroundCompositing() returns false.

[Radar WebKit Bug Importer]

<rdar://problem/34220451>

[Simon Fraser (smfr)]

Some WIP (needed to fix root layer opaqueness):

diff --git a/Source/WebCore/rendering/RenderLayerBacking.cpp b/Source/WebCore/rendering/RenderLayerBacking.cpp
index 32c24fa21a31aff1aa9b0345382c5c1a2d31de23..7776955752fcf3f0d3b4f46aa2384acb28a057f8 100644
--- a/Source/WebCore/rendering/RenderLayerBacking.cpp
+++ b/Source/WebCore/rendering/RenderLayerBacking.cpp
@@ -1023,7 +1023,7 @@ void RenderLayerBacking::updateGeometry()
         positionOverflowControlsLayers();
     }
 
-    if (!m_isMainFrameRenderViewLayer) {
+    if (!m_isFrameLayerWithTiledBacking) {
         // For non-root layers, background is always painted by the primary graphics layer.
         ASSERT(!m_backgroundLayer);
         // Subpixel offset from graphics layer or size changed.
@@ -1966,12 +1966,14 @@ void RenderLayerBacking::updateRootLayerConfiguration()
     bool viewIsTransparent = compositor().viewHasTransparentBackground(&backgroundColor);
 
     if (m_backgroundLayerPaintsFixedRootBackground && m_backgroundLayer) {
-        m_backgroundLayer->setBackgroundColor(backgroundColor);
-        m_backgroundLayer->setContentsOpaque(!viewIsTransparent);
+        if (m_isMainFrameRenderViewLayer) {
+            m_backgroundLayer->setBackgroundColor(backgroundColor);
+            m_backgroundLayer->setContentsOpaque(!viewIsTransparent);
+        }
 
         m_graphicsLayer->setBackgroundColor(Color());
         m_graphicsLayer->setContentsOpaque(false);
-    } else {
+    } else if (m_isMainFrameRenderViewLayer) {
         m_graphicsLayer->setBackgroundColor(backgroundColor);
         m_graphicsLayer->setContentsOpaque(!viewIsTransparent);
     }

[Simon Fraser (smfr)]

Created attachment 319899 [details]
Patch

[Build Bot]

Attachment 319899 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Frédéric Wang (:fredw)]

Hi Simon. I was testing your WIP and was improving the test from bug 173644 to consider transparent, opaque and fixed backgrounds. I can provide it if you want. In any case, the expectation will have to be updated now that the opaque white background is removed.

[Frédéric Wang (:fredw)]

Created attachment 319902 [details]
Experimental test adjustments

Feel free to use it as you want.

[Build Bot]

Comment on attachment 319899 [details]
Patch

Attachment 319899 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4454784

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 319904 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319899 [details]
Patch

Attachment 319899 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4454841

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 319908 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319899 [details]
Patch

Attachment 319899 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4454848

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html

[Build Bot]

Created attachment 319911 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319899 [details]
Patch

Attachment 319899 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4454858

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html

[Build Bot]

Created attachment 319913 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Frédéric Wang (:fredw)]

Created attachment 319992 [details]
Part 1: Extend tiled-drawing-async-frame-scrolling to check different kind of backgrounds.

Sorry, my previous patch was not quite correct, here is a better one. Note that the gradient background test exhibits a crash.

[Frédéric Wang (:fredw)]

Created attachment 319993 [details]
Part 2: Simon's patch with test expectation adjustments.

This is attachment 319899 [details], with some adjustments to test expectations. It applies on top of attachment 319992 [details]. Note how the backgroundColor and contentsOpaque are removed. The crash due to ASSERT(!m_backgroundLayer) is also fixed.

[Frédéric Wang (:fredw)]

Created attachment 319995 [details]
Part 1 + Part 2

[Build Bot]

Comment on attachment 319995 [details]
Part 1 + Part 2

Attachment 319995 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4461719

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 319999 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319995 [details]
Part 1 + Part 2

Attachment 319995 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4461659

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320001 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319995 [details]
Part 1 + Part 2

Attachment 319995 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4461880

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html

[Build Bot]

Created attachment 320006 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Frédéric Wang (:fredw)]

Comment on attachment 319992 [details]
Part 1: Extend tiled-drawing-async-frame-scrolling to check different kind of backgrounds.

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

> LayoutTests/compositing/tiling/tiled-drawing-async-frame-scrolling.html:29
> +  <body onload="setTimeout(doTest, 0)">

OK the ios bot is failing so probably we still need to check that all iframes are loaded before calling doTest.

[Frédéric Wang (:fredw)]

Created attachment 320254 [details]
Patch

[Build Bot]

Comment on attachment 320254 [details]
Patch

Attachment 320254 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4486103

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320255 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320254 [details]
Patch

Attachment 320254 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4486093

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320256 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Frédéric Wang (:fredw)]

Created attachment 320264 [details]
Patch

[Build Bot]

Comment on attachment 320264 [details]
Patch

Attachment 320264 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4487612

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html

[Build Bot]

Created attachment 320267 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320264 [details]
Patch

Attachment 320264 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4487762

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html

[Build Bot]

Created attachment 320268 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Frédéric Wang (:fredw)]

Created attachment 320269 [details]
Patch

Taking bug, per discusson with Simon yesterday.

[Ali Juma]

Comment on attachment 320269 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:1477
> +        renderer().view().frameView().removeSlowRepaintObject(renderer().view().rendererForRootBackground());

This logic assumes that the backing won't ever be destroyed while the frame is still scrollable and has a fixed root background (since otherwise something needs to add back the slow repaint object). It looks like this assumption is currently true, but I wonder if it would be possible to add an assertion somewhere to verify this?

Also, if we're making this kind of assumption about backing lifetime, we could alternatively change RenderLayerCompositor::supportsFixedRootBackgroundCompositing to depend on the same settings we use for computing the value of m_isFrameLayerWithTiledBacking, and then we'd avoid adding the slow repaint object in the first place.

[Frédéric Wang (:fredw)]

Comment on attachment 320269 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1477
>> +        renderer().view().frameView().removeSlowRepaintObject(renderer().view().rendererForRootBackground());
> 
> This logic assumes that the backing won't ever be destroyed while the frame is still scrollable and has a fixed root background (since otherwise something needs to add back the slow repaint object). It looks like this assumption is currently true, but I wonder if it would be possible to add an assertion somewhere to verify this?
> 
> Also, if we're making this kind of assumption about backing lifetime, we could alternatively change RenderLayerCompositor::supportsFixedRootBackgroundCompositing to depend on the same settings we use for computing the value of m_isFrameLayerWithTiledBacking, and then we'd avoid adding the slow repaint object in the first place.

Thanks for the review. Sorry, I checked that last week but forgot to reply.

As I see, the RenderLayerBacking is only released in the destructor of RenderLayer, so that should not be a problem. Normally, the slowRepainObject is added/removed in RenderElement::styleWillChange, maybe we should have an "addSlowRepaint" call in RenderLayerBacking::setBackgroundLayerPaintsFixedRootBackground too, although I don't know if that will be needed.

I believe it would indeed be possible to have RenderLayerCompositor::supportsFixedRootBackgroundCompositing use the same computation as for m_isFrameLayerWithTiledBacking, however we also need to know whether a RenderLayerBacking will be created and I'm not sure it is straightforward as RenderLayerCompositor::updateBacking does quite a fair amount of work.

[Antonio Gomes]

Comment on attachment 320269 [details]
Patch

r=me.

This is based on Simon's original patch, plus a nice set of tests added by Fred. I also test on a spare Macbook Pro I have, and it is really a progression on the opaque background case.

[Frédéric Wang (:fredw)]

Committed r225089: <https://trac.webkit.org/changeset/225089>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 179937

[Frédéric Wang (:fredw)]

removeSlowRepaintObject was changed two days ago to take a reference https://trac.webkit.org/changeset/225052 :-( ; will reland soon.

[Frédéric Wang (:fredw)]

Committed r225092: <https://trac.webkit.org/changeset/225092>