897776 - MediaRecorder infinite recursion with requestData() calls in "dataavailable" event

Status: VERIFIED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

First I wasn't sure whether this is a potential bug or not but since this only re-produces with two requestData() calls inside a "dataavailable" event it lead to my believe that this is perhaps not an intended behavior.

Testcase triggers after 5 seconds.

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "too much recursion" {file: "file:///Users/cdiehl/Desktop/mediarecorder-testcase.html" line: 16}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///Users/cdiehl/Desktop/mediarecorder-testcase.html :: <TOP_LEVEL> :: line 16"  data: yes]

Comment 1

[Christoph Diehl [:posidron]]

Attachment: media.zip

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Christoph,
From the API spec
void requestData()
    When a MediaRecorderobject’s requestData() method is invoked, the UA must queue a task, using the DOM manipulation task source, that runs the following steps:

        If state is not "recording" raise a DOM InvalidState error and terminate these steps. Otherwise:
        Raise a dataavailable event containing the current Blob of saved data.


And the test case wrote:
	o2.addEventListener("dataavailable", function(e) { 
		o2.requestData()

Should we block this behavior?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah. CreateAndDispatchDOMEvent does not queue a task to dispatch the event, it dispatches one synchronously. So, RequestData should use NS_NewRunnableMethod(CreateAndDispatchBlobEvent) to create a runnable and dispatch that to the main thread.

Comment 4

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The test step will become 
1. start record with initial timeslice
2. on dataavailable, also call the requestData again. 
Now our implement will fire a dataavaiable event with blob size = 0 to UA, 
So the UA call the request data again, then...loop forever.
Should I avoid to fire a dataavaiable event with blob size = 0 ?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, I think we have to fire an event when requestData is called. If the dataavailable event handler calls requestData, that's not nice but we should handle it OK because we'll return to the event loop between each iteration of the dataavailable event handler.

Comment 6

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

OK,I will implement a method to prevent this problem happen.

Comment 7

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

Try to block this behavior by throwing exception.

Comment 8

[Jason Smith [:jsmith]]

Comment on attachment 785661 [details] [diff] [review]
patch v1

Review of attachment 785661 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_mediarecorder_avoid_recursive.html
@@ +18,5 @@
> +  element.token = token;
> +  manager.started(token);
> +  element.test = test;
> +  element.play();
> +  element.onended = function () {  manager.finished(token);}

Don't think this should be needed.

@@ +23,5 @@
> +  element.oncanplay = function () {
> +    element.mr = new MediaRecorder(element.stream);
> +    element.mr.ondataavailable = function () {
> +      try {
> +        element.mr.requestData();

There's nothing in the test that acts as a failure condition. You should add a failure condition after requestData and finish the test gracefully in a finally block.

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2

modify test case.
try result
https://tbpl.mozilla.org/?tree=Try&rev=f0276575385c
hit this one
147486 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_reload_crash.html | Assertion count 1 is greater than expected range 0-0 assertions.
 [Parent 1188] ###!!! ASSERTION: still has data in EncodedBuffers!: 'mDataSize == 0', file ../../../content/media/EncodedBufferCache.h, line 35

Can be solve by this bug 900708.

Comment 10

[Jason Smith [:jsmith]]

Comment on attachment 786163 [details] [diff] [review]
patch v2

Review of attachment 786163 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_mediarecorder_avoid_recursive.html
@@ +10,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +var count = 0;
> +function startTest() {
> +  navigator.mozGetUserMedia({audio: true, fake: true}, function(stream) {

This particular test should be able to rely on the general gMediaRecorderTest workflow to get better codec coverage.

@@ +16,5 @@
> +    mediaRecorder.start();
> +    mediaRecorder.ondataavailable = function () {
> +      if (count++ > 100) {
> +        ok(false, "shouldn't fire so many ondataavailable event!!");
> +        mediaRecorder.stop();

This will result in a followup ondataavailable event firing, which will cause SimpleTest.finish to fire twice.

@@ +22,5 @@
> +      } else {
> +        try {
> +          mediaRecorder.requestData();
> +        } catch (e){
> +          is(e.name, 'NS_ERROR_FAILURE', 'catch this exception, pass this test');

Why is NS_ERROR_FAILURE being used as the exception name? Couldn't we pick something more friendly?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> No, I think we have to fire an event when requestData is called. If the
> dataavailable event handler calls requestData, that's not nice but we should
> handle it OK because we'll return to the event loop between each iteration
> of the dataavailable event handler.

I think you misunderstood what I said here. RequestData needs to dispatch the dataavailable event asynchronously. So you need to create an nsRunnable to fire the event and dispatch it to the main thread. You don't need to fire any exceptions or use this zero-blob-limit.

Comment 12

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

So if UA call the RequestData method, fire a dataavailable event, then UA call the RequestData again, could this way can avoid this recursion problem. Do the js engine has the mechanism to prevent this problem?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There's no recursion. The page calls requestData(), then returns to the event loop. Then we run the queued nsRunnable that fires dataavailable, which calls requestData() again, then we return to the event loop again. Then we carry on doing that. Not only is there no recursion, but the browser remains responsive because we can process other events on the event loop.

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

OK, understood. :) I will do it right away.

Comment 15

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v3

Works fine by this way.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 786841 [details] [diff] [review]
patch v3

Review of attachment 786841 [details] [diff] [review]:
-----------------------------------------------------------------

Please add the test.

Comment 17

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case

Should apply bug 900419 first, also test this case:
Media Recording - MediaRecorder's state is wrong when stopping media stream during recording

Comment 18

[Jason Smith [:jsmith]]

Comment on attachment 787312 [details] [diff] [review]
test case

Review of attachment 787312 [details] [diff] [review]:
-----------------------------------------------------------------

Did you confirm this mochitest will trigger the bug without the bug fix included and fail gracefully?

Comment 19

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Would enter timeout case. 
 9:35.44 TEST-UNEXPECTED-FAIL | automation.py | application timed out after 330 seconds with no output

Comment 20

[Jason Smith [:jsmith]]

Comment on attachment 787312 [details] [diff] [review]
test case

Review of attachment 787312 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good then. The concept of the test here looks fine.

Note - There's some cleanup needed before review.

::: content/media/test/test_mediarecorder_avoid_recursion.html
@@ +11,5 @@
> +a Bug 897776</a>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +function startTest() {
> +  navigator.mozGetUserMedia({audio: true, fake: true}, function(stream) {

I'd suggest using the gMediaRecorder workflow here to get better codec coverage.

@@ +15,5 @@
> +  navigator.mozGetUserMedia({audio: true, fake: true}, function(stream) {
> +    var mediaRecorder = new MediaRecorder(stream);
> +    var count = 0;
> +    mediaRecorder.start();
> +    mediaRecorder.ondataavailable = function (e) {

Nit on ondataavailable & onstop - fix indentation

@@ +25,5 @@
> +                                                    }
> +                                                  }
> +    mediaRecorder.requestData();
> +    mediaRecorder.onstop = function () {
> +                                          ok(true, "can use this way to control the recorder object");

Can you improve the message here? Maybe something like:

'requestData within ondataavailable successfully avoided infinite recursion'

@@ +27,5 @@
> +    mediaRecorder.requestData();
> +    mediaRecorder.onstop = function () {
> +                                          ok(true, "can use this way to control the recorder object");
> +                                          SimpleTest.finish();
> +                                          console.log("onstop");

Nit - this won't ever be executed, so this can be removed.

Comment 21

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case v2

I found the media stream from audio tag isn't trigger encoder to stop recording, But fake gUM can. so I use the fake gUM to capture this bug and are debugging another one.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #21)
> Created attachment 787965 [details] [diff] [review]
> test case v2
> 
> I found the media stream from audio tag isn't trigger encoder to stop
> recording, But fake gUM can. so I use the fake gUM to capture this bug and
> are debugging another one.

Ah okay, so doing it via gMediaRecorderTests isn't possible right now due to a known bug then, right?

Comment 23

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Ya, we are looking the problem which can't use the gMediaRecorderTests module to test the recorder object.

Comment 24

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: checkin -2 test case

try result
https://hg.mozilla.org/try/rev/4169a8106cae

Comment 25

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: checkin-1 patch

Comment 26

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Please help to check in two patches,  
try result log.
https://tbpl.mozilla.org/?tree=Try&rev=4169a8106cae

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6bf6013084ca
https://hg.mozilla.org/integration/b2g-inbound/rev/16d14f3212fc

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6bf6013084ca
https://hg.mozilla.org/mozilla-central/rev/16d14f3212fc

Comment 29

[Jason Smith [:jsmith]]

Verified per clean landing with mochitest running.