REGRESSION (r238090): Toggling visibility on the <html> element can result in a blank...
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Feb 2019 01:39:34 +0000 (01:39 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Feb 2019 01:39:34 +0000 (01:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194827
rdar://problem/47620594

Reviewed by Antti Koivisto.

Source/WebCore:

Incremental compositing updates, added in rr238090, use repaints as a trigger for re-evaluating
layer configurations, since a repaint implies that a layer gains painted content. This is done
via the call to setNeedsCompositingConfigurationUpdate() in RenderLayerBacking::setContentsNeedDisplay{InRect}.
The RenderView's layer is opted out of this to avoid doing lots of redundant layer config recomputation
for the root. The configuration state that matters here is whether the layer contains painted content,
and therefore needs backing store; this is computed by RenderLayerBacking::isSimpleContainerCompositingLayer(),
and feeds into GraphicsLayer::drawsContent().

However, if <html> starts as "visibility:hidden" or "opacity:0", as some sites do to hide incremental loading,
then we'll fail to recompute 'drawsContent' for the root and leave the root with drawsContent=false, which
causes RenderLayerBacking::setContentsNeedDisplay{InRect} to short-circuit, and then we paint nothing.

Ironically, 'drawsContent' doesn't actually save any backing store for the root, since it has no affect on
the root tile caches; we always make tiles. So the simple fix here is to change RenderLayerBacking::isSimpleContainerCompositingLayer()
to always return false for the RenderView's layer (the root).

Testing this was tricky; ref testing doesn't work because we force repaint, and we normally skip
properties of the root in layer tree dumps to hide WK1/WK2 differences. Therefore I had to add
LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES and fix RenderLayerBacking::shouldDumpPropertyForLayer to
respect it.

Test: compositing/visibility/root-visibility-toggle.html

* page/Frame.h:
* platform/graphics/GraphicsLayer.cpp:
(WebCore::GraphicsLayer::dumpProperties const):
* platform/graphics/GraphicsLayerClient.h:
(WebCore::GraphicsLayerClient::shouldDumpPropertyForLayer const):
* rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::isSimpleContainerCompositingLayer const):
(WebCore::RenderLayerBacking::shouldDumpPropertyForLayer const):
* rendering/RenderLayerBacking.h:
* rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::layerTreeAsText):
* testing/Internals.cpp:
(WebCore::toLayerTreeFlags):
* testing/Internals.h:
* testing/Internals.idl:

LayoutTests:

Test dumps layer tree with RenderLayerBacking::shouldDumpPropertyForLayer to show that the root has (drawsContent 1)

* compositing/visibility/root-visibility-toggle-expected.txt: Added.
* compositing/visibility/root-visibility-toggle.html: Added.
* platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@241788 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
LayoutTests/ChangeLog
LayoutTests/compositing/visibility/root-visibility-toggle-expected.txt [new file with mode: 0644]
LayoutTests/compositing/visibility/root-visibility-toggle.html [new file with mode: 0644]
LayoutTests/platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/Frame.h
Source/WebCore/platform/graphics/GraphicsLayer.cpp
Source/WebCore/platform/graphics/GraphicsLayerClient.h
Source/WebCore/rendering/RenderLayerBacking.cpp
Source/WebCore/rendering/RenderLayerBacking.h
Source/WebCore/rendering/RenderLayerCompositor.cpp
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl

index 84c521d..f1100e8 100644 (file)
@@ -1,3 +1,17 @@
+2019-02-19  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r238090): Toggling visibility on the <html> element can result in a blank web view
+        https://bugs.webkit.org/show_bug.cgi?id=194827
+        rdar://problem/47620594
+
+        Reviewed by Antti Koivisto.
+
+        Test dumps layer tree with RenderLayerBacking::shouldDumpPropertyForLayer to show that the root has (drawsContent 1)
+
+        * compositing/visibility/root-visibility-toggle-expected.txt: Added.
+        * compositing/visibility/root-visibility-toggle.html: Added.
+        * platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt: Added.
+
 2019-02-19  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Improve ES6 Class instances in Heap Snapshot instances view
