HTMLMediaElement::clearMediaPlayer should acquire MediaElementAudioSourceNode::lock()

HTMLMediaElement::clearMediaPlayer should acquire MediaElementAudioSourceNode::lock()

Crash report: https://cluster-fuzz.appspot.com/testcase?key=5123869056696320

There is threading race between HTMLMediaElement::clearMediaPlayer and other methods which try to use HTMLMediaElement::m_player. clearMediaPlayer has to acquire a lock before clearing m_player, just like createMediaPlayer is acquiring the lock before clearing m_player.

c.f., https://chromiumcodereview.appspot.com/23691033/ is a CL that added the lock to createMediaPlayer.

BUG=320344
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162730

[haraken, 2013-11-26 12:51:56]

PTAL

[acolwell GONE FROM CHROMIUM, 2013-11-26 16:08:58]

There is a code smell around this locking business that I can't quite put my
finger on.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3400: audioSourceProvider()->setClient(0);
How does this ever get executed? Doesn't audioSourceProvider() return 0 since
m_player is null at this point.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3403: #if ENABLE(WEB_AUDIO)
nit: Remove this line and the #endif right above it.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3721:
audioSourceProvider()->setClient(m_audioSourceNode);
Why don't you need a lock around this call of setClient()?

[DaleCurtis, 2013-11-26 18:36:14]

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3400: audioSourceProvider()->setClient(0);
On 2013/11/26 16:08:58, acolwell wrote:
> How does this ever get executed? Doesn't audioSourceProvider() return 0 since
> m_player is null at this point.

Hah, wow, yeah that's true even in the original CL.  I missed that. The code
should be above the .clear() ? It must introduce enough "hand-waving" to prevent
the race the previous Cl was fixing.

[Ken Russell (switch to Gerrit), 2013-11-26 20:09:24]

Needs another iteration.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3390: m_audioSourceNode->lock();
The manual calls to lock() and unlock() are unfortunate. It would be much better
to use a Locker class like in wtf/Locker.h. Unfortunately, that class deals only
with references, not with pointers that might be NULL.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3400: audioSourceProvider()->setClient(0);
On 2013/11/26 18:36:15, DaleCurtis wrote:
> On 2013/11/26 16:08:58, acolwell wrote:
> > How does this ever get executed? Doesn't audioSourceProvider() return 0
since
> > m_player is null at this point.
> 
> Hah, wow, yeah that's true even in the original CL.  I missed that. The code
> should be above the .clear() ? It must introduce enough "hand-waving" to
prevent
> the race the previous Cl was fixing.

This should be fixed, and further, HTMLMediaElement::~HTMLMediaElement has the
same problem. Please refactor this code to share the m_player.clear() and
audioSourceProvider()->setClient(0) calls between these two methods.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3721:
audioSourceProvider()->setClient(m_audioSourceNode);
On 2013/11/26 16:08:58, acolwell wrote:
> Why don't you need a lock around this call of setClient()?

If the lock above is necessary then it's necessary here too.

[acolwell GONE FROM CHROMIUM, 2013-11-26 20:18:49]

On 2013/11/26 20:09:24, Ken Russell wrote:
> Needs another iteration.
> 
I agree. I think all the locking and such can be contained within the
m_audioSourceNode implementation. Really the node only cares about when the
provider comes and goes so it seems like a setAudioSourceProvider() method on
the node would properly contain all this locking business. I think this also
means that the node wouldn't have to hold a reference to the media element
itself which would help it avoid a race on the
HTMLMediaElement::audioSourceProvider() accessor.

[haraken, 2013-11-27 01:10:28]

PTAL. Addressed comments except for the below comment.

If you want to fix the below comment in this CL, I'm happy to do that, but I
want to understand what kind of class structure you have in mind more
specifically. (Note: This CL is for fixing a release block bug.)

> > Needs another iteration.
> > 
> I agree. I think all the locking and such can be contained within the
> m_audioSourceNode implementation. Really the node only cares about when the
> provider comes and goes so it seems like a setAudioSourceProvider() method on
> the node would properly contain all this locking business. I think this also
> means that the node wouldn't have to hold a reference to the media element
> itself which would help it avoid a race on the
> HTMLMediaElement::audioSourceProvider() accessor.

Agreed, it's ugly to have hand-written locks in HTMLMediaElement.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3400: audioSourceProvider()->setClient(0);
On 2013/11/26 20:09:24, Ken Russell wrote:
> On 2013/11/26 18:36:15, DaleCurtis wrote:
> > On 2013/11/26 16:08:58, acolwell wrote:
> > > How does this ever get executed? Doesn't audioSourceProvider() return 0
> since
> > > m_player is null at this point.
> > 
> > Hah, wow, yeah that's true even in the original CL.  I missed that. The code
> > should be above the .clear() ? It must introduce enough "hand-waving" to
> prevent
> > the race the previous Cl was fixing.
> 
> This should be fixed, and further, HTMLMediaElement::~HTMLMediaElement has the
> same problem. Please refactor this code to share the m_player.clear() and
> audioSourceProvider()->setClient(0) calls between these two methods.

Done.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3403: #if ENABLE(WEB_AUDIO)
On 2013/11/26 16:08:58, acolwell wrote:
> nit: Remove this line and the #endif right above it.

Done.

https://codereview.chromium.org/88183002/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3721:
audioSourceProvider()->setClient(m_audioSourceNode);
On 2013/11/26 20:09:24, Ken Russell wrote:
> On 2013/11/26 16:08:58, acolwell wrote:
> > Why don't you need a lock around this call of setClient()?
> 
> If the lock above is necessary then it's necessary here too.

Done.

[Ken Russell (switch to Gerrit), 2013-11-27 01:13:28]

Thanks for updating the CL. LGTM.

[commit-bot: I haz the power, 2013-11-27 01:15:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/88183002/20001

[commit-bot: I haz the power, 2013-11-27 02:59:19]

Change committed as 162730