User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36 Steps to reproduce: I am testing some MediaRecorder functionality on FireFox Dev Edition 48.0a2 (2016-05-26) on OS X 10.11.5 using this https://jsfiddle.net/rshodaw8/9/ Run that fiddle and it will try to get one default resolution video stream and one 160x90 video stream. It then uses MediaRecorder to record the first stream for 5 seconds and then it swaps out the first video track with the second. NOTE: Whether this operation should work or not is a question for another ticket and I will raise that elsewhere. Actual results: I see the following output in the console: Use of getAttributeNode() is deprecated. Use getAttribute() instead._dist-editor.js:2:25246 Starting recorder Recording... Waiting for 5 seconds... done Replacing track with low resolution Waiting for 5 seconds... ERROR: error { target: MediaRecorder, isTrusted: true, name: "GenericError", currentTarget: MediaRecorder, eventPhase: 2, bubbles: false, cancelable: false, defaultPrevented: false, timeStamp: 1464257988236837, originalTarget: MediaRecorder, explicitOriginalTarget: MediaRecorder } Recording stopped done Stopping recorder ERROR: InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable So, sometime after swapping the tracks but during the second 5s timeout, an error is raised. This error is not instanceof Error. It has no message property. It doesn't seem to adhere to the spec, unless I am mistaken. Expected results: Either there should be no error and the track switching would be supported, or more likely right now the IllegalStreamModification error should have been raised: https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/onerror

