Don't run removeFinishedSourceNodes with the context lock.

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 615 matching lines @@
 void BaseAudioContext::notifyStateChange() {
   dispatchEvent(Event::create(EventTypeNames::statechange));
 }
 
 void BaseAudioContext::notifySourceNodeFinishedProcessing(
     AudioHandler* handler) {
   DCHECK(isAudioThread());
   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
   // manageable, especially |m_finishedSourceNodes| is likely to be short.
   for (AudioNode* node : m_finishedSourceNodes) {
     size_t i = m_activeSourceNodes.find(node);
     if (i != kNotFound)
       m_activeSourceNodes.remove(i);
   }
   m_finishedSourceNodes.clear();
 }
 
-void BaseAudioContext::releaseFinishedSourceNodes() {
+bool BaseAudioContext::releaseFinishedSourceNodes() {
   ASSERT(isGraphOwner());
   DCHECK(isAudioThread());
   bool didRemove = false;
   for (AudioHandler* handler : m_finishedSourceHandlers) {
     for (AudioNode* node : m_activeSourceNodes) {
       if (m_finishedSourceNodes.contains(node))
         continue;
       if (handler == &node->handler()) {
         handler->breakConnection();
         m_finishedSourceNodes.add(node);
         didRemove = true;
         break;
       }
     }
   }
-  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) {
   DCHECK(isMainThread());
   AutoLocker locker(this);
 
   m_activeSourceNodes.push_back(node);
   node->handler().makeConnection();
 }
 
@@ skipping 52 matching lines @@
 }
 
 void BaseAudioContext::handlePostRenderTasks() {
   DCHECK(isAudioThread());
 
   // Must use a tryLock() here too.  Don't worry, the lock will very rarely be
   // contended and this method is called frequently.  The worst that can happen
   // 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() {
   DCHECK(isMainThread());
   AutoLocker locker(this);
 
   for (auto& resolver : m_resumeResolvers) {
     if (m_contextState == Closed) {
       resolver->reject(DOMException::create(
           InvalidStateError, "Cannot resume a context that has been closed"));
@@ skipping 126 matching lines @@
 }
 
 SecurityOrigin* BaseAudioContext::getSecurityOrigin() const {
   if (getExecutionContext())
     return getExecutionContext()->getSecurityOrigin();
 
   return nullptr;
 }
 
 }  // namespace blink