[chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
authorenne@google.com <enne@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Sep 2012 18:22:51 +0000 (18:22 +0000)
committerenne@google.com <enne@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Sep 2012 18:22:51 +0000 (18:22 +0000)
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):

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

Source/Platform/ChangeLog
Source/Platform/chromium/public/WebVideoFrameProvider.h
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h

index 6e913e7..c9d7e94 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         [chromium] MediaStream API: Remove the Descriptor postfix
index 4d656a2..24552e8 100644 (file)
@@ -46,7 +46,8 @@ public:
     class Client {
     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;
 
         // Notifies the provider's client that a call to getCurrentFrame() will return new data.
@@ -57,17 +58,20 @@ public:
         virtual void didUpdateMatrix(const float*) = 0;
     };
 
-    // 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;
 
     // This function places a lock on the current frame and returns a pointer to it.
     // 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;
 };
 
index 2a62145..2b1a691 100644 (file)
@@ -1,3 +1,37 @@
+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>
 
         [chromium] Don't use WebGestureEvent.boundingBox for touch adjustment
index e570dca..e75c739 100644 (file)
@@ -92,9 +92,11 @@ WebMediaPlayer* WebMediaPlayerClientImpl::mediaPlayer() const
 WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl()
 {
 #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);
 #endif
@@ -312,8 +314,8 @@ void WebMediaPlayerClientImpl::load(const String& url)
 {
     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.
 #endif
@@ -325,7 +327,6 @@ void WebMediaPlayerClientImpl::load(const String& url)
 
 void WebMediaPlayerClientImpl::loadInternal()
 {
-    MutexLocker locker(m_compositingMutex);
 #if ENABLE(WEB_AUDIO)
     m_audioSourceProvider.wrap(0); // Clear weak reference to m_webMediaPlayer's WebAudioSourceProvider.
 #endif
@@ -766,7 +767,7 @@ bool WebMediaPlayerClientImpl::acceleratedRenderingInUse()
 
 void WebMediaPlayerClientImpl::setVideoFrameProviderClient(WebVideoFrameProvider::Client* client)
 {
-    MutexLocker locker(m_compositingMutex);
+    MutexLocker locker(m_webMediaPlayerMutex);
     if (m_videoFrameProviderClient)
         m_videoFrameProviderClient->stopUsingProvider();
     m_videoFrameProviderClient = client;
@@ -776,8 +777,10 @@ void WebMediaPlayerClientImpl::setVideoFrameProviderClient(WebVideoFrameProvider
 
 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();
     return m_currentVideoFrame;
@@ -785,8 +788,10 @@ WebVideoFrame* WebMediaPlayerClientImpl::getCurrentFrame()
 
 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;
     if (m_webMediaPlayer)
index dd170e5..73eb78f 100644 (file)
@@ -197,7 +197,7 @@ private:
     bool acceleratedRenderingInUse();
 #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;
     WebVideoFrame* m_currentVideoFrame;