Don't run removeFinishedSourceNodes with the context lock.

Don't run removeFinishedSourceNodes with the context lock.

releaseFinishedSourceNodes is run from handlePostRenderTask on the
audio thread which has the context lock.  releaseFinishedSourceNodes
can call removeFinishedSourceNodes which runs on the main thread which
will also get the context lock.  Depending on how fast things are,
this second attempt to get the lock may happen while
handlePostRenderTask still has the lock, causing a cycle.

Therefore, move the call to removeFinishedSourceNodes outside the
lock.  This is ok, since removeFinishedSourceNodes doesn't do anything
without acquiring the context lock.

BUG=681316
TEST=test case from the bug passes on a tsan build

Review-Url: https://codereview.chromium.org/2638413004
Cr-Commit-Position: refs/heads/master@{#445467}
Committed: https://chromium.googlesource.com/chromium/src/+/b82438a6b9cf9324f9d2b3780ea270b7bff62a93

[Raymond Toy, 2017-01-18 23:10:11]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 23:10:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raymond Toy, 2017-01-18 23:12:31]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2017-01-18 23:12:32]

PTAL at this preliminary patch.

I think we may want to add another function named, say,
requestRemovalOfFinishedSourceNodesOnMainThread (is that too long) to do the
actual postTask and everything.  Then we can easily add a check that we're not
running with the context lock.

[commit-bot: I haz the power, 2017-01-19 01:25:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 01:25:48]

Dry run: This issue passed the CQ dry run.

[hongchan, 2017-01-19 12:47:43]

https://codereview.chromium.org/2638413004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (left):

https://codereview.chromium.org/2638413004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:671:
m_finishedSourceHandlers.clear();
We don't have this line in the patchset anymore. Is that okay?

[Raymond Toy, 2017-01-20 19:36:10]

https://codereview.chromium.org/2638413004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (left):

https://codereview.chromium.org/2638413004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:671:
m_finishedSourceHandlers.clear();
On 2017/01/19 12:47:43, hongchan wrote:
> We don't have this line in the patchset anymore. Is that okay?

Fixed.

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

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
removeFinishedSourceNodes(didRemove);
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:16]

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
removeFinishedSourceNodes(didRemove);
On 2017/01/20 23:27:05, hongchan wrote:
> 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.

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]

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
removeFinishedSourceNodes(didRemove);
On 2017/01/21 00:12:15, Raymond Toy wrote:
> On 2017/01/20 23:27:05, hongchan wrote:
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
removeFinishedSourceNodes(didRemove);
On 2017/01/23 17:38:41, hongchan wrote:
> On 2017/01/21 00:12:15, Raymond Toy wrote:
> > On 2017/01/20 23:27:05, hongchan wrote:
> > > 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.
> > 
> > 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.
> 
> What I meant by the comment above is: why do we have to pass |didRemove| to
the
> function?

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.

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

Since there's an unlock() at line 762, the lock ends at the end of the if
(tryLock()) block.

[hongchan, 2017-01-23 17:47:52]

On 2017/01/23 17:45:01, Raymond Toy wrote:
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):
> 
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
> removeFinishedSourceNodes(didRemove);
> On 2017/01/23 17:38:41, hongchan wrote:
> > On 2017/01/21 00:12:15, Raymond Toy wrote:
> > > On 2017/01/20 23:27:05, hongchan wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > What I meant by the comment above is: why do we have to pass |didRemove| to
> the
> > function?
> 
> 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.

Then returning to the square one: why do we need another function call only for
postTask()?

 
> > 
> > What's the scope of tryLock() here? Is it inside of the if statement? Or up
to
> > the end of handlePostRenderTasks()?
> 
> Since there's an unlock() at line 762, the lock ends at the end of the if
> (tryLock()) block.

Yes, that's my understanding and I wasn't sure why calling another function
fixes the issue of postTask().

[Raymond Toy, 2017-01-23 17:55:55]

On 2017/01/23 17:47:52, hongchan wrote:
> On 2017/01/23 17:45:01, Raymond Toy wrote:
> >
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
> > removeFinishedSourceNodes(didRemove);
> > On 2017/01/23 17:38:41, hongchan wrote:
> > > On 2017/01/21 00:12:15, Raymond Toy wrote:
> > > > On 2017/01/20 23:27:05, hongchan wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > What I meant by the comment above is: why do we have to pass |didRemove|
to
> > the
> > > function?
> > 
> > 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.
> 
> Then returning to the square one: why do we need another function call only
for
> postTask()?

