Changeset 170862 in webkit

Timestamp:

Jul 7, 2014, 4:44:49 PM (12 years ago)

Author:

Simon Fraser

Message:

[UI-side compositing] Crash when starting a filter transition on a reflected layer
​https://bugs.webkit.org/show_bug.cgi?id=134694

Reviewed by Tim Horton.

Source/WebCore:

Don't call the owner if we failed to find the animation key (which actually
isn't used by PlatformCALayerMac anyway).

• platform/graphics/ca/mac/PlatformCALayerMac.mm:

(-[WebAnimationDelegate animationDidStart:]):

Source/WebKit2:

When cloned layers had animations, we would fire two animationDidStart callbacks,
but the second would pass an empty animationKey string to the web process, resulting
in a crash.

Fix by not blindly copying all layer properties when cloning PlatformCALayerRemotes,
since the clone would include addedAnimations, and then get the same animations
added on top by the caller.

Also protect against an empty animation key in the animationDidStart callback.

• UIProcess/mac/RemoteLayerTreeHost.mm:

(WebKit::RemoteLayerTreeHost::animationDidStart):

• WebProcess/WebPage/mac/PlatformCALayerRemote.cpp:

(WebKit::PlatformCALayerRemote::PlatformCALayerRemote):
(WebKit::PlatformCALayerRemote::clone): Don't copy all the properties; copy
them manually as PlatformCALayerMac does. Only copy the big things if they don't
have their default values.
(WebKit::PlatformCALayerRemote::copyFiltersFrom): Need an implementation of this
for clone() to call.

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm (modified) (1 diff)
• WebKit2/ChangeLog (modified) (1 diff)
• WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm (modified) (1 diff)
• WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-07-07  Simon Fraser  <simon.fraser@apple.com>
+
+        [UI-side compositing] Crash when starting a filter transition on a reflected layer
+        https://bugs.webkit.org/show_bug.cgi?id=134694
+
+        Reviewed by Tim Horton.
+
+        Don't call the owner if we failed to find the animation key (which actually
+        isn't used by PlatformCALayerMac anyway).
+
+        * platform/graphics/ca/mac/PlatformCALayerMac.mm:
+        (-[WebAnimationDelegate animationDidStart:]):
+
 2014-07-07  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm

@@ -133 +133 @@
         }
 
-        m_owner->animationStarted(animationKey, startTime);
+        if (!animationKey.isEmpty())
+            m_owner->animationStarted(animationKey, startTime);
     }
 }

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2014-07-07  Simon Fraser  <simon.fraser@apple.com>
+
+        [UI-side compositing] Crash when starting a filter transition on a reflected layer
+        https://bugs.webkit.org/show_bug.cgi?id=134694
+
+        Reviewed by Tim Horton.
+        
+        When cloned layers had animations, we would fire two animationDidStart callbacks,
+        but the second would pass an empty animationKey string to the web process, resulting
+        in a crash.
+        
+        Fix by not blindly copying all layer properties when cloning PlatformCALayerRemotes,
+        since the clone would include addedAnimations, and then get the same animations
+        added on top by the caller.
+        
+        Also protect against an empty animation key in the animationDidStart callback.
+
+        * UIProcess/mac/RemoteLayerTreeHost.mm:
+        (WebKit::RemoteLayerTreeHost::animationDidStart):
+        * WebProcess/WebPage/mac/PlatformCALayerRemote.cpp:
+        (WebKit::PlatformCALayerRemote::PlatformCALayerRemote):
+        (WebKit::PlatformCALayerRemote::clone): Don't copy all the properties; copy
+        them manually as PlatformCALayerMac does. Only copy the big things if they don't
+        have their default values.
+        (WebKit::PlatformCALayerRemote::copyFiltersFrom): Need an implementation of this
+        for clone() to call.
+
 2014-07-07  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm

@@ -149 +149 @@
     }
 
-    m_drawingArea.acceleratedAnimationDidStart(layerID, animationKey, startTime);
+    if (!animationKey.isEmpty())
+        m_drawingArea.acceleratedAnimationDidStart(layerID, animationKey, startTime);
 }
 

trunk/Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp

@@ -91 +91 @@
 PlatformCALayerRemote::PlatformCALayerRemote(const PlatformCALayerRemote& other, PlatformCALayerClient* owner, RemoteLayerTreeContext& context)
     : PlatformCALayer(other.layerType(), owner)
-    , m_properties(other.m_properties)
     , m_superlayer(nullptr)
     , m_maskLayer(nullptr)
@@ -103 +102 @@
     RefPtr<PlatformCALayerRemote> clone = PlatformCALayerRemote::create(*this, client, *m_context);
 
-    clone->m_properties.notePropertiesChanged(static_cast<RemoteLayerTreeTransaction::LayerChange>(m_properties.everChangedProperties & ~RemoteLayerTreeTransaction::BackingStoreChanged));
+    clone->setPosition(position());
+    clone->setBounds(bounds());
+    clone->setAnchorPoint(anchorPoint());
+
+    if (m_properties.transform)
+        clone->setTransform(*m_properties.transform);
+
+    if (m_properties.sublayerTransform)
+        clone->setSublayerTransform(*m_properties.sublayerTransform);
+
+    clone->setContents(contents());
+    clone->setMasksToBounds(masksToBounds());
+    clone->setDoubleSided(isDoubleSided());
+    clone->setOpaque(isOpaque());
+    clone->setBackgroundColor(backgroundColor());
+    clone->setContentsScale(contentsScale());
+#if ENABLE(CSS_FILTERS)
+    if (m_properties.filters)
+        clone->copyFiltersFrom(this);
+#endif
+    clone->updateCustomAppearance(customAppearance());
 
     clone->setClonedLayer(this);
@@ -578 +597 @@
 void PlatformCALayerRemote::copyFiltersFrom(const PlatformCALayer* sourceLayer)
 {
-    ASSERT_NOT_REACHED();
+    if (const FilterOperations* filters = toPlatformCALayerRemote(sourceLayer)->m_properties.filters.get())
+        setFilters(*filters);
+    else if (m_properties.filters)
+        m_properties.filters = nullptr;
+
+    m_properties.notePropertiesChanged(RemoteLayerTreeTransaction::FiltersChanged);
 }