Bug 96010 - [chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
Summary: [chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentF...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-06 12:58 PDT by Adrienne Walker
Modified: 2012-09-10 11:39 PDT (History)
13 users (show)

See Also:


Attachments
Patch (2.94 KB, patch)
2012-09-06 13:22 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (7.99 KB, patch)
2012-09-06 15:08 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (8.58 KB, patch)
2012-09-07 10:33 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch for landing (8.58 KB, patch)
2012-09-10 10:32 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-09-06 12:58:51 PDT
[chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
Comment 1 Adrienne Walker 2012-09-06 13:22:18 PDT
Created attachment 162565 [details]
Patch
Comment 2 Adrienne Walker 2012-09-06 13:26:31 PDT
See: http://code.google.com/p/chromium/issues/detail?id=146859
Comment 3 James Robinson 2012-09-06 14:47:13 PDT
Comment on attachment 162565 [details]
Patch

Looks good to me. Fingers crossed!
Comment 4 Adrienne Walker 2012-09-06 14:56:15 PDT
I have convinced myself that the client/provider lifetimes and locks are good with this patch, but I'm a little unsure about the interaction between load() on the main thread which creates/destroys m_webMediaPlayer and getCurrentFrame()/putCurrentFrame() which is called from the compositor thread and accesses m_webMediaPlayer.

I'm wondering if we should switch out m_compositorMutex in load()/loadInternal() for this new m_mediaPlayerMutex and also lock it in getCurrentFrame/putCurrentFrame.  I think that would be sufficient.
Comment 5 Daniel Sievers 2012-09-06 15:00:53 PDT
(In reply to comment #4)
> I have convinced myself that the client/provider lifetimes and locks are good with this patch, but I'm a little unsure about the interaction between load() on the main thread which creates/destroys m_webMediaPlayer and getCurrentFrame()/putCurrentFrame() which is called from the compositor thread and accesses m_webMediaPlayer.
> 
> I'm wondering if we should switch out m_compositorMutex in load()/loadInternal() for this new m_mediaPlayerMutex and also lock it in getCurrentFrame/putCurrentFrame.  I think that would be sufficient.

Should setVideoFrameProviderClient() also acquire m_mediaPlayerMutex then? Since it gets called from the impl thread and might race with load.
Comment 6 Adrienne Walker 2012-09-06 15:08:20 PDT
Created attachment 162598 [details]
Patch
Comment 7 WebKit Review Bot 2012-09-06 15:12:58 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 8 Min Qin 2012-09-06 15:18:15 PDT
WebMediaPlayerClientImpl::load() should be called before getCurrentFrame() and PutCurrentFrame() gets called. 
And I think load() cannot be called twice, as HTMLMediaElement will construct a new WebMediaPlayerClientImpl if src changes or media.load() is called again.

(In reply to comment #4)
> I have convinced myself that the client/provider lifetimes and locks are good with this patch, but I'm a little unsure about the interaction between load() on the main thread which creates/destroys m_webMediaPlayer and getCurrentFrame()/putCurrentFrame() which is called from the compositor thread and accesses m_webMediaPlayer.
> 
> I'm wondering if we should switch out m_compositorMutex in load()/loadInternal() for this new m_mediaPlayerMutex and also lock it in getCurrentFrame/putCurrentFrame.  I think that would be sufficient.
Comment 9 Adrienne Walker 2012-09-06 15:19:56 PDT
(In reply to comment #8)
> WebMediaPlayerClientImpl::load() should be called before getCurrentFrame() and PutCurrentFrame() gets called. 
> And I think load() cannot be called twice, as HTMLMediaElement will construct a new WebMediaPlayerClientImpl if src changes or media.load() is called again.

How is the ordering of load() and getCurrentFrame() guaranteed? Aren't they coming from different threads?
Comment 10 Min Qin 2012-09-06 15:26:20 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > WebMediaPlayerClientImpl::load() should be called before getCurrentFrame() and PutCurrentFrame() gets called. 
> > And I think load() cannot be called twice, as HTMLMediaElement will construct a new WebMediaPlayerClientImpl if src changes or media.load() is called again.
> 
> How is the ordering of load() and getCurrentFrame() guaranteed? Aren't they coming from different threads?

VideoLayerChromium will not get created until the first readyStateChanged() gets called. 
That function is called by m_webmediaplayer(), and it has to happen after load()
Comment 11 Daniel Sievers 2012-09-06 17:09:13 PDT
Comment on attachment 162598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162598&action=review

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:770
>      m_videoFrameProviderClient = client;

Maybe also acquire m_webMediaPlayerMutex before accessing m_webMediaPlayer here since this is the other of the 3 VideoFrameProvider APIs being called from the impl thread?
But then scope each of the two locks, so we don't get another deadlock :)
Comment 12 Ami Fischman 2012-09-07 04:36:28 PDT
Comment on attachment 162598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162598&action=review

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:95
> +    MutexLocker locker(m_clientMutex);

Using a lock in a dtor is a pretty strong smell (one I suspect I originally authored in some ancestor of this code; sorry).

If something else can race with this dtor (and thus actually require mutual exclusion), that means a use-after-free is just waiting to happen (when the dtor acquires the lock first and the other task ends up waiting for the dtor to return).
Is this only needed as a fence/barrier, not a mutex?
Comment 13 Adrienne Walker 2012-09-07 09:35:28 PDT
Comment on attachment 162598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162598&action=review

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:95
>> +    MutexLocker locker(m_clientMutex);
> 
> Using a lock in a dtor is a pretty strong smell (one I suspect I originally authored in some ancestor of this code; sorry).
> 
> If something else can race with this dtor (and thus actually require mutual exclusion), that means a use-after-free is just waiting to happen (when the dtor acquires the lock first and the other task ends up waiting for the dtor to return).
> Is this only needed as a fence/barrier, not a mutex?

It's true that this is a bad smell.  The more I think about it, maybe we don't need m_clientMutex at all (at least based on the calling conventions that currently exist).

m_clientMutex right now guards setVideoFrameProviderClient calls from the impl thread and ~WebMediaPlayerClientImpl calls from the main thread.  Right now, setVideoFrameProviderClient only gets called during tree sync when the main thread is blocked.  Arguably, we could insert some asserts here that the main thread is actually blocked and just remove the mutex entirely.
Comment 14 Adrienne Walker 2012-09-07 10:33:19 PDT
Created attachment 162809 [details]
Patch
Comment 15 Min Qin 2012-09-09 10:07:21 PDT
LGTM with 1 nit.

In WebMediaPlayerImplClient.h:.
199	    Mutex m_webMediaPlayerMutex; // Guards the m_webMediaPlayerMutex
should be m_webMediaPlayer
Comment 16 Adrienne Walker 2012-09-10 10:32:35 PDT
Created attachment 163160 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-09-10 11:39:50 PDT
Comment on attachment 163160 [details]
Patch for landing

Clearing flags on attachment: 163160

Committed r128082: <http://trac.webkit.org/changeset/128082>
Comment 18 WebKit Review Bot 2012-09-10 11:39:54 PDT
All reviewed patches have been landed.  Closing bug.