Don't run removeFinishedSourceNodes with the context lock.

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 44c5badf83d62db681b89090707876641dba8236..29b446600e43a8984068cd45b4bb43c42dedf32d 100644
--- a/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
+++ b/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
@@ -633,7 +633,19 @@ void BaseAudioContext::notifySourceNodeFinishedProcessing(
   m_finishedSourceHandlers.push_back(handler);
 }
 
-void BaseAudioContext::removeFinishedSourceNodes() {
+void BaseAudioContext::removeFinishedSourceNodes(bool needsRemoval) {
+  DCHECK(isAudioThread());
+
+  if (needsRemoval) {
+    Platform::current()->mainThread()->getWebTaskRunner()->postTask(
+        BLINK_FROM_HERE,
+        crossThreadBind(
+            &BaseAudioContext::removeFinishedSourceNodesOnMainThread,
+            wrapCrossThreadPersistent(this)));
+  }
+}
+
+void BaseAudioContext::removeFinishedSourceNodesOnMainThread() {
   DCHECK(isMainThread());
   AutoLocker locker(this);
   // Quadratic worst case, but sizes of both vectors are considered
@@ -646,7 +658,7 @@ void BaseAudioContext::removeFinishedSourceNodes() {
   m_finishedSourceNodes.clear();
 }
 
-void BaseAudioContext::releaseFinishedSourceNodes() {
+bool BaseAudioContext::releaseFinishedSourceNodes() {
   ASSERT(isGraphOwner());
   DCHECK(isAudioThread());
   bool didRemove = false;
@@ -662,13 +674,8 @@ void BaseAudioContext::releaseFinishedSourceNodes() {
       }
     }
   }
-  if (didRemove)
-    Platform::current()->mainThread()->getWebTaskRunner()->postTask(
-        BLINK_FROM_HERE,
-        crossThreadBind(&BaseAudioContext::removeFinishedSourceNodes,
-                        wrapCrossThreadPersistent(this)));
-
   m_finishedSourceHandlers.clear();
+  return didRemove;
 }
 
 void BaseAudioContext::notifySourceNodeStartedProcessing(AudioNode* node) {
@@ -741,18 +748,21 @@ void BaseAudioContext::handlePostRenderTasks() {
   // is that there will be some nodes which will take slightly longer than usual
   // to be deleted or removed from the render graph (in which case they'll
   // render silence).
+  bool didRemove = false;
   if (tryLock()) {
     // Take care of AudioNode tasks where the tryLock() failed previously.
     deferredTaskHandler().breakConnections();
 
     // Dynamically clean up nodes which are no longer needed.
-    releaseFinishedSourceNodes();
+    didRemove = releaseFinishedSourceNodes();
 
     deferredTaskHandler().handleDeferredTasks();
     deferredTaskHandler().requestToDeleteHandlersOnMainThread();
 
     unlock();
   }
+
+  removeFinishedSourceNodes(didRemove);

[hongchan, 2017/01/20 23:27:05]

What's the difference between doing the above and this?

if (didRemove)
  removeFinishedSourceNodes();

Furthermore, I am not sure how this change helps in fixing the problem.

[Raymond Toy, 2017/01/21 00:12:15]

Mostly less duplication in the two places that call this.

By moving the remove outside the tryLock, we're not inside the graph lock when
removeFinishedSourceNodesOnMainThread is run which tries to grab the lock on the
main thread.  I think before removeFinishedSourceNodes could run on the main
thread while the tryLock was still active (holding on to the graph lock).  So
two different threads were contending for the same lock, but the calls are in a
cycle.

[hongchan, 2017/01/23 17:38:41]

What I meant by the comment above is: why do we have to pass |didRemove| to the
function?

What's the scope of tryLock() here? Is it inside of the if statement? Or up to
the end of handlePostRenderTasks()?

[Raymond Toy, 2017/01/23 17:45:01]

Just to make the logic of what to do with didRemove the same so the caller
doesn't need to do the check.  I'm fine having the check in the caller or the
callee.
Since there's an unlock() at line 762, the lock ends at the end of the if
(tryLock()) block.

[hongchan, 2017/01/23 18:08:06]

Okay, probably this will be the last question; can we move source nodes handling
logic to DFH? All other post-render tasks are handled by DFH, not the context.

[Raymond Toy, 2017/01/23 18:11:20]

Let me try.  You mean DeferredTaskHandler? (Not sure what DFH is).
 But I don't quite follow the DFH comment since releaseFinishedSourceNodes is in
BaseAudioContext, DeferredTaskHandler.

 }
 
 void BaseAudioContext::resolvePromisesForResumeOnMainThread() {