Fix threading races on ConvolverNode::m_reverb in ConvolverNode::tailTime()

Fix threading races on ConvolverNode::m_reverb in ConvolverNode::tailTime()

According to the crash report (https://cluster-fuzz.appspot.com/testcase?key=6515787040817152),
ConvolverNode::m_reverb races between ConvolverNode::tailTime() and ConvolverNode::setBuffer().
This CL adds a proper lock for ConvolverNode::m_reverb.

BUG=284785
No tests because the crash depends on threading races and thus not reproducible.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157243

[haraken, 2013-09-04 00:30:32]

abarth: would you take a look? crogers is no longer in the team.

[abarth-chromium, 2013-09-04 00:46:45]

Perhaps Kenneth Russell would be a better reviewer?  I'm not familiar with the
details of how this code works.

[haraken, 2013-09-04 00:47:28]

kbr: would you take a look?

[Raymond Toy, 2013-09-04 16:18:14]

https://codereview.chromium.org/23926002/diff/1/Source/modules/webaudio/Convo...
File Source/modules/webaudio/ConvolverNode.cpp (right):

https://codereview.chromium.org/23926002/diff/1/Source/modules/webaudio/Convo...
Source/modules/webaudio/ConvolverNode.cpp:161: MutexLocker
locker(m_processLock);
I don't think we want to lock this way because tailTime is called in the audio
thread.  I think a tryLock is better.  I guess we can return infinity or some
large value if we can't acquire a lock.

[haraken, 2013-09-04 16:25:03]

On 2013/09/04 16:18:14, rtoy1 wrote:
>
https://codereview.chromium.org/23926002/diff/1/Source/modules/webaudio/Convo...
> File Source/modules/webaudio/ConvolverNode.cpp (right):
> 
>
https://codereview.chromium.org/23926002/diff/1/Source/modules/webaudio/Convo...
> Source/modules/webaudio/ConvolverNode.cpp:161: MutexLocker
> locker(m_processLock);
> I don't think we want to lock this way because tailTime is called in the audio
> thread.  I think a tryLock is better.  I guess we can return infinity or some
> large value if we can't acquire a lock.

Thanks for reviewing! Makes sense. I'll update the CL (and other related CLs)
accordingly.

[haraken, 2013-09-04 16:57:12]

On 2013/09/04 16:25:03, haraken wrote:
> On 2013/09/04 16:18:14, rtoy1 wrote:
> >
>
https://codereview.chromium.org/23926002/diff/1/Source/modules/webaudio/Convo...
> > File Source/modules/webaudio/ConvolverNode.cpp (right):
> > 
> >
>
https://codereview.chromium.org/23926002/diff/1/Source/modules/webaudio/Convo...
> > Source/modules/webaudio/ConvolverNode.cpp:161: MutexLocker
> > locker(m_processLock);
> > I don't think we want to lock this way because tailTime is called in the
audio
> > thread.  I think a tryLock is better.  I guess we can return infinity or
some
> > large value if we can't acquire a lock.
> 
> Thanks for reviewing! Makes sense. I'll update the CL (and other related CLs)
> accordingly.

Fixed. PTAL.

[Raymond Toy (Google), 2013-09-04 17:07:50]

On 2013/09/04 16:57:12, haraken wrote:
> On 2013/09/04 16:25:03, haraken wrote:
> > On 2013/09/04 16:18:14, rtoy1 wrote:
> > >
> >
>
https://codereview.chromium.org/23926002/diff/1/Source/modules/webaudio/Convo...
> > > File Source/modules/webaudio/ConvolverNode.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23926002/diff/1/Source/modules/webaudio/Convo...
> > > Source/modules/webaudio/ConvolverNode.cpp:161: MutexLocker
> > > locker(m_processLock);
> > > I don't think we want to lock this way because tailTime is called in the
> audio
> > > thread.  I think a tryLock is better.  I guess we can return infinity or
> some
> > > large value if we can't acquire a lock.
> > 
> > Thanks for reviewing! Makes sense. I'll update the CL (and other related
CLs)
> > accordingly.
> 
> Fixed. PTAL.

lgtm, but instead of DBL_MAX, perhaps we can use
std::numeric_limits<double>::infinity(), as done in ScriptProcessorNode.  Not
sure which is better.

[Ken Russell (switch to Gerrit), 2013-09-04 18:12:31]

LGTM with rtoy's feedback addressed.

For these simple values I wonder whether it's worth caching them in the
ConvolverNode so that tailTime() and latencyTime() can avoid dereferencing
m_reverb altogether.

[commit-bot: I haz the power, 2013-09-04 19:44:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/23926002/13001

[haraken, 2013-09-04 19:45:42]

Thanks for reviewing! Changed it to std::numeric_limits<double>::infinity().

[commit-bot: I haz the power, 2013-09-04 20:59:05]

Change committed as 157243