Require use of AudioBus::create() to avoid ref-counting issues

Require use of
AudioBus::create() to avoid ref-counting issues

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

[Chris Rogers, 2013-05-02 17:48:46]

Thanks Xingnan, just a few minor comments.

https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/Au...
File Source/core/platform/audio/AudioFIFO.cpp (right):

https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/Au...
Source/core/platform/audio/AudioFIFO.cpp:38: : m_fifoAudioBus(new
AudioBus(numberOfChannels, fifoLength))
need to wrap the new in adoptRef()

https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/Au...
File Source/core/platform/audio/AudioPullFIFO.cpp (right):

https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/Au...
Source/core/platform/audio/AudioPullFIFO.cpp:41: , m_tempBus(new
AudioBus(numberOfChannels, providerSize))
adoptRef()

https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/ch...
File Source/core/platform/audio/chromium/AudioDestinationChromium.cpp (right):

https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/ch...
Source/core/platform/audio/chromium/AudioDestinationChromium.cpp:58: ,
m_sampleRate(sampleRate)
adoptRef()

https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/ch...
Source/core/platform/audio/chromium/AudioDestinationChromium.cpp:86:
RefPtr<AudioBus> silence = adoptRef<AudioBus>(new AudioBus(2,
renderBufferSize));
Does this need to be adoptRef<AudioBus> or can it just be adoptRef()?

https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioDestinationNode.h:70: : m_sourceBus(new AudioBus(2,
AudioNode::ProcessingSizeInFrames)) // FIXME: handle non-stereo local input.
adoptRef()

https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Media...
File Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp (right):

https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Media...
Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:48: , m_mixBus(new
AudioBus(numberOfChannels, ProcessingSizeInFrames))
adoptRef()

https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Scrip...
File Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Scrip...
Source/modules/webaudio/ScriptProcessorNode.cpp:82: , m_internalInputBus(new
AudioBus(numberOfInputChannels, AudioNode::ProcessingSizeInFrames, false))
adoptRef()

[xingnan.wang, 2013-05-03 03:18:51]

Done.

On 2013/05/02 17:48:46, Chris Rogers wrote:
> Thanks Xingnan, just a few minor comments.
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/Au...
> File Source/core/platform/audio/AudioFIFO.cpp (right):
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/Au...
> Source/core/platform/audio/AudioFIFO.cpp:38: : m_fifoAudioBus(new
> AudioBus(numberOfChannels, fifoLength))
> need to wrap the new in adoptRef()
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/Au...
> File Source/core/platform/audio/AudioPullFIFO.cpp (right):
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/Au...
> Source/core/platform/audio/AudioPullFIFO.cpp:41: , m_tempBus(new
> AudioBus(numberOfChannels, providerSize))
> adoptRef()
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/ch...
> File Source/core/platform/audio/chromium/AudioDestinationChromium.cpp (right):
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/ch...
> Source/core/platform/audio/chromium/AudioDestinationChromium.cpp:58: ,
> m_sampleRate(sampleRate)
> adoptRef()
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/core/platform/audio/ch...
> Source/core/platform/audio/chromium/AudioDestinationChromium.cpp:86:
> RefPtr<AudioBus> silence = adoptRef<AudioBus>(new AudioBus(2,
> renderBufferSize));
> Does this need to be adoptRef<AudioBus> or can it just be adoptRef()?
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Audio...
> File Source/modules/webaudio/AudioDestinationNode.h (right):
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioDestinationNode.h:70: : m_sourceBus(new
AudioBus(2,
> AudioNode::ProcessingSizeInFrames)) // FIXME: handle non-stereo local input.
> adoptRef()
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Media...
> File Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp (right):
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Media...
> Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:48: , m_mixBus(new
> AudioBus(numberOfChannels, ProcessingSizeInFrames))
> adoptRef()
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Scrip...
> File Source/modules/webaudio/ScriptProcessorNode.cpp (right):
> 
>
https://codereview.chromium.org/14628008/diff/1/Source/modules/webaudio/Scrip...
> Source/modules/webaudio/ScriptProcessorNode.cpp:82: , m_internalInputBus(new
> AudioBus(numberOfInputChannels, AudioNode::ProcessingSizeInFrames, false))
> adoptRef()

[Chris Rogers, 2013-05-03 20:24:55]

Sorry Xingnan, I should have realized this earlier in my last comments.  To be
more resilient and protect against these kinds of bugs in the future where we
create AudioBus objects as member variables or as stack-based objects we need to
make the AudioBus constructor private and add an AudioBus::create() method. 
Then we use the create() method instead of the pattern:

adoptRef(new AudioBus(numberOfChannels, fifoLength))

Also, please update the description for this CL (using the "Edit Issue" in
upper-left corner) from:

"Add RefPtr for the AudioBus which is class number" -> "Require use of
AudioBus::create() to avoid ref-counting issues"

[Chris Rogers, 2013-05-03 20:25:42]

You can use AudioBuffer:create() as an example...

[Chris Rogers, 2013-05-06 20:26:26]

Thanks for the changes - lgtm

[commit-bot: I haz the power, 2013-05-06 20:27:46]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[aarya, 2013-05-06 20:28:34]

lgtm

[commit-bot: I haz the power, 2013-05-06 20:28:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xingnan.wang@intel.com/14628008/21001

[aarya, 2013-05-06 20:30:16]

On 2013/05/06 20:27:46, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Chris, looks like owners file has your chromium account :( so, i did the lgtm
for cq to work.

[commit-bot: I haz the power, 2013-05-06 21:37:50]

Change committed as 149804