Changeset 116833 in webkit

Timestamp:

May 11, 2012 7:04:46 PM (12 years ago)

Author:

enne@google.com

Message:

[chromium] Prevent deadlock on CCVideoLayerImpl destruction
​https://bugs.webkit.org/show_bug.cgi?id=86258

Reviewed by James Robinson.

~CCVideoLayerImpl had a common deadlock issue where if it got
destroyed before WebMediaPlayerClientImpl, it would take a lock,
call WebMediaPlayerClientImpl::setVideoFrameProviderClient(0),
which in turn would call CCVideoLayerImpl::stopUsingProvider(),
which would try to take the same lock and would deadlock.

CCVideoLayerImpl is only created and destroyed during tree
synchronization in a commit or during synchronous compositor thread
destruction. In either case, the main thread is blocked, and so no
lock needs to be taken at all.

• platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:

(WebCore::CCVideoLayerImpl::CCVideoLayerImpl):
(WebCore::CCVideoLayerImpl::~CCVideoLayerImpl):
(WebCore::CCVideoLayerImpl::stopUsingProvider):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/chromium/cc/CCVideoLayerImpl.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-05-11  Adrienne Walker  <enne@google.com>
+
+        [chromium] Prevent deadlock on CCVideoLayerImpl destruction
+        https://bugs.webkit.org/show_bug.cgi?id=86258
+
+        Reviewed by James Robinson.
+
+        ~CCVideoLayerImpl had a common deadlock issue where if it got
+        destroyed before WebMediaPlayerClientImpl, it would take a lock,
+        call WebMediaPlayerClientImpl::setVideoFrameProviderClient(0),
+        which in turn would call CCVideoLayerImpl::stopUsingProvider(),
+        which would try to take the same lock and would deadlock.
+
+        CCVideoLayerImpl is only created and destroyed during tree
+        synchronization in a commit or during synchronous compositor thread
+        destruction. In either case, the main thread is blocked, and so no
+        lock needs to be taken at all.
+
+        * platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:
+        (WebCore::CCVideoLayerImpl::CCVideoLayerImpl):
+        (WebCore::CCVideoLayerImpl::~CCVideoLayerImpl):
+        (WebCore::CCVideoLayerImpl::stopUsingProvider):
+
 2012-05-11  Jeffrey Pfau  <jpfau@apple.com>
 

trunk/Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp

@@ -81 +81 @@
 {
     memcpy(m_streamTextureMatrix, flipTransform, sizeof(m_streamTextureMatrix));
-    provider->setVideoFrameProviderClient(this);
+
+    // This only happens during a commit on the compositor thread while the main
+    // thread is blocked. That makes this a thread-safe call to set the video
+    // frame provider client that does not require a lock. The same is true of
+    // the call in the destructor.
+    m_provider->setVideoFrameProviderClient(this);
 }
 
 CCVideoLayerImpl::~CCVideoLayerImpl()
 {
-    MutexLocker locker(m_providerMutex);
+    // See comment in constructor for why this doesn't need a lock.
     if (m_provider) {
         m_provider->setVideoFrameProviderClient(0);
@@ -97 +102 @@
 void CCVideoLayerImpl::stopUsingProvider()
 {
+    // Block the provider from shutting down until this client is done
+    // using the frame.
     MutexLocker locker(m_providerMutex);
+    ASSERT(!m_frame);
     m_provider = 0;
 }