`setValueCurve` should throw on non-finite elements.

RESOLVED FIXED in Firefox 54

Status

()

Core
Web Audio
P2
normal
Rank:
20
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: padenot, Assigned: beekill, Mentored)

Tracking

(Depends on: 1 bug, {dev-doc-complete, good-first-bug})

Trunk
mozilla54
dev-doc-complete, good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Rank: 20
(Reporter)

Updated

a year ago
Depends on: 1184057

Updated

a year ago
Mentor: dminor@mozilla.com
Keywords: good-first-bug
(Assignee)

Comment 1

10 months ago
Hi Dan, the bug seems to depends on bug 1184057. Is it ok to work on this bug?
Flags: needinfo?(dminor)

Comment 2

10 months ago
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!
Flags: needinfo?(dminor)
(Assignee)

Comment 3

10 months ago
Sure, I'll work on this.

Comment 4

10 months ago
Thanks!
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 months ago
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
Flags: needinfo?(dminor)

Comment 6

10 months ago
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?
Flags: needinfo?(dminor) → needinfo?(padenot)
(Reporter)

Comment 7

10 months ago
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.
Flags: needinfo?(padenot)
(Assignee)

Comment 8

10 months ago
(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.
(Assignee)

Comment 9

10 months ago
Created attachment 8833588 [details] [diff] [review]
set_value_curve.patch

I changed the SyntaxError to TypeError
Attachment #8833588 - Flags: review?(dminor)
(Assignee)

Comment 10

10 months ago
Created attachment 8833589 [details] [diff] [review]
set_value_curve_testcase.patch

I added a test case to check whether TypeError is thrown when calling setValueCurve with Infinity and Nan
Attachment #8833589 - Flags: review?(dminor)

Comment 11

10 months ago
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".
Attachment #8833588 - Flags: review?(padenot)
Attachment #8833588 - Flags: review?(dminor)
Attachment #8833588 - Flags: review+

Comment 12

10 months ago
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"
Attachment #8833589 - Flags: review?(padenot)
Attachment #8833589 - Flags: review?(dminor)
Attachment #8833589 - Flags: review+
(Reporter)

Updated

10 months ago
Attachment #8833588 - Flags: review?(padenot) → review+
(Reporter)

Updated

10 months ago
Attachment #8833589 - Flags: review?(padenot) → review+
(Assignee)

Comment 13

10 months ago
Created attachment 8834014 [details] [diff] [review]
set_value_curve.patch

I changed the commit message like you said.
Attachment #8833588 - Attachment is obsolete: true
Attachment #8834014 - Flags: review?(padenot)
Attachment #8834014 - Flags: review?(dminor)
(Assignee)

Comment 14

10 months ago
Created attachment 8834015 [details] [diff] [review]
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
Attachment #8833589 - Attachment is obsolete: true
Attachment #8834015 - Flags: review?(padenot)
Attachment #8834015 - Flags: review?(dminor)

Updated

10 months ago
Attachment #8834014 - Flags: review?(dminor) → review+

Updated

10 months ago
Attachment #8834015 - Flags: review?(dminor) → review+
(Reporter)

Updated

10 months ago
Attachment #8834014 - Flags: review?(padenot) → review+
(Reporter)

Updated

10 months ago
Attachment #8834015 - Flags: review?(padenot) → review+
(Assignee)

Comment 15

10 months ago
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!
Flags: needinfo?(dminor)

Comment 16

10 months ago
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!
Flags: needinfo?(dminor)
(Assignee)

Comment 17

10 months ago
Created attachment 8834423 [details] [diff] [review]
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.
Attachment #8834423 - Flags: review?(padenot)
Attachment #8834423 - Flags: review?(dminor)
(Reporter)

Comment 18

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

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

Thanks !
Attachment #8834423 - Flags: review?(padenot) → review+

Comment 19

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

lgtm
Attachment #8834423 - Flags: review?(dminor) → review+
(Assignee)

Comment 20

10 months ago
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.
Flags: needinfo?(dminor)

Comment 21

10 months ago
Those all look like intermittent failures to me. Thanks!
Flags: needinfo?(dminor)
Keywords: checkin-needed

Comment 22

10 months ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 23

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59f634bb78c4
https://hg.mozilla.org/mozilla-central/rev/845451c03a19
https://hg.mozilla.org/mozilla-central/rev/9066a2ed8493
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Documentation updated:

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

https://developer.mozilla.org/en-US/Firefox/Releases/54
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.