Enable Oilpan by default for webaudio/

Enable Oilpan by default for webaudio/

(1) This CL basically does the following two things:

- Drop "WillBe" types from webaudio/.

- Replace the hand-written reference counting system with RefCountedGarbageCollected.

(2) Given that AudioNode inherits from EventTarget, which is still on-heap
in non-oilpan builds, we need to make AudioNode RefCountedGarbageCollected.

(3) Ideally the following collections should use strong Members,
but we cannot do that because these collections are touched by audio threads
(which are not registered to oilpan). Thus we use raw pointers
with GC_PLUGIN_IGNORE.

- AudioNodeInput::m_disabledOutputs
- AudioSummingJunction::m_outputs
- AudioSummingJunction::m_renderingOutputs
- AudioContext::m_finishedNodes
- AudioContext::m_referencedNodes
- AudioContext::m_nodesMarkForDeletion
- AudioContext::m_nodesToDelete
- AudioContext::m_dirtySummingJunctions
- AudioContext::m_dirtyAudioNodeOutputs
- AudioContext::m_automaticPullNodes
- AudioContext::m_renderingAutomaticPullNodes
- AudioContext::m_deferredBreakConnectionList
- AudioContext::m_deferredFinishDerefList

(4) Ideally AudioDSPKernel should be moved to the heap and
AudioDSPKernel::m_kernelProcessor should be a Member.
However, we cannot do that because AudioDSPKernel can be allocated
by audio threads. Thus we leave AudioDSPKernel::m_kernelProcessor
as a raw pointer. This raw pointer is guaranteed to be safe.

(5) This CL fixes leaks of webaudio tests in LeakExpectations.


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

[haraken, 2014-08-05 12:23:03]

This CL is not yet for review.

While I'm writing the CL, I noticed a couple of issues to be handled before
enabling Oilpan. I'll handle the issues first, and then get back to this CL.

[haraken, 2014-08-11 06:44:39]

Hmm, this is complicated. I'm getting to understand what's going on.

In short, it's hard to move the AudioNode hierarchy until we move EventTarget to
the heap.

The problem is that in non-oilpan builds, AudioNode needs to be
"RefCounted"GarbageCollected because AudioNode inherits from EventTarget. This
means that we need to re-introduce the self-keep-alive persistent handle to the
AudioNode to support AudioNode::ref/deref.

However, we cannot easily re-introduce the self-keep-alive persistent handle,
because it causes leaks (Remember that tkent-san refactored the ref-counting
mechanism a lot in order to remove the self-keep-alive handle and get rid of the
leaks). If we simply re-introduce the self-keep-alive, we'll end up causing the
leaks again.

tkent-san: Should we try to re-introduce the self-persistent handle so that it
won't cause leaks? Or would it be better just to suspend moving AudioNode to the
heap until we move EventTarget to the heap? Or any better idea?

[tkent, 2014-08-11 08:50:10]

I think making AudioNode RefCountedGarbageCollected won't cause any problems.
The past self-keep-alive leaked everything because its implementation was not
good.

[haraken, 2014-08-11 08:50:59]

On 2014/08/11 08:50:10, tkent wrote:
> I think making AudioNode RefCountedGarbageCollected won't cause any problems.
> The past self-keep-alive leaked everything because its implementation was not
> good.

Thanks, then I'll try that.

[haraken, 2014-08-12 08:25:18]

The patch set 6 fixes all crashes in layout tests. However, it looks like all
AudioNodes are leaking :) Investigating.

[haraken, 2014-08-12 11:43:40]

OK, the patch set 8 will work. It passes all layout tests without leaking any
AudioNode.

That being said, the patch set 8 changes the reference counting system
dramatically, so I'll split the change into pieces and land them incrementally.

[haraken, 2014-08-15 07:07:08]

OK, the patch set 10 is now ready for review.

tkent-san, rtoy: Would you take a look when you have a cycle?

[tkent, 2014-08-15 07:39:01]

I wonder why we don't use RefCountedGarbageCollected for AudioNode.  ref() and
deref() are called only in the main thread, and I thought
RefCountedGarbageCollected should work.

