Closed Bug 1472095 Opened 3 years ago Closed 3 years ago

setValueCurveAtTime throws incorrect exceptions

Categories

(Core :: Web Audio, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bzbarsky, Assigned: padenot)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

In bug 1308437 it was switched from throwing a DOMException named "SyntaxError" to throwing a DOMException named "TypeError".

But it should be throwing a TypeError, not a DOMException to start with.  That is, it should use the ThrowTypeError() API on ErrorResult.  NS_ERROR_TYPE_ERR is a confusing abomination that needs to die; see bug 927610.
Flags: needinfo?(nnn_bikiu0707)
Flags: needinfo?(dminor)
Blocks: 927610
Flags: needinfo?(nnn_bikiu0707)
Flags: needinfo?(dminor)
Boris, for now this is using a nsAutoString for the message, but I'm afraid it won't be possible to localize the error. I haven't found a mechanism to write custom error messages for type errors, in a file that can be picked up by our localization tools, have I missed something?

Maybe it's good enough though.
Flags: needinfo?(bzbarsky)
Assignee: nobody → padenot
Rank: 15
Priority: -- → P2
Comment on attachment 8989159 [details]
Bug 1472095 - Update the web-platform-tests for the Web Audio API to expect the right kind of type error.

https://reviewboard.mozilla.org/r/254226/#review261172

This needs tests.

In theory, the wpts in webaudio/ would catch this, since they test this codepath, but they don't do so because the throw() method in webaudio/resources/audit.js is too loose: it just compares the exception's .name instead of testing that the exception is the right sort of exception.

Do you mind updating the test bits so they would have caught this?

I guess it doesn't help that it just takes a string, which makes it impossible to tell whether a DOMException or an ES Error subtype is expected.  Ideally there would be a throw_domexception() which takes the DOMException .name and a throw() which takes the constructor for the expected ES Error and tests `error.constructor === expected` or so.