To make things common truly common.  Otherwise we duplicate the code in two
places, begging the question if they are truly duplicates or not.

> 
>  
> > > 
> > > What's the scope of tryLock() here? Is it inside of the if statement? Or
up
> to
> > > the end of handlePostRenderTasks()?
> > 
> > Since there's an unlock() at line 762, the lock ends at the end of the if
> > (tryLock()) block.
> 
> Yes, that's my understanding and I wasn't sure why calling another function
> fixes the issue of postTask().

Because if we don't, the postTask is called inside the tryLock.  If things are
fast and the other function calls are slow, the postTask could run while the
lock is still held by the audio thread.  Then the main thread will try to grab
the lock that is held by the audio thread.  This could be a potential deadlock.
(Although, I'm not sure that can really happen since the audio thread will
eventually release the lock that it is holding.)

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

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
removeFinishedSourceNodes(didRemove);
On 2017/01/23 17:45:01, Raymond Toy wrote:
> On 2017/01/23 17:38:41, hongchan wrote:
> > On 2017/01/21 00:12:15, Raymond Toy wrote:
> > > On 2017/01/20 23:27:05, hongchan wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > What I meant by the comment above is: why do we have to pass |didRemove| to
> the
> > function?
> 
> 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.
> 
> > 
> > What's the scope of tryLock() here? Is it inside of the if statement? Or up
to
> > the end of handlePostRenderTasks()?
> 
> Since there's an unlock() at line 762, the lock ends at the end of the if
> (tryLock()) block.
> 
> 

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:21]

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
removeFinishedSourceNodes(didRemove);
On 2017/01/23 18:08:06, hongchan wrote:
> On 2017/01/23 17:45:01, Raymond Toy wrote:
> > On 2017/01/23 17:38:41, hongchan wrote:
> > > On 2017/01/21 00:12:15, Raymond Toy wrote:
> > > > On 2017/01/20 23:27:05, hongchan wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > What I meant by the comment above is: why do we have to pass |didRemove|
to
> > the
> > > function?
> > 
> > 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.
> > 
> > > 
> > > What's the scope of tryLock() here? Is it inside of the if statement? Or
up
> to
> > > the end of handlePostRenderTasks()?
> > 
> > Since there's an unlock() at line 762, the lock ends at the end of the if
> > (tryLock()) block.
> > 
> > 
> 
> 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.

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.

[hongchan, 2017-01-23 18:15:21]

On 2017/01/23 18:11:21, Raymond Toy wrote:
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):
> 
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
> removeFinishedSourceNodes(didRemove);
> On 2017/01/23 18:08:06, hongchan wrote:
> > On 2017/01/23 17:45:01, Raymond Toy wrote:
> > > On 2017/01/23 17:38:41, hongchan wrote:
> > > > On 2017/01/21 00:12:15, Raymond Toy wrote:
> > > > > On 2017/01/20 23:27:05, hongchan wrote:
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > What I meant by the comment above is: why do we have to pass |didRemove|
> to
> > > the
> > > > function?
> > > 
> > > 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.
> > > 
> > > > 
> > > > What's the scope of tryLock() here? Is it inside of the if statement? Or
> up
> > to
> > > > the end of handlePostRenderTasks()?
> > > 
> > > Since there's an unlock() at line 762, the lock ends at the end of the if
> > > (tryLock()) block.
> > > 
> > > 
> > 
> > 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.
> 
> 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.

Sorry. Yes, I meant DTH. I am naively asking why 'finished source nodes' got to
stay in the context while other 'deferrable' tasks are in the DTH.

[Raymond Toy, 2017-01-23 18:17:34]

On 2017/01/23 18:15:21, hongchan wrote:
> On 2017/01/23 18:11:21, Raymond Toy wrote:
> >
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
> > removeFinishedSourceNodes(didRemove);
> > On 2017/01/23 18:08:06, hongchan wrote:
> > > On 2017/01/23 17:45:01, Raymond Toy wrote:
> > > > On 2017/01/23 17:38:41, hongchan wrote:
> > > > > On 2017/01/21 00:12:15, Raymond Toy wrote:
> > > > > > On 2017/01/20 23:27:05, hongchan wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > What I meant by the comment above is: why do we have to pass
|didRemove|
> > to
> > > > the
> > > > > function?
> > > > 
> > > > 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.
> > > > 
> > > > > 
> > > > > What's the scope of tryLock() here? Is it inside of the if statement?
Or
> > up
> > > to
> > > > > the end of handlePostRenderTasks()?
> > > > 
> > > > Since there's an unlock() at line 762, the lock ends at the end of the
if
> > > > (tryLock()) block.
> > > > 
> > > > 
> > > 
> > > 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.
> > 
> > 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.
> 
> Sorry. Yes, I meant DTH. I am naively asking why 'finished source nodes' got
to
> stay in the context while other 'deferrable' tasks are in the DTH.