Our current implementation is [1]. I'll also note that the spec has changed here and we're not up to date with it yet. [1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/RecordErrorEvent.webidl

This is an HTML/JS file to ease debugging. It does the same as the fiddle which is apparently difficult to test.

Firefox currently fires a non-spec event with a name property. Used like this: recorder.onerror = e => { throw new Error(e.name); } Instead, we need to update to the latest spec and implement MediaRecorderErrorEvent [1]: recorder.onerror = e => { throw e.error }; Part of the implementation work will be to capture the callstack of recorder.start(), to include as stack in the e.error DOMException object, like the spec encourages [2]. [1] https://w3c.github.io/mediacapture-record/MediaRecorder.html#idl-def-ErrorEvent [2] https://w3c.github.io/mediacapture-record/MediaRecorder.html#erroreventinit

Workaround: recorder.onerror = e => { throw e.error || new Error(e.name); };

I've got some further work in progress around capturing the stack trace for the DOMException. However I've got some leaks in there that I'm yet to sort out. As such I'm opting for review of the existing, (hopefully) working code, and will follow up with capturing the stack traces.

Comment on attachment 8894284 [details] Bug 1275856 - Add MediaRecorderErrorEvent webidl files and update moz.build. https://reviewboard.mozilla.org/r/165368/#review170856 Lgtm with duplicate entry removed. Note you'll need a DOM reviewer as well for touching webidl. ::: dom/webidl/moz.build:684 (Diff revision 2) > 'MediaKeyStatusMap.webidl', > 'MediaKeySystemAccess.webidl', > 'MediaList.webidl', > 'MediaQueryList.webidl', > 'MediaRecorder.webidl', > + 'MediaRecorderErrorEvent.webidl', The pattern here appears to be: put it either under WEBIDL_FILES or GENERATED_EVENTS_WEBIDL_FILES, but not both. I suspect we want the latter?

Comment on attachment 8894285 [details] Bug 1275856 - Fire MediaRecorderErrorEvent in media recorder. https://reviewboard.mozilla.org/r/165370/#review171014 Need to fire only valid errors, populate message field, and add stack. Also, more work (in new or same issue): A comparison to the spec reveals that firing an event is supposed to be terminal, and "set state to inactive" [1]. For SecurityError we should also "discard any data that it has gathered". ::: dom/media/MediaRecorder.cpp:1401 (Diff revision 4) > + case NS_ERROR_DOM_INVALID_STATE_ERR: > + case NS_ERROR_DOM_NOT_SUPPORTED_ERR: You've added two new errors here. Why? According to the spec [1], the only two errors that can be fired are "SecurityError", and "UnknownError" for everything else. This means any gecko-specifc debug info about the actual failure (like the aRv number) would need to be captured in the error.message field instead. [1] https://w3c.github.io/mediacapture-record/MediaRecorder.html#dom-mediarecorder-start ::: dom/media/MediaRecorder.cpp:1406 (Diff revision 4) > - default: > + default: > - errorMsg = NS_LITERAL_STRING("GenericError"); > + init.mError = DOMException::Create(NS_ERROR_DOM_ABORT_ERR); > } Missing the stack part. ::: dom/media/MediaRecorder.cpp:1407 (Diff revision 4) > - break; > - case NS_ERROR_OUT_OF_MEMORY: > - errorMsg = NS_LITERAL_STRING("OutOfMemoryError"); > - break; > + break; > - default: > + default: > - errorMsg = NS_LITERAL_STRING("GenericError"); > + init.mError = DOMException::Create(NS_ERROR_DOM_ABORT_ERR); NS_ERROR_DOM_UNKNOWN_ERR

Comment on attachment 8894286 [details] Bug 1275856 - Remove now unused RecrodErrorEvent. https://reviewboard.mozilla.org/r/165372/#review171018

Comment on attachment 8894287 [details] Bug 1275856- Update tests to reflect new MediaRecorderErrorEvent. https://reviewboard.mozilla.org/r/165374/#review171020 Some nits and some more changes needed if we end up fixing the state change in this issue. ::: commit-message-7a18e:3 (Diff revision 4) > +With the MediaRecorderErrorEvent now being fired upon async errors the existing > +tests need to be updated to reflect this. s/upon/with/ I think? ::: dom/media/test/test_mediarecorder_creation_fail.html:24 (Diff revision 4) > var stream = new AudioContext().createMediaStreamDestination().stream; > var mediaRecorder = new MediaRecorder(stream); > > mediaRecorder.onerror = function (e) { > is(callbackStep, 0, 'should fired onstop callback'); > - is(e.name, 'GenericError', 'error name should be GenericError'); > + is(e.error.name, 'AbortError', 'error name should be AbortError'); UnknownError ::: dom/media/test/test_mediarecorder_creation_fail.html:24 (Diff revision 4) > - is(e.name, 'GenericError', 'error name should be GenericError'); > + is(e.error.name, 'AbortError', 'error name should be AbortError'); > is(mediaRecorder.mimeType, '', 'mimetype should be empty'); > is(mediaRecorder.state, 'recording', 'state is recording'); There appears to be a discrepancy between how our mediaRecorder works here and the spec, since the spec says the state should be "inactive" here. With Chrome and Firefox the only implementations, we should double-check how Chrome behaves here. If both implementations agree, we should consider whether updating the spec instead would be preferable. ::: dom/media/test/test_mediarecorder_getencodeddata.html:52 (Diff revision 4) > - is(evt.name, 'GenericError', > - 'Event name is GenericError'); > + ok(evt.error instanceof DOMException, > + 'Events fired from onerror should have a DOMException in their error member'); We should check evt.error.name and evt.error.message as well. ::: dom/media/test/test_mediarecorder_principals.html:94 (Diff revision 4) > hasStopped = new Promise(resolve => rec.onstop = resolve); > video.play(); > }) > - .then(() => ok(true, msgNoThrow), e => is(e.name, null, msgNoThrow)) > + .then(() => ok(true, msgNoThrow), e => is(e.error.name, null, msgNoThrow)) > .then(() => Promise.race([ > - new Promise((_, reject) => rec.onerror = e => reject(new DOMException("", e.name))), > + new Promise((_, reject) => rec.onerror = e => reject(new DOMException("", e.error.name))), No need to create a new DOMException here anymore. ::: dom/media/test/test_mediarecorder_unsupported_src.html:45 (Diff revision 4) > } catch(e) { > ok(false, 'Should not get exception in pause call.'); > } > } > ok(callbackStep < 3, 'onerror callback fired as expected.'); > - is(e.name, 'GenericError', 'Error name should be GenericError.'); > + is(e.error.name, 'AbortError', 'Error name should be AbortError.'); UnknownError

Comment on attachment 8894285 [details] Bug 1275856 - Fire MediaRecorderErrorEvent in media recorder. https://reviewboard.mozilla.org/r/165370/#review171014 > You've added two new errors here. Why? > > According to the spec [1], the only two errors that can be fired are "SecurityError", and "UnknownError" for everything else. > > This means any gecko-specifc debug info about the actual failure (like the aRv number) would need to be captured in the error.message field instead. > > [1] https://w3c.github.io/mediacapture-record/MediaRecorder.html#dom-mediarecorder-start I'd assumed based on the previous errors being raised in the recorder and that since we call NotifyError outside of start that it was possible to expose any of the exceptions detailed in the spec[1] on the error event. Will revise. Should we remove the possibility of other operations firing an error event? [1] https://www.w3.org/TR/mediastream-recording/#exception-summary > Missing the stack part. I've got a further patch which I'll add to the review containing the changes for this.

Comment on attachment 8894287 [details] Bug 1275856- Update tests to reflect new MediaRecorderErrorEvent. https://reviewboard.mozilla.org/r/165374/#review171020 > s/upon/with/ I think? I'm trying to say that we'll fire the event in response to an error above, rather than wrapping an error. I will reword this to make both cases more clear. > There appears to be a discrepancy between how our mediaRecorder works here and the spec, since the spec says the state should be "inactive" here. > > With Chrome and Firefox the only implementations, we should double-check how Chrome behaves here. If both implementations agree, we should consider whether updating the spec instead would be preferable. Shall we follow this up in another bug?

Comment on attachment 8895154 [details] Bug 1275856 - Capture MediaRecorder DOMExceptions early in order to capture JS traces. https://reviewboard.mozilla.org/r/166286/#review171500 ::: dom/media/MediaRecorder.cpp:1147 (Diff revision 1) > } > > MOZ_ASSERT(mSessions.Length() > 0); > nsresult rv = mSessions.LastElement()->Pause(); > if (NS_FAILED(rv)) { > + InitializeDomExceptions(); Is this sensible? Would we rather not re-init here and let the exceptions only be set in start? Should we not be throwing here all together (the spec doesn't specify the error event as coming from Pause. Right now it's possible to not have the start Exception present when reaching this code, which is part of why I'm doing the init here. I've also got guards in the throwing code, with some asserts. There's some overlap with this code in terms of defensive programming. Figured I'd go conservative initially and we can back off as appropriate. ::: dom/media/MediaRecorder.cpp:1166 (Diff revision 1) > } > > MOZ_ASSERT(mSessions.Length() > 0); > nsresult rv = mSessions.LastElement()->Resume(); > if (NS_FAILED(rv)) { > + InitializeDomExceptions(); As with pause comment. ::: dom/media/MediaRecorder.cpp:1183 (Diff revision 1) > return; > } > MOZ_ASSERT(mSessions.Length() > 0); > nsresult rv = mSessions.LastElement()->RequestData(); > if (NS_FAILED(rv)) { > + InitializeDomExceptions(); As with pause comment.

Comment on attachment 8894285 [details] Bug 1275856 - Fire MediaRecorderErrorEvent in media recorder. https://reviewboard.mozilla.org/r/165370/#review171516

Comment on attachment 8895154 [details] Bug 1275856 - Capture MediaRecorder DOMExceptions early in order to capture JS traces. https://reviewboard.mozilla.org/r/166286/#review171500 > Is this sensible? Would we rather not re-init here and let the exceptions only be set in start? Should we not be throwing here all together (the spec doesn't specify the error event as coming from Pause. Right now it's possible to not have the start Exception present when reaching this code, which is part of why I'm doing the init here. > > I've also got guards in the throwing code, with some asserts. There's some overlap with this code in terms of defensive programming. Figured I'd go conservative initially and we can back off as appropriate. We only get here if state == Recording, which is only set from start(), so the exception should be present. I'd remove it. > As with pause comment. ditto > As with pause comment. ditto

Comment on attachment 8895154 [details] Bug 1275856 - Capture MediaRecorder DOMExceptions early in order to capture JS traces. https://reviewboard.mozilla.org/r/166286/#review171524 ::: dom/media/MediaRecorder.cpp:1006 (Diff revision 2) > + // Make sure the exceptions are initialized in case we error before we get > + // another chance. > + InitializeDomExceptions(); Can this happen? I'm a bit concerned about the overhead of creating these already, so I'm hoping it can't.

Comment on attachment 8894287 [details] Bug 1275856- Update tests to reflect new MediaRecorderErrorEvent. https://reviewboard.mozilla.org/r/165374/#review171534 ::: dom/media/test/test_mediarecorder_getencodeddata.html:54 (Diff revisions 4 - 6) > + is(evt.error.name, 'UnknownError', 'Error name should be UnknownError.'); > + is(evt.error.message, 'The operation failed for an unknown transient reason'); Do we want to test that e.stack points to start() in this file? It's not spec'ed, but we should probably cover it so it won't break on us.

Comment on attachment 8895154 [details] Bug 1275856 - Capture MediaRecorder DOMExceptions early in order to capture JS traces. https://reviewboard.mozilla.org/r/166286/#review171500 > We only get here if state == Recording, which is only set from start(), so the exception should be present. I'd remove it. Roger. I'll change the assertions in NotifyError into logs. It's possible right now for the same Exception type to happen multiple times for one call of start(), see `test_mediarecorder_unsupported_src.html`. The first time the Exception is thrown it will be nulled, and then the next time it will need to be reinitialized. I think this is not what should happen per spec. As you've indicated in another comment, we're not moving the recorder to inactive state at times were it appears we should. I'm going to follow up on this in another bug rather than complicate this bug by bringing that work in.

Comment on attachment 8895154 [details] Bug 1275856 - Capture MediaRecorder DOMExceptions early in order to capture JS traces. https://reviewboard.mozilla.org/r/166286/#review171524 > Can this happen? I'm a bit concerned about the overhead of creating these already, so I'm hoping it can't. With the defensive initialization code at the time the event is fired this can be removed anyway, so will get rid of it. Per the spec I agree it shouldn't happen, will test as such once I've made the alterations.

Comment on attachment 8895266 [details] Bug 1275856 - Expand MediaRecorder tests to check stack traces when MediaRecorderErrorEvent is fired. https://reviewboard.mozilla.org/r/166426/#review171590 ::: dom/media/test/test_mediarecorder_creation_fail.html:25 (Diff revision 1) > var mediaRecorder = new MediaRecorder(stream); > > mediaRecorder.onerror = function (e) { > is(callbackStep, 0, 'should fired onstop callback'); > is(e.error.name, 'UnknownError', 'error name should be UnknownError'); > + ok(e.error.stack.includes('test_mediarecorder_creation_fail.html'), In my local tests I wasn't getting the `start` function name logged in the trace. Correct line number, but function not listed. I'm not sure if this is a known issue. However, it's meant I've made these checks a bit less strict than they could be.

Comment on attachment 8895154 [details] Bug 1275856 - Capture MediaRecorder DOMExceptions early in order to capture JS traces. https://reviewboard.mozilla.org/r/166286/#review171892 Looks good. Kinda wish we still had the assert, but this is probably more robust against future changes. ::: dom/media/MediaRecorder.cpp:1419 (Diff revisions 2 - 3) > default: > - MOZ_ASSERT(mUnkownDomException); > if (!mUnkownDomException) { > + LOG(LogLevel::Debug, ("MediaRecorder.NotifyError: " > + "mUnkownDomException was not initialized")); > mUnkownDomException = DOMException::Create(NS_ERROR_DOM_UNKNOWN_ERR); > } > init.mError = mUnkownDomException.forget(); Just wondering, would it be helpful for debugging to get the real aRv number value into the UnknownError's message? We could probably just subclass DOMException here to get access to the protected member. That, or maybe log it (unless errors are logged where they're thrown)?

Comment on attachment 8895266 [details] Bug 1275856 - Expand MediaRecorder tests to check stack traces when MediaRecorderErrorEvent is fired. https://reviewboard.mozilla.org/r/166426/#review171894

Comment on attachment 8894287 [details] Bug 1275856- Update tests to reflect new MediaRecorderErrorEvent. https://reviewboard.mozilla.org/r/165374/#review171020 > Shall we follow this up in another bug? https://bugzilla.mozilla.org/show_bug.cgi?id=1388909 raised to track this.

Comment on attachment 8895154 [details] Bug 1275856 - Capture MediaRecorder DOMExceptions early in order to capture JS traces. https://reviewboard.mozilla.org/r/166286/#review171892 > Just wondering, would it be helpful for debugging to get the real aRv number value into the UnknownError's message? > > We could probably just subclass DOMException here to get access to the protected member. > > That, or maybe log it (unless errors are logged where they're thrown)? Added logging here (also realized I've typoed mUnknownDomException, so have fixed that up).

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 6 changesets with 14 changes to 8 files remote: remote: WebIDL file dom/webidl/RecordErrorEvent.webidl altered in changeset 0271692503de without DOM peer review remote: WebIDL file dom/webidl/MediaRecorderErrorEvent.webidl altered in changeset 5d2156c05a51 without DOM peer review remote: remote: remote: remote: ************************** ERROR **************************** remote: remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=... remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding.. remote: remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.d_webidl hook failed abort: push failed on remote

Comment on attachment 8894284 [details] Bug 1275856 - Add MediaRecorderErrorEvent webidl files and update moz.build. https://reviewboard.mozilla.org/r/165368/#review172262

Comment on attachment 8894286 [details] Bug 1275856 - Remove now unused RecrodErrorEvent. https://reviewboard.mozilla.org/r/165372/#review172264 Hopefully we won't break websites with these backwards incompatible changes :/

Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/407dc5f8de5b Add MediaRecorderErrorEvent webidl files and update moz.build. r=jib,smaug https://hg.mozilla.org/integration/autoland/rev/1c8e6dd99100 Fire MediaRecorderErrorEvent in media recorder. r=jib https://hg.mozilla.org/integration/autoland/rev/38739c5a3ae8 Remove now unused RecrodErrorEvent. r=jib,smaug https://hg.mozilla.org/integration/autoland/rev/210e8d6d5f85 Update tests to reflect new MediaRecorderErrorEvent. r=jib https://hg.mozilla.org/integration/autoland/rev/2fb8d23f1ece Capture MediaRecorder DOMExceptions early in order to capture JS traces. r=jib https://hg.mozilla.org/integration/autoland/rev/d229d269ac55 Expand MediaRecorder tests to check stack traces when MediaRecorderErrorEvent is fired. r=jib

I had to back these out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=122405486&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=122407157&repo=autoland https://hg.mozilla.org/integration/autoland/rev/8a97c4d1ab15dbbf8f148805d58326e48c20a95c

Comment on attachment 8896531 [details] Bug 1275856 - Update dom event tests to reflect removal of RecorderErrorEvent and new MediaRecorderErrorEvent. https://reviewboard.mozilla.org/r/167796/#review173390

Comment on attachment 8896532 [details] Bug 1275856 - Update expected outcomes of webplatform test for MediaRecorder. https://reviewboard.mozilla.org/r/167798/#review174076

Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ebe132a416d4 Add MediaRecorderErrorEvent webidl files and update moz.build. r=jib,smaug https://hg.mozilla.org/integration/autoland/rev/2f9f9eab06b2 Fire MediaRecorderErrorEvent in media recorder. r=jib https://hg.mozilla.org/integration/autoland/rev/39674f7a8148 Remove now unused RecrodErrorEvent. r=jib,smaug https://hg.mozilla.org/integration/autoland/rev/3c105bee024c Update tests to reflect new MediaRecorderErrorEvent. r=jib https://hg.mozilla.org/integration/autoland/rev/2d3c818bf51d Capture MediaRecorder DOMExceptions early in order to capture JS traces. r=jib https://hg.mozilla.org/integration/autoland/rev/c735325c95be Expand MediaRecorder tests to check stack traces when MediaRecorderErrorEvent is fired. r=jib https://hg.mozilla.org/integration/autoland/rev/0153cadbabe3 Update dom event tests to reflect removal of RecorderErrorEvent and new MediaRecorderErrorEvent. r=smaug https://hg.mozilla.org/integration/autoland/rev/f51e7690796d Update expected outcomes of webplatform test for MediaRecorder. r=jib

https://hg.mozilla.org/mozilla-central/rev/ebe132a416d4 https://hg.mozilla.org/mozilla-central/rev/2f9f9eab06b2 https://hg.mozilla.org/mozilla-central/rev/39674f7a8148 https://hg.mozilla.org/mozilla-central/rev/3c105bee024c https://hg.mozilla.org/mozilla-central/rev/2d3c818bf51d https://hg.mozilla.org/mozilla-central/rev/c735325c95be https://hg.mozilla.org/mozilla-central/rev/0153cadbabe3 https://hg.mozilla.org/mozilla-central/rev/f51e7690796d

Commits pushed to master at https://github.com/mdn/kumascript https://github.com/mdn/kumascript/commit/abc0e9e088a97e80b3ee91689d7a6624a8ac839c Fix bug 1275856 - added MediaRecorderErrorEvent Added this interface to the list of interfaces under the MediaStream Recording API. https://github.com/mdn/kumascript/commit/ef840bf177023ba454ab65527c493ae517921da0 Merge Fix bug 1275856 - added MediaRecorderErrorEvent to interface list (#339) Fix bug 1275856 - added MediaRecorderErrorEvent

Documentation has been updated and written for this. Pages that have been created: https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorderErrorEvent https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorderErrorEvent/MediaRecorderErrorEvent https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorderErrorEvent/error Reference pages updated to refer to this new content as appropriate: https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/start https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorde/onerror https://developer.mozilla.org/en-US/docs/Web/Events/error Other pages updated to refer to this new content: https://developer.mozilla.org/en-US/docs/Web/API/MediaStream_Recording_API And mentioned on Firefox 57 for developers.