::: dom/media/webaudio/AudioEventTimeline.h:161
(Diff revision 1)
>          return false;
>        }
>        for (uint32_t i = 0; i < aEvent.mCurveLength; ++i) {
>          if (!IsValid(aEvent.mCurve[i])) {
> -          aRv.Throw(NS_ERROR_TYPE_ERR);
> +          nsAutoString message;
> +          message.AppendPrintf("Index %u of the array passed to setValueCurveAtTime", i);

s/Index/Element/, right?

I guess we need to do this check manually because our setValueCuveAtTime takes Float32Array where the spec takes `sequence<float>` (which would do the check in the bindings)?  Is there a bug to align with the  spec here?
Attachment #8989159 - Flags: review?(bzbarsky) → review-
> but I'm afraid it won't be possible to localize the error

We don't localize web-visible exception strings, for various reasons.  The most important is that it could make sites fail only in one particular localization of the browser, depending on what processing they do on their exceptions.
Flags: needinfo?(bzbarsky)
Comment on attachment 8989158 [details]
Bug 1472095 - Update web-platform-test's audit.js file throw() method to be able to pass in exactly the error or exception to expect.

https://reviewboard.mozilla.org/r/254224/#review261218

::: dom/media/webaudio/gtest/TestAudioEventTimeline.cpp:14
(Diff revision 1)
>  #include "gtest/gtest.h"
> +#include "ErrorList.h"
> +
> +namespace mozilla {
> +  namespace dom {
> +    const int MSG_NOT_FINITE = -1;

I guess this works until something in AudioEventTimeline.cpp is tested?

Do you recall why ErrorResult.h can't be used in the gtest?
Attachment #8989158 - Flags: review?(karlt) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Comment on attachment 8989159 [details]
> Bug 1472095 - Throw a proper TypeError instead of a DOMException with
> NS_ERROR_TYPE_ERR when passing an array containing non-finite values in
> AudioParam.setValueCurveAtTime.
> 
> https://reviewboard.mozilla.org/r/254226/#review261172
> 
> This needs tests.
> 
> In theory, the wpts in webaudio/ would catch this, since they test this
> codepath, but they don't do so because the throw() method in
> webaudio/resources/audit.js is too loose: it just compares the exception's
> .name instead of testing that the exception is the right sort of exception.
> 
> Do you mind updating the test bits so they would have caught this?
> 
> I guess it doesn't help that it just takes a string, which makes it
> impossible to tell whether a DOMException or an ES Error subtype is
> expected.  Ideally there would be a throw_domexception() which takes the
> DOMException .name and a throw() which takes the constructor for the
> expected ES Error and tests `error.constructor === expected` or so.

I'm going to have a go at this yes.
 
> I guess we need to do this check manually because our setValueCuveAtTime
> takes Float32Array where the spec takes `sequence<float>` (which would do
> the check in the bindings)?  Is there a bug to align with the  spec here?

Indeed better handled that way. I've posted a patch in bug 1421091 that was opened a short while ago by the chromium folks, and will remove our custom code in this bug.
I ended up removing the patch that was manually throwing the type error altogether, in favor of directly fixing bug 1421091 (and adjusting the wpt expectations there). This bug is now about fixing audit.js in the first patch, and changing all the call sites in the second patch (which is simply the result of the execution of a command, which is in the commit message).

I've made the exception/error more strict it via dispatching on the argument, seemingly in line with the style of the file. I think what I'm using to comparing the "types" works, but I'm not too knowledgeable, maybe I'm missing an edge case.
Comment on attachment 8989158 [details]
Bug 1472095 - Update web-platform-test's audit.js file throw() method to be able to pass in exactly the error or exception to expect.

https://reviewboard.mozilla.org/r/254224/#review263234

Do we still need the code at https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/dom/media/webaudio/AudioEventTimeline.h#156-161 (the IsValid checks for entries of aEvent.mCurve in AudioEventTimeline::ValidateEvent)?

r=me with the nits addressed.

::: testing/web-platform/tests/webaudio/resources/audit.js:313
(Diff revision 2)
>              ': "' + error.message + '"';
>          if (this._expected === null || this._expected === undefined) {
>            // The expected error type was not given.
>            didThrowCorrectly = true;
>            passDetail = '${actual} threw ' + error.name + errorMessage + '.';
> -        } else if (error.name === this._expected) {
> +        } else if (this._expected.constructor == String &&

typeof(this._expected) == "string" is a better test here, no?

::: testing/web-platform/tests/webaudio/resources/audit.js:320
(Diff revision 2)
> +                   error.name === this._expected) {
> +          // A DOMException was thrown and expected, and the names match
> +          didThrowCorrectly = true;
> +          passDetail = '${actual} threw ${expected}' + errorMessage + '.';
> +        } else if (this._expected.prototype &&
> +                   this._expected.prototype.constructor == error.constructor &&

Why are we checking for the "this._expected.prototype" thing?

And if we do want to test it, why are we then getting its .constructor?  Seems like we should just test:

    if (this._expected == error.constructor && this._expected.name == error.name)
Attachment #8989158 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989159 [details]
Bug 1472095 - Update the web-platform-tests for the Web Audio API to expect the right kind of type error.

https://reviewboard.mozilla.org/r/254226/#review263236
Attachment #8989159 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989158 [details]
Bug 1472095 - Update web-platform-test's audit.js file throw() method to be able to pass in exactly the error or exception to expect.

https://reviewboard.mozilla.org/r/254224/#review263614

::: testing/web-platform/tests/webaudio/resources/audit.js:82
(Diff revision 2)
> +      case 'function':
> +        if (Error.isPrototypeOf(target)) {
> +          targetString = "EcmaScript error " + target.name;
> +        } else {
> +          targetString = target.name;

Is it this change that caused subtests of delaynode-maxdelaylimit.html
previously named

`() => context.createDelay(180) threw NotSupportedError: "Operation is not supported".`	
`() => context.createDelay(0) threw NotSupportedError: "Operation is not supported".`
`() => context.createDelay(-1) threw NotSupportedError: "Operation is not supported".`
`() => context.createDelay(NaN) threw TypeError: "Argument 1 of BaseAudioContext.createDelay is not a finite floating-point value.".`

to now be named?

`   threw NotSupportedError: "Operation is not supported".`

Unnamed functions should be converted to strings.
That seems like a regression that should be fixed, even if the known
symptoms would be papered over by
https://reviewboard.mozilla.org/r/256532/diff/1

::: testing/web-platform/tests/webaudio/resources/audit.js
(Diff revision 2)
> -      if (args.length > 0)
> -        this._expected = args[0];
> +      this._expected = args[0];

"|expected| is optional" and so is this change necessary?
Indeed. I've fixed this, and removed the removal of the if. It was unnecessary.
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/721346b51bf3
Explicitely label the assertions for DelayNode.delayTime tests, and test the error type. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cfa0dff8393
Update web-platform-test's audit.js file throw() method to be able to pass in exactly the error or exception to expect. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f89fc574e6c
Update the web-platform-tests for the Web Audio API to expect the right kind of type error. r=bz
Flags: needinfo?(padenot)
Comment on attachment 8991942 [details]
Bug 1472095 - Explicitely label the assertions for DelayNode.delayTime tests, and test the error type.

https://reviewboard.mozilla.org/r/256850/#review263710

r+ed by karlt in bug 1472095, where he suggested to move this patch here.
Attachment #8991942 - Flags: review+
Attachment #8991942 - Flags: review?(karlt)
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/ff364dae8e3d
Explicitely label the assertions for DelayNode.delayTime tests, and test the error type. r=padenot
https://hg.mozilla.org/integration/autoland/rev/ae918d7261dd
Update web-platform-test's audit.js file throw() method to be able to pass in exactly the error or exception to expect. r=bz
https://hg.mozilla.org/integration/autoland/rev/15f3954dd4ef
Update the web-platform-tests for the Web Audio API to expect the right kind of type error. r=bz
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11983 for changes under testing/web-platform/tests
Upstream PR was closed without merging
See Also: → 1485198
Probably ought to make sure the docs mention the correct exceptions.
Keywords: dev-doc-needed
The doc is correct already. Added a note to Firefox 63 for developers.
You need to log in before you can comment on or make changes to this bug.