Most likely because the member variables that are needed live in the context not
the handler.  Perhaps these variables don't really make sense in the DTH?

[Raymond Toy, 2017-01-23 18:22:43]

On 2017/01/23 18:17:34, Raymond Toy wrote:
> On 2017/01/23 18:15:21, hongchan wrote:
> > On 2017/01/23 18:11:21, Raymond Toy wrote:
> > >
> >
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2638413004/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765:
> > > removeFinishedSourceNodes(didRemove);
> > > On 2017/01/23 18:08:06, hongchan wrote:
> > > > On 2017/01/23 17:45:01, Raymond Toy wrote:
> > > > > On 2017/01/23 17:38:41, hongchan wrote:
> > > > > > On 2017/01/21 00:12:15, Raymond Toy wrote:
> > > > > > > On 2017/01/20 23:27:05, hongchan wrote:
> > > > > > > > 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.
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > What I meant by the comment above is: why do we have to pass
> |didRemove|
> > > to
> > > > > the
> > > > > > function?
> > > > > 
> > > > > 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.
> > > > > 
> > > > > > 
> > > > > > What's the scope of tryLock() here? Is it inside of the if
statement?
> Or
> > > up
> > > > to
> > > > > > the end of handlePostRenderTasks()?
> > > > > 
> > > > > Since there's an unlock() at line 762, the lock ends at the end of the
> if
> > > > > (tryLock()) block.
> > > > > 
> > > > > 
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > Sorry. Yes, I meant DTH. I am naively asking why 'finished source nodes' got
> to
> > stay in the context while other 'deferrable' tasks are in the DTH.
> 
> Most likely because the member variables that are needed live in the context
not
> the handler.  Perhaps these variables don't really make sense in the DTH?

From a quick offline discussion, moving these things around is a separate issue,
not directly related to fixing the lock order issue.  Thus, that refactoring is
a different CL.

[hongchan, 2017-01-23 18:23:06]

lgtm

[Raymond Toy, 2017-01-23 18:26:41]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-01-23 18:27:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 20:28:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485196001419170,
"parent_rev": "421a2db9f7cb0ff46bd0d1a89f9ab314737b7308", "commit_rev":
"b82438a6b9cf9324f9d2b3780ea270b7bff62a93"}

[commit-bot: I haz the power, 2017-01-23 20:29:12]

Description was changed from

==========
Don't run removeFinishedSourceNodes with the context lock.

releaseFinishedSourceNodes is run from handlePostRenderTask on the
audio thread which has the context lock.  releaseFinishedSourceNodes
can call removeFinishedSourceNodes which runs on the main thread which
will also get the context lock.  Depending on how fast things are,
this second attempt to get the lock may happen while
handlePostRenderTask still has the lock, causing a cycle.

Therefore, move the call to removeFinishedSourceNodes outside the
lock.  This is ok, since removeFinishedSourceNodes doesn't do anything
without acquiring the context lock.

BUG=681316
TEST=test case from the bug passes on a tsan build
==========

to

==========
Don't run removeFinishedSourceNodes with the context lock.

releaseFinishedSourceNodes is run from handlePostRenderTask on the
audio thread which has the context lock.  releaseFinishedSourceNodes
can call removeFinishedSourceNodes which runs on the main thread which
will also get the context lock.  Depending on how fast things are,
this second attempt to get the lock may happen while
handlePostRenderTask still has the lock, causing a cycle.

Therefore, move the call to removeFinishedSourceNodes outside the
lock.  This is ok, since removeFinishedSourceNodes doesn't do anything
without acquiring the context lock.

BUG=681316
TEST=test case from the bug passes on a tsan build

Review-Url: https://codereview.chromium.org/2638413004
Cr-Commit-Position: refs/heads/master@{#445467}
Committed:
https://chromium.googlesource.com/chromium/src/+/b82438a6b9cf9324f9d2b3780ea2...
==========

[commit-bot: I haz the power, 2017-01-23 20:29:13]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b82438a6b9cf9324f9d2b3780ea2...