[chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame

[Adrienne Walker]

Created attachment 162565 [details]
Patch

[Adrienne Walker]

See: http://code.google.com/p/chromium/issues/detail?id=146859

[James Robinson]

Comment on attachment 162565 [details]
Patch

Looks good to me. Fingers crossed!

[Adrienne Walker]

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.

[Daniel Sievers]

(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.

[Adrienne Walker]

Created attachment 162598 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Min Qin]

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.

[Adrienne Walker]

(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?

[Min Qin]

(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()

[Daniel Sievers]

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 :)

[Ami Fischman]

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?

[Adrienne Walker]

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.

[Adrienne Walker]

Created attachment 162809 [details]
Patch

[Min Qin]

LGTM with 1 nit.

In WebMediaPlayerImplClient.h:.
199	    Mutex m_webMediaPlayerMutex; // Guards the m_webMediaPlayerMutex
should be m_webMediaPlayer

[Adrienne Walker]

Created attachment 163160 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 163160 [details]
Patch for landing

Clearing flags on attachment: 163160

Committed r128082: <http://trac.webkit.org/changeset/128082>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.