MediaRecorder fires start event when erroring

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: bryce, Assigned: bryce)

Tracking

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

There are several error paths that result in the start event being fired by the MediaRecorder. This will often lead to a second start event being fired after the recorder has already started. This is undesirable for a number of reasons such as  not meeting the spec, and resulting in the onstart handler being called again.

JSFiddle showing the issue: https://jsfiddle.net/SingingTree/LrpdLnr6/ See the web console for "onstart".

The cause is that in many failure cases the MediaRecorder makes an internal call to Session::DoSessionEndTask[1]. This method fires a start event to cover the case where a start was not already fired, it then sets a flag indicating that DoSessionEndTask is not required (which is also set elsewhere).

There is inconsistent checking around the flag that gates calls to DoSessionEndTask such that DoSessionEndTask will be called without first checking that flag in many cases.

To complicate matters, DoSessionEndTask performs other functionality that is important. It's not clear to me that the firing of start and these other functions should be coupled as it is. However, I've not had time to investigate further.

[1]: https://searchfox.org/mozilla-central/source/dom/media/MediaRecorder.cpp#817
Rank: 25
Priority: -- → P2
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8904347 [details]
Bug 1395022 - Fix MediaRecorder firing unnecessary start event when erroring.

https://reviewboard.mozilla.org/r/176134/#review181194
Attachment #8904347 - Flags: review?(apehrson) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8904348 [details]
Bug 1395022 - Add test to check only one start event is fired by MediaRecorder when erroring.

https://reviewboard.mozilla.org/r/176136/#review181196

::: dom/media/test/test_mediarecorder_fires_start_event_once_when_erroring.html:7
(Diff revision 1)
> +<html>
> +<head>
> +  <title>Bug 1395022 - MediaRecorder fires start event when erroring.</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +  <script type="text/javascript" src="manifest.js"></script>

I don't think you need manifest.js here. It's for managing testing multiple media files in the same test.

::: dom/media/test/test_mediarecorder_fires_start_event_once_when_erroring.html:21
(Diff revision 1)
> +  let oscilator2 = audioContext.createOscillator();
> +  oscilator2.connect(destination2);
> +  oscilator2.start();

Shouldn't be a need for `oscilator2` to trigger an error.

Unsure if `oscilator1` is needed. Doubtful but maybe.
Attachment #8904348 - Flags: review?(apehrson) → review+
Comment hidden (mozreview-request)
Assignee

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8904348 [details]
Bug 1395022 - Add test to check only one start event is fired by MediaRecorder when erroring.

https://reviewboard.mozilla.org/r/176136/#review181196

> Shouldn't be a need for `oscilator2` to trigger an error.
> 
> Unsure if `oscilator1` is needed. Doubtful but maybe.

Removed oscialtor 2. Leaving 1 in as without some data source feeding into the MediaStreamDestination we never fire start (this may change in future, as Chrome don't bother waiting).

Comment 9

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a4fafdfadf35 -d 08d59706aac1: rebasing 418132:a4fafdfadf35 "Bug 1395022 - Fix MediaRecorder firing unnecessary start event when erroring. r=pehrsons"
merging dom/media/MediaRecorder.cpp
warning: conflicts while merging dom/media/MediaRecorder.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35a2a0327e51
Fix MediaRecorder firing unnecessary start event when erroring. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/dfc055f4d93e
Add test to check only one start event is fired by MediaRecorder when erroring. r=pehrsons
Assignee: nobody → bvandyk
You need to log in before you can comment on or make changes to this bug.