> (3) Ideally the following collections should use strong Members,
> but we cannot do that because these collections are touched by audio threads
> (which are not registered to oilpan). Thus we use raw pointers
> with GC_PLUGIN_IGNORE.

Is GC_PLUGIN_IGNORE necessary?  If so, we should avoid empty string argument,
should file a bug and fill the bug URL.

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

https://codereview.chromium.org/438293003/diff/180001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:75: class AudioContext : public
ThreadSafeRefCountedGarbageCollected<AudioContext>, public ActiveDOMObject,
public EventTargetWithInlineData {
should be ThreadSafeRefCountedGarbageCollectedWillBeGarbageCollectedFinalized ?

https://codereview.chromium.org/438293003/diff/180001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:275: // This RefPtr is connection
reference. We must call AudioNode::
Remove this paragraph.

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

https://codereview.chromium.org/438293003/diff/180001/Source/modules/webaudio...
Source/modules/webaudio/AudioNodeOutput.h:136: // This RefPtr<AudioNode> is
connection reference. We must call AudioNode::
Remove this paragraph.

https://codereview.chromium.org/438293003/diff/180001/Source/platform/audio/A...
File Source/platform/audio/AudioDSPKernel.h (right):

https://codereview.chromium.org/438293003/diff/180001/Source/platform/audio/A...
Source/platform/audio/AudioDSPKernel.h:70: // Oilpan: This raw pointer is safe
because the AudioDSPKernelProcessor object is guaranteed
nit: I recommend to wrap code comments in 80 columns.

[haraken, 2014-08-15 08:08:00]

> I wonder why we don't use RefCountedGarbageCollected for AudioNode.  ref() and
> deref() are called only in the main thread, and I thought
> RefCountedGarbageCollected should work.

This is a very good point. I think we can (should) do that, but would it be OK
to do that in a separate CL? It's not a trivial change because
RefCountedGarbageCollected starts with refcount==1 whereas the current AudioNode
starts with refcount==0. Added a FIXME.


> Is GC_PLUGIN_IGNORE necessary?  If so, we should avoid empty string argument,
> should file a bug and fill the bug URL.

GC_PLUGIN_IGNORE is necessary because the plugin complains about raw pointers to
on-heap objects in non-oilpan builds.

That being said, those raw pointers are not bugs, as I commented on each
collection that has GC_PLUGIN_IGNORE. I'd suggest just to drop the requirement
that GC_PLUGIN_IGNORE has to have a bug number. If we want to add a bug number,
we can add it as a comment.

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

https://codereview.chromium.org/438293003/diff/180001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:75: class AudioContext : public
ThreadSafeRefCountedGarbageCollected<AudioContext>, public ActiveDOMObject,
public EventTargetWithInlineData {
On 2014/08/15 07:39:00, tkent wrote:
> should be ThreadSafeRefCountedGarbageCollectedWillBeGarbageCollectedFinalized
?

Sounds reasonable. However, this change also needs some non-trivial fix. I'd
like to handle it in a separate CL. Added a FIXME.

https://codereview.chromium.org/438293003/diff/180001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:275: // This RefPtr is connection
reference. We must call AudioNode::
On 2014/08/15 07:39:00, tkent wrote:
> Remove this paragraph.

Done.

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

https://codereview.chromium.org/438293003/diff/180001/Source/modules/webaudio...
Source/modules/webaudio/AudioNodeOutput.h:136: // This RefPtr<AudioNode> is
connection reference. We must call AudioNode::
On 2014/08/15 07:39:00, tkent wrote:
> Remove this paragraph.

Done.

https://codereview.chromium.org/438293003/diff/180001/Source/platform/audio/A...
File Source/platform/audio/AudioDSPKernel.h (right):

https://codereview.chromium.org/438293003/diff/180001/Source/platform/audio/A...
Source/platform/audio/AudioDSPKernel.h:70: // Oilpan: This raw pointer is safe
because the AudioDSPKernelProcessor object is guaranteed
On 2014/08/15 07:39:01, tkent wrote:
> nit: I recommend to wrap code comments in 80 columns.

Done.

[tkent, 2014-08-15 08:19:35]

On 2014/08/15 08:08:00, haraken wrote:
> > I wonder why we don't use RefCountedGarbageCollected for AudioNode.  ref()
and
> > deref() are called only in the main thread, and I thought
> > RefCountedGarbageCollected should work.
> 
> This is a very good point. I think we can (should) do that, but would it be OK
> to do that in a separate CL? It's not a trivial change because
> RefCountedGarbageCollected starts with refcount==1 whereas the current
AudioNode
> starts with refcount==0. Added a FIXME.

adoptRefCountedGarbageCollected resolves this issue.
Re-introducing m_keepAlive is a unnecessary step if we can use
RefCountedGarbageCollected.

[haraken, 2014-08-15 09:00:49]

On 2014/08/15 08:19:35, tkent wrote:
> On 2014/08/15 08:08:00, haraken wrote:
> > > I wonder why we don't use RefCountedGarbageCollected for AudioNode.  ref()
> and
> > > deref() are called only in the main thread, and I thought
> > > RefCountedGarbageCollected should work.
> > 
> > This is a very good point. I think we can (should) do that, but would it be
OK
> > to do that in a separate CL? It's not a trivial change because
> > RefCountedGarbageCollected starts with refcount==1 whereas the current
> AudioNode
> > starts with refcount==0. Added a FIXME.
> 
> adoptRefCountedGarbageCollected resolves this issue.
> Re-introducing m_keepAlive is a unnecessary step if we can use
> RefCountedGarbageCollected.

You're right. Done.

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

In the end, I realized that we are able to remove all ENABLE(OILPAN)s from
webaudio/ (except for the one in ~AudioContext(), which I'll address in a
follow-up).

[zerny-chromium, 2014-08-15 10:29:54]

> > Is GC_PLUGIN_IGNORE necessary?  If so, we should avoid empty string
argument,
> > should file a bug and fill the bug URL.
> 
> GC_PLUGIN_IGNORE is necessary because the plugin complains about raw pointers
to
> on-heap objects in non-oilpan builds.
> 
> That being said, those raw pointers are not bugs, as I commented on each
> collection that has GC_PLUGIN_IGNORE. I'd suggest just to drop the requirement
> that GC_PLUGIN_IGNORE has to have a bug number. If we want to add a bug
number,
> we can add it as a comment.

I would prefer that we create a bug and link to that in the ignore. This case is
so special that I think the GC plugin should build in the exception (ie,
disallow raw ptrs, except to AudioNodes or something similar). The explicit bug
would help to track these cases. Also, there should be no PLUGIN_IGNORES in the
code base, so each is either a bug of the plugin, a bug in the code, or a
temporary hack with some future way of removing it. In each case, a tracking bug
is a good thing to have.

[Raymond Toy, 2014-08-15 16:20:10]

lgtm.

I didn't completely follow all of the previous changes that lead up to this, but
this looks good. This is really, really exciting.

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

https://codereview.chromium.org/438293003/diff/260001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:75: // FIXME: Oilpan: This should be
ThreadSafeRefCountedGarbageCollectedWillBeGarbageCollectedFinalized.
Perhaps it's obvious, but why should this be GarbageCollectedFinalized? And what
is preventing it from being so?

[tkent, 2014-08-18 04:18:24]

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

https://codereview.chromium.org/438293003/diff/260001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:52: return
adoptRefCountedGarbageCollected(new AudioBufferSourceNode(context, sampleRate));
adoptRefCountedGarbageCollected should be
adoptRefCountedGarbageCollectedWillBeNoop

[haraken, 2014-08-18 05:09:40]

I confirmed that the patch set 15 passes all webaudio/ tests in both oilpan and
non-oilpan builds. PTAL.


On 2014/08/15 10:29:54, zerny-chromium wrote:
> > > Is GC_PLUGIN_IGNORE necessary?  If so, we should avoid empty string
> argument,
> > > should file a bug and fill the bug URL.
> > 
> > GC_PLUGIN_IGNORE is necessary because the plugin complains about raw
pointers
> to
> > on-heap objects in non-oilpan builds.
> > 
> > That being said, those raw pointers are not bugs, as I commented on each
> > collection that has GC_PLUGIN_IGNORE. I'd suggest just to drop the
requirement
> > that GC_PLUGIN_IGNORE has to have a bug number. If we want to add a bug
> number,
> > we can add it as a comment.
> 
> I would prefer that we create a bug and link to that in the ignore. This case
is
> so special that I think the GC plugin should build in the exception (ie,
> disallow raw ptrs, except to AudioNodes or something similar). The explicit
bug
> would help to track these cases. Also, there should be no PLUGIN_IGNORES in
the
> code base, so each is either a bug of the plugin, a bug in the code, or a
> temporary hack with some future way of removing it. In each case, a tracking
bug
> is a good thing to have.

Done. Filed a bug: https://code.google.com/p/chromium/issues/detail?id=404527

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

https://codereview.chromium.org/438293003/diff/260001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:52: return
adoptRefCountedGarbageCollected(new AudioBufferSourceNode(context, sampleRate));
On 2014/08/18 04:18:24, tkent wrote:
> adoptRefCountedGarbageCollected should be
> adoptRefCountedGarbageCollectedWillBeNoop

Done.

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

https://codereview.chromium.org/438293003/diff/260001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:75: // FIXME: Oilpan: This should be
ThreadSafeRefCountedGarbageCollectedWillBeGarbageCollectedFinalized.
On 2014/08/15 16:20:10, Raymond Toy wrote:
> Perhaps it's obvious, but why should this be GarbageCollectedFinalized? And
what
> is preventing it from being so?

In oilpan builds, this should be GarbageCollectedFinalized.

In non-oilpan builds, this should be ThreadSafeRefCountedGarbageCollected. This
is because EventTargetWithInlineData is still off-heap and ref-counted in
non-oilpan builds. (Actually I think AudioContext no longer needs to be
"ThreadSafe"RefCounted, but either way let me address the issue in a follow-up
CL.)

[tkent, 2014-08-18 08:54:52]

lgtm

https://codereview.chromium.org/438293003/diff/300001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/438293003/diff/300001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:595: GC_PLUGIN_IGNORE("")
bug URL?

https://codereview.chromium.org/438293003/diff/300001/Source/platform/audio/A...
File Source/platform/audio/AudioDSPKernel.h (right):

https://codereview.chromium.org/438293003/diff/300001/Source/platform/audio/A...
Source/platform/audio/AudioDSPKernel.h:76: GC_PLUGIN_IGNORE("")
Bug URL?

[haraken, 2014-08-18 09:11:03]

https://codereview.chromium.org/438293003/diff/300001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/438293003/diff/300001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:595: GC_PLUGIN_IGNORE("")
On 2014/08/18 08:54:51, tkent wrote:
> bug URL?

Filed issue 404577 and added the bug number here.

https://codereview.chromium.org/438293003/diff/300001/Source/platform/audio/A...
File Source/platform/audio/AudioDSPKernel.h (right):

https://codereview.chromium.org/438293003/diff/300001/Source/platform/audio/A...
Source/platform/audio/AudioDSPKernel.h:76: GC_PLUGIN_IGNORE("")
On 2014/08/18 08:54:51, tkent wrote:
> Bug URL?

Filed issue 404578 and added the bug number here.

[haraken, 2014-08-18 09:11:10]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-18 09:11:36]

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

[commit-bot: I haz the power, 2014-08-18 10:18:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23177)

[commit-bot: I haz the power, 2014-08-18 11:23:53]

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

[commit-bot: I haz the power, 2014-08-18 11:23:54]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23194)

[haraken, 2014-08-18 12:18:43]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-18 12:19:42]

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

[commit-bot: I haz the power, 2014-08-18 13:30:42]

Committed patchset #17 (320001) as 180457