1308437 - `setValueCurve` should throw on non-finite elements.

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Paul Adenot (:padenot)]

If the curve contains non-finite values, setValueCurveAtTime must signal a TypeError.  These values are illegal.

Spec changes at https://github.com/webaudio/web-audio-api/commit/a067bb9e273656becad3b3d660c20850516a175b.

Comment 1

[Bao Quan [:beekill]]

Hi Dan, the bug seems to depends on bug 1184057. Is it ok to work on this bug?

Comment 2

[Dan Minor [:dminor]]

Hi Bao, in this case I think it is fine, because you should only need to change the point where the values are first set and I don't believe that will conflict with the work planned for Bug 1184057. Please let me know if you are interested in working on this!

Comment 3

[Bao Quan [:beekill]]

Sure, I'll work on this.

Comment 4

[Dan Minor [:dminor]]

Thanks!

Comment 5

[Bao Quan [:beekill]]

The commit [1] referenced by Paul was denied [2]. So I guess we don’t have to do anything :)

Please correct me if I'm wrong.

[1] https://github.com/webaudio/web-audio-api/commit/a067bb9e273656becad3b3d660c20850516a175b
[2] https://github.com/WebAudio/web-audio-api/pull/819

Comment 6

[Dan Minor [:dminor]]

Looks like you're right :) I checked the spec and it looks like the WebIDL in the spec matches what is in tree, so maybe the planned changes were never made. :padenot, is this another one we can just close or does this require some spec work?

Comment 7

[Paul Adenot (:padenot)]

Indeed, it looks like we do the right thing, apart from the exception, that is not the right one: TypeError should be thrown, instead of SyntaxError:

http://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioEventTimeline.h#196

A test (probably web-platform test) would be a good addition as well.

Comment 8

[Bao Quan [:beekill]]

(In reply to Paul Adenot (:padenot) from comment #7)
> Indeed, it looks like we do the right thing, apart from the exception, that
> is not the right one: TypeError should be thrown, instead of SyntaxError:
> 
> http://searchfox.org/mozilla-central/source/dom/media/webaudio/
> AudioEventTimeline.h#196
> 
> A test (probably web-platform test) would be a good addition as well.

OK, I’ll modify the incorrect error and write a test case.

Comment 9

[Bao Quan [:beekill]]

Attachment: set_value_curve.patch

I changed the SyntaxError to TypeError

Comment 10

[Bao Quan [:beekill]]

Attachment: set_value_curve_testcase.patch

I added a test case to check whether TypeError is thrown when calling setValueCurve with Infinity and Nan

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 8833588 [details] [diff] [review]
set_value_curve.patch

Review of attachment 8833588 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Please update your commit message to be more descriptive, e.g. "Change the exception thrown by `setValueCurve` on non-finite elements to TypeError".

Comment 12

[Dan Minor [:dminor]]

Comment on attachment 8833589 [details] [diff] [review]
set_value_curve_testcase.patch

Review of attachment 8833589 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: dom/media/webaudio/test/test_setValueCurveWithNonFiniteElements.html
@@ +21,5 @@
> +  arr[4] = 0.5;
> +
> +  try {
> +    audioParam.setValueCurveAtTime(arr, audioContext.currentTime(), 2);
> +    ok(false, "We can't setValueCurve with Infinity but we can");

Please change "We can't" to "We shouldn't be able to call"

@@ +38,5 @@
> +  arr[4] = 0.5;
> +
> +  try {
> +    audioParam.setValueCurveAtTime(arr, audioContext.currentTime(), 2);
> +    ok(false, "We can't setValueCurve with NaN but we can");

Please change "We can't" to "We shouldn't be able to call"

Comment 13

[Bao Quan [:beekill]]

Attachment: set_value_curve.patch

I changed the commit message like you said.

Comment 14

[Bao Quan [:beekill]]

Attachment: set_value_curve_testcase.patch

I changed "We can't" to "We shouldn't be able to".

I also pushed the patches to try server. Here's the link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5ec32989bbae408c75069d5b9679eb2d3a32d

Comment 15

[Bao Quan [:beekill]]

Hi Dan,
Here's the try server's result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5ec32989bbae408c75069d5b9679eb2d3a32d.

Looks like there are a lot of failed tests but none of them related to my patches, I guess. Can you have a look at the link? Thanks!

Comment 16

[Dan Minor [:dminor]]

Hi Bao,

The gtest failures look significant. My guess is that you would have to update the exception type in this test http://searchfox.org/mozilla-central/source/dom/media/webaudio/gtest/TestAudioEventTimeline.cpp#157.
The rest look like normal intermittent failures in the tests. Thanks!

Comment 17

[Bao Quan [:beekill]]

Attachment: set_value_curve_update_gtest.patch

Thanks Dan for pointing that out. I updated the expected error value to TypeError and pushed the patch to try server. 

I'll post the result back asap.

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8834423 [details] [diff] [review]
set_value_curve_update_gtest.patch

Review of attachment 8834423 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks !

Comment 19

[Dan Minor [:dminor]]

Comment on attachment 8834423 [details] [diff] [review]
set_value_curve_update_gtest.patch

lgtm

Comment 20

[Bao Quan [:beekill]]

Hi Dan, here's the latest result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5c47533dc212bb4b398a1aa3503a6a62579ccc

All the previous GTest failures are gone. Can you have a look a the link to see if there's anything wrong? Thank you.

Comment 21

[Dan Minor [:dminor]]

Those all look like intermittent failures to me. Thanks!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59f634bb78c4
Part 1: Change the exception thrown by 'setValueCurve' on non-finite elements to TypeError. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/845451c03a19
Part 2: Add test case to verify TypeError is thrown when calling setValueCurve on non-finite elements. r=dminor r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/9066a2ed8493
Part 3: Change expected error to TypeError when testing setValueCurveAtTime with non-finite elements in TestAudioEventTimeline.cpp. r=dminor, a=padenot

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59f634bb78c4
https://hg.mozilla.org/mozilla-central/rev/845451c03a19
https://hg.mozilla.org/mozilla-central/rev/9066a2ed8493

Comment 24

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/AudioParam/setValueCurveAtTime

https://developer.mozilla.org/en-US/Firefox/Releases/54