Avoid unsafe heap access from audio thread.

third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp

Index: third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
diff --git a/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp b/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
index 607f2e6c32ad832bcd59b703c02c60340ed66c7a..42c0a9b1df0a1d9955b7db8de559aefc47d80ce4 100644
--- a/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
+++ b/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
@@ -680,6 +680,10 @@ bool BaseAudioContext::ReleaseFinishedSourceNodes() {
   DCHECK(IsAudioThread());
   bool did_remove = false;
   for (AudioHandler* handler : finished_source_handlers_) {
+    // See HandleStoppableSourceNodes() comment for why
+    // this is needed.
+    ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState());

[hongchan, 2017/06/01 16:08:55]

rtoy@ If we have this lock, perhaps we don't need the graph owner lock anymore?

[Raymond Toy, 2017/06/01 18:20:47]

Based on the name of the class, I think we still need the graph owner lock in
general.  Can't have both the audio thread and the main thread modifying the
graph at the same time.  This lock seems to held if GC is happening on the main
thread.

Is that right sof@ and haraken@?

[sof, 2017/06/01 21:10:05]

That's correct, it prevents the audio thread from accessing the heap while the
GC runs (and vice versa), when the main thread isn't touching the audio node
graph. 

[haraken, 2017/06/02 02:42:25]

I'm a bit confused.

Is it possible that a GC runs on the main thread while the audio thread is
holding the graph owner lock?

I guess it would be problematic. The fact that the audio thread is holding the
graph owner lock would mean that it may touch objects shared with the main
thread.

[sof, 2017/06/02 05:39:40]

Yes, that can happen - the lock is over an off-heap graph of nodes, however.

Coordination between main thread (mutator) & audio thread about updating that
node graph is handled by way of that lock. But as the graph is off-heap, the
main thread GC may proceed without knowing anything about that lock while it
operates (which now includes moving objects.)

+
     for (AudioNode* node : active_source_nodes_) {
       if (finished_source_nodes_.Contains(node))
         continue;
@@ -712,8 +716,13 @@ void BaseAudioContext::ReleaseActiveSourceNodes() {
 }
 
 void BaseAudioContext::HandleStoppableSourceNodes() {
+  DCHECK(IsAudioThread());
   DCHECK(IsGraphOwner());
 
+  // A main thread GC must not be running while the audio
+  // thread iterates over the |active_source_nodes_| heap object.
+  ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState());

[Raymond Toy, 2017/06/01 18:20:47]

Is it possible to make this a trylock where we can just return if we can't get
the lock?  It's ok if we don't process the nodes right now in the audio thread. 
There's no requirement that the processing needs to happen right now as long as
it happens eventually.

[sof, 2017/06/01 21:10:05]

I see, thanks - a tryLock() operation could be provided. But, if promptly
stopping isn't vital, would it be plausible to move this active_source_nodes_
iteration to an expanded version of RemoveFinishedSourceNodesOnMainThread()
instead? Thereby being performed by a task on the main thread, which avoids
having to use this GC lock.

+
   // Find AudioBufferSourceNodes to see if we can stop playing them.
   for (AudioNode* node : active_source_nodes_) {
     // If the AudioNode has been marked as finished and released by