Fix crash when the OfflineAudioContext constructor is given 0 as numberOfFrames

Fix crash when the OfflineAudioContext constructor is given 0 as numberOfFrames

Fix crash when the OfflineAudioContext constructor is given 0 as numberOfFrames.
The constructor now throws an exception in this case since we cannot construct
an AudioBuffer with zero frames.

R=kbr@chromium.org
BUG=306536
TEST=webaudio/offlineaudiocontext-constructor.html

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

[Inactive, 2013-10-11 16:01:11]

See associated bug for stack trace.

[Ken Russell (switch to Gerrit), 2013-10-11 18:57:43]

LGTM, but please wait for review from rtoy as well.

It looks like the error conditions need to be specified in
http://webaudio.github.io/web-audio-api/ .

[Raymond Toy (Google), 2013-10-11 20:04:39]

https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
File LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html (right):

https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html:8:
description("Checks that the OfflineAudioContext does not crash on bad input");
I think this file should not include "crash" in the name. Maybe name it
offlineaudiocontext-constructor.html. The description probably should not say
crash either.

https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html:11:
shouldThrow("new webkitOfflineAudioContext(0, 0, 46000)");
Why 46000 for the sample rate? I would choose a more typical value like 44100.

Also, it's not clear what 0 channels should do for an offline context. (Spec
doesn't say). So to be safe, I would use 1 channel.

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

https://codereview.chromium.org/27039002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:159: ASSERT(m_destinationNode);
Did these two asserts catch the crash?

I'm just concerned that debug code will find this, but release code will not and
crash in some strange place.

It would be nice if we threw an error in these cases too.

[Inactive, 2013-10-11 20:14:30]

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

https://codereview.chromium.org/27039002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:159: ASSERT(m_destinationNode);
On 2013/10/11 20:04:40, rtoy wrote:
> Did these two asserts catch the crash?
> 
> I'm just concerned that debug code will find this, but release code will not
and
> crash in some strange place.
> 
> It would be nice if we threw an error in these cases too.

Yes, these assertions would have caught the crash. The issue was that
AudioBuffer::create() would fail due to bad input and return 0.
Then the OfflineAudioDestinationNode constructor would try the use the null
renderTarget.

It is up to the factory methods to validate the input properly so that the
constructor does not fail. I think those assertion are useful indicators that
the factory did not validate properly. We cannot throw js exceptions here, they
should be thrown earlier in the factory.

[Inactive, 2013-10-11 20:48:17]

https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
File LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html (right):

https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html:8:
description("Checks that the OfflineAudioContext does not crash on bad input");
On 2013/10/11 20:04:40, rtoy wrote:
> I think this file should not include "crash" in the name. Maybe name it
> offlineaudiocontext-constructor.html. The description probably should not say
> crash either.  

Done.

https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html:11:
shouldThrow("new webkitOfflineAudioContext(0, 0, 46000)");
On 2013/10/11 20:04:40, rtoy wrote:
> Why 46000 for the sample rate? I would choose a more typical value like 44100.
> 
> Also, it's not clear what 0 channels should do for an offline context. (Spec
> doesn't say). So to be safe, I would use 1 channel.

Done.

[Raymond Toy (Google), 2013-10-11 21:02:08]

On 2013/10/11 20:14:30, Chris Dumez wrote:
>
https://codereview.chromium.org/27039002/diff/1/Source/modules/webaudio/Audio...
> File Source/modules/webaudio/AudioContext.cpp (right):
> 
>
https://codereview.chromium.org/27039002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioContext.cpp:159: ASSERT(m_destinationNode);
> On 2013/10/11 20:04:40, rtoy wrote:
> > Did these two asserts catch the crash?
> > 
> > I'm just concerned that debug code will find this, but release code will not
> and
> > crash in some strange place.
> > 
> > It would be nice if we threw an error in these cases too.
> 
> Yes, these assertions would have caught the crash. The issue was that
> AudioBuffer::create() would fail due to bad input and return 0.
> Then the OfflineAudioDestinationNode constructor would try the use the null
> renderTarget.
> 
> It is up to the factory methods to validate the input properly so that the
> constructor does not fail. I think those assertion are useful indicators that
> the factory did not validate properly. We cannot throw js exceptions here,
they
> should be thrown earlier in the factory.

Or they could be caught in OfflineAudioContext::create, which can throw
exceptions.

[Raymond Toy (Google), 2013-10-11 21:02:32]

On 2013/10/11 20:48:17, Chris Dumez wrote:
>
https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
> File LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html (right):
> 
>
https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
> LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html:8:
> description("Checks that the OfflineAudioContext does not crash on bad
input");
> On 2013/10/11 20:04:40, rtoy wrote:
> > I think this file should not include "crash" in the name. Maybe name it
> > offlineaudiocontext-constructor.html. The description probably should not
say
> > crash either.  
> 
> Done.
> 
>
https://codereview.chromium.org/27039002/diff/1/LayoutTests/webaudio/offlinea...
> LayoutTests/webaudio/offlineaudiocontext-constructor-crash.html:11:
> shouldThrow("new webkitOfflineAudioContext(0, 0, 46000)");
> On 2013/10/11 20:04:40, rtoy wrote:
> > Why 46000 for the sample rate? I would choose a more typical value like
44100.
> > 
> > Also, it's not clear what 0 channels should do for an offline context. (Spec
> > doesn't say). So to be safe, I would use 1 channel.
> 
> Done.

LGTM

[Inactive, 2013-10-11 21:22:12]

On 2013/10/11 21:02:08, rtoy wrote:
> On 2013/10/11 20:14:30, Chris Dumez wrote:
> >
>
https://codereview.chromium.org/27039002/diff/1/Source/modules/webaudio/Audio...
> > File Source/modules/webaudio/AudioContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/27039002/diff/1/Source/modules/webaudio/Audio...
> > Source/modules/webaudio/AudioContext.cpp:159: ASSERT(m_destinationNode);
> > On 2013/10/11 20:04:40, rtoy wrote:
> > > Did these two asserts catch the crash?
> > > 
> > > I'm just concerned that debug code will find this, but release code will
not
> > and
> > > crash in some strange place.
> > > 
> > > It would be nice if we threw an error in these cases too.
> > 
> > Yes, these assertions would have caught the crash. The issue was that
> > AudioBuffer::create() would fail due to bad input and return 0.
> > Then the OfflineAudioDestinationNode constructor would try the use the null
> > renderTarget.
> > 
> > It is up to the factory methods to validate the input properly so that the
> > constructor does not fail. I think those assertion are useful indicators
that
> > the factory did not validate properly. We cannot throw js exceptions here,
> they
> > should be thrown earlier in the factory.
> 
> Or they could be caught in OfflineAudioContext::create, which can throw
> exceptions.

Yes, this is what I meant by factory method (e.g.
OfflineAudioContext::create()). We already validate input in the factories and
throw exceptions if needed. The issue what that we failed to validate the number
of frames in OfflineAudioContext::create().

[commit-bot: I haz the power, 2013-10-11 21:23:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/27039002/10001

[commit-bot: I haz the power, 2013-10-12 05:16:27]

Change committed as 159499