952232 - [B2G][API Mediarecorder] "OnError" is not fired up in logcat when receiving/answering a phone call while media is recording

Status: RESOLVED INVALID

(Core :: WebRTC, defect)

Description

[sarsenyev]

Attachment: log12-19.txt

Description:
When receiving & answer an incoming call
Then logcat doesn't show evidence that onerror fired 

Repro Steps:
1) Updated Buri to BuildID: 20131218004002
2) Open "Browser" and type in URL: http://mozilla.github.io/qa-testcase-data/webapi/mediarecorder/index.html
3) Select microphone, and select setup and grant permission
4) Select to start recording
5) Receive & answer an incoming call

Actual:
Logcat doesn't show evidence that onerror fired 

Expected:
Logcat shows evidence that onerror fired

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20131218004002
Gaia: a99b23e73fe5630a877004746d0e7fcec1b6d653
Gecko: 369bdbff6c38
Version: 28.0a2
Firmware Version:v1.2_20131115

Notes:
Repro frequency: 100%
Link to failed test case:
https://moztrap.mozilla.org/manage/cases/?filter-id=10154
See attached: logcat

Comment 1

[sarsenyev]

It does reproduce with 1.2 build, logcat doesn't show "onerror" in logcat

Device: Buri 1.2 COM
BuildID: 20131218004002
Gaia: 38338b31b554cdb8c6242b15a966f171a0e7abaa
Gecko: dcffefec8206
Version: 26.0
RIL Version: 01.02.00.019.102
Firmware Version:v1.2_20131115

Comment 2

[Jason Smith [:jsmith]]

The logcat here isn't right - it's missing messages all together when media recording active. Can you attach a new logcat with this bug reproducing that shows evidence of "Media Recorder created on index" in the logcat?

Comment 3

[sarsenyev]

No active recording message appear when starting recording, 
https://bugzilla.mozilla.org/show_bug.cgi?id=951008

Comment 4

[Jason Smith [:jsmith]]

(In reply to sarsenyev from comment #3)
> No active recording message appear when starting recording, 
> https://bugzilla.mozilla.org/show_bug.cgi?id=951008

I think that's because you don't have console enabled set to true in the debug settings. If that's not enabled, then you won't get the logcat output. Try again with console enabled set.

Comment 5

[sarsenyev]

Attachment: log12-20.txt

Logcat with "console enabled"

Comment 6

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

The getUserMedia onerror callback should also fired this kind of event and pass through media stream.

Comment 7

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

Hi Benjamin, 
Please help to handle this, thanks a lot.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: bug-952232.patch

1. Add |mErrorCallbacks| member to store the GUM's error callback.
2. Fire GUM's error callback when answering a phone call.

By apply this patch, we can see the message in logcat:
E/GeckoConsole(12266): Content JS LOG at http://mozilla.github.io/qa-testcase-data/webapi/mediarecorder/common.js:76 in createGUMStream/<: IMCOMING_CALL

Comment 9

[Benjamin Chen [:bechen]]

Hi Sarsenyev:
Is it ok if we fire the GUM's error callback when answering a phone call?

Comment 10

[Jason Smith [:jsmith]]

(In reply to Benjamin Chen [:bechen] from comment #9)
> Hi Sarsenyev:
> Is it ok if we fire the GUM's error callback when answering a phone call?

I can answer this - I'm the owner of those test cases.

Yes that's fine.

Comment 11

[Benjamin Chen [:bechen]]

Comment on attachment 8372091 [details] [diff] [review]
bug-952232.patch

Hi Randell:
Would you please help to review this patch?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8372091 [details] [diff] [review]
bug-952232.patch

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

::: dom/media/MediaManager.cpp
@@ +1893,5 @@
>      array->GetElementAt(i, getter_AddRefs(window));
>      nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(window));
>      if (win) {
> +#ifdef MOZ_WIDGET_GONK
> +      // fire onErrorCallback

Please note that this is to cause any un-answered gUM requests to have their error callbacks fired when access to the mic is overridden by a higher-priority audio request.  (and add comments about what the overall StopMediaStreams() call is for, and make the entire function #ifdef MOZ_WIDGET_GONK - since it can only be called from GONK, it's dead code elsewhere for now).

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8372091 [details] [diff] [review]
bug-952232.patch

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

Per discussion with jib: please make sure the error callback is freed once the success callback is called.  If it's complicated or there's something to worry about, please let me know and re can re-evaluate.

Comment 14

[Benjamin Chen [:bechen]]

(In reply to Randell Jesup [:jesup] from comment #13)
> Comment on attachment 8372091 [details] [diff] [review]
> bug-952232.patch
> 
> Review of attachment 8372091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Per discussion with jib: please make sure the error callback is freed once
> the success callback is called.  If it's complicated or there's something to
> worry about, please let me know and re can re-evaluate.
Why do we need to clear error callbacks after success callback is called?

In this case, when user granted the permission, MediaManager clears the callbacks in |mActiveCallbacks|.
After that we don't have any callbacks can be fired when answering a phone call.
That's why I add |mErrorCallbacks| to store the error callbacks and doesn't clear it when observe "getUserMedia:response:allow".

Comment 15

[Benjamin Chen [:bechen]]

Hi Jason:
Is it possible to change the test case to "No error callback when answering a phone" or "Don't check the error callback" ?
Because 
1. When the user answering the phone call, the MediaStream will be stopped, so the recording functionality should be fine.
2. My patch to store the error callbacks seems not a correct solution for this bug, and the bug can't be fix in a short time if we need a error callback under this case.

Comment 16

[Jason Smith [:jsmith]]

Rob - What do you think here? Should we fire an error in this case or not? I'm in the middle on this one, as I could see a case for including onerror to indicate to the developer that an incoming call killed the recording vs. not including it aligns with stop() getting called on a media stream.

Comment 17

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

I don't think we should fire an error. The microphone input should be suspended temporarily while you're on the call, but afterwards it should resume as if nothing happened.

Comment 18

[Jason Smith [:jsmith]]

That makes sense to me. Okay, I'll fix the test case here.

Comment 19

[Jason Smith [:jsmith]]

Updated test cases.