Closed
Bug 1472095
Opened 6 years ago
Closed 6 years ago
setValueCurveAtTime throws incorrect exceptions
Categories
(Core :: Web Audio, defect, P2)
Core
Web Audio
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(nnn_bikiu0707)
Flags: needinfo?(dminor)
Assignee | ||
Comment 3•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → padenot
Updated•6 years ago
|
Rank: 15
Priority: -- → P2
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-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/#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-
Reporter | ||
Comment 5•6 years ago
|
||
> 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 6•6 years ago
|
||
mozreview-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/#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+
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
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.
Reporter | ||
Comment 11•6 years ago
|
||
mozreview-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/#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+
Reporter | ||
Comment 12•6 years ago
|
||
mozreview-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 13•6 years ago
|
||
mozreview-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?
Assignee | ||
Comment 14•6 years ago
|
||
Indeed. I've fixed this, and removed the removal of the if. It was unnecessary.
Comment 15•6 years ago
|
||
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
Comment 16•6 years ago
|
||
Backed out for gtest failures on AudioEventTimeline. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=1f89fc574e6ccffbdf96ae74e1151c2a8442c7c3&tochange=9d4c8ab1fa61392f2c7449ed0cc5cafa091bd1cc&filter-searchStr=gtest&selectedJob=188035489 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=188035489&repo=mozilla-inbound&lineNumber=22821 Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d4c8ab1fa61392f2c7449ed0cc5cafa091bd1cc Please also take a look at these wpt failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=1f89fc574e6ccffbdf96ae74e1151c2a8442c7c3&tochange=9d4c8ab1fa61392f2c7449ed0cc5cafa091bd1cc&filter-searchStr=wpt&selectedJob=188035510 https://treeherder.mozilla.org/logviewer.html#?job_id=188035510&repo=mozilla-inbound&lineNumber=8231
Flags: needinfo?(padenot)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(padenot)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•6 years ago
|
Attachment #8991942 -
Flags: review?(karlt)
Comment 21•6 years ago
|
||
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
Comment 23•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ff364dae8e3d https://hg.mozilla.org/mozilla-central/rev/ae918d7261dd https://hg.mozilla.org/mozilla-central/rev/15f3954dd4ef
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR was closed without merging
Upstream PR merged
Comment 26•6 years ago
|
||
Probably ought to make sure the docs mention the correct exceptions.
Keywords: dev-doc-needed
Comment 27•6 years ago
|
||
The doc is correct already. Added a note to Firefox 63 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•