Changeset 128082 in webkit

Timestamp:

Sep 10, 2012 11:22:51 AM (12 years ago)

Author:

enne@google.com

Message:

[chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
​https://bugs.webkit.org/show_bug.cgi?id=96010

Reviewed by James Robinson.

Source/Platform:

Add some additional clarifying comments.

• chromium/public/WebVideoFrameProvider.h:

(Client):
(WebVideoFrameProvider):

Source/WebKit/chromium:

The key fix here is that the destructor no longer has a mutex.
The m_compositingMutex was supposedly protecting races between
~WebMediaPlayerClientImpl and setVideoFrameProviderClient. The
former is only called from the main thread and the latter is called
from the compositor thread only when the main thread is blocked (and
it already asserts that this is the case).

In addition, the m_providerMutex in CCVideoLayerImpl prevents the
destruction of WebMediaPlayerClientImpl, thus keeping the frame
acquired via getCurrentFrame alive until putCurrentFrame is called.
These functions are only called by the client, and comments are added
to the interface to better document this.

To prevent a race between load() and getCurrentFrame/putCurrentFrame
(which are called from different threads) a new m_webMediaPlayerMutex
to replace part of what the old m_compositingMutex was doing.

• src/WebMediaPlayerClientImpl.cpp:

(WebKit::WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl):
(WebKit::WebMediaPlayerClientImpl::load):
(WebKit::WebMediaPlayerClientImpl::loadInternal):
(WebKit::WebMediaPlayerClientImpl::setVideoFrameProviderClient):
(WebKit::WebMediaPlayerClientImpl::getCurrentFrame):
(WebKit::WebMediaPlayerClientImpl::putCurrentFrame):

• src/WebMediaPlayerClientImpl.h:

(WebMediaPlayerClientImpl):

Location:

trunk/Source

Files:

• Platform/ChangeLog (modified) (1 diff)
• Platform/chromium/public/WebVideoFrameProvider.h (modified) (3 diffs)
• WebKit/chromium/ChangeLog (modified) (1 diff)
• WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (modified) (6 diffs)
• WebKit/chromium/src/WebMediaPlayerClientImpl.h (modified) (1 diff)

trunk/Source/Platform/ChangeLog

@@ +1 @@
+2012-09-10  Adrienne Walker  <enne@google.com>
+
+        [chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
+        https://bugs.webkit.org/show_bug.cgi?id=96010
+
+        Reviewed by James Robinson.
+
+        Add some additional clarifying comments.
+
+        * chromium/public/WebVideoFrameProvider.h:
+        (Client):
+        (WebVideoFrameProvider):
+
 2012-09-10  Tommy Widenflycht  <tommyw@google.com>
 

trunk/Source/Platform/chromium/public/WebVideoFrameProvider.h

@@ -47 +47 @@
     public:
         // Provider will call this method to tell the client to stop using it.
-        // stopUsingProvider() may be called from any thread.
+        // stopUsingProvider() may be called from any thread. The client should
+        // block until it has putCurrentFrame any outstanding frames.
         virtual void stopUsingProvider() = 0;
 
@@ -58 +59 @@
     };
 
-    // May be called from any thread.
+    // May be called from any thread, but there must be some external guarantee
+    // that the provider is not destroyed before this call returns.
     virtual void setVideoFrameProviderClient(Client*) = 0;
 
@@ -64 +66 @@
     // Calls to this method should always be followed with a call to putCurrentFrame().
     // The ownership of the object is not transferred to the caller and
-    // the caller should not free the returned object.
+    // the caller should not free the returned object. Only the current provider
+    // client should call this function.
     virtual WebVideoFrame* getCurrentFrame() = 0;
     // This function releases the lock on the video frame in chromium. It should
     // always be called after getCurrentFrame(). Frames passed into this method
-    // should no longer be referenced after the call is made.
+    // should no longer be referenced after the call is made. Only the current
+    // provider client should call this function.
     virtual void putCurrentFrame(WebVideoFrame*) = 0;
 };

trunk/Source/WebKit/chromium/ChangeLog

