Closed Bug 1275856 Opened 8 years ago Closed 7 years ago

Update MediaRecorder to spec by firing MediaRecorderErrorEvent for errors

Categories

(Core :: Audio/Video: Recording, defect, P2)

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: robert.swain, Assigned: bryce)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(9 files)

2.89 KB, text/html
Details
59 bytes, text/x-review-board-request
jib
: review+
smaug
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
smaug
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
smaug
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
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
Component: Untriaged → Audio/Video: Recording
OS: Unspecified → Mac OS X
Product: Firefox → Core
Hardware: Unspecified → x86_64
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
Blocks: 1224079
Status: UNCONFIRMED → NEW
Rank: 35
Ever confirmed: true
Priority: -- → P2
Rank: 35 → 25
Attached file track-switch.html
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
Rank: 25 → 21
Summary: MediaRecorder error callback called with object that is not instanceof Error → Update MediaRecorder to spec by firing MediaRecorderErrorEvent for errors
OS: Mac OS X → All
Hardware: x86_64 → All
Workaround:

    recorder.onerror = e => { throw e.error || new Error(e.name); };
Assignee: nobody → bvandyk
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?
Attachment #8894284 - Flags: review?(jib) → review+
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
Attachment #8894285 - Flags: review?(jib) → review-
Comment on attachment 8894286 [details]
Bug 1275856 - Remove now unused RecrodErrorEvent.

https://reviewboard.mozilla.org/r/165372/#review171018
Attachment #8894286 - Flags: review?(jib) → review+
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
Attachment #8894287 - Flags: review?(jib) → review-
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
Attachment #8894285 - Flags: review?(jib) → review+
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.
Attachment #8895154 - Flags: review?(jib) → review-
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.
Attachment #8894287 - Flags: review?(jib) → review+
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)?
Attachment #8895154 - Flags: review?(jib) → review+
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
Attachment #8895266 - Flags: review?(jib) → review+
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
Attachment #8894284 - Flags: review?(bugs) → review+
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 :/
Attachment #8894286 - Flags: review?(bugs) → review+
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
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
Attachment #8896531 - Flags: review?(bugs) → review+
Comment on attachment 8896532 [details]
Bug 1275856 - Update expected outcomes of webplatform test for MediaRecorder.

https://reviewboard.mozilla.org/r/167798/#review174076
Attachment #8896532 - Flags: review?(jib) → review+
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
Flags: needinfo?(bvandyk)
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: