Fix threading races on WebAudioSourceProviderImpl::provideInput

Fix threading races on WebAudioSourceProviderImpl::provideInput

According to the crash report (https://cluster-fuzz.appspot.com/testcase?key=4697390229487616),
there is a threading race. Specifically, WebAudioSourceProviderImpl can be destructed by the main thread while WebAudioSourceProviderImpl::Stop() is being called by the audio thread.

The core problem is that we're not calling WebAudioSourceProviderImpl::setClient(NULL) when HTMLMediaElement clears the audio source provider.

BUG=284786
No tests because the crash depends on threading races and thus not reproducible.

[haraken, 2013-09-04 01:41:11]

dalecurtis, scherkus: Would you mind taking a look? This is for fixing a P1 bug.

[Ken Russell (switch to Gerrit), 2013-09-04 17:09:35]

https://codereview.chromium.org/23691033/diff/1/content/renderer/media/webaud...
File content/renderer/media/webaudiosourceprovider_impl.cc (right):

https://codereview.chromium.org/23691033/diff/1/content/renderer/media/webaud...
content/renderer/media/webaudiosourceprovider_impl.cc:104: bus_wrapper_->Zero();
This looks wrong. If the trylock fails, then now, bus_wrapper_ might not have
been allocated here.

[DaleCurtis, 2013-09-04 18:31:29]

I don't think this fixes the issue, just a symptom of it. There will still be a
call into provideInput() after destruction has occurred, no?

The real issue seems to be that WebAudioSourceProviderImpl is being destructed
before Stop() has been called on the underlying AudioOutputDevice.

The try lock implementation should be fine because there are never supposed to
be multiple concurrent callers into provideInput(). bus_wrapper_ is allocated
and only used there. The bus is not intended to be protected by the lock, the
lock is there to protect state_ and client_.

[DaleCurtis, 2013-09-04 18:50:56]

Here's an instance of a similar issue crogers@ fixed previously:
https://codereview.chromium.org/15077011

[haraken, 2013-09-04 20:11:14]

On 2013/09/04 18:50:56, DaleCurtis wrote:
> Here's an instance of a similar issue crogers@ fixed previously:
> https://codereview.chromium.org/15077011

hmm, I'm not sure how I can fix the issue.

In WebAudioSourceProviderImpl, there is already an 'AutoLock sink_lock_' that
protects Stop() and provideInput(). This corresponds to the lock that crogers@
implemented in https://codereview.chromium.org/15077011...

[DaleCurtis, 2013-09-04 20:16:26]

Can we require setClient(NULL) to be called before destruction?

[haraken, 2013-09-04 21:45:34]

On 2013/09/04 20:16:26, DaleCurtis wrote:
> Can we require setClient(NULL) to be called before destruction?

Thanks for the advice. Makes sense to me. By calling setClient(NULL), we can
make sure that WebAudioSourceProviderImpl won't be destructed while Stop() is
being called.

Updated the CL. PTAL.

[DaleCurtis, 2013-09-04 21:53:39]

This should work, but may be considered papering over the problem. Should
HTMLMediaElement::clearMediaPlayer() instead call setClient(0) ? 

There's already some special case code in createMediaPlayer():
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[haraken, 2013-09-04 22:26:00]

On 2013/09/04 21:53:39, DaleCurtis wrote:
> This should work, but may be considered papering over the problem. Should
> HTMLMediaElement::clearMediaPlayer() instead call setClient(0) ? 
> 
> There's already some special case code in createMediaPlayer():
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

You're right. Done in the latest patch set.

[DaleCurtis, 2013-09-04 22:33:09]

lgtm % #if protections

https://codereview.chromium.org/23691033/diff/16001/Source/core/html/HTMLMedi...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/23691033/diff/16001/Source/core/html/HTMLMedi...
Source/core/html/HTMLMediaElement.cpp:346: if (audioSourceProvider())
Both of these need protection via #if ENABLE(WEB_AUDIO)

[DaleCurtis, 2013-09-04 22:33:35]

Also description needs an update.

[haraken, 2013-09-04 22:38:32]

On 2013/09/04 22:33:35, DaleCurtis wrote:
> Also description needs an update.

Done. Thanks for all quick reviewing! (I'm not familiar with web audio, so sorry
for the iterative patch sets.)

[Ken Russell (switch to Gerrit), 2013-09-04 22:43:36]

LGTM

[commit-bot: I haz the power, 2013-09-04 22:44:29]

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

[commit-bot: I haz the power, 2013-09-04 22:44:34]

Failed to apply patch for Source/core/html/HTMLMediaElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         Source
  Created missing directory Source.
  A         Source/core
  Created missing directory Source/core.
  A         Source/core/html
  Created missing directory Source/core/html.
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/core/html/HTMLMediaElement.cpp
  |diff --git a/Source/core/html/HTMLMediaElement.cpp
b/Source/core/html/HTMLMediaElement.cpp
  |index
719d388704c908c3a68a8c9e1e54cd295fd7447a..a9b98e35e2ff94a6068766ec05b510b8ab92e4d7
100644
  |--- a/Source/core/html/HTMLMediaElement.cpp
  |+++ b/Source/core/html/HTMLMediaElement.cpp
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       Source/core/html/HTMLMediaElement.cpp
Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp
b/Source/core/html/HTMLMediaElement.cpp
index
719d388704c908c3a68a8c9e1e54cd295fd7447a..a9b98e35e2ff94a6068766ec05b510b8ab92e4d7
100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -343,6 +343,10 @@ HTMLMediaElement::~HTMLMediaElement()
     // See http://crbug.com/233654 for more details.
     m_completelyLoaded = true;
     m_player.clear();
+#if ENABLE(WEB_AUDIO)
+    if (audioSourceProvider())
+        audioSourceProvider()->setClient(0);
+#endif
 }
 
 void HTMLMediaElement::didMoveToNewDocument(Document* oldDocument)
