Move webaudio to oilpan

Move webaudio to oilpan transitional types

We don't allow on-heap allocations in the audio thread. We change AsyncAudioDecoder::decode so we won't allocate an AudioBuffer in the audio thread.

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

[keishi, 2014-03-19 20:33:44]

https://codereview.chromium.org/205173002/diff/1/Source/modules/webaudio/Asyn...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/1/Source/modules/webaudio/Asyn...
Source/modules/webaudio/AsyncAudioDecoder.cpp:44: m_thread->postTask(new
Task(WTF::bind(&AsyncAudioDecoder::setupAudioDecoderThread)));
Adding this line fixes an assertion failure but causes many tests to timeout.
The tests seem to get stuck in
WebCore::SafePointBarrier::parkOthers(ThreadState.cpp:131).
I'm not sure how to fix this. Maybe I'm not calling ThreadState::detach
properly?

[haraken, 2014-03-19 21:03:47]

Just a quick comment:

WebAudio is tricky in a sense that we're not allowed to stop the audio thread
while doing garbage collection. For that objective, we need to forbid the audio
thread to allocate objects on oilpan's heap; i.e., we need to keep using
reference counting in the audio thread.

This implies that we need to mix reference counting and GC tracing in objects
which are touched by both the audio thread and the main thread. In my
understanding, AudioNode is the only object that has the issue, and we've
already addressed the issue in our experimental branch. So you might want to
take a look at how AudioNode is implemented in the experimental branch and apply
the same trick to this CL.

Mads and Slava should know the issue better and will give you more practical
hints :)

https://codereview.chromium.org/205173002/diff/1/Source/modules/webaudio/Asyn...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/1/Source/modules/webaudio/Asyn...
Source/modules/webaudio/AsyncAudioDecoder.cpp:44: m_thread->postTask(new
Task(WTF::bind(&AsyncAudioDecoder::setupAudioDecoderThread)));
> Adding this line fixes an assertion failure but causes many tests to timeout.
> The tests seem to get stuck in
> WebCore::SafePointBarrier::parkOthers(ThreadState.cpp:131).
> I'm not sure how to fix this. Maybe I'm not calling ThreadState::detach
> properly?

Looks like you're hitting dead lock.

Isn't there any place where the audio thread waits for something to happen? Then
you need to enter a safe point before doing the wait. Otherwise, a dead lock can
happen because other threads wait for the audio thread to enter a safe point but
the audio thread is waiting for something (and thus never enters the safe
point).

See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Vyacheslav Egorov (Google), 2014-03-20 07:06:53]

Haraken is right, we want to avoid attaching the audio thread to the OilPan heap
 because we don't ever want to pause it. Deadlock you are seeing indicates that
we are actually trying to pause it (and points for that are missing somewhere).

[keishi, 2014-03-20 07:30:51]

On 2014/03/20 07:06:53, Vyacheslav Egorov (Google) wrote:
> Haraken is right, we want to avoid attaching the audio thread to the OilPan
heap
>  because we don't ever want to pause it. Deadlock you are seeing indicates
that
> we are actually trying to pause it (and points for that are missing
somewhere).

Thanks. I've changed it so we don't attach the audio decode thread.

I've also moved AudioNodes to the heap using the technique used in the branch.

[Mads Ager (chromium), 2014-03-20 08:29:59]

First round of comments. This is complicated stuff. Thanks for taking it on
Keishi. :)

There are a couple of changes here that do not look oilpan related
(AudioBus/AudioBuffer). Can we avoid doing those as part of this change so we
can focus on the oilpan parts?

https://codereview.chromium.org/205173002/diff/20001/Source/heap/Heap.h
File Source/heap/Heap.h (left):

https://codereview.chromium.org/205173002/diff/20001/Source/heap/Heap.h#oldco...
Source/heap/Heap.h:1020: ASSERT(ptr->hasOneRef());
All ref counted object should start with ref count one and be adopted as the
first thing. Therefore if we get an object that does not have one ref here that
is usually a problem. What are the issues caused by this?

