[chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
Created attachment 162565 [details] Patch
See: http://code.google.com/p/chromium/issues/detail?id=146859
Comment on attachment 162565 [details] Patch Looks good to me. Fingers crossed!
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.
(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.
Created attachment 162598 [details] Patch
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.
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.
(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?
(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 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 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 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.
Created attachment 162809 [details] Patch
LGTM with 1 nit. In WebMediaPlayerImplClient.h:. 199 Mutex m_webMediaPlayerMutex; // Guards the m_webMediaPlayerMutex should be m_webMediaPlayer
Created attachment 163160 [details] Patch for landing
Comment on attachment 163160 [details] Patch for landing Clearing flags on attachment: 163160 Committed r128082: <http://trac.webkit.org/changeset/128082>
All reviewed patches have been landed. Closing bug.