Oilpan: WebAudio: Apply the weak HashMap pattern to remove an entry from AudioContext::m_dirtyAudio…

Oilpan: WebAudio: Apply the weak HashMap pattern to remove an entry from AudioContext::m_dirtyAudioSummingJunctions.

r178238, which made AudioSummingJunction::m_context a weak member, didn't
work. If the AudioContext and an AudioSummingJunction object become unreachable
togeter, AudioSummingJunction::m_context is not cleared and their destuction
order is not deterministic. We must not touch weak members in destructors.

With this CL, we register all of AudioSummingJunction objects to AudioContext,
and applies the weak HashMap pattern to clear m_dirtyAudioSummingJunctions.

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

[tkent, 2014-07-18 09:08:06]

Please take a look at this.

[haraken, 2014-07-18 09:49:31]

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:774: AudioContext::AutoLocker
locker(m_junction.context());

Won't this cause a dead lock? removeMarkedSummingJunction also tries to acquire
a lock on the AudioContext.

[tkent, 2014-07-18 09:52:16]

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:774: AudioContext::AutoLocker
locker(m_junction.context());
On 2014/07/18 09:49:30, haraken wrote:
> 
> Won't this cause a dead lock? removeMarkedSummingJunction also tries to
acquire
> a lock on the AudioContext.

No.  AutoLocker supports reentrancy.  If the current thread already has the
lock, AutoLocker doesn't block the thread.

[tkent, 2014-07-18 09:54:22]

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:774: AudioContext::AutoLocker
locker(m_junction.context());
On 2014/07/18 09:52:16, tkent wrote:
> On 2014/07/18 09:49:30, haraken wrote:
> > 
> > Won't this cause a dead lock? removeMarkedSummingJunction also tries to
> acquire
> > a lock on the AudioContext.
> 
> No.  AutoLocker supports reentrancy.  If the current thread already has the
> lock, AutoLocker doesn't block the thread.
> 

Anyway, this AutoLocker is redundant and unnecessary.

[haraken, 2014-07-18 10:01:01]

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:774: AudioContext::AutoLocker
locker(m_junction.context());
On 2014/07/18 09:52:16, tkent wrote:
> On 2014/07/18 09:49:30, haraken wrote:
> > 
> > Won't this cause a dead lock? removeMarkedSummingJunction also tries to
> acquire
> > a lock on the AudioContext.
> 
> No.  AutoLocker supports reentrancy.  If the current thread already has the
> lock, AutoLocker doesn't block the thread.
> 

Makes sense. However, I think you can just drop this AutoLocker, since
removeMarkedSummingJunction always acquires the lock.

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:840: #if !ENABLE(OILPAN)

Probably can we keep this code? In terms of performance, it would be better to
explicitly remove entries rather than relying on weak processing.

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioSummingJunction.cpp (right):

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioSummingJunction.cpp:43:
m_context->registerLiveAudioSummingJunction(*this);

Instead of registering an AudioSummingJunction when it's created, would it be
possible to only register the AudioSummingJunction when
markSummingJunctionDirty() is called?

What m_liveAudioSummingJunctions has to manage is a list of
AudioSummingJunctions that are in m_dirtySummingJunctions (not a list of all
AudioSummingJunctions).

[tkent, 2014-07-18 13:43:09]

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:774: AudioContext::AutoLocker
locker(m_junction.context());
On 2014/07/18 10:01:01, haraken wrote:
> On 2014/07/18 09:52:16, tkent wrote:
> > On 2014/07/18 09:49:30, haraken wrote:
> > > 
> > > Won't this cause a dead lock? removeMarkedSummingJunction also tries to
> > acquire
> > > a lock on the AudioContext.
> > 
> > No.  AutoLocker supports reentrancy.  If the current thread already has the
> > lock, AutoLocker doesn't block the thread.
> > 
> 
> Makes sense. However, I think you can just drop this AutoLocker, since
> removeMarkedSummingJunction always acquires the lock.

Indeed.

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:840: #if !ENABLE(OILPAN)
On 2014/07/18 10:01:00, haraken wrote:
> 
> Probably can we keep this code? In terms of performance, it would be better to
> explicitly remove entries rather than relying on weak processing.

Yeah, It's ok to keep this.

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioSummingJunction.cpp (right):

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioSummingJunction.cpp:43:
m_context->registerLiveAudioSummingJunction(*this);
On 2014/07/18 10:01:01, haraken wrote:
> 
> Instead of registering an AudioSummingJunction when it's created, would it be
> possible to only register the AudioSummingJunction when
> markSummingJunctionDirty() is called?
> 
> What m_liveAudioSummingJunctions has to manage is a list of
> AudioSummingJunctions that are in m_dirtySummingJunctions (not a list of all
> AudioSummingJunctions).

We can't do it.  markSummingJunctionDirty can be called in audio thread, we want
to avoid multi-thread access to m_liveAudioSummingJunctions.

[haraken, 2014-07-18 13:49:01]

LGTM with the nit comments addressed.

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioSummingJunction.cpp (right):

https://codereview.chromium.org/393133003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioSummingJunction.cpp:43:
m_context->registerLiveAudioSummingJunction(*this);
On 2014/07/18 13:43:09, tkent wrote:
> On 2014/07/18 10:01:01, haraken wrote:
> > 
> > Instead of registering an AudioSummingJunction when it's created, would it
be
> > possible to only register the AudioSummingJunction when
> > markSummingJunctionDirty() is called?
> > 
> > What m_liveAudioSummingJunctions has to manage is a list of
> > AudioSummingJunctions that are in m_dirtySummingJunctions (not a list of all
> > AudioSummingJunctions).
> 
> We can't do it.  markSummingJunctionDirty can be called in audio thread, we
want
> to avoid multi-thread access to m_liveAudioSummingJunctions.

Makes sense. Probably we can add a comment and mention that.
m_liveAudioSummingJunctions can contain AudioSummingJunctions that are not in
m_dirtySummingJunctions. Thus we can m_dirtySummingJunctions.remove() for
AudioSummingJunctions that are not contained in the m_dirtySummingJunctions, but
that's no problem.

[tkent, 2014-07-20 00:02:01]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-07-20 00:03:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/393133003/20001

[commit-bot: I haz the power, 2014-07-20 00:05:51]

Change committed as 178521