https://codereview.chromium.org/205173002/diff/20001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1148: ASSERT(state);
I would remove this assert. The assert below will cause a null pointer exception
when !state and that should be enough to catch issues.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
false, sampleRate);
This change seems unrelated to Oilpan.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AsyncAudioDecoder.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AsyncAudioDecoder.h:52:
PassRefPtrWillBeRawPtr<AudioBuffer> createAudioBufferFromAudioBus(AudioBus*);
This change seems independent of Oilpan? Can you explain why you need the
restructuring? Maybe you can split that out as a separate change?

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:77: class AudioContext : public
ActiveDOMObject, public ScriptWrappable, public
ThreadSafeRefCountedWillBeRefCountedGarbageCollected<AudioContext>, public
EventTargetWithInlineData {
If this is still ref counted it also still needs to be thread safe. On the
branch we also had a class called ThreadSafeRefCountedGarbageCollected. We need
to move that over as well.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:276:
RefPtrWillBeRawPtr<AudioDestinationNode> m_destinationNode;
If this was a RefPtr before it needs to be a Member now.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:282: // We don't use
RefPtrWillBeRawPtr<AudioNode> here because AudioNode has a more complex ref() /
deref() implementation
I would leave the comment as is.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:53: class AudioNode : public
GarbageCollectedFinalized<AudioNode>, public ScriptWrappable, public
EventTargetWithInlineData {
We don't want to move this into the oilpan heap when !ENABLE(OILPAN), therefore,
this should be public NoBaseWillBeGarbageCollectedFinalized.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:217: OwnPtr<Persistent<AudioNode> >
m_keepAlive;
This should be guarded with #if ENABLE(OILPAN)

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNodeOutput.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNodeOutput.h:143:
PersistentHeapHashSet<Member<AudioParam> > m_params;
Need to use transition types.
WillBePersistentHeapHashSet<RefPtrWillBeMember<AudioParam> >

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioScheduledSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioScheduledSourceNode.h:65:
RefPtrWillBeRawPtr<AudioScheduledSourceNode> m_scheduledNode;
This should be trace. It should be a member. Is NotifyEndedTask stack allocated?
If it is you can add STACK_ALLOCATED() and just make this a RefPtrWillBeMember.

[Mads Ager (chromium), 2014-03-20 08:33:27]

A couple more comments.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/OscillatorNode.cpp (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:121: #if !ENABLE(OILPAN)
We should get rid of this. We don't want to create a new one here every time
with Oilpan. If PeriodicWave is in the heap this just needs to be a static
Persistent to the PeriodicWave which we can then use.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/ScriptProcessorNode.h:81:
PersistentHeapVector<Member<AudioBuffer> > m_inputBuffers;
Transition types.

[sof, 2014-03-22 14:06:13]

Just some code details..

I went through the destructors and they all are either non-problematic or where
they are (AudioSummingJunction), the object isn't garbage collected & finalized.

I couldn't see a pattern to how "heap/Handle.h" was #include'd. You may either
want to consider adding it to all header files that use transition types, or
just include it in the base classes (e.g. AudioNode).

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBuffer.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBuffer.h:44: class AudioBuffer : public
ScriptWrappable, public RefCountedWillBeGarbageCollectedFinalized<AudioBuffer> {
Swap the order of these two? (i.e., I don't think we've dropped the constraint
that the GarbageCollected<> class needs to be leftmost. And it would be
consistent with how we generally order these.)

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioListener.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioListener.h:42: class AudioListener : public
ScriptWrappable, public RefCountedWillBeGarbageCollectedFinalized<AudioListener>
{
Swap order here also?

[haraken, 2014-03-23 14:25:59]

> I couldn't see a pattern to how "heap/Handle.h" was #include'd. You may either
> want to consider adding it to all header files that use transition types, or
> just include it in the base classes (e.g. AudioNode).

Nit: I guess that "if it compiles, it's OK" is our pattern and we're not going
to have a general pattern for heap/Handle.h #include (just like v8.h #include).
Forcing some pattern would just complicate oilpan programming without any
benefit.

[keishi, 2014-03-27 07:39:36]

https://codereview.chromium.org/205173002/diff/20001/Source/heap/Heap.h
File Source/heap/Heap.h (left):

https://codereview.chromium.org/205173002/diff/20001/Source/heap/Heap.h#oldco...
Source/heap/Heap.h:1020: ASSERT(ptr->hasOneRef());
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> All ref counted object should start with ref count one and be adopted as the
> first thing. Therefore if we get an object that does not have one ref here
that
> is usually a problem. What are the issues caused by this?

I was hitting this at one point but the issue seems to have gone away. Reverted.

https://codereview.chromium.org/205173002/diff/20001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1148: ASSERT(state);
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> I would remove this assert. The assert below will cause a null pointer
exception
> when !state and that should be enough to catch issues.

Removed.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
false, sampleRate);
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> This change seems unrelated to Oilpan.

By doing this I am removing allocating garbage collected AudioBuffer inside the
audio decoder thread. Otherwise I was hitting an assert because I haven't called
ThreadState::attach.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBuffer.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBuffer.h:44: class AudioBuffer : public
ScriptWrappable, public RefCountedWillBeGarbageCollectedFinalized<AudioBuffer> {
On 2014/03/22 14:06:13, sof wrote:
> Swap the order of these two? (i.e., I don't think we've dropped the constraint
> that the GarbageCollected<> class needs to be leftmost. And it would be
> consistent with how we generally order these.)

Done.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:77: class AudioContext : public
ActiveDOMObject, public ScriptWrappable, public
ThreadSafeRefCountedWillBeRefCountedGarbageCollected<AudioContext>, public
EventTargetWithInlineData {
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> If this is still ref counted it also still needs to be thread safe. On the
> branch we also had a class called ThreadSafeRefCountedGarbageCollected. We
need
> to move that over as well.

Done.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:276:
RefPtrWillBeRawPtr<AudioDestinationNode> m_destinationNode;
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> If this was a RefPtr before it needs to be a Member now.

Done.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:282: // We don't use
RefPtrWillBeRawPtr<AudioNode> here because AudioNode has a more complex ref() /
deref() implementation
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> I would leave the comment as is.

Done.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioListener.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioListener.h:42: class AudioListener : public
ScriptWrappable, public RefCountedWillBeGarbageCollectedFinalized<AudioListener>
{
On 2014/03/22 14:06:13, sof wrote:
> Swap order here also?

Done.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:53: class AudioNode : public
GarbageCollectedFinalized<AudioNode>, public ScriptWrappable, public
EventTargetWithInlineData {
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> We don't want to move this into the oilpan heap when !ENABLE(OILPAN),
therefore,
> this should be public NoBaseWillBeGarbageCollectedFinalized.

Done.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNodeOutput.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNodeOutput.h:143:
PersistentHeapHashSet<Member<AudioParam> > m_params;
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> Need to use transition types.
> WillBePersistentHeapHashSet<RefPtrWillBeMember<AudioParam> >

Done.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioScheduledSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioScheduledSourceNode.h:65:
RefPtrWillBeRawPtr<AudioScheduledSourceNode> m_scheduledNode;
On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> This should be trace. It should be a member. Is NotifyEndedTask stack
allocated?
> If it is you can add STACK_ALLOCATED() and just make this a
RefPtrWillBeMember.

I don't think this is stack allocated. It is allocated in the AudioContext audio
thread(AudioScheduledSourceNode::finish) and keeps the AudioScheduledSourceNode
alive until it is destroyed in the main
thread(AudioScheduledSourceNode::notifyEndedDispatch).

This is tricky. Still working on this.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/OscillatorNode.cpp (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:121: #if !ENABLE(OILPAN)
On 2014/03/20 08:33:28, Mads Ager (chromium) wrote:
> We should get rid of this. We don't want to create a new one here every time
> with Oilpan. If PeriodicWave is in the heap this just needs to be a static
> Persistent to the PeriodicWave which we can then use.

Done.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/ScriptProcessorNode.h:81:
PersistentHeapVector<Member<AudioBuffer> > m_inputBuffers;
On 2014/03/20 08:33:28, Mads Ager (chromium) wrote:
> Transition types.

Done.

[Mads Ager (chromium), 2014-03-27 11:06:48]

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
false, sampleRate);
On 2014/03/27 07:39:37, keishi1 wrote:
> On 2014/03/20 08:30:00, Mads Ager (chromium) wrote:
> > This change seems unrelated to Oilpan.
> 
> By doing this I am removing allocating garbage collected AudioBuffer inside
the
> audio decoder thread. Otherwise I was hitting an assert because I haven't
called
> ThreadState::attach.

OK, that means that we will have to keep AudioBus off-heap which probably makes
sense. Is the idea here to keep all the platform objects off-heap and move the
modules objects into the heap?

I think it would make sense to do this change as a separate small patch first
and get Raymond's input on this change in isolation.

https://codereview.chromium.org/205173002/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AsyncAudioDecoder.cpp:83:
RefPtrWillBeRawPtr<AudioBuffer> audioBuffer =
AudioBuffer::createFromAudioBus(audioBus);
Can we add ASSERT(isMainThread()) here?

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Handle.h#new...
Source/heap/Handle.h:679: #define
ThreadSafeRefCountedWillBeRefCountedGarbageCollected
WebCore::RefCountedGarbageCollected
Let's remove these macros. It seems dangerous to use them and I don't think your
current patch set uses them?

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Handle.h#new...
Source/heap/Handle.h:758: #define
ThreadSafeRefCountedWillBeRefCountedGarbageCollected WTF::ThreadSafeRefCounted
And remove this one too.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:548: class ThreadSafeRefCountedGarbageCollected : public
WTF::ThreadSafeRefCountedBase, public GarbageCollectedFinalized<T> {
I think it was like this on the branch as well, but could you swap the order of
the super classes so we have GarbageCollectedFinalized first? :)

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1073: int refCount()
Do we need this? When something is RefCountedGarbageCollected that usually means
that we will start moving away from RefPtr to Member/Persistent. At that point
all asserts about refCount() are wrong. So I would prefer not to add this method
if we can avoid it?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:912: #endif
You need to keep the code for the non-oilpan version here.

#else
    // Finally, delete it.
    delete node;
#endif

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:278:
RefPtrWillBeRawPtr<AudioDestinationNode> m_destinationNode;
This should be RefPtrWillBeMember and it should be traced in the trace method.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioProcessingEvent.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioProcessingEvent.h:55:
RefPtrWillBePersistent<AudioBuffer> m_inputBuffer;
With Haraken's change to make Event garbage collected again you should make
these Members and trace them in the trace method.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioScheduledSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioScheduledSourceNode.h:65:
RefPtrWillBeRawPtr<AudioScheduledSourceNode> m_scheduledNode;
I'm not entirely sure, but it looks like this object is create on the audio
thread and then passed over to the main thread. In that case we should probably
just leave this a RefPtr to the AudioNode? The alternative would be to use a
CrossThreadPersistent that we should be able to allocate on any thread and
deallocate on any thread.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/OfflineAudioCompletionEvent.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OfflineAudioCompletionEvent.h:54:
RefPtrWillBePersistent<AudioBuffer> m_renderedBuffer;
Member and trace once you merge with Haraken's change to make Event garbage
collected again.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/OscillatorNode.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:370: AudioNode::trace(visitor);
We should call the immediate super class trace here:
AudioScheduledSourceNode::trace(visitor)

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/ScriptProcessorNode.h:81:
WillBePersistentHeapVector<RefPtrWillBeMember<AudioBuffer> > m_inputBuffers;
This is an AudioNode and is therefore in the heap. You should just use
WillBeHeapVector<RefPtrWillBeMember<AudioBuffer> > here and you should then
implement a trace method that traces m_inputBuffers and m_outputBuffers.

[haraken, 2014-03-27 11:44:04]

This is a complicated CL.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:542: // thread safe.

Probably you can eliminate this restriction by using a CrossThreadPersistent for
the keep alive handle?

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:543: // This is currently used by the WebAudio code and the
WebDatabase code.

Remove the "WebDatabase code" part. I think tkent-san has fixed the issue in the
database thread.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:581: if (derefBase()) {

I suspect this leaks memory, because currently derefBase() does not decrement
the refcount from 1 to 0. I guess you need to make a change to
RefCounted::derefBase() so that it does the decrement.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:583: m_keepAlive = 0;

m_keepAlive = nullptr;

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:594: Persistent<T>* m_keepAlive;

Shall we use an OwnPtr<Persistent<T> > ?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AnalyserNode.idl (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AnalyserNode.idl:26: WillBeGarbageCollected,

[WillBeGarbageCollected] is inherited from AudioNode, so you don't need to add
it here.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
false, sampleRate);

What is this change for?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:483: void
AudioBufferSourceNode::clearPannerNode()

If you move m_pannerNode to RawPtrWillBeMember<>, you need to be careful about
what clearPannerNode() does because clearPannerNode() is called in
~AudioBufferSourceNode().

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:503:
AudioNode::trace(visitor);

AudioScheduledSourceNode::trace(visitor) ?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.h:142: PannerNode* m_pannerNode;

I think PannerNode and AudioBufferSourceNode should die together. If so, you
need to change this to a RawPtrWillBeMember<PannerNode>.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:912: #endif

You'll need to call 'delete node' in #if !ENABLE(OILPAN).

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:278:
RefPtrWillBeRawPtr<AudioDestinationNode> m_destinationNode;

Shouldn't this be a RefPtrWillBeMember? If no, I want to have a comment about
why it needs to be a RawPtr.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:287: Vector<AudioNode*>
m_referencedNodes;

AudioContext has a bunch of collections of AudioNode*. Are you sure that these
raw pointers are safe?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioDestinationNode.idl (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioDestinationNode.idl:26: WillBeGarbageCollected,

You don't need this.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:53: class AudioNode : public
NoBaseWillBeGarbageCollectedFinalized<AudioNode>, public ScriptWrappable, public
EventTargetWithInlineData {

Shouldn't this be NoBaseWillBeRefCountedGarbageCollected because AudioNode
inherits EventTarget? Actually I don't understand why this CL compiles with the
AudioNode being NoBaseWillBeGarbageCollectedFinalized.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:213: // until the reference count reaches
zero.

If this statement is true, shouldn't we add an OwnPtr<Persistent<AudioNode>> to
ScriptProcessorNode instead of adding a keep alive handle to AudioNode?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:219: // will not go above zero again.

Can we add an ASSERT about the fact?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNodeOutput.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNodeOutput.h:144: typedef
WillBePersistentHeapHashSet<RefPtrWillBeMember<AudioParam> >::iterator
ParamsIterator;

It looks like this is unused. You can remove this typedef.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioProcessingEvent.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioProcessingEvent.h:56:
RefPtrWillBePersistent<AudioBuffer> m_outputBuffer;

These should be RefPtrWillBeMembers, and needs to be traced.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:208:
event->setTarget(adoptRefWillBeNoop(m_scheduledNode.get()));

Why do you need to call adoptRef?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioScheduledSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioScheduledSourceNode.h:65:
RefPtrWillBeRawPtr<AudioScheduledSourceNode> m_scheduledNode;

Shouldn't this be RefPtrWillBePersistent ? Otherwise, I guess m_scheduledNode
will be prematurely collected.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/ConvolverNode.idl (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/ConvolverNode.idl:27: WillBeGarbageCollected,

Remove. The same comment for other IDL files.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/MediaStreamAudioSourceNode.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/MediaStreamAudioSourceNode.cpp:61: uninitialize();

Not related to your CL, it looks like that all subclasses of AudioNode are
calling uninitialize(). How about calling that in ~AudioNode()?

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/OscillatorNode.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:125: periodicWave =
PeriodicWave::createSine(sampleRate);

You need to store the created pointer into a static persistent handle.
Otherwise, it will leak.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:134: Persistent<PeriodicWave>
periodicWaveSquare = PeriodicWave::createSquare(sampleRate);

Use DEFINE_STATIC_LOCAL.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:144: Persistent<PeriodicWave>
periodicWaveSawtooth = PeriodicWave::createSawtooth(sampleRate);

Ditto.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:154: Persistent<PeriodicWave>
periodicWaveTriangle = PeriodicWave::createTriangle(sampleRate);

Ditto.

Probably it might be worth introducing DEFINE_STATIC_REF_WILL_BE_PERSISTENT.
It's better to use #if ENABLE_OILPAN flag. We have this patter in several places
in the code base.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/PeriodicWave.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/PeriodicWave.h:43: class PeriodicWave : public
ScriptWrappable, public RefCountedWillBeGarbageCollectedFinalized<PeriodicWave>
{

You need to put RefCountedWillBeGarbageCollected in the left-most class.

[haraken, 2014-03-27 11:49:44]

Actually I don't fully understand what objects are touched by the audio thread
and thus need to be kept being RefCounted. I'd be happy if you could add the
explanation to the CL description. I'd like to understand a list of objects that
need non-trivial handling (e.g., objects created in the main thread and passed
to the audio thread etc).

[Raymond Toy, 2014-03-27 17:39:01]

On 2014/03/27 11:49:44, haraken wrote:
> Actually I don't fully understand what objects are touched by the audio thread
> and thus need to be kept being RefCounted. I'd be happy if you could add the
> explanation to the CL description. I'd like to understand a list of objects
that
> need non-trivial handling (e.g., objects created in the main thread and passed
> to the audio thread etc).

I believe the only objects that are created in the main thread are the ones
listed in AudioContext.idl with the names create*.  Any other objects are
internal to the webaudio implementation.

[haraken, 2014-03-28 00:48:43]

On 2014/03/27 17:39:01, Raymond Toy wrote:
> On 2014/03/27 11:49:44, haraken wrote:
> > Actually I don't fully understand what objects are touched by the audio
thread
> > and thus need to be kept being RefCounted. I'd be happy if you could add the
> > explanation to the CL description. I'd like to understand a list of objects
> that
> > need non-trivial handling (e.g., objects created in the main thread and
passed
> > to the audio thread etc).
> 
> I believe the only objects that are created in the main thread are the ones
> listed in AudioContext.idl with the names create*.  Any other objects are
> internal to the webaudio implementation.

This is an easy model to understand :) Thanks for making it clear!

[keishi, 2014-04-03 06:53:18]

WIP. Changing AudioNode to RefCountedGarbageCollected has caused many tests to
fail and I'm trying to figure out why.

I'm getting this error when using the gc plugin. How do I silence this?

../../third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:47:1:
error: [blink-gc] Class 'ScriptProcessorNode' contains GC root in field
'm_keepAlive'.
class ScriptProcessorNode FINAL : public AudioNode {
^
../../third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:111:5:
note: [blink-gc] Field 'm_keepAlive' defining a GC root declared here:
    OwnPtr<Persistent<AudioNode> > m_keepAlive;
    ^
1 error generated.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Handle.h#new...
Source/heap/Handle.h:679: #define
ThreadSafeRefCountedWillBeRefCountedGarbageCollected
WebCore::RefCountedGarbageCollected
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> Let's remove these macros. It seems dangerous to use them and I don't think
your
> current patch set uses them?

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Handle.h#new...
Source/heap/Handle.h:758: #define
ThreadSafeRefCountedWillBeRefCountedGarbageCollected WTF::ThreadSafeRefCounted
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> And remove this one too.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:542: // thread safe.
On 2014/03/27 11:44:05, haraken wrote:
> 
> Probably you can eliminate this restriction by using a CrossThreadPersistent
for
> the keep alive handle?

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:543: // This is currently used by the WebAudio code and the
WebDatabase code.
On 2014/03/27 11:44:05, haraken wrote:
> 
> Remove the "WebDatabase code" part. I think tkent-san has fixed the issue in
the
> database thread.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:548: class ThreadSafeRefCountedGarbageCollected : public
WTF::ThreadSafeRefCountedBase, public GarbageCollectedFinalized<T> {
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> I think it was like this on the branch as well, but could you swap the order
of
> the super classes so we have GarbageCollectedFinalized first? :)

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:581: if (derefBase()) {
On 2014/03/27 11:44:05, haraken wrote:
> 
> I suspect this leaks memory, because currently derefBase() does not decrement
> the refcount from 1 to 0. I guess you need to make a change to
> RefCounted::derefBase() so that it does the decrement.

haraken fixed this in r170256

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:583: m_keepAlive = 0;
On 2014/03/27 11:44:05, haraken wrote:
> 
> m_keepAlive = nullptr;

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:594: Persistent<T>* m_keepAlive;
On 2014/03/27 11:44:05, haraken wrote:
> 
> Shall we use an OwnPtr<Persistent<T> > ?

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/heap/Heap.h#newco...
Source/heap/Heap.h:1073: int refCount()
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> Do we need this? When something is RefCountedGarbageCollected that usually
means
> that we will start moving away from RefPtr to Member/Persistent. At that point
> all asserts about refCount() are wrong. So I would prefer not to add this
method
> if we can avoid it?

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AnalyserNode.idl (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AnalyserNode.idl:26: WillBeGarbageCollected,
On 2014/03/27 11:44:05, haraken wrote:
> 
> [WillBeGarbageCollected] is inherited from AudioNode, so you don't need to add
> it here.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
false, sampleRate);
On 2014/03/27 11:44:05, haraken wrote:
> 
> What is this change for?

By doing this I am removing allocating garbage collected AudioBuffer inside the
audio decoder thread. Otherwise I was hitting an assert because I haven't called
ThreadState::attach.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:503:
AudioNode::trace(visitor);
On 2014/03/27 11:44:05, haraken wrote:
> 
> AudioScheduledSourceNode::trace(visitor) ?

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:912: #endif
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> You need to keep the code for the non-oilpan version here.
> 
> #else
>     // Finally, delete it.
>     delete node;
> #endif

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:278:
RefPtrWillBeRawPtr<AudioDestinationNode> m_destinationNode;
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> This should be RefPtrWillBeMember and it should be traced in the trace method.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioDestinationNode.idl (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioDestinationNode.idl:26: WillBeGarbageCollected,
On 2014/03/27 11:44:05, haraken wrote:
> 
> You don't need this.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:53: class AudioNode : public
NoBaseWillBeGarbageCollectedFinalized<AudioNode>, public ScriptWrappable, public
EventTargetWithInlineData {
On 2014/03/27 11:44:05, haraken wrote:
> 
> Shouldn't this be NoBaseWillBeRefCountedGarbageCollected because AudioNode
> inherits EventTarget? Actually I don't understand why this CL compiles with
the
> AudioNode being NoBaseWillBeGarbageCollectedFinalized.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:213: // until the reference count reaches
zero.
On 2014/03/27 11:44:05, haraken wrote:
> 
> If this statement is true, shouldn't we add an OwnPtr<Persistent<AudioNode>>
to
> ScriptProcessorNode instead of adding a keep alive handle to AudioNode?

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNodeOutput.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNodeOutput.h:144: typedef
WillBePersistentHeapHashSet<RefPtrWillBeMember<AudioParam> >::iterator
ParamsIterator;
On 2014/03/27 11:44:05, haraken wrote:
> 
> It looks like this is unused. You can remove this typedef.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioProcessingEvent.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioProcessingEvent.h:55:
RefPtrWillBePersistent<AudioBuffer> m_inputBuffer;
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> With Haraken's change to make Event garbage collected again you should make
> these Members and trace them in the trace method.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioScheduledSourceNode.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioScheduledSourceNode.cpp:208:
event->setTarget(adoptRefWillBeNoop(m_scheduledNode.get()));
On 2014/03/27 11:44:05, haraken wrote:
> 
> Why do you need to call adoptRef?

My mistake. removed.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioScheduledSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioScheduledSourceNode.h:65:
RefPtrWillBeRawPtr<AudioScheduledSourceNode> m_scheduledNode;
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> I'm not entirely sure, but it looks like this object is create on the audio
> thread and then passed over to the main thread. In that case we should
probably
> just leave this a RefPtr to the AudioNode? The alternative would be to use a
> CrossThreadPersistent that we should be able to allocate on any thread and
> deallocate on any thread.

yes, this is created on the render thread and passed to the main thread. I used
a RefPtr

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/ConvolverNode.idl (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/ConvolverNode.idl:27: WillBeGarbageCollected,
On 2014/03/27 11:44:05, haraken wrote:
> 
> Remove. The same comment for other IDL files.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/OfflineAudioCompletionEvent.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OfflineAudioCompletionEvent.h:54:
RefPtrWillBePersistent<AudioBuffer> m_renderedBuffer;
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> Member and trace once you merge with Haraken's change to make Event garbage
> collected again.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/OscillatorNode.cpp (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:125: periodicWave =
PeriodicWave::createSine(sampleRate);
On 2014/03/27 11:44:05, haraken wrote:
> 
> You need to store the created pointer into a static persistent handle.
> Otherwise, it will leak.

Done. Created DEFINE_STATIC_REF_WILL_BE_PERSISTENT

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:134: Persistent<PeriodicWave>
periodicWaveSquare = PeriodicWave::createSquare(sampleRate);
On 2014/03/27 11:44:05, haraken wrote:
> 
> Use DEFINE_STATIC_LOCAL.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:144: Persistent<PeriodicWave>
periodicWaveSawtooth = PeriodicWave::createSawtooth(sampleRate);
On 2014/03/27 11:44:05, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:154: Persistent<PeriodicWave>
periodicWaveTriangle = PeriodicWave::createTriangle(sampleRate);
On 2014/03/27 11:44:05, haraken wrote:
> 
> Ditto.
> 
> Probably it might be worth introducing DEFINE_STATIC_REF_WILL_BE_PERSISTENT.
> It's better to use #if ENABLE_OILPAN flag. We have this patter in several
places
> in the code base.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/OscillatorNode.cpp:370: AudioNode::trace(visitor);
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> We should call the immediate super class trace here:
> AudioScheduledSourceNode::trace(visitor)

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/PeriodicWave.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/PeriodicWave.h:43: class PeriodicWave : public
ScriptWrappable, public RefCountedWillBeGarbageCollectedFinalized<PeriodicWave>
{
On 2014/03/27 11:44:05, haraken wrote:
> 
> You need to put RefCountedWillBeGarbageCollected in the left-most class.

Done.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/ScriptProcessorNode.h:81:
WillBePersistentHeapVector<RefPtrWillBeMember<AudioBuffer> > m_inputBuffers;
On 2014/03/27 11:06:49, Mads Ager (chromium) wrote:
> This is an AudioNode and is therefore in the heap. You should just use
> WillBeHeapVector<RefPtrWillBeMember<AudioBuffer> > here and you should then
> implement a trace method that traces m_inputBuffers and m_outputBuffers.

Done.

[haraken, 2014-04-03 06:55:35]

On 2014/04/03 06:53:18, keishi1 wrote:
> WIP. Changing AudioNode to RefCountedGarbageCollected has caused many tests to
> fail and I'm trying to figure out why.
> 
> I'm getting this error when using the gc plugin. How do I silence this?
> 
> ../../third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:47:1:
> error: [blink-gc] Class 'ScriptProcessorNode' contains GC root in field
> 'm_keepAlive'.
> class ScriptProcessorNode FINAL : public AudioNode {
> ^
> ../../third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:111:5:
> note: [blink-gc] Field 'm_keepAlive' defining a GC root declared here:
>     OwnPtr<Persistent<AudioNode> > m_keepAlive;
>     ^
> 1 error generated.

Can you add:

GC_PLUGIN_IGNORE("http://crbug.com/353083")

?

[Mads Ager (chromium), 2014-04-03 06:59:52]

On Thu, Apr 3, 2014 at 8:55 AM, <haraken@chromium.org> wrote:

> On 2014/04/03 06:53:18, keishi1 wrote:
>
>> WIP. Changing AudioNode to RefCountedGarbageCollected has caused many
>> tests to
>> fail and I'm trying to figure out why.
>>
>
>  I'm getting this error when using the gc plugin. How do I silence this?
>>
>
>  ../../third_party/WebKit/Source/modules/webaudio/
>> ScriptProcessorNode.h:47:1:
>> error: [blink-gc] Class 'ScriptProcessorNode' contains GC root in field
>> 'm_keepAlive'.
>> class ScriptProcessorNode FINAL : public AudioNode {
>> ^
>> ../../third_party/WebKit/Source/modules/webaudio/
>> ScriptProcessorNode.h:111:5:
>> note: [blink-gc] Field 'm_keepAlive' defining a GC root declared here:
>>      OwnPtr<Persistent<AudioNode> > m_keepAlive;
>>      ^
>> 1 error generated.
>>
>
> Can you add:
>
> GC_PLUGIN_IGNORE("http://crbug.com/353083")
>

You probably shouldn't do that. The ScriptProcessorNode is now in the heap
and should not have a persistent here. It should be a member and be traced.
I'll have a closer look after our meeting in a couple of minutes. :)

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Mads Ager (chromium), 2014-04-03 08:39:32]

https://codereview.chromium.org/205173002/diff/120001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/120001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.h:53: class AudioNode : public
NoBaseWillBeRefCountedGarbageCollected<AudioNode>, public ScriptWrappable,
public EventTargetWithInlineData {
I don't think we want this to be RefCountedGarbageCollected, that will cause the
wrong type of reference counting to be used. This is not RefCounted currently,
and we shouldn't make it RefCounted this way. It is using ref counting but
manages itself. You need to go back to NoBaseWillBeGarbageCollectedFinalized
here.

https://codereview.chromium.org/205173002/diff/120001/Source/modules/webaudio...
File Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/205173002/diff/120001/Source/modules/webaudio...
Source/modules/webaudio/ScriptProcessorNode.h:111: OwnPtr<Persistent<AudioNode>
> m_keepAlive;
This has to be in AudioNode. When the ref count in AudioNode is larger than 0 it
should have this keep alive. When it is 0 the keep alive should be deallocated.
Once this is in AudioNode you can use the GC_PLUGIN_IGNORE to get the plugin to
allow it.

[keishi, 2014-04-08 02:33:39]

Thanks. All webaudi/ tests are passing locally now so please take a look.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.h:142: PannerNode* m_pannerNode;
On 2014/03/27 11:44:05, haraken wrote:
> 
> I think PannerNode and AudioBufferSourceNode should die together. If so, you
> need to change this to a RawPtrWillBeMember<PannerNode>.

AudioNodes use their own ref counting so I don't think we need to do this.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.h:287: Vector<AudioNode*>
m_referencedNodes;
On 2014/03/27 11:44:05, haraken wrote:
> 
> AudioContext has a bunch of collections of AudioNode*. Are you sure that these
> raw pointers are safe?

I think this is safe because this keeps a set of AudioNodes that the
AudioContext has a RefTypeConnection reference to.

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/80001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNode.h:219: // will not go above zero again.
On 2014/03/27 11:44:05, haraken wrote:
> 
> Can we add an ASSERT about the fact?

Done. Added an ASSERT to AudioNode::ref

https://codereview.chromium.org/205173002/diff/120001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/120001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.h:53: class AudioNode : public
NoBaseWillBeRefCountedGarbageCollected<AudioNode>, public ScriptWrappable,
public EventTargetWithInlineData {
On 2014/04/03 08:39:33, Mads Ager (chromium) wrote:
> I don't think we want this to be RefCountedGarbageCollected, that will cause
the
> wrong type of reference counting to be used. This is not RefCounted currently,
> and we shouldn't make it RefCounted this way. It is using ref counting but
> manages itself. You need to go back to NoBaseWillBeGarbageCollectedFinalized
> here.

Done.

https://codereview.chromium.org/205173002/diff/120001/Source/modules/webaudio...
File Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/205173002/diff/120001/Source/modules/webaudio...
Source/modules/webaudio/ScriptProcessorNode.h:111: OwnPtr<Persistent<AudioNode>
> m_keepAlive;
On 2014/04/03 08:39:33, Mads Ager (chromium) wrote:
> This has to be in AudioNode. When the ref count in AudioNode is larger than 0
it
> should have this keep alive. When it is 0 the keep alive should be
deallocated.
> Once this is in AudioNode you can use the GC_PLUGIN_IGNORE to get the plugin
to
> allow it.

Done.

[haraken, 2014-04-08 02:45:50]

> Thanks. All webaudi/ tests are passing locally now so please take a look.

Sounds great! I'll take a look at this CL and the mediastream CL after lunch.

[haraken, 2014-04-08 06:02:07]

Thanks for working on this complicated piece. I think we're getting close.

One thing you might want to try is to run layout tests with your CL with ASAN
enabled. ASAN can detect errors such as touching on-heap objects inside
destructors etc.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
false, sampleRate);

> By doing this I am removing allocating garbage collected AudioBuffer inside
the
> audio decoder thread. Otherwise I was hitting an assert because I haven't
called
> ThreadState::attach.

I don't quite understand this. This change is just changing
createFromAudioFileData to createBusFromInMemoryAudioFile, and I'm not sure how
it is resolving the threading issue. Would you elaborate on it?

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.h:28: #include "heap/Handle.h"

platform/heap/Handle.h. The same comment for other places in this CL.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBufferSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.h:141: // the pitch rate. We
manually manage ref-counting because we want to use RefTypeConnection.

Let me double-confirm: It is safe to keep raw pointers from the AudioNode
hierarchy to the AudioNode hierarchy, because the lifetime of the raw pointers
are cared by RefTypeConnection, right?

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:77: class AudioContext : public
ThreadSafeRefCountedWillBeThreadSafeRefCountedGarbageCollected<AudioContext>,
public ActiveDOMObject, public ScriptWrappable, public EventTargetWithInlineData
{

Just to confirm: The fact that this class is ThreadSafeRefCounted means that
this class can be touched by the audio thread (which is not attached to oilpan).
However, it is no problem as long as the audio thread does not allocate the
AudioContext (i.e., the audio thread cannot allocate the AudioContext, but can
touch the AudioContext). Am I understanding correctly?

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:303: HashSet<AudioNodeOutput*>
m_dirtyAudioNodeOutputs;

At this point AudioSummingJunction and AudioNodeOutput are off-heap, and thus it
makes sense to keep using raw pointers for these. However, when we move
AudioSummingJunction and AudioNodeOutput to the heap, we need to change these
raw pointers with Members. If we forget to change them, is there any way to
notice the error? (Can the plugin detect the error?)

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.cpp:479: ASSERT(m_keepAlive);

Add #if ENABLE(OILPAN)

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.h:53: class AudioNode : public
NoBaseWillBeGarbageCollectedFinalized<AudioNode>, public ScriptWrappable, public
EventTargetWithInlineData {

Add a comment about why this doesn't need to be RefCountedGarbageCollected.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.h:213: // until the reference count reaches
zero.

EventTarget is not yet on-heap, so this comment doesn't make much sense. The
comment could be something like:

"AudioNodes are in the oilpan heap but they are still reference counted at the
same time. This is because we are not allowed to stop the audio thread and thus
the audio thread cannot allocate objects in the oilpan heap.

The m_keepAlive handle is used to keep a persistent reference to this AudioNode
while someone has a reference to this AudioNode through a RefPtr."

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.h:219: // will not go above zero again.

I'd put this comment in clearKeepAlive() in the cpp file.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioParam.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioParam.h:46: class AudioParam FINAL : public
RefCountedWillBeGarbageCollectedFinalized<AudioParam>, public ScriptWrappable,
public AudioSummingJunction {

You might want to move AudioSummingJunction to the heap in a follow-up CL.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioSourceNode.idl (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioSourceNode.idl:30: WillBeGarbageCollected,

You don't need [WillBeGarbageCollected] in sub-classes of AudioNode. The same
comment in other places in this CL.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/MediaStreamAudioDestinationNode.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/MediaStreamAudioDestinationNode.h:60:
RefPtr<MediaStream> m_stream;

You'll need to change this to RefPtrWillBeMember once you land the mediastream
CL.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/MediaStreamAudioSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/MediaStreamAudioSourceNode.h:64: RefPtr<MediaStream>
m_mediaStream;

Ditto.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/OfflineAudioDestinationNode.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/OfflineAudioDestinationNode.h:72:
OwnPtr<blink::WebThread> m_renderThread;

Just help me understand: Is this a webaudio thread?

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:769: static Persistent<type> name = arguments;

Can we use DEFINE_STATIC_LOCAL instead of using 'static ...' ?

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:832: static type* name =
PassRefPtr<type>(arguments).leakRef();

Ditto.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:540: // FIXME(oilpan): The current implementation
requires the deallocating

FIXME: oilpan:

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:543: // thread safe.

Do we still have this restriction? I guess that this restriction is gone because
we now use a CrossThreadPersistent for the self keep handle.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:572: // ASSERT(m_threadState ==
ThreadState::current());

Do we need this ASSERT?

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:583: ASSERT(m_threadState ==
ThreadState::current());

Do we need this ASSERT?

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:583: ASSERT(m_threadState ==
ThreadState::current());

Add ASSERT(m_keepAlive).

[haraken, 2014-04-08 06:08:10]

Also, let's explain non-trivial changes in the CL description.

[keishi, 2014-04-21 17:10:11]

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
false, sampleRate);
On 2014/04/08 06:02:08, haraken wrote:
> 
> > By doing this I am removing allocating garbage collected AudioBuffer inside
> the
> > audio decoder thread. Otherwise I was hitting an assert because I haven't
> called
> > ThreadState::attach.
> 
> I don't quite understand this. This change is just changing
> createFromAudioFileData to createBusFromInMemoryAudioFile, and I'm not sure
how
> it is resolving the threading issue. Would you elaborate on it?

AsyncAudioDecoder::decode runs in m_thread. AudioBuffer is on-heap so if we
create it in m_thread, Heap::allocate will fail because ThreadState is null.
I think by refactoring this to use AudioBus in m_thread and allocating
AudioBuffer when we are back in the main thread, I think I am avoiding the
complexity of attaching a thread state.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.h:28: #include "heap/Handle.h"
On 2014/04/08 06:02:08, haraken wrote:
> 
> platform/heap/Handle.h. The same comment for other places in this CL.

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBufferSourceNode.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.h:141: // the pitch rate. We
manually manage ref-counting because we want to use RefTypeConnection.
On 2014/04/08 06:02:08, haraken wrote:
> 
> Let me double-confirm: It is safe to keep raw pointers from the AudioNode
> hierarchy to the AudioNode hierarchy, because the lifetime of the raw pointers
> are cared by RefTypeConnection, right?
Right.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.cpp:479: ASSERT(m_keepAlive);
On 2014/04/08 06:02:08, haraken wrote:
> 
> Add #if ENABLE(OILPAN)

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.h:53: class AudioNode : public
NoBaseWillBeGarbageCollectedFinalized<AudioNode>, public ScriptWrappable, public
EventTargetWithInlineData {
On 2014/04/08 06:02:08, haraken wrote:
> 
> Add a comment about why this doesn't need to be RefCountedGarbageCollected.

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.h:213: // until the reference count reaches
zero.
On 2014/04/08 06:02:08, haraken wrote:
> 
> EventTarget is not yet on-heap, so this comment doesn't make much sense. The
> comment could be something like:
> 
> "AudioNodes are in the oilpan heap but they are still reference counted at the
> same time. This is because we are not allowed to stop the audio thread and
thus
> the audio thread cannot allocate objects in the oilpan heap.
> 
> The m_keepAlive handle is used to keep a persistent reference to this
AudioNode
> while someone has a reference to this AudioNode through a RefPtr."

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.h:219: // will not go above zero again.
On 2014/04/08 06:02:08, haraken wrote:
> 
> I'd put this comment in clearKeepAlive() in the cpp file.

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioParam.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioParam.h:46: class AudioParam FINAL : public
RefCountedWillBeGarbageCollectedFinalized<AudioParam>, public ScriptWrappable,
public AudioSummingJunction {
On 2014/04/08 06:02:08, haraken wrote:
> 
> You might want to move AudioSummingJunction to the heap in a follow-up CL.

I just tried to move AudioSummingJunction to the heap but I wasn't able to come
up with the right *WillBe* trick.

I think AudioSummingJunction and AudioParam both needs to be
GarbageCollectedFinalized but currently AudioParam is RefCounted.
If I make AudioSummingJunction NoBaseWillBeGarbageCollectedFinalized and
AudioParam RefCountedWillBeGarbageCollectedFinalized the compiler gives me an
error like error: deleted function '~AudioParam' cannot override a non-deleted
function

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioSourceNode.idl (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioSourceNode.idl:30: WillBeGarbageCollected,
On 2014/04/08 06:02:08, haraken wrote:
> 
> You don't need [WillBeGarbageCollected] in sub-classes of AudioNode. The same
> comment in other places in this CL.

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/OfflineAudioDestinationNode.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/OfflineAudioDestinationNode.h:72:
OwnPtr<blink::WebThread> m_renderThread;
On 2014/04/08 06:02:08, haraken wrote:
> 
> Just help me understand: Is this a webaudio thread?

Yes. The audio node renders in this thread so I think this is an audio thread
too.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:769: static Persistent<type> name = arguments;
On 2014/04/08 06:02:08, haraken wrote:
> 
> Can we use DEFINE_STATIC_LOCAL instead of using 'static ...' ?

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:832: static type* name =
PassRefPtr<type>(arguments).leakRef();
On 2014/04/08 06:02:08, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:540: // FIXME(oilpan): The current implementation
requires the deallocating
On 2014/04/08 06:02:08, haraken wrote:
> 
> FIXME: oilpan: 

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:543: // thread safe.
On 2014/04/08 06:02:08, haraken wrote:
> 
> Do we still have this restriction? I guess that this restriction is gone
because
> we now use a CrossThreadPersistent for the self keep handle.
> 

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:572: // ASSERT(m_threadState ==
ThreadState::current());
On 2014/04/08 06:02:08, haraken wrote:
> 
> Do we need this ASSERT?

This was in the branch but I don't think we should have this.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:583: ASSERT(m_threadState ==
ThreadState::current());
On 2014/04/08 06:02:08, haraken wrote:
> 
> Add ASSERT(m_keepAlive).

Done.

https://codereview.chromium.org/205173002/diff/150001/Source/platform/heap/He...
Source/platform/heap/Heap.h:583: ASSERT(m_threadState ==
ThreadState::current());
On 2014/04/08 06:02:08, haraken wrote:
> 
> Do we need this ASSERT?

Removed.

[haraken, 2014-04-22 05:37:18]

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
false, sampleRate);
On 2014/04/21 17:10:11, keishi1 wrote:
> On 2014/04/08 06:02:08, haraken wrote:
> > 
> > > By doing this I am removing allocating garbage collected AudioBuffer
inside
> > the
> > > audio decoder thread. Otherwise I was hitting an assert because I haven't
> > called
> > > ThreadState::attach.
> > 
> > I don't quite understand this. This change is just changing
> > createFromAudioFileData to createBusFromInMemoryAudioFile, and I'm not sure
> how
> > it is resolving the threading issue. Would you elaborate on it?
> 
> AsyncAudioDecoder::decode runs in m_thread. AudioBuffer is on-heap so if we
> create it in m_thread, Heap::allocate will fail because ThreadState is null.
> I think by refactoring this to use AudioBus in m_thread and allocating
> AudioBuffer when we are back in the main thread, I think I am avoiding the
> complexity of attaching a thread state.

Makes sense, thanks for the clarification!

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
File Source/modules/webaudio/AudioParam.h (right):

https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
Source/modules/webaudio/AudioParam.h:46: class AudioParam FINAL : public
RefCountedWillBeGarbageCollectedFinalized<AudioParam>, public ScriptWrappable,
public AudioSummingJunction {
On 2014/04/21 17:10:11, keishi1 wrote:
> On 2014/04/08 06:02:08, haraken wrote:
> > 
> > You might want to move AudioSummingJunction to the heap in a follow-up CL.
> 
> I just tried to move AudioSummingJunction to the heap but I wasn't able to
come
> up with the right *WillBe* trick.
> 
> I think AudioSummingJunction and AudioParam both needs to be
> GarbageCollectedFinalized but currently AudioParam is RefCounted.
> If I make AudioSummingJunction NoBaseWillBeGarbageCollectedFinalized and
> AudioParam RefCountedWillBeGarbageCollectedFinalized the compiler gives me an
> error like error: deleted function '~AudioParam' cannot override a non-deleted
> function

hmm, this is weird.

I think that in the end we want to have the following class structure:

class AudioParam : public AudioSummingJunction, public ScriptWrappable { ... };

class AudioSummingJunction : public
RefCountedWillBeGarbageCollectedFinalized<AudioSummingJunction> { ... };

Would it be possible to refactor the current class structure to:

class AudioParam : public AudioSummingJunction, public ScriptWrappable { ... };

class AudioNodeInput : public AudioSummingJunction { ... };

class AudioSummingJunction : public RefCounted<AudioSummingJunction> { ... };

before landing this CL?

[haraken, 2014-04-23 12:52:08]

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:876: 

Shall we land this macro ahead of time? Recently I've seen a couple of CLs that
want to use this macro :)

[tkent, 2014-04-24 04:02:39]

lgtm

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (left):

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:252: 
This change is unnecessary.

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:340: 
This change is unnecessary.

[tkent, 2014-04-24 04:04:15]

On 2014/04/21 17:10:11, keishi1 wrote:
>
https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
> File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):
> 
>
https://codereview.chromium.org/205173002/diff/150001/Source/modules/webaudio...
> Source/modules/webaudio/AsyncAudioDecoder.cpp:68: RefPtr<AudioBus> bus =
> createBusFromInMemoryAudioFile(audioData->data(), audioData->byteLength(),
> false, sampleRate);
> On 2014/04/08 06:02:08, haraken wrote:
> > 
> > > By doing this I am removing allocating garbage collected AudioBuffer
inside
> > the
> > > audio decoder thread. Otherwise I was hitting an assert because I haven't
> > called
> > > ThreadState::attach.
> > 
> > I don't quite understand this. This change is just changing
> > createFromAudioFileData to createBusFromInMemoryAudioFile, and I'm not sure
> how
> > it is resolving the threading issue. Would you elaborate on it?
> 
> AsyncAudioDecoder::decode runs in m_thread. AudioBuffer is on-heap so if we
> create it in m_thread, Heap::allocate will fail because ThreadState is null.
> I think by refactoring this to use AudioBus in m_thread and allocating
> AudioBuffer when we are back in the main thread, I think I am avoiding the
> complexity of attaching a thread state.

This should be explained in the CL description.

[Mads Ager (chromium), 2014-04-24 09:05:36]

LGTM! Thanks for iterating on this! :-)

https://codereview.chromium.org/205173002/diff/190001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/205173002/diff/190001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.cpp:605: // of a ScriptProcessorNode reaches
zero. At that point, the
ScriptProcessorNode -> AudioNode (and one occurrence on the next line as well)

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:750: #define
NoBaseWillBeRefCountedGarbageCollected WebCore::RefCountedGarbageCollected
I don't think this is used in this changelist and we shouldn't need it (if
something is not ref counted now we should not make it ref counted when oilpan).
Can we remove this?

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:838: #define
NoBaseWillBeRefCountedGarbageCollected WebCore::DummyBase
Ditto.

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/He...
Source/platform/heap/Heap.h:557: m_threadState = ThreadState::current();
In the code on the branch I used this threadState to assert that the persistent
was created and deleted on the same thread.

You have now moved to using a cross-thread persistent for this which is good, we
did not have that on the branch. :-)

Let's remove the m_threadState since it is unused?

[keishi, 2014-05-06 20:00:02]

I tried to move AudioSummingJunction, AudioNodeInput, AudioParam to the heap but
couldn't.

I uploaded my try here.
https://codereview.chromium.org/270103005/

AudioSummingJunction::~AudioSummingJunction touches AudioContext so I tried to
use WeakMember to remove that. AudioSummingJunction::changedOutputs is called
from the audio thread but we don't allow on-heap allocation in the audio thread
so that didn't work.

https://codereview.chromium.org/205173002/diff/190001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/205173002/diff/190001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.cpp:605: // of a ScriptProcessorNode reaches
zero. At that point, the
On 2014/04/24 09:05:37, Mads Ager (chromium) wrote:
> ScriptProcessorNode -> AudioNode (and one occurrence on the next line as well)

Done.

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:750: #define
NoBaseWillBeRefCountedGarbageCollected WebCore::RefCountedGarbageCollected
On 2014/04/24 09:05:37, Mads Ager (chromium) wrote:
> I don't think this is used in this changelist and we shouldn't need it (if
> something is not ref counted now we should not make it ref counted when
oilpan).
> Can we remove this?

Done.

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:838: #define
NoBaseWillBeRefCountedGarbageCollected WebCore::DummyBase
On 2014/04/24 09:05:37, Mads Ager (chromium) wrote:
> Ditto.

Done.

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/He...
Source/platform/heap/Heap.h:557: m_threadState = ThreadState::current();
On 2014/04/24 09:05:37, Mads Ager (chromium) wrote:
> In the code on the branch I used this threadState to assert that the
persistent
> was created and deleted on the same thread.
> 
> You have now moved to using a cross-thread persistent for this which is good,
we
> did not have that on the branch. :-)
> 
> Let's remove the m_threadState since it is unused?

Done.

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (left):

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:252: 
On 2014/04/24 04:02:40, tkent (ooo until May 6) wrote:
> This change is unnecessary.

Done.

https://codereview.chromium.org/205173002/diff/190001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:340: 
On 2014/04/24 04:02:40, tkent (ooo until May 6) wrote:
> This change is unnecessary.

Done.

[haraken, 2014-05-07 00:51:27]

On 2014/05/06 20:00:02, keishi1 wrote:
> I tried to move AudioSummingJunction, AudioNodeInput, AudioParam to the heap
but
> couldn't.
> 
> I uploaded my try here.
> https://codereview.chromium.org/270103005/
> 
> AudioSummingJunction::~AudioSummingJunction touches AudioContext so I tried to
> use WeakMember to remove that. AudioSummingJunction::changedOutputs is called
> from the audio thread but we don't allow on-heap allocation in the audio
thread
> so that didn't work.

Let's discuss offline. It's weird to keep AudioSummingJunction off-heap but move
AudioParam to the heap, but it will work. So let's land this CL first and fix
the issue later. I'll take another look at this CL today.

[haraken, 2014-05-07 04:02:15]

LGTM with comments.

rtoy@, kbr@: We're going to land this CL, but would it be OK?

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.cpp:81: RefPtr<AudioBus> audioBusRef =
adoptRef(audioBus);

RefPtr<AudioBus> protect(audioBus);

would be better.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBuffer.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.cpp:64: return buffer;

buffer.release();

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.cpp:73: return buffer;

buffer.release();

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.cpp:85: return buffer;

buffer.release();

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:305: return audioBuffer;

audioBuffer.release();

Not related to your CL, but it's better to have .release(). The same comment for
other create methods in this CL.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:899: for (HashSet<AudioSummingJunction*
>::iterator i = m_dirtySummingJunctions.begin(); i !=
m_dirtySummingJunctions.end(); ++i)

mis-edit.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:1000:
visitor->trace(m_dirtySummingJunctions);

m_dirtySummingJunctions should not be traced, since it's still off heap.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:294: HashSet<AudioSummingJunction* >
m_dirtySummingJunctions;

This is mis-edit.

It looks safe to use raw pointers here because m_dirtySummingJunctions is
explicitly cleared when the AudioSummingJunction is destructed. Let's move
AudioSummingJunction to the heap in a follow-up CL.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:295: HashSet<AudioNodeOutput*>
m_dirtyAudioNodeOutputs;

These raw pointers look safe as well, but let's move AudioNodeOutput to the heap
in a follow-up.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.cpp:603: {

Add ASSERT(m_keepAlive);

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/OfflineAudioContext.idl (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/OfflineAudioContext.idl:26: WillBeGarbageCollected,

This is not necessary, since AudioContext has [WillBeGarbageCollected].

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/PeriodicWave.cpp:56: return periodicWave;

periodicWave.release().

The same comment for other create methods in this file.

[keishi, 2014-05-07 07:59:23]

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.cpp:81: RefPtr<AudioBus> audioBusRef =
adoptRef(audioBus);
On 2014/05/07 04:02:16, haraken wrote:
> 
> RefPtr<AudioBus> protect(audioBus);
> 
> would be better.

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBuffer.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.cpp:64: return buffer;
On 2014/05/07 04:02:16, haraken wrote:
> 
> buffer.release();

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.cpp:73: return buffer;
On 2014/05/07 04:02:16, haraken wrote:
> 
> buffer.release();

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.cpp:85: return buffer;
On 2014/05/07 04:02:16, haraken wrote:
> 
> buffer.release();

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:305: return audioBuffer;
On 2014/05/07 04:02:16, haraken wrote:
> 
> audioBuffer.release();
> 
> Not related to your CL, but it's better to have .release(). The same comment
for
> other create methods in this CL.

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:899: for (HashSet<AudioSummingJunction*
>::iterator i = m_dirtySummingJunctions.begin(); i !=
m_dirtySummingJunctions.end(); ++i)
On 2014/05/07 04:02:16, haraken wrote:
> 
> mis-edit.

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:1000:
visitor->trace(m_dirtySummingJunctions);
On 2014/05/07 04:02:16, haraken wrote:
> 
> m_dirtySummingJunctions should not be traced, since it's still off heap.

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:294: HashSet<AudioSummingJunction* >
m_dirtySummingJunctions;
On 2014/05/07 04:02:16, haraken wrote:
> 
> This is mis-edit.
> 
> It looks safe to use raw pointers here because m_dirtySummingJunctions is
> explicitly cleared when the AudioSummingJunction is destructed. Let's move
> AudioSummingJunction to the heap in a follow-up CL.

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/AudioNode.cpp:603: {
On 2014/05/07 04:02:16, haraken wrote:
> 
> Add ASSERT(m_keepAlive);

Already there on line 609

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/OfflineAudioContext.idl (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/OfflineAudioContext.idl:26: WillBeGarbageCollected,
On 2014/05/07 04:02:16, haraken wrote:
> 
> This is not necessary, since AudioContext has [WillBeGarbageCollected].

Done.

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
File Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/205173002/diff/230001/Source/modules/webaudio...
Source/modules/webaudio/PeriodicWave.cpp:56: return periodicWave;
On 2014/05/07 04:02:16, haraken wrote:
> 
> periodicWave.release().
> 
> The same comment for other create methods in this file.

Done.

[Ken Russell (switch to Gerrit), 2014-05-07 22:07:19]

Very nice work transitioning this complex module to Oilpan. LGTM overall. A
couple of questions and comments.

rtoy@ should review this too.

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.h:52:
PassRefPtrWillBeRawPtr<AudioBuffer> createAudioBufferFromAudioBus(AudioBus*);
This declaration looks stale; there's no implementation in the .cpp file.

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBuffer.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.h:67: void trace(Visitor*) { }
Just confirming -- it isn't necessary to do anything with the m_channels array?
Each element is still a RefPtr, and none of those objects are exposed directly
to JavaScript, even via getChannelData.

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
File Source/modules/webaudio/AudioListener.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
Source/modules/webaudio/AudioListener.h:82: void trace(Visitor*) { }
Is it necessary to do anything with m_panners?

https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
Source/platform/heap/Heap.h:567: // audio thread.
Agreed, though the audio thread needs to traverse the AudioNode graph directly.
This seems to require that Oilpan would not use a compacting GC for the
garbage-collected objects in its heap. Is this the case?

How could the Audio thread indicate to the main thread that it's OK to perform a
GC?

[haraken, 2014-05-08 00:52:08]

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBuffer.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
Source/modules/webaudio/AudioBuffer.h:67: void trace(Visitor*) { }
On 2014/05/07 22:07:20, Ken Russell wrote:
> Just confirming -- it isn't necessary to do anything with the m_channels
array?
> Each element is still a RefPtr, and none of those objects are exposed directly
> to JavaScript, even via getChannelData.

m_channels is not on heap, so we don't need to trace it. We just need to trace
on-heap things such as Member and HeapVector.

We'll move Float32Array to the heap at some point and trace it.

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
File Source/modules/webaudio/AudioListener.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
Source/modules/webaudio/AudioListener.h:82: void trace(Visitor*) { }
On 2014/05/07 22:07:20, Ken Russell wrote:
> Is it necessary to do anything with m_panners?

Not necessary, since m_panners is not on heap. We just need to trace on-heap
things, like Member, HeapVector etc.

https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
Source/platform/heap/Heap.h:567: // audio thread.
On 2014/05/07 22:07:20, Ken Russell wrote:
> Agreed, though the audio thread needs to traverse the AudioNode graph
directly.
> This seems to require that Oilpan would not use a compacting GC for the
> garbage-collected objects in its heap. Is this the case?

Yes. Oilpan won't introduce a compacting GC. Copying Blink objects causes a ton
of issues and we won't take that way.

> How could the Audio thread indicate to the main thread that it's OK to perform
a
> GC?

One way to do that is using an Interrupter. By using the Interruptor, the audio
thread can indicate that "I'm currently at a point where GC can happen" (this is
called a "safe point").

The audio thread needs to enter the safe point very frequently. Otherwise, the
main thread has to wait for a long time to start a GC, which will increase GC
pause. For more details, you can see a comment in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
.

[Ken Russell (switch to Gerrit), 2014-05-08 02:01:19]

https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
Source/platform/heap/Heap.h:567: // audio thread.
On 2014/05/08 00:52:09, haraken wrote:
> One way to do that is using an Interrupter. By using the Interruptor, the
audio
> thread can indicate that "I'm currently at a point where GC can happen" (this
is
> called a "safe point").
> 
> The audio thread needs to enter the safe point very frequently. Otherwise, the
> main thread has to wait for a long time to start a GC, which will increase GC
> pause. For more details, you can see a comment in
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> .

We will need to be careful to not introduce hitches or dropouts in the
high-priority audio thread. If keeping a persistent handle to the audio nodes
currently being used by the audio thread is a solution that avoids any
possibility of pausing the audio thread, we should consider making that solution
permanent.

[haraken, 2014-05-08 02:29:55]

On 2014/05/08 02:01:19, Ken Russell wrote:
>
https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
> File Source/platform/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
> Source/platform/heap/Heap.h:567: // audio thread.
> On 2014/05/08 00:52:09, haraken wrote:
> > One way to do that is using an Interrupter. By using the Interruptor, the
> audio
> > thread can indicate that "I'm currently at a point where GC can happen"
(this
> is
> > called a "safe point").
> > 
> > The audio thread needs to enter the safe point very frequently. Otherwise,
the
> > main thread has to wait for a long time to start a GC, which will increase
GC
> > pause. For more details, you can see a comment in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > .
> 
> We will need to be careful to not introduce hitches or dropouts in the
> high-priority audio thread. If keeping a persistent handle to the audio nodes
> currently being used by the audio thread is a solution that avoids any
> possibility of pausing the audio thread, we should consider making that
solution
> permanent.

Agreed. Using an Interruptor is one option, but I think it would be more
reasonable to keep the reference counting in order to ensure realtime
responsiveness of the audio thread. Oilpan is not "all-or-nothing", and we don't
need to move everything to Oilpan. It's completely fine to leave reference
counting in places where it does make sense.

[Mads Ager (chromium), 2014-05-08 05:50:14]

https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
Source/platform/heap/Heap.h:567: // audio thread.
On 2014/05/08 02:01:20, Ken Russell wrote:
> On 2014/05/08 00:52:09, haraken wrote:
> > One way to do that is using an Interrupter. By using the Interruptor, the
> audio
> > thread can indicate that "I'm currently at a point where GC can happen"
(this
> is
> > called a "safe point").
> > 
> > The audio thread needs to enter the safe point very frequently. Otherwise,
the
> > main thread has to wait for a long time to start a GC, which will increase
GC
> > pause. For more details, you can see a comment in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > .
> 
> We will need to be careful to not introduce hitches or dropouts in the
> high-priority audio thread. If keeping a persistent handle to the audio nodes
> currently being used by the audio thread is a solution that avoids any
> possibility of pausing the audio thread, we should consider making that
solution
> permanent.

Thanks for your comments Ken. You are absolutely right that we cannot do a
compacting collector for AudioNodes as long as they are handled this way.  We
have no current plans of registering the audio thread with oilpan. The real-time
requirements are too strict. If we do not register the audio thread with oilpan
we cannot get it to a safe-point and can't move AudioNodes. However, compaction
doesn't have to be all or nothing. We can split heap segments by object type (we
are going to do that for security anyway). There could be a space for objects
that are not allowed to move. AudioNodes could be in that space. I'm not yet
willing to give up on a compacting collector down the line! :-)

The FIXME comment here should probably be removed. I added it when we did the
first version on this on the oilpan branch. The ref/derefs from the audio thread
*are* the message from the audio thread about life-time and I agree that we
should just keep that setup permanently.

[Raymond Toy, 2014-05-08 17:09:23]

On 2014/05/08 00:52:08, haraken wrote:
>
https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
> File Source/modules/webaudio/AudioBuffer.h (right):
> 
>
https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
> Source/modules/webaudio/AudioBuffer.h:67: void trace(Visitor*) { }
> On 2014/05/07 22:07:20, Ken Russell wrote:
> > Just confirming -- it isn't necessary to do anything with the m_channels
> array?
> > Each element is still a RefPtr, and none of those objects are exposed
directly
> > to JavaScript, even via getChannelData.
> 
> m_channels is not on heap, so we don't need to trace it. We just need to trace
> on-heap things such as Member and HeapVector.
> 
> We'll move Float32Array to the heap at some point and trace it.
> 
>
https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
> File Source/modules/webaudio/AudioListener.h (right):
> 
>
https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
> Source/modules/webaudio/AudioListener.h:82: void trace(Visitor*) { }
> On 2014/05/07 22:07:20, Ken Russell wrote:
> > Is it necessary to do anything with m_panners?
> 
> Not necessary, since m_panners is not on heap. We just need to trace on-heap
> things, like Member, HeapVector etc.
> 
>
https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
> File Source/platform/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
> Source/platform/heap/Heap.h:567: // audio thread.
> On 2014/05/07 22:07:20, Ken Russell wrote:
> > Agreed, though the audio thread needs to traverse the AudioNode graph
> directly.
> > This seems to require that Oilpan would not use a compacting GC for the
> > garbage-collected objects in its heap. Is this the case?
> 
> Yes. Oilpan won't introduce a compacting GC. Copying Blink objects causes a
ton
> of issues and we won't take that way.
> 
> > How could the Audio thread indicate to the main thread that it's OK to
perform
> a
> > GC?
> 
> One way to do that is using an Interrupter. By using the Interruptor, the
audio
> thread can indicate that "I'm currently at a point where GC can happen" (this
is
> called a "safe point").
> 
> The audio thread needs to enter the safe point very frequently. Otherwise, the
> main thread has to wait for a long time to start a GC, which will increase GC
> pause. For more details, you can see a comment in
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> .

There should be a safepoint, I think, after every audio processing block of 128
frames (about every 3 ms for a 48kHz sample rate).

[Raymond Toy, 2014-05-08 17:11:47]

On 2014/05/08 05:50:14, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
> File Source/platform/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
> Source/platform/heap/Heap.h:567: // audio thread.
> On 2014/05/08 02:01:20, Ken Russell wrote:
> > On 2014/05/08 00:52:09, haraken wrote:
> > > One way to do that is using an Interrupter. By using the Interruptor, the
> > audio
> > > thread can indicate that "I'm currently at a point where GC can happen"
> (this
> > is
> > > called a "safe point").
> > > 
> > > The audio thread needs to enter the safe point very frequently. Otherwise,
> the
> > > main thread has to wait for a long time to start a GC, which will increase
> GC
> > > pause. For more details, you can see a comment in
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > .
> > 
> > We will need to be careful to not introduce hitches or dropouts in the
> > high-priority audio thread. If keeping a persistent handle to the audio
nodes
> > currently being used by the audio thread is a solution that avoids any
> > possibility of pausing the audio thread, we should consider making that
> solution
> > permanent.
> 
> Thanks for your comments Ken. You are absolutely right that we cannot do a
> compacting collector for AudioNodes as long as they are handled this way.  We
> have no current plans of registering the audio thread with oilpan. The
real-time
> requirements are too strict. If we do not register the audio thread with
oilpan
> we cannot get it to a safe-point and can't move AudioNodes. However,
compaction
> doesn't have to be all or nothing. We can split heap segments by object type
(we
> are going to do that for security anyway). There could be a space for objects
> that are not allowed to move. AudioNodes could be in that space. I'm not yet
> willing to give up on a compacting collector down the line! :-)
> 
> The FIXME comment here should probably be removed. I added it when we did the
> first version on this on the oilpan branch. The ref/derefs from the audio
thread
> *are* the message from the audio thread about life-time and I agree that we
> should just keep that setup permanently.

lgtm.

It's really exciting to see oilpan for webaudio!

[Mads Ager (chromium), 2014-05-08 17:23:59]

On 2014/05/08 17:09:23, Raymond Toy wrote:
> On 2014/05/08 00:52:08, haraken wrote:
> >
>
https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
> > File Source/modules/webaudio/AudioBuffer.h (right):
> > 
> >
>
https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
> > Source/modules/webaudio/AudioBuffer.h:67: void trace(Visitor*) { }
> > On 2014/05/07 22:07:20, Ken Russell wrote:
> > > Just confirming -- it isn't necessary to do anything with the m_channels
> > array?
> > > Each element is still a RefPtr, and none of those objects are exposed
> directly
> > > to JavaScript, even via getChannelData.
> > 
> > m_channels is not on heap, so we don't need to trace it. We just need to
trace
> > on-heap things such as Member and HeapVector.
> > 
> > We'll move Float32Array to the heap at some point and trace it.
> > 
> >
>
https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
> > File Source/modules/webaudio/AudioListener.h (right):
> > 
> >
>
https://codereview.chromium.org/205173002/diff/250001/Source/modules/webaudio...
> > Source/modules/webaudio/AudioListener.h:82: void trace(Visitor*) { }
> > On 2014/05/07 22:07:20, Ken Russell wrote:
> > > Is it necessary to do anything with m_panners?
> > 
> > Not necessary, since m_panners is not on heap. We just need to trace on-heap
> > things, like Member, HeapVector etc.
> > 
> >
>
https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
> > File Source/platform/heap/Heap.h (right):
> > 
> >
>
https://codereview.chromium.org/205173002/diff/250001/Source/platform/heap/He...
> > Source/platform/heap/Heap.h:567: // audio thread.
> > On 2014/05/07 22:07:20, Ken Russell wrote:
> > > Agreed, though the audio thread needs to traverse the AudioNode graph
> > directly.
> > > This seems to require that Oilpan would not use a compacting GC for the
> > > garbage-collected objects in its heap. Is this the case?
> > 
> > Yes. Oilpan won't introduce a compacting GC. Copying Blink objects causes a
> ton
> > of issues and we won't take that way.
> > 
> > > How could the Audio thread indicate to the main thread that it's OK to
> perform
> > a
> > > GC?
> > 
> > One way to do that is using an Interrupter. By using the Interruptor, the
> audio
> > thread can indicate that "I'm currently at a point where GC can happen"
(this
> is
> > called a "safe point").
> > 
> > The audio thread needs to enter the safe point very frequently. Otherwise,
the
> > main thread has to wait for a long time to start a GC, which will increase
GC
> > pause. For more details, you can see a comment in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > .
> 
> There should be a safepoint, I think, after every audio processing block of
128
> frames (about every 3 ms for a 48kHz sample rate).

Yeah, the problem is not finding a good place for a safepoint. The problem is
that a safepoint is where a thread is blocked during garbage collection.
Therefore, if we add safepoints in the audio thread, it will block during
garbage collection. Garbage collection can easily block for more time than the
audio thread has to process a frame, so we can't add safepoints. Therefore, this
patch is the best option that I see at this point and I think it will work well.

[haraken, 2014-05-08 23:09:54]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-08 23:11:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/205173002/250001

[commit-bot: I haz the power, 2014-05-08 23:12:25]

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

[commit-bot: I haz the power, 2014-05-08 23:12:26]

Failed to apply patch for Source/modules/webaudio/AudioBuffer.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/webaudio/AudioBuffer.cpp
  Hunk #1 FAILED at 52.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/webaudio/AudioBuffer.cpp.rej

Patch:       Source/modules/webaudio/AudioBuffer.cpp
Index: Source/modules/webaudio/AudioBuffer.cpp
diff --git a/Source/modules/webaudio/AudioBuffer.cpp
b/Source/modules/webaudio/AudioBuffer.cpp
index
52a18a3b88044e44d0aca021b10e884760f352db..7951f6962e8a6fc4f2fe0074837c6905e5b77155
100644
--- a/Source/modules/webaudio/AudioBuffer.cpp
+++ b/Source/modules/webaudio/AudioBuffer.cpp
@@ -52,30 +52,40 @@ float AudioBuffer::maxAllowedSampleRate()
     return 192000;
 }
 
-PassRefPtr<AudioBuffer> AudioBuffer::create(unsigned numberOfChannels, size_t
numberOfFrames, float sampleRate)
+PassRefPtrWillBeRawPtr<AudioBuffer> AudioBuffer::create(unsigned
numberOfChannels, size_t numberOfFrames, float sampleRate)
 {
     if (sampleRate < minAllowedSampleRate() || sampleRate >
maxAllowedSampleRate() || numberOfChannels > AudioContext::maxNumberOfChannels()
|| !numberOfFrames)
         return nullptr;
 
-    RefPtr<AudioBuffer> buffer = adoptRef(new AudioBuffer(numberOfChannels,
numberOfFrames, sampleRate));
+    RefPtrWillBeRawPtr<AudioBuffer> buffer = adoptRefWillBeNoop(new
AudioBuffer(numberOfChannels, numberOfFrames, sampleRate));
 
     if (!buffer->createdSuccessfully(numberOfChannels))
         return nullptr;
-    return buffer;
+    return buffer.release();
 }
 
-PassRefPtr<AudioBuffer> AudioBuffer::createFromAudioFileData(const void* data,
size_t dataSize, bool mixToMono, float sampleRate)
+PassRefPtrWillBeRawPtr<AudioBuffer> AudioBuffer::createFromAudioFileData(const
void* data, size_t dataSize, bool mixToMono, float sampleRate)
 {
     RefPtr<AudioBus> bus = createBusFromInMemoryAudioFile(data, dataSize,
mixToMono, sampleRate);
     if (bus.get()) {
-        RefPtr<AudioBuffer> buffer = adoptRef(new AudioBuffer(bus.get()));
+        RefPtrWillBeRawPtr<AudioBuffer> buffer = adoptRefWillBeNoop(new
AudioBuffer(bus.get()));
         if (buffer->createdSuccessfully(bus->numberOfChannels()))
-            return buffer;
+            return buffer.release();
     }
 
     return nullptr;
 }
 
+PassRefPtrWillBeRawPtr<AudioBuffer> AudioBuffer::createFromAudioBus(AudioBus*
bus)
+{
+    if (!bus)
+        return nullptr;
+    RefPtrWillBeRawPtr<AudioBuffer> buffer = adoptRefWillBeNoop(new
AudioBuffer(bus));
+    if (buffer->createdSuccessfully(bus->numberOfChannels()))
+        return buffer.release();
+    return nullptr;
+}
+
 bool AudioBuffer::createdSuccessfully(unsigned desiredNumberOfChannels) const
 {
     return numberOfChannels() == desiredNumberOfChannels;

[keishi, 2014-05-09 04:55:14]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-05-09 04:56:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/205173002/270001

[commit-bot: I haz the power, 2014-05-09 06:10:20]

Change committed as 173725