@@ +1 @@
+2012-09-10  Adrienne Walker  <enne@google.com>
+
+        [chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
+        https://bugs.webkit.org/show_bug.cgi?id=96010
+
+        Reviewed by James Robinson.
+
+        The key fix here is that the destructor no longer has a mutex.
+        The m_compositingMutex was supposedly protecting races between
+        ~WebMediaPlayerClientImpl and setVideoFrameProviderClient. The
+        former is only called from the main thread and the latter is called
+        from the compositor thread only when the main thread is blocked (and
+        it already asserts that this is the case).
+
+        In addition, the m_providerMutex in CCVideoLayerImpl prevents the
+        destruction of WebMediaPlayerClientImpl, thus keeping the frame
+        acquired via getCurrentFrame alive until putCurrentFrame is called.
+        These functions are only called by the client, and comments are added
+        to the interface to better document this.
+
+        To prevent a race between load() and getCurrentFrame/putCurrentFrame
+        (which are called from different threads) a new m_webMediaPlayerMutex
+        to replace part of what the old m_compositingMutex was doing.
+
+        * src/WebMediaPlayerClientImpl.cpp:
+        (WebKit::WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl):
+        (WebKit::WebMediaPlayerClientImpl::load):
+        (WebKit::WebMediaPlayerClientImpl::loadInternal):
+        (WebKit::WebMediaPlayerClientImpl::setVideoFrameProviderClient):
+        (WebKit::WebMediaPlayerClientImpl::getCurrentFrame):
+        (WebKit::WebMediaPlayerClientImpl::putCurrentFrame):
+        * src/WebMediaPlayerClientImpl.h:
+        (WebMediaPlayerClientImpl):
+
 2012-09-10  Rick Byers  <rbyers@chromium.org>
 

trunk/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp

@@ -93 +93 @@
 {
 #if USE(ACCELERATED_COMPOSITING)
-    MutexLocker locker(m_compositingMutex);
     if (m_videoFrameProviderClient)
         m_videoFrameProviderClient->stopUsingProvider();
+    // No need for a lock here, as getCurrentFrame/putCurrentFrame can't be
+    // called now that the client is no longer using this provider. Also, load()
+    // and this destructor are called from the same thread.
     if (m_webMediaPlayer)
         m_webMediaPlayer->setStreamTextureClient(0);
@@ -313 +315 @@
     m_url = url;
 
+    MutexLocker locker(m_webMediaPlayerMutex);
     if (m_preload == MediaPlayer::None) {
-        MutexLocker locker(m_compositingMutex);
 #if ENABLE(WEB_AUDIO)
         m_audioSourceProvider.wrap(0); // Clear weak reference to m_webMediaPlayer's WebAudioSourceProvider.
@@ -326 +328 @@
 void WebMediaPlayerClientImpl::loadInternal()
 {
-    MutexLocker locker(m_compositingMutex);
 #if ENABLE(WEB_AUDIO)
     m_audioSourceProvider.wrap(0); // Clear weak reference to m_webMediaPlayer's WebAudioSourceProvider.
@@ -767 +768 @@
 void WebMediaPlayerClientImpl::setVideoFrameProviderClient(WebVideoFrameProvider::Client* client)
 {
-    MutexLocker locker(m_compositingMutex);
+    MutexLocker locker(m_webMediaPlayerMutex);
     if (m_videoFrameProviderClient)
         m_videoFrameProviderClient->stopUsingProvider();
@@ -777 +778 @@
 WebVideoFrame* WebMediaPlayerClientImpl::getCurrentFrame()
 {
-    MutexLocker locker(m_compositingMutex);
+    // This function is called only by the client.
+    MutexLocker locker(m_webMediaPlayerMutex);
     ASSERT(!m_currentVideoFrame);
+    ASSERT(m_videoFrameProviderClient);
     if (m_webMediaPlayer)
         m_currentVideoFrame = m_webMediaPlayer->getCurrentFrame();
@@ -786 +789 @@
 void WebMediaPlayerClientImpl::putCurrentFrame(WebVideoFrame* videoFrame)
 {
-    MutexLocker locker(m_compositingMutex);
+    // This function is called only by the client.
+    MutexLocker locker(m_webMediaPlayerMutex);
     ASSERT(videoFrame == m_currentVideoFrame);
+    ASSERT(m_videoFrameProviderClient);
     if (!videoFrame)
         return;

trunk/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h

@@ -198 +198 @@
 #endif
 
-    Mutex m_compositingMutex; // Guards m_currentVideoFrame and m_videoFrameProviderClient.
+    Mutex m_webMediaPlayerMutex; // Guards the m_webMediaPlayer
     WebCore::MediaPlayer* m_mediaPlayer;
     OwnPtr<WebMediaPlayer> m_webMediaPlayer;