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 6bdb579eca2a0cc64a87bfb3f1e8d371cfcb76f3..e85477d35e90aef77fac6846b08a4e8c32e138c9 100644
--- a/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
+++ b/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
@@ -95,6 +95,7 @@ BaseAudioContext::BaseAudioContext(Document* document)
       destination_node_(nullptr),
       is_cleared_(false),
       is_resolving_resume_promises_(false),
+      has_posted_cleanup_task_(false),
       user_gesture_required_(false),
       connection_count_(0),
       deferred_task_handler_(DeferredTaskHandler::Create()),
@@ -759,6 +760,10 @@ bool BaseAudioContext::ReleaseFinishedSourceNodes() {
   DCHECK(IsAudioThread());
   bool did_remove = false;
   for (AudioHandler* handler : finished_source_handlers_) {
+    // 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());

[haraken, 2017/06/08 01:07:55]

Add a TODO to remove this somehow. We should refactor code so that the audio
thread doesn't need to access data structures on Oilpan's heap.

GCLockScope is not nice because the audio thread needs to stop while the main
thread is running a GC. This breaks the realtime-ness of the audio thread.

Also it violates Blink's messaging passing model. In Blink a thread should not
access data structures held by a different thread.

+
     for (AudioNode* node : active_source_nodes_) {
       if (finished_source_nodes_.Contains(node))
         continue;
@@ -791,23 +796,10 @@ void BaseAudioContext::ReleaseActiveSourceNodes() {
 }
 
 void BaseAudioContext::HandleStoppableSourceNodes() {
+  DCHECK(IsAudioThread());
   DCHECK(IsGraphOwner());
 
-  // 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) {
-      AudioBufferSourceNode* source_node =
-          static_cast<AudioBufferSourceNode*>(node);
-      source_node->GetAudioBufferSourceHandler().HandleStoppableSourceNode();
-    }
-  }
+  ScheduleMainThreadCleanup();
 }
 
 void BaseAudioContext::HandlePreRenderTasks(
@@ -861,22 +853,51 @@ void BaseAudioContext::HandlePostRenderTasks() {
   RemoveFinishedSourceNodes(did_remove);
 }
 
-void BaseAudioContext::ResolvePromisesForResumeOnMainThread() {
+void BaseAudioContext::PerformCleanupOnMainThread() {
   DCHECK(IsMainThread());
   AutoLocker locker(this);
 
-  for (auto& resolver : resume_resolvers_) {
-    if (context_state_ == kClosed) {
-      resolver->Reject(DOMException::Create(
-          kInvalidStateError, "Cannot resume a context that has been closed"));
-    } else {
-      SetContextState(kRunning);
-      resolver->Resolve();
+  if (is_resolving_resume_promises_) {
+    for (auto& resolver : resume_resolvers_) {
+      if (context_state_ == kClosed) {
+        resolver->Reject(DOMException::Create(
+            kInvalidStateError,
+            "Cannot resume a context that has been closed"));
+      } else {
+        SetContextState(kRunning);
+        resolver->Resolve();
+      }
     }
+    resume_resolvers_.clear();
+    is_resolving_resume_promises_ = false;
   }
 
-  resume_resolvers_.clear();
-  is_resolving_resume_promises_ = false;
+  // 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) {
+      AudioBufferSourceNode* source_node =
+          static_cast<AudioBufferSourceNode*>(node);
+      source_node->GetAudioBufferSourceHandler().HandleStoppableSourceNode();
+    }
+  }
+  has_posted_cleanup_task_ = false;
+}
+
+void BaseAudioContext::ScheduleMainThreadCleanup() {
+  if (has_posted_cleanup_task_)
+    return;
+  Platform::Current()->MainThread()->GetWebTaskRunner()->PostTask(
+      BLINK_FROM_HERE,
+      CrossThreadBind(&BaseAudioContext::PerformCleanupOnMainThread,
+                      WrapCrossThreadPersistent(this)));
+  has_posted_cleanup_task_ = true;
 }
 
 void BaseAudioContext::ResolvePromisesForResume() {
@@ -890,10 +911,7 @@ void BaseAudioContext::ResolvePromisesForResume() {
   // often and it takes some time to resolve the promises in the main thread.
   if (!is_resolving_resume_promises_ && resume_resolvers_.size() > 0) {
     is_resolving_resume_promises_ = true;
-    Platform::Current()->MainThread()->GetWebTaskRunner()->PostTask(
-        BLINK_FROM_HERE,
-        CrossThreadBind(&BaseAudioContext::ResolvePromisesForResumeOnMainThread,
-                        WrapCrossThreadPersistent(this)));
+    ScheduleMainThreadCleanup();
   }
 }