[CoordinatedGraphics] CompositingCoordinator destructor is scheduling layer flushes
authorzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Feb 2016 08:41:57 +0000 (08:41 +0000)
committerzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Feb 2016 08:41:57 +0000 (08:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=153823

Reviewed by Carlos Garcia Campos.

Purging the backing stores during the CompositingCoordinator destructor
is also scheduling layer flushes in the object's client, which is an object
of the LayerTreeHost-deriving class that owns the CompositingCoordinator
object in question and is also being destroyed.

In case of ThreadedCoordinatedLayerTreeHost, this scheduling can access
the RunLoop::Timer object which has already been destroyed, causing a
crash. Another problem with this is that we're invoking a virtual function
on an object that's being destructed, which works well enough in this case
but should be discouraged in general.

In order to avoid this, add the m_isDestructing boolean to the
CompositingCoordinator class, flip it to true during the destruction,
and check for its falseness before scheduling a layer flush.

* platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:
(WebCore::CompositingCoordinator::CompositingCoordinator):
(WebCore::CompositingCoordinator::~CompositingCoordinator):
(WebCore::CompositingCoordinator::notifyFlushRequired):
* platform/graphics/texmap/coordinated/CompositingCoordinator.h:

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp
Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h

index 0180e9e..74aea89 100644 (file)
@@ -1,5 +1,33 @@
 2016-02-03  Zan Dobersek  <zdobersek@igalia.com>
 
+        [CoordinatedGraphics] CompositingCoordinator destructor is scheduling layer flushes
+        https://bugs.webkit.org/show_bug.cgi?id=153823
+
+        Reviewed by Carlos Garcia Campos.
+
+        Purging the backing stores during the CompositingCoordinator destructor
+        is also scheduling layer flushes in the object's client, which is an object
+        of the LayerTreeHost-deriving class that owns the CompositingCoordinator
+        object in question and is also being destroyed.
+
+        In case of ThreadedCoordinatedLayerTreeHost, this scheduling can access
+        the RunLoop::Timer object which has already been destroyed, causing a
+        crash. Another problem with this is that we're invoking a virtual function
+        on an object that's being destructed, which works well enough in this case
+        but should be discouraged in general.
+
+        In order to avoid this, add the m_isDestructing boolean to the
+        CompositingCoordinator class, flip it to true during the destruction,
+        and check for its falseness before scheduling a layer flush.
+
+        * platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:
+        (WebCore::CompositingCoordinator::CompositingCoordinator):
+        (WebCore::CompositingCoordinator::~CompositingCoordinator):
+        (WebCore::CompositingCoordinator::notifyFlushRequired):
+        * platform/graphics/texmap/coordinated/CompositingCoordinator.h:
+
+2016-02-03  Zan Dobersek  <zdobersek@igalia.com>
+
         [TexMap] Don't use RELEASE_ASSERT in TextureMapperLayer::computeTransformsRecursive()
         https://bugs.webkit.org/show_bug.cgi?id=153822
 
index c53bc06..0bf037d 100644 (file)
 
 namespace WebCore {
 
-CompositingCoordinator::~CompositingCoordinator()
-{
-    purgeBackingStores();
-
-    for (auto& registeredLayer : m_registeredLayers.values())
-        registeredLayer->setCoordinator(0);
-}
-
 CompositingCoordinator::CompositingCoordinator(Page* page, CompositingCoordinator::Client* client)
     : m_page(page)
     , m_client(client)
-    , m_rootCompositingLayer(0)
+    , m_rootCompositingLayer(nullptr)
+    , m_isDestructing(false)
     , m_isPurging(false)
     , m_isFlushingLayerChanges(false)
     , m_shouldSyncFrame(false)
@@ -66,6 +59,16 @@ CompositingCoordinator::CompositingCoordinator(Page* page, CompositingCoordinato
 {
 }
 
+CompositingCoordinator::~CompositingCoordinator()
+{
+    m_isDestructing = true;
+
+    purgeBackingStores();
+
+    for (auto& registeredLayer : m_registeredLayers.values())
+        registeredLayer->setCoordinator(nullptr);
+}
+
 void CompositingCoordinator::setRootCompositingLayer(GraphicsLayer* compositingLayer, GraphicsLayer* overlayLayer)
 {
     if (m_rootCompositingLayer)
@@ -240,11 +243,10 @@ void CompositingCoordinator::notifyAnimationStarted(const GraphicsLayer*, const
 
 void CompositingCoordinator::notifyFlushRequired(const GraphicsLayer*)
 {
-    if (!isFlushingLayerChanges())
+    if (!m_isDestructing && !isFlushingLayerChanges())
         m_client->notifyFlushRequired();
 }
 
-
 void CompositingCoordinator::paintContents(const GraphicsLayer* graphicsLayer, GraphicsContext& graphicsContext, GraphicsLayerPaintingPhase, const FloatRect& clipRect)
 {
     m_client->paintLayerContents(graphicsLayer, graphicsContext, enclosingIntRect(clipRect));
index fe10a3e..79e76ba 100644 (file)
@@ -139,6 +139,7 @@ private:
     Vector<std::unique_ptr<UpdateAtlas>> m_updateAtlases;
 
     // We don't send the messages related to releasing resources to renderer during purging, because renderer already had removed all resources.
+    bool m_isDestructing;
     bool m_isPurging;
     bool m_isFlushingLayerChanges;