diff --git a/LayoutTests/compositing/visibility/root-visibility-toggle-expected.txt b/LayoutTests/compositing/visibility/root-visibility-toggle-expected.txt
new file mode 100644 (file)
index 0000000..9420fd1
--- /dev/null
@@ -0,0 +1,15 @@
+This text should be visible.
+
+(GraphicsLayer
+  (anchor 0.00 0.00)
+  (bounds 800.00 600.00)
+  (children 1
+    (GraphicsLayer
+      (bounds 800.00 600.00)
+      (contentsOpaque 1)
+      (drawsContent 1)
+      (backgroundColor #FFFFFF)
+    )
+  )
+)
+
diff --git a/LayoutTests/compositing/visibility/root-visibility-toggle.html b/LayoutTests/compositing/visibility/root-visibility-toggle.html
new file mode 100644 (file)
index 0000000..b57fe66
--- /dev/null
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        html {
+            visibility: hidden;
+        }
+    </style>
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function doTest()
+        {
+            requestAnimationFrame(() => {
+                document.documentElement.style.visibility = 'visible';
+
+                if (window.internals)
+                    document.getElementById('layers').textContent = window.internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES);
+
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            });
+        }
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+<p>This text should be visible.</p>
+<pre id="layers"></pre>
+</body>
+</html>
diff --git a/LayoutTests/platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt b/LayoutTests/platform/mac-wk1/compositing/visibility/root-visibility-toggle-expected.txt
new file mode 100644 (file)
index 0000000..db472d9
--- /dev/null
@@ -0,0 +1,3 @@
+This text should be visible.
+
+
index c25dffb..05648c5 100644 (file)
@@ -1,3 +1,50 @@
+2019-02-19  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r238090): Toggling visibility on the <html> element can result in a blank web view
+        https://bugs.webkit.org/show_bug.cgi?id=194827
+        rdar://problem/47620594
+
+        Reviewed by Antti Koivisto.
+
+        Incremental compositing updates, added in rr238090, use repaints as a trigger for re-evaluating
+        layer configurations, since a repaint implies that a layer gains painted content. This is done
+        via the call to setNeedsCompositingConfigurationUpdate() in RenderLayerBacking::setContentsNeedDisplay{InRect}.
+        The RenderView's layer is opted out of this to avoid doing lots of redundant layer config recomputation
+        for the root. The configuration state that matters here is whether the layer contains painted content,
+        and therefore needs backing store; this is computed by RenderLayerBacking::isSimpleContainerCompositingLayer(),
+        and feeds into GraphicsLayer::drawsContent().
+
+        However, if <html> starts as "visibility:hidden" or "opacity:0", as some sites do to hide incremental loading,
+        then we'll fail to recompute 'drawsContent' for the root and leave the root with drawsContent=false, which
+        causes RenderLayerBacking::setContentsNeedDisplay{InRect} to short-circuit, and then we paint nothing.
+
+        Ironically, 'drawsContent' doesn't actually save any backing store for the root, since it has no affect on
+        the root tile caches; we always make tiles. So the simple fix here is to change RenderLayerBacking::isSimpleContainerCompositingLayer()
+        to always return false for the RenderView's layer (the root).
+        
+        Testing this was tricky; ref testing doesn't work because we force repaint, and we normally skip
+        properties of the root in layer tree dumps to hide WK1/WK2 differences. Therefore I had to add
+        LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES and fix RenderLayerBacking::shouldDumpPropertyForLayer to
+        respect it.
+
+        Test: compositing/visibility/root-visibility-toggle.html
+
+        * page/Frame.h:
+        * platform/graphics/GraphicsLayer.cpp:
+        (WebCore::GraphicsLayer::dumpProperties const):
+        * platform/graphics/GraphicsLayerClient.h:
+        (WebCore::GraphicsLayerClient::shouldDumpPropertyForLayer const):
+        * rendering/RenderLayerBacking.cpp:
+        (WebCore::RenderLayerBacking::isSimpleContainerCompositingLayer const):
+        (WebCore::RenderLayerBacking::shouldDumpPropertyForLayer const):
+        * rendering/RenderLayerBacking.h:
+        * rendering/RenderLayerCompositor.cpp:
+        (WebCore::RenderLayerCompositor::layerTreeAsText):
+        * testing/Internals.cpp:
+        (WebCore::toLayerTreeFlags):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2019-02-19  Ryosuke Niwa  <rniwa@webkit.org>
 
         REGRESSION(r240909): Release assertion in FrameLoader::loadPostRequest when opening new window
index 74f6f4f..83dc483 100644 (file)
@@ -114,6 +114,7 @@ enum {
     LayerTreeFlagsIncludeContentLayers = 1 << 5,
     LayerTreeFlagsIncludeAcceleratesDrawing = 1 << 6,
     LayerTreeFlagsIncludeBackingStoreAttached = 1 << 7,
+    LayerTreeFlagsIncludeRootLayerProperties = 1 << 8,
 };
 typedef unsigned LayerTreeFlags;
 
index 08f013d..6b5cf2d 100644 (file)
@@ -836,7 +836,7 @@ void GraphicsLayer::dumpProperties(TextStream& ts, LayerTreeAsTextBehavior behav
     if (m_preserves3D)
         ts << indent << "(preserves3D " << m_preserves3D << ")\n";
 
-    if (m_drawsContent && client().shouldDumpPropertyForLayer(this, "drawsContent"))
+    if (m_drawsContent && client().shouldDumpPropertyForLayer(this, "drawsContent", behavior))
         ts << indent << "(drawsContent " << m_drawsContent << ")\n";
 
     if (!m_contentsVisible)
@@ -850,7 +850,7 @@ void GraphicsLayer::dumpProperties(TextStream& ts, LayerTreeAsTextBehavior behav
         ts << indent << "(client " << static_cast<void*>(m_client) << ")\n";
     }
 
-    if (m_backgroundColor.isValid() && client().shouldDumpPropertyForLayer(this, "backgroundColor"))
+    if (m_backgroundColor.isValid() && client().shouldDumpPropertyForLayer(this, "backgroundColor", behavior))
         ts << indent << "(backgroundColor " << m_backgroundColor.nameForRenderTreeAsText() << ")\n";
 
     if (behavior & LayerTreeAsTextIncludeAcceleratesDrawing && m_acceleratesDrawing)
@@ -904,7 +904,7 @@ void GraphicsLayer::dumpProperties(TextStream& ts, LayerTreeAsTextBehavior behav
         ts << ")\n";
     }
 
-    if (behavior & LayerTreeAsTextIncludeRepaintRects && repaintRectMap().contains(this) && !repaintRectMap().get(this).isEmpty() && client().shouldDumpPropertyForLayer(this, "repaintRects")) {
+    if (behavior & LayerTreeAsTextIncludeRepaintRects && repaintRectMap().contains(this) && !repaintRectMap().get(this).isEmpty() && client().shouldDumpPropertyForLayer(this, "repaintRects", behavior)) {
         ts << indent << "(repaint rects\n";
         for (size_t i = 0; i < repaintRectMap().get(this).size(); ++i) {
             if (repaintRectMap().get(this)[i].isEmpty())
index 1d0ceb1..63fd9ed 100644 (file)
@@ -72,6 +72,7 @@ enum LayerTreeAsTextBehaviorFlags {
     LayerTreeAsTextIncludePageOverlayLayers     = 1 << 6,
     LayerTreeAsTextIncludeAcceleratesDrawing    = 1 << 7,
     LayerTreeAsTextIncludeBackingStoreAttached  = 1 << 8,
+    LayerTreeAsTextIncludeRootLayerProperties   = 1 << 9,
     LayerTreeAsTextShowAll                      = 0xFFFF
 };
 typedef unsigned LayerTreeAsTextBehavior;
@@ -123,7 +124,7 @@ public:
     virtual bool isTrackingRepaints() const { return false; }
 
     virtual bool shouldSkipLayerInDump(const GraphicsLayer*, LayerTreeAsTextBehavior) const { return false; }
-    virtual bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char*) const { return true; }
+    virtual bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char*, LayerTreeAsTextBehavior) const { return true; }
 
     virtual bool shouldAggressivelyRetainTiles(const GraphicsLayer*) const { return false; }
     virtual bool shouldTemporarilyRetainTileCohorts(const GraphicsLayer*) const { return true; }
index 7d129b6..387a9d4 100644 (file)
@@ -2099,6 +2099,9 @@ static bool isCompositedPlugin(RenderObject& renderer)
 // This is a useful optimization, because it allows us to avoid allocating backing store.
 bool RenderLayerBacking::isSimpleContainerCompositingLayer(PaintedContentsInfo& contentsInfo) const
 {
+    if (m_owningLayer.isRenderViewLayer())
+        return false;
+
     if (renderer().isRenderReplaced() && (!isCompositedPlugin(renderer()) || isRestartedPlugin(renderer())))
         return false;
 
@@ -2114,29 +2117,6 @@ bool RenderLayerBacking::isSimpleContainerCompositingLayer(PaintedContentsInfo&
     if (renderer().isDocumentElementRenderer() && m_owningLayer.isolatesCompositedBlending())
         return false;
 
-    if (renderer().isRenderView()) {
-        // Look to see if the root object has a non-simple background
-        auto* rootObject = renderer().document().documentElement() ? renderer().document().documentElement()->renderer() : nullptr;
-        if (!rootObject)
-            return false;
-        
-        // Reject anything that has a border, a border-radius or outline,
-        // or is not a simple background (no background, or solid color).
-        if (hasPaintedBoxDecorationsOrBackgroundImage(rootObject->style()))
-            return false;
-        
-        // Now look at the body's renderer.
-        auto* body = renderer().document().body();
-        if (!body)
-            return false;
-        auto* bodyRenderer = body->renderer();
-        if (!bodyRenderer)
-            return false;
-        
-        if (hasPaintedBoxDecorationsOrBackgroundImage(bodyRenderer->style()))
-            return false;
-    }
-
     return true;
 }
 
@@ -2724,11 +2704,11 @@ bool RenderLayerBacking::shouldSkipLayerInDump(const GraphicsLayer* layer, Layer
     return m_isMainFrameRenderViewLayer && layer && layer == m_childContainmentLayer.get();
 }
 
-bool RenderLayerBacking::shouldDumpPropertyForLayer(const GraphicsLayer* layer, const char* propertyName) const
+bool RenderLayerBacking::shouldDumpPropertyForLayer(const GraphicsLayer* layer, const char* propertyName, LayerTreeAsTextBehavior flags) const
 {
     // For backwards compatibility with WebKit1 and other platforms,
     // skip some properties on the root tile cache.
-    if (m_isMainFrameRenderViewLayer && layer == m_graphicsLayer.get()) {
+    if (m_isMainFrameRenderViewLayer && layer == m_graphicsLayer.get() && !(flags & LayerTreeAsTextIncludeRootLayerProperties)) {
         if (!strcmp(propertyName, "drawsContent"))
             return false;
 
index 10d1d77..71ba37f 100644 (file)
@@ -227,7 +227,7 @@ public:
 
     bool isTrackingRepaints() const override;
     bool shouldSkipLayerInDump(const GraphicsLayer*, LayerTreeAsTextBehavior) const override;
-    bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char* propertyName) const override;
+    bool shouldDumpPropertyForLayer(const GraphicsLayer*, const char* propertyName, LayerTreeAsTextBehavior) const override;
 
     bool shouldAggressivelyRetainTiles(const GraphicsLayer*) const override;
     bool shouldTemporarilyRetainTileCohorts(const GraphicsLayer*) const override;
index 19dab8a..703457c 100644 (file)
@@ -1918,6 +1918,8 @@ String RenderLayerCompositor::layerTreeAsText(LayerTreeFlags flags)
         layerTreeBehavior |= LayerTreeAsTextIncludeAcceleratesDrawing;
     if (flags & LayerTreeFlagsIncludeBackingStoreAttached)
         layerTreeBehavior |= LayerTreeAsTextIncludeBackingStoreAttached;
+    if (flags & LayerTreeFlagsIncludeRootLayerProperties)
+        layerTreeBehavior |= LayerTreeAsTextIncludeRootLayerProperties;
 
     // We skip dumping the scroll and clip layers to keep layerTreeAsText output
     // similar between platforms.
@@ -1925,7 +1927,7 @@ String RenderLayerCompositor::layerTreeAsText(LayerTreeFlags flags)
 
     // Dump an empty layer tree only if the only composited layer is the main frame's tiled backing,
     // so that tests expecting us to drop out of accelerated compositing when there are no layers succeed.
-    if (!hasContentCompositingLayers() && documentUsesTiledBacking() && !(layerTreeBehavior & LayerTreeAsTextIncludeTileCaches))
+    if (!hasContentCompositingLayers() && documentUsesTiledBacking() && !(layerTreeBehavior & LayerTreeAsTextIncludeTileCaches) && !(layerTreeBehavior & LayerTreeAsTextIncludeRootLayerProperties))
         layerTreeText = emptyString();
 
     // The true root layer is not included in the dump, so if we want to report
index b06a1f8..73b6776 100644 (file)
@@ -2515,6 +2515,8 @@ static LayerTreeFlags toLayerTreeFlags(unsigned short flags)
         layerTreeFlags |= LayerTreeFlagsIncludeAcceleratesDrawing;
     if (flags & Internals::LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED)
         layerTreeFlags |= LayerTreeFlagsIncludeBackingStoreAttached;
+    if (flags & Internals::LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES)
+        layerTreeFlags |= LayerTreeFlagsIncludeRootLayerProperties;
 
     return layerTreeFlags;
 }
index c19382f..8430fd9 100644 (file)
@@ -347,6 +347,7 @@ public:
         LAYER_TREE_INCLUDES_CONTENT_LAYERS = 16,
         LAYER_TREE_INCLUDES_ACCELERATES_DRAWING = 32,
         LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED = 64,
+        LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES = 128,
     };
     ExceptionOr<String> layerTreeAsText(Document&, unsigned short flags) const;
     ExceptionOr<uint64_t> layerIDForElement(Element&);
index e638512..3bc508d 100644 (file)
@@ -372,6 +372,7 @@ enum CompositingPolicy {
     const unsigned short LAYER_TREE_INCLUDES_CONTENT_LAYERS = 16;
     const unsigned short LAYER_TREE_INCLUDES_ACCELERATES_DRAWING = 32;
     const unsigned short LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED = 64;
+    const unsigned short LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES = 128;
     [MayThrowException] DOMString layerTreeAsText(Document document, optional unsigned short flags = 0);
 
     [MayThrowException] unsigned long long layerIDForElement(Element element);