Closed Bug 1095381 Opened 10 years ago Closed 10 years ago

[b2g] The encoder error lead to the unexpected crash in the procedure of the MediaRecorder API

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: alwu, Assigned: alwu)

Details

Attachments

(1 file, 2 obsolete files)

It's relative to the Bug1090130.

STR.
1. goto http://people.mozilla.org/~rlin/MediaRecorder.html
2. play 720WebM
3. start record, found browser crashed.

The crash happened in the encoding procedure. 

In bug1090130, sometime we didn't crash directly even we accessed the wrong memory address. Then we still got the error message when we put the data of this buffer into the Codec. It would arouse the NotifyError() to call alert() of the js file. The strange point is that alert() change the order of the normal procedure, it arouse the DestroyRunnable before we done the task of the PushBlobRunnable. 

It resulted in that we couldn't find the MediaRecorder when we continued to execute the PushBlobRunnable, then the program would crash.
Assignee: nobody → alwu
Crash reason :
Main thread task blocking leads to the error of runnable execution order.

Details :
In the media recording procedure, we would send the error message to the client if the encoding error happened. After the error notification, we needed to send data available event and stop event to the client. Therefore, we send the pushBlobRunnable and DestroyRunnable to the waiting queue of the main thread. 

The pushBlobRunnable was responsible for the error notification and sending data available event. In this case, error notification would arouse the alert() function in the client. This function would blocked the task running on main thread and executed the next event from the waiting queue.

Therefore, the event of data available could not be send before the stop event. The destroyRunnable would be arouse before sending data available event. That was why the crash happened.
Attached patch StableState.patch (obsolete) — Splinter Review
I tried to use the runInStableState() [1] to ensure the calling could followed the order, but it counld not work. 

(1)runInStableState() ensures the starting order of the runnables, but it doesn't ensure the time of arousing next event is later than the ending time of previous one. But the thing we needed is the correct execution order, not a starting order.

(2) The NotifyError() in PushBlobRunnable leads infinite syncLoop recursion. The implement details of runInStableState() is to save the runnables into a syncSection array, then executes them by order. I observed that the PushBlobRunnable would be executed repeatly. Everytime it executed the NotifyError(), it would be restarted and ran again. It would not execute the remaining content of PushBlobRunnable. Until the system error, "js : too much recursion", happened, the PushRunnable could be ended. And it resulted to another crash.

So I would try another solution. Split the error notification out from the PushBlobRunnable.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAppShell#runInStableState%28%29
Attached patch Bug1095381_v1.patch (obsolete) — Splinter Review
Hi, randy,
Could you help me review my patch?
Below is the short introduction about this patch.

--------------------------------------------------------------------------
The defined notification order by W3C is "the error" first, then "the data availiable", final "stop".

Because alert() would block "data availiable event" and arouse "stop" before "data availiable event" finished, I divided the error notification from PushBlobRunnable so that alert() couldn't block "data availiable event" ever.

[The original situation of waiting queue]
{PushBlob} , {Destroy} 
※ Blocking happened in the middle of PushBlob, so we couldn't send data availiable event

[The revised situation of waiting queue]
{NotifyError}, {PushBlob}, {Destroy}
※ Blocking happened in the end of NotifyError, so we could arouse PushBlob successfully.
--------------------------------------------------------------------------

Thanks a lot :)
Attachment #8524273 - Attachment is obsolete: true
Attachment #8524316 - Flags: review?(rlin)
Attachment #8524316 - Flags: review?(rlin) → review+
Attachment #8524316 - Attachment is obsolete: true
Keywords: checkin-needed
Hi Alastor,
Please post the try server test link, thanks.
Here is the try server test result.
https://tbpl.mozilla.org/?tree=Try&rev=133caba7af64
https://hg.mozilla.org/mozilla-central/rev/0cc609ae9395
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.