@@ -3454,6 +3458,10 @@ void HTMLMediaElement::clearMediaPlayer(int flags)
     closeMediaSource();
 
     m_player.clear();
+#if ENABLE(WEB_AUDIO)
+    if (audioSourceProvider())
+        audioSourceProvider()->setClient(0);
+#endif
     stopPeriodicTimers();
     m_loadTimer.stop();

[commit-bot: I haz the power, 2013-09-04 22:47:36]

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

[commit-bot: I haz the power, 2013-09-04 22:47:37]

Failed to apply patch for Source/core/html/HTMLMediaElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         Source
  Created missing directory Source.
  A         Source/core
  Created missing directory Source/core.
  A         Source/core/html
  Created missing directory Source/core/html.
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/core/html/HTMLMediaElement.cpp
  |diff --git a/Source/core/html/HTMLMediaElement.cpp
b/Source/core/html/HTMLMediaElement.cpp
  |index
719d388704c908c3a68a8c9e1e54cd295fd7447a..a9b98e35e2ff94a6068766ec05b510b8ab92e4d7
100644
  |--- a/Source/core/html/HTMLMediaElement.cpp
  |+++ b/Source/core/html/HTMLMediaElement.cpp
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       Source/core/html/HTMLMediaElement.cpp
Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp
b/Source/core/html/HTMLMediaElement.cpp
index
719d388704c908c3a68a8c9e1e54cd295fd7447a..a9b98e35e2ff94a6068766ec05b510b8ab92e4d7
100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -343,6 +343,10 @@ HTMLMediaElement::~HTMLMediaElement()
     // See http://crbug.com/233654 for more details.
     m_completelyLoaded = true;
     m_player.clear();
+#if ENABLE(WEB_AUDIO)
+    if (audioSourceProvider())
+        audioSourceProvider()->setClient(0);
+#endif
 }
 
 void HTMLMediaElement::didMoveToNewDocument(Document* oldDocument)
@@ -3454,6 +3458,10 @@ void HTMLMediaElement::clearMediaPlayer(int flags)
     closeMediaSource();
 
     m_player.clear();
+#if ENABLE(WEB_AUDIO)
+    if (audioSourceProvider())
+        audioSourceProvider()->setClient(0);
+#endif
     stopPeriodicTimers();
     m_loadTimer.stop();

[DaleCurtis, 2013-09-04 22:50:30]

Base URL still points to chromium (from the original patch set) rather than
blink; I'm not sure if that's fixable without a new issue.

[haraken, 2013-09-04 22:50:59]

On 2013/09/04 22:50:30, DaleCurtis wrote:
> Base URL still points to chromium (from the original patch set) rather than
> blink; I'm not sure if that's fixable without a new issue.

ok, will upload to a new issue.