Avoid unsafe heap access from audio thread.

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

 /*
  * Copyright (C) 2010, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1.  Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 662 matching lines @@
       active_source_nodes_.erase(i);
   }
   finished_source_nodes_.clear();
 }
 
 bool BaseAudioContext::ReleaseFinishedSourceNodes() {
   DCHECK(IsGraphOwner());
   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;
       if (handler == &node->Handler()) {
         handler->BreakConnection();
         finished_source_nodes_.insert(node);
         did_remove = true;
         break;
       }
     }
@@ skipping 12 matching lines @@
 
 void BaseAudioContext::ReleaseActiveSourceNodes() {
   DCHECK(IsMainThread());
   for (auto& source_node : active_source_nodes_)
     source_node->Handler().BreakConnection();
 
   active_source_nodes_.clear();
 }
 
 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
     // the audio thread, but not yet removed by the main thread
     // (see releaseActiveSourceNodes() above), |node| must not be
     // touched as its handler may have been released already.
     if (finished_source_nodes_.Contains(node))
       continue;
     if (node->Handler().GetNodeType() ==
         AudioHandler::kNodeTypeAudioBufferSource) {
@@ skipping 194 matching lines @@
 }
 
 SecurityOrigin* BaseAudioContext::GetSecurityOrigin() const {
   if (GetExecutionContext())
     return GetExecutionContext()->GetSecurityOrigin();
 
   return nullptr;
 }
 
 }  // namespace blink