Attachment 343307 Details for Bug 186909 – Patch

[patch] Patch

bug-186909-20180621213402.patch (text/plain), 16.68 KB, created by Simon Fraser (smfr) on 2018-06-21 21:34:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2018-06-21 21:34:03 PDT

Size: 16.68 KB

patch

obsolete

>Subversion Revision: 232942
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index fad856ef01281b41ec26948b2882932d1a3875e8..9acb9f49a4695f25c3c9953714468829cad0bd2b 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,43 @@
>+2018-06-21  Simon Fraser  <simon.fraser@apple.com>
>+
>+        https://hackernoon.com/ uses lots of layer backing store
>+        https://bugs.webkit.org/show_bug.cgi?id=186909
>+        rdar://problem/40257540
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        The existing "backing store detached" logic, which was used to eliminate backing store
>+        for compositing layers outside the viewport, had a number of bugs that allowed layers
>+        to have backing store when they should not.
>+        
>+        Specifically, any code path that ended up in setNeedsDisplay{InRect}() in PlatformCALayer
>+        could trigger backing store creation on layers that should have never had any.
>+        
>+        Rather than monkeypatch all the GraphicsLayerCA call sites that call setNeedsDisplay{InRect}(),
>+        just bail early from the PlatformCALayer* methods that trigger repaints.
>+        
>+        Tests didn't catch this because they just dumped the state of the backingStoreAttached flag. To fix this,
>+        create backingStoreAttachedForTesting() which also tests whether the layer has contents.
>+
>+        Test: compositing/backing/backing-store-attachment-outside-viewport.html
>+
>+        * platform/graphics/GraphicsLayer.cpp:
>+        (WebCore::GraphicsLayer::dumpProperties const):
>+        (showGraphicsLayerTree):
>+        * platform/graphics/GraphicsLayer.h:
>+        (WebCore::GraphicsLayer::backingStoreAttachedForTesting const):
>+        * platform/graphics/GraphicsLayerClient.h:
>+        * platform/graphics/ca/GraphicsLayerCA.cpp:
>+        (WebCore::GraphicsLayerCA::backingStoreAttachedForTesting const):
>+        (WebCore::GraphicsLayerCA::setNeedsDisplay):
>+        * platform/graphics/ca/GraphicsLayerCA.h:
>+        * platform/graphics/ca/PlatformCALayer.h:
>+        * platform/graphics/ca/cocoa/PlatformCALayerCocoa.h:
>+        * platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:
>+        (PlatformCALayerCocoa::setNeedsDisplay):
>+        (PlatformCALayerCocoa::setNeedsDisplayInRect):
>+        (PlatformCALayerCocoa::hasContents const):
>+
> 2018-06-07  Simon Fraser  <simon.fraser@apple.com>
> 
>         Add support for dumping GC heap snapshots, and a viewer
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 34429f3a236e2febcafd829521ed434841343c19..c673747f1efa4fdbb4b2b87fd5f736e306914ec7 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-06-21  Simon Fraser  <simon.fraser@apple.com>
>+
>+        https://hackernoon.com/ uses lots of layer backing store
>+        https://bugs.webkit.org/show_bug.cgi?id=186909
>+        rdar://problem/40257540
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp:
>+        (WebKit::PlatformCALayerRemote::setNeedsDisplayInRect):
>+        (WebKit::PlatformCALayerRemote::setNeedsDisplay):
>+        (WebKit::PlatformCALayerRemote::hasContents const):
>+        * WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h:
>+
> 2018-06-18  Jiewen Tan  <jiewen_tan@apple.com>
> 
>         Add a graceful exit for AuthenticationManager::initializeConnection
>diff --git a/Source/bmalloc/ChangeLog b/Source/bmalloc/ChangeLog
>index 8b6caf2c57f800cd0f3572235b59b90a6b21e31b..e21ceed641c54852595a284a821d806eceab75b3 100644
>--- a/Source/bmalloc/ChangeLog
>+++ b/Source/bmalloc/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-06-21  Simon Fraser  <simon.fraser@apple.com>
>+
>+        https://hackernoon.com/ uses lots of layer backing store
>+        https://bugs.webkit.org/show_bug.cgi?id=186909
>+        rdar://problem/40257540
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        Drive-by typo fix.
>+
>+        * bmalloc/Scavenger.cpp:
>+        (bmalloc::dumpStats):
>+
> 2018-06-09  Dan Bernstein  <mitz@apple.com>
> 
>         [Xcode] Clean up and modernize some build setting definitions
>diff --git a/Source/WebCore/platform/graphics/GraphicsLayer.cpp b/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>index 99e98ba909d43e90d4ebb7eef91d21b0df83f458..33fdd82328c6f26ce49dd6fbcb00771841cd6e69 100644
>--- a/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>+++ b/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>@@ -787,7 +787,7 @@ void GraphicsLayer::dumpProperties(TextStream& ts, LayerTreeAsTextBehavior behav
>         ts << indent << "(acceleratesDrawing " << m_acceleratesDrawing << ")\n";
> 
>     if (behavior & LayerTreeAsTextIncludeBackingStoreAttached)
>-        ts << indent << "(backingStoreAttached " << backingStoreAttached() << ")\n";
>+        ts << indent << "(backingStoreAttached " << backingStoreAttachedForTesting() << ")\n";
> 
>     if (!m_transform.isIdentity()) {
>         ts << indent << "(transform ";
>@@ -931,7 +931,7 @@ void showGraphicsLayerTree(const WebCore::GraphicsLayer* layer)
>     if (!layer)
>         return;
> 
>-    String output = layer->layerTreeAsText(WebCore::LayerTreeAsTextDebug | WebCore::LayerTreeAsTextIncludeVisibleRects | WebCore::LayerTreeAsTextIncludeTileCaches | WebCore::LayerTreeAsTextIncludeContentLayers);
>+    String output = layer->layerTreeAsText(WebCore::LayerTreeAsTextShowAll);
>     fprintf(stderr, "%s\n", output.utf8().data());
> }
> #endif
>diff --git a/Source/WebCore/platform/graphics/GraphicsLayer.h b/Source/WebCore/platform/graphics/GraphicsLayer.h
>index 10eda15fc3f48fcabf123df167cf9908626cbb4b..58b02e9f082afbb4a2dc4548ae5b4cf467f0542a 100644
>--- a/Source/WebCore/platform/graphics/GraphicsLayer.h
>+++ b/Source/WebCore/platform/graphics/GraphicsLayer.h
>@@ -552,6 +552,7 @@ public:
>     WEBCORE_EXPORT virtual double backingStoreMemoryEstimate() const;
> 
>     virtual bool backingStoreAttached() const { return true; }
>+    virtual bool backingStoreAttachedForTesting() const { return backingStoreAttached(); }
> 
>     void setCanDetachBackingStore(bool b) { m_canDetachBackingStore = b; }
>     bool canDetachBackingStore() const { return m_canDetachBackingStore; }
>diff --git a/Source/WebCore/platform/graphics/GraphicsLayerClient.h b/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>index b2fc8f6e7da8d2e581e8b05d1afc5f6073db2913..c3aa277467a14b7b76eaf59846a8a1ea3f8f7bc1 100644
>--- a/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>+++ b/Source/WebCore/platform/graphics/GraphicsLayerClient.h
>@@ -72,6 +72,7 @@ enum LayerTreeAsTextBehaviorFlags {
>     LayerTreeAsTextIncludePageOverlayLayers     = 1 << 6,
>     LayerTreeAsTextIncludeAcceleratesDrawing    = 1 << 7,
>     LayerTreeAsTextIncludeBackingStoreAttached  = 1 << 8,
>+    LayerTreeAsTextShowAll                      = 0xFFFF
> };
> typedef unsigned LayerTreeAsTextBehavior;
> 
>diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>index a0d642751f1e7f10d662304b20d17819d39ecf4d..7468af9017223981f936a029b3c7f196f173101f 100644
>--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>@@ -881,14 +881,16 @@ bool GraphicsLayerCA::backingStoreAttached() const
>     return m_layer->backingStoreAttached();
> }
> 
>+bool GraphicsLayerCA::backingStoreAttachedForTesting() const
>+{
>+    return m_layer->backingStoreAttached() || m_layer->hasContents();
>+}
>+
> void GraphicsLayerCA::setNeedsDisplay()
> {
>     if (!drawsContent())
>         return;
> 
>-    if (!backingStoreAttached())
>-        return;
>-
>     m_needsFullRepaint = true;
>     m_dirtyRects.clear();
>     noteLayerPropertyChanged(DirtyRectsChanged);
>diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>index ee41cd43d468ad62bc4130ca9b74b62027f29de9..296da67eb972ba08f6ca9f325a7595b0ffe8653b 100644
>--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>@@ -264,6 +264,7 @@ private:
>     bool isRunningTransformAnimation() const;
> 
>     WEBCORE_EXPORT bool backingStoreAttached() const override;
>+    WEBCORE_EXPORT bool backingStoreAttachedForTesting() const override;
> 
>     bool animationIsRunning(const String& animationName) const
>     {
>diff --git a/Source/WebCore/platform/graphics/ca/PlatformCALayer.h b/Source/WebCore/platform/graphics/ca/PlatformCALayer.h
>index 744662b2b425fef4f07d5a82930d4610851250d0..d7074db6df3f57aa36faec60fb38b7454b882e9d 100644
>--- a/Source/WebCore/platform/graphics/ca/PlatformCALayer.h
>+++ b/Source/WebCore/platform/graphics/ca/PlatformCALayer.h
>@@ -180,6 +180,7 @@ public:
>     virtual bool supportsSubpixelAntialiasedText() const = 0;
>     virtual void setSupportsSubpixelAntialiasedText(bool) = 0;
> 
>+    virtual bool hasContents() const = 0;
>     virtual CFTypeRef contents() const = 0;
>     virtual void setContents(CFTypeRef) = 0;
> 
>diff --git a/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h b/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h
>index e59cb0dc71b65223781b4bd10ac6bc5c0494dd21..13446cddb332517f8b18c57b3cc9260e34b875cc 100644
>--- a/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h
>+++ b/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h
>@@ -116,6 +116,7 @@ public:
>     bool supportsSubpixelAntialiasedText() const override;
>     void setSupportsSubpixelAntialiasedText(bool) override;
> 
>+    bool hasContents() const override;
>     CFTypeRef contents() const override;
>     void setContents(CFTypeRef) override;
> 
>diff --git a/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm b/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm
>index ca1c45f13219c6a97599c1d1e9e7fd4ca0e11d96..07587c2688db49a76f5df6fdd6d6f230280fb4c9 100644
>--- a/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm
>+++ b/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm
>@@ -391,6 +391,9 @@ void PlatformCALayerCocoa::animationEnded(const String& animationKey)
> 
> void PlatformCALayerCocoa::setNeedsDisplay()
> {
>+    if (!m_backingStoreAttached)
>+        return;
>+
>     BEGIN_BLOCK_OBJC_EXCEPTIONS
>     [m_layer setNeedsDisplay];
>     END_BLOCK_OBJC_EXCEPTIONS
>@@ -398,6 +401,9 @@ void PlatformCALayerCocoa::setNeedsDisplay()
> 
> void PlatformCALayerCocoa::setNeedsDisplayInRect(const FloatRect& dirtyRect)
> {
>+    if (!m_backingStoreAttached)
>+        return;
>+
>     BEGIN_BLOCK_OBJC_EXCEPTIONS
>     [m_layer setNeedsDisplayInRect:dirtyRect];
>     END_BLOCK_OBJC_EXCEPTIONS
>@@ -739,6 +745,11 @@ void PlatformCALayerCocoa::setSupportsSubpixelAntialiasedText(bool supportsSubpi
>     updateContentsFormat();
> }
> 
>+bool PlatformCALayerCocoa::hasContents() const
>+{
>+    return [m_layer contents];
>+}
>+
> CFTypeRef PlatformCALayerCocoa::contents() const
> {
>     return (__bridge CFTypeRef)[m_layer contents];
>diff --git a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp
>index 53968fcdb38ccdf4266746f3f1712c09eaadeee8..ee34e3bb6302f37ade8360d1a988a93929cdb1f0 100644
>--- a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp
>@@ -207,6 +207,9 @@ void PlatformCALayerRemote::updateBackingStore()
> 
> void PlatformCALayerRemote::setNeedsDisplayInRect(const FloatRect& rect)
> {
>+    if (!m_properties.backingStoreAttached)
>+        return;
>+
>     ensureBackingStore();
> 
>     // FIXME: Need to map this through contentsRect/etc.
>@@ -215,6 +218,9 @@ void PlatformCALayerRemote::setNeedsDisplayInRect(const FloatRect& rect)
> 
> void PlatformCALayerRemote::setNeedsDisplay()
> {
>+    if (!m_properties.backingStoreAttached)
>+        return;
>+
>     ensureBackingStore();
> 
>     m_properties.backingStore->setNeedsDisplay();
>@@ -617,6 +623,11 @@ void PlatformCALayerRemote::setSupportsSubpixelAntialiasedText(bool)
> {
> }
> 
>+bool PlatformCALayerRemote::hasContents() const
>+{
>+    return !!m_properties.backingStore;
>+}
>+
> CFTypeRef PlatformCALayerRemote::contents() const
> {
>     return nullptr;
>diff --git a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h
>index 8f84ca3d24cda1e0f4e1d556aeab7f1e8d5a47ce..39e97a4ad8e67fe81a27b7f62f92de573a965e2f 100644
>--- a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h
>+++ b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h
>@@ -120,6 +120,7 @@ public:
>     bool supportsSubpixelAntialiasedText() const override;
>     void setSupportsSubpixelAntialiasedText(bool) override;
> 
>+    bool hasContents() const override;
>     CFTypeRef contents() const override;
>     void setContents(CFTypeRef) override;
> 
>diff --git a/Source/bmalloc/bmalloc/Scavenger.cpp b/Source/bmalloc/bmalloc/Scavenger.cpp
>index de99c176bc9cc89453d354e2804b5b794a89cf67..edb014de9b2fcbcae545cc7b9afc41d7bf3ac3f2 100644
>--- a/Source/bmalloc/bmalloc/Scavenger.cpp
>+++ b/Source/bmalloc/bmalloc/Scavenger.cpp
>@@ -158,7 +158,7 @@ inline void dumpStats()
>     task_vm_info_data_t vmInfo;
>     mach_msg_type_number_t vmSize = TASK_VM_INFO_COUNT;
>     if (KERN_SUCCESS == task_info(mach_task_self(), TASK_VM_INFO, (task_info_t)(&vmInfo), &vmSize)) {
>-        dump("phys_footrpint", vmInfo.phys_footprint);
>+        dump("phys_footprint", vmInfo.phys_footprint);
>         dump("internal+compressed", vmInfo.internal + vmInfo.compressed);
>     }
> #endif
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 5ce41036d34895fc5b1e9d090af3ad8e74d9a276..fc2b87564d96dbd95c386c8b22bf8da20297dc6b 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-06-21  Simon Fraser  <simon.fraser@apple.com>
>+
>+        https://hackernoon.com/ uses lots of layer backing store
>+        https://bugs.webkit.org/show_bug.cgi?id=186909
>+        rdar://problem/40257540
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        New test.
>+
>+        * compositing/backing/backing-store-attachment-outside-viewport-expected.txt: Added.
>+        * compositing/backing/backing-store-attachment-outside-viewport.html: Added.
>+
> 2018-06-18  Said Abou-Hallawa  <sabouhallawa@apple.com>
> 
>         Document should not be mutated under SMILTimeContainer::updateAnimations()
>diff --git a/LayoutTests/compositing/backing/backing-store-attachment-outside-viewport-expected.txt b/LayoutTests/compositing/backing/backing-store-attachment-outside-viewport-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..5000839a61149164c6f528c66413223544a98bdf
>--- /dev/null
>+++ b/LayoutTests/compositing/backing/backing-store-attachment-outside-viewport-expected.txt
>@@ -0,0 +1,35 @@
>+(GraphicsLayer
>+  (anchor 0.00 0.00)
>+  (bounds 785.00 3821.00)
>+  (backingStoreAttached 1)
>+  (children 1
>+    (GraphicsLayer
>+      (bounds 785.00 3821.00)
>+      (contentsOpaque 1)
>+      (backingStoreAttached 1)
>+      (children 3
>+        (GraphicsLayer
>+          (position 8.00 13.00)
>+          (bounds 600.00 600.00)
>+          (drawsContent 1)
>+          (backingStoreAttached 1)
>+        )
>+        (GraphicsLayer
>+          (position 8.00 1613.00)
>+          (bounds 600.00 600.00)
>+          (drawsContent 1)
>+          (backingStoreAttached 0)
>+        )
>+        (GraphicsLayer
>+          (position 8.00 3213.00)
>+          (bounds 600.00 600.00)
>+          (drawsContent 1)
>+          (backingStoreAttached 0)
>+        )
>+      )
>+    )
>+  )
>+)
>+I'm attached.
>+I'm detached.
>+I'm detached. Triggers repaint
>diff --git a/LayoutTests/compositing/backing/backing-store-attachment-outside-viewport.html b/LayoutTests/compositing/backing/backing-store-attachment-outside-viewport.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..6191cea98ff896201676290209fee50d3e2db755
>--- /dev/null
>+++ b/LayoutTests/compositing/backing/backing-store-attachment-outside-viewport.html
>@@ -0,0 +1,41 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<style>
>+.layerized {
>+    transform: translateZ(0);
>+    width: 600px;
>+    height: 600px;
>+}
>+.vspace {
>+    height: 1000px;
>+}
>+</style>
>+<script>
>+if (window.testRunner) {
>+    testRunner.dumpAsText();
>+    testRunner.waitUntilDone();
>+}
>+
>+window.onload = function() {
>+    if (!window.testRunner)
>+        return;
>+
>+    document.getElementById('repainting').style.color = 'blue';
>+    setTimeout(function() {
>+        var out = document.getElementById('out');
>+        out.innerText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_BACKING_STORE_ATTACHED);
>+        testRunner.notifyDone();
>+    }, 0);
>+};
>+</script>
>+</head>
>+<body>
>+<pre id="out"></pre>
>+<div class="layerized">I'm attached.</div>
>+<div class="vspace"></div>
>+<div class="layerized">I'm detached.</div>
>+<div class="vspace"></div>
>+<div class="layerized">I'm detached. <span id="repainting">Triggers repaint</span></div>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 186909: 343307 | 343309 | 343310 | 343311 | 343316 | 343678 | 343719 | 343720