Closed Bug 1388909 Opened 7 years ago Closed 7 years ago

MediaRecorder state is not set to inactive before onerror handler

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bryce, Assigned: bryce)

References

Details

(Keywords: stale-bug)

Attachments

(2 files)

The MediaRecorder does not transition to an inactive state from recording when an error is encountered. As per the spec we should be transitioning to inactive in these cases[1]. This has a knock on effect since the recorder is still in a recording state: if we attempt other operations after such an error we do not get the expected InvalidState DOMException, but may instead fire an error event wrapping a different DOMException.

[1]: https://w3c.github.io/mediacapture-record/MediaRecorder.html#mediarecorder-methods
Assignee: nobody → bvandyk
Rank: 19
Depends on: 1275856
Priority: -- → P1
Looking into this further, my initial analysis was incorrect: we will typically transition to inactive, but we don't do so in a spec compliant way. The spec indicates the following steps for errors[1]:

>  ...queue a task, using the DOM manipulation task source, that runs the following steps:
>    Set state to inactive.
>    Fire an error event named <appropriate error type> at target.
>    Fire a blob event named dataavailable at target with blob.
>    Fire an event named stop at target. 

Right now the order in which we do these is a bit screwy, and they're not all in the same task. In most cases it looks like we have 3 separate tasks that do:

- Fire error event
- Fire blob event (if not encountering a sec error -- which is kinda against the spec, but that's an issue for another day)
- Go inactive and fire stop event

I believe they will be queued in the above order. This results in the state transition not having happened by the time onerror is called. This means that you get weird behaviour in onerror handlers that interact with state. We currently expect some of this behaviour in our mochitests[2].

[1]: https://w3c.github.io/mediacapture-record/MediaRecorder.html#mediarecorder-methods
[2]: https://searchfox.org/mozilla-central/source/dom/media/test/test_mediarecorder_creation_fail.html
Summary: MediaRecorder state is not set to inactive upon error during recording → MediaRecorder state is not set to inactive before onerror handler
Comment on attachment 8904081 [details]
Bug 1388909 - Rework MediaRecorder to tranisition to inactive upon error.

https://reviewboard.mozilla.org/r/175830/#review180926

Thanks! r+ with nits.

::: dom/media/MediaRecorder.cpp:1479
(Diff revision 1)
>    MOZ_ASSERT(mAudioNode != nullptr);
>    return mPipeStream ? mPipeStream.get() : mAudioNode->GetStream();
>  }
>  
>  void
> +MediaRecorder::GotSessionError()

This name explains why it's called. I'd prefer a name that explains what it does instead. A comment could give an example of a case when it's called. A reason for its existence if you will.

Perhaps `ForceInactive()`?

::: dom/media/MediaRecorder.cpp:1490
(Diff revision 1)
> +void
> +MediaRecorder::StopForSessionDestruction()
> +{
> +  LOG(LogLevel::Debug, ("MediaRecorder.StopForSessionDestruction %p", this));
> +  MediaRecorderReporter::RemoveMediaRecorder(this);
> +  // We do not do perform a mState != RecordingState::Recording) check here as

s/do not do/do not/g

::: dom/media/MediaRecorder.cpp:1491
(Diff revision 1)
> +MediaRecorder::StopForSessionDestruction()
> +{
> +  LOG(LogLevel::Debug, ("MediaRecorder.StopForSessionDestruction %p", this));
> +  MediaRecorderReporter::RemoveMediaRecorder(this);
> +  // We do not do perform a mState != RecordingState::Recording) check here as
> +  // we may already be inactive due to StopForSessionError().

Update `StopForSessionError` to reflect what name you landed on above.
Attachment #8904081 - Flags: review?(apehrson) → review+
Comment on attachment 8904082 [details]
Bug 1388909 - Update tests to reflect updated MediaRecorder error handling.

https://reviewboard.mozilla.org/r/175832/#review180928

Looks good. r+ with nits

::: dom/media/test/test_mediarecorder_getencodeddata.html:62
(Diff revision 1)
>          is(evt.error.message, 'The operation failed for an unknown transient reason');
>          ok(evt.error.stack.includes('test_mediarecorder_getencodeddata.html'),
>            'Events fired from onerror should include an error with a stack trace indicating ' +
>            'an error in this test');
> +        try {
> +          mediaRecorder.requestData();

Add an `ok(false, ...)` here to check that we really do get an exception.

::: dom/media/test/test_mediarecorder_getencodeddata.html:64
(Diff revision 1)
> +          ok(e instanceof DOMException, 'requestdata should fire an exception ' +
> +            'if called on an incative recorder');

Please also check that this DOMException has the name "InvalidStateError"

::: dom/media/test/test_mediarecorder_getencodeddata.html:65
(Diff revision 1)
>            'an error in this test');
> +        try {
> +          mediaRecorder.requestData();
> +        } catch (e) {
> +          ok(e instanceof DOMException, 'requestdata should fire an exception ' +
> +            'if called on an incative recorder');

s/incative/inactive/

::: dom/media/test/test_mediarecorder_unsupported_src.html:38
(Diff revision 1)
>  
>        mediaRecorder.onerror = function (e) {
>          callbackStep++;
>          if (callbackStep == 1) {
>            try {
>              mediaRecorder.pause();

Same here re an `ok(false, ...)`.

::: dom/media/test/test_mediarecorder_unsupported_src.html:40
(Diff revision 1)
>          callbackStep++;
>          if (callbackStep == 1) {
>            try {
>              mediaRecorder.pause();
>            } catch(e) {
> -            ok(false, 'Should not get exception in pause call.');
> +            ok(e instanceof DOMException, 'pause should fire an exception ' +

Same here re "InvalidStateError"

::: dom/media/test/test_mediarecorder_unsupported_src.html:41
(Diff revision 1)
>          if (callbackStep == 1) {
>            try {
>              mediaRecorder.pause();
>            } catch(e) {
> -            ok(false, 'Should not get exception in pause call.');
> +            ok(e instanceof DOMException, 'pause should fire an exception ' +
> +            'if called on an incative recorder');

s/incative/inactive/g

+ indentation
Attachment #8904082 - Flags: review?(apehrson) → review+
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/963d871f9bc0
Rework MediaRecorder to tranisition to inactive upon error. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/268d45f4eca3
Update tests to reflect updated MediaRecorder error handling. r=pehrsons
You need to log in before you can comment on or make changes to this bug.