924724 - Shutdown hang in Media Encoder when running state machine mochitest in bug 920595

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

STR

1. Run the mochitest in bug 920595 locally

Expected

The test passes with no errors.

Actual

The test passes, but the process fails to shutdown. See https://bugzilla.mozilla.org/show_bug.cgi?id=920595#c8 for more information.

Comment 1

[Ivan Tsay (:ITsay)]

Plus it since it is the hang issue.

Comment 2

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

It seems encoder doesn't receive the track_end notification when user close the browser while recording the media stream comes from audio tag /audioContext. gUM don't have this problem.

Comment 3

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

Attachment: patch v1

Try to remove encoder from media stream listener to avoid this problem.

Comment 4

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

try result
https://tbpl.mozilla.org/?tree=Try&rev=241e318ef09b

Comment 5

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

Test pasa of Running the mochitest in bug 920595 locally.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b83bf0fa7adc

Comment 7

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

Hi Ryan, 
Please help to backout this in inbound due to bug 920595 can't pass on try server.
Sorry for the inconvenience.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d7bd3ebe59cf

Comment 9

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

I found the DisconnectFromOwner fired while the page is on the focus window,
So close the recording page can exit record session successfully.
But if I try to record first and navigate to another URL, this event won't be triggered and keep hang...

Comment 10

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

May refer the AudioContext to Shutdown the instance while close the window, let me try...

Comment 11

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

Another case is 1. during recording and 2. directly navigate another URL 3. close firefox and found it still hang. the recording object can't get the cleanUp notification from GlobalWindow.

Comment 12

[Alex Keybl [:akeybl]]

This bug is not marked as a regression, and we're only down to critical blockers at this point. We need to limit risk/blockers.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Alex Keybl [:akeybl] from comment #12)
> This bug is not marked as a regression, and we're only down to critical
> blockers at this point. We need to limit risk/blockers.

That's incorrect. This is a hang on a 1.2 feature that has confirmed through Randy's testing through real world use cases of using the Media Recording API. We have target partner apps that need to work with this API, so risking shutdown hangs isn't acceptable. Renoming.

Comment 15

[Jason Smith [:jsmith]]

Reduced test case in the dupe.

Comment 16

[Jason Smith [:jsmith]]

Apparently the dupe confirmed this was a regression during the 1.2 timeframe.

Comment 17

[Jason Smith [:jsmith]]

Out of the three candidate bugs on the dupe, bug 896353 seems the most likely bug that caused this regression. The other two don't seem likely - bug 900419 is fixing an attribute state bug & bug 898952 is ensuring we are throwing an exception in one case. bug 896353 involves actual shutdown code, which makes it a very likely candidate.

Specifically: https://hg.mozilla.org/mozilla-central/rev/980905e0308e

Comment 18

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

We found the same call stack of those hang prlblem and still try to find solution for this object release problem.

Comment 19

[Shelly Lin [:shelly]]

Due to the change in bug 909640, how we destroy a MediaRecorder::Session totally relies on whether the source media stream coming from MediaStreamGraph is destroyed or not. There is no way to release the session if it is not connected with MediaStreamGraph, which is the case of recording an audio context or an audio tag.

MediaRecorder creates a Session with raw pointer, but this sessions uses nsRefPtr to hold a reference of MediaRecorder, increments the ref count of MediaRecorder to two. This is why closing the browser while recording an audio context fail to call any of these destructors.

Comment 20

[Shelly Lin [:shelly]]

Correcting my previous comment, if recording an audio context or an audio tag, MediaEncoder is still hooked up with MediaStreamGraph, but the source stream is not created by gUM.

Comment 21

[u459114]

(In reply to Jason Smith [:jsmith] from comment #17)
> Out of the three candidate bugs on the dupe, bug 896353 seems the most
> likely bug that caused this regression. The other two don't seem likely -
> bug 900419 is fixing an attribute state bug & bug 898952 is ensuring we are
> throwing an exception in one case. bug 896353 involves actual shutdown code,
> which makes it a very likely candidate.
> 
> Specifically: https://hg.mozilla.org/mozilla-central/rev/980905e0308e

Thanks, bug 896353 is the root cause, but the true cause is another place inside webaudio module.
We need to figure out how to send out stop event from media stream which source is an audio destination node.

Comment 22

[u459114]

While browser close, the desired behavior is

In MSG thread:
  a. AudioTrackEncoder::NotifyQueuedTrackChanges(MediaStreamListener::TRACK_EVENT_ENDED)
  b. AudioTrackEncoder::NotifyEndOfStream

In MediaRecorder::Session::ExtractThread:
  c. Unlock mEncoder->GetEncodedData(&encodedBuf, mimeType)
  d. Destroy Session in NS_DispatchToMainThread(new DestroyRunnable(this))
  e. Session destroy MediaRecorder

The problem is #a does not happened at all if the source is MediaStreamAudioDestinationNode.
JW, please help to figure out code inside webaudio

Comment 23

[Jason Smith [:jsmith]]

(In reply to C.J. Ku[:CJKu] from comment #21)
> (In reply to Jason Smith [:jsmith] from comment #17)
> > Out of the three candidate bugs on the dupe, bug 896353 seems the most
> > likely bug that caused this regression. The other two don't seem likely -
> > bug 900419 is fixing an attribute state bug & bug 898952 is ensuring we are
> > throwing an exception in one case. bug 896353 involves actual shutdown code,
> > which makes it a very likely candidate.
> > 
> > Specifically: https://hg.mozilla.org/mozilla-central/rev/980905e0308e
> 
> Thanks, bug 896353 is the root cause, but the true cause is another place
> inside webaudio module.
> We need to figure out how to send out stop event from media stream which
> source is an audio destination node.

I don't think this is related to Web Audio. The mochitest I have that also reproduces this bug doesn't use any Web Audio APIs.

Comment 24

[u459114]

(In reply to Jason Smith [:jsmith] from comment #23)
> I don't think this is related to Web Audio. The mochitest I have that also
> reproduces this bug doesn't use any Web Audio APIs.
Double confirm, While you testing, did you ever run
1. content/media/test/test_mediarecorder_record_audiocontext.html
2. any test cases in content/media/webaudio

Any test cases in #1 or #2 cause in undead thread in https://bugzilla.mozilla.org/show_bug.cgi?id=920595#c8

Comment 25

[u459114]

sorry, please ignore #2 in previous comment

Comment 26

[u459114]

If you didn't run test_mediarecorder_record_audiocontext.html, and browser still has problem to shutdown, then we have another root cause here.

Comment 27

[Jason Smith [:jsmith]]

(In reply to C.J. Ku[:CJKu] from comment #24)
> (In reply to Jason Smith [:jsmith] from comment #23)
> > I don't think this is related to Web Audio. The mochitest I have that also
> > reproduces this bug doesn't use any Web Audio APIs.
> Double confirm, While you testing, did you ever run
> 1. content/media/test/test_mediarecorder_record_audiocontext.html
> 2. any test cases in content/media/webaudio
> 
> Any test cases in #1 or #2 cause in undead thread in
> https://bugzilla.mozilla.org/show_bug.cgi?id=920595#c8

Nope. I ran the mochitest in bug 920595 as a standalone - no other mochitest was ran in the process.

I saw this also happen btw by doing the following STR:

1. Go to http://mozilla.github.io/qa-testcase-data/webapi/mediarecorder/index.html in the browser
2. Select setup under setup opus stream for media recorder
3. Select play for each set of media controls
4. Select start recording
5. Quit the existing content process

That STR doesn't use WebAudio at all, so this isn't a Web Audio bug.

Comment 28

[Jason Smith [:jsmith]]

Blocking+ again in triage - can't shutdown an active process if media recording is active is really bad.

Comment 29

[Benjamin Kerensa [:bkerensa]]

roc: Do we have any examples of this in the wild and how likely is it to impact our desktop users?

Comment 30

[Jason Smith [:jsmith]]

(In reply to Benjamin Kerensa from comment #29)
> roc: Do we have any examples of this in the wild and how likely is it to
> impact our desktop users?

This is a new API, so I don't see this being out in the wild right now & hit by our desktop users. The only people that will hit this will be bleeding edge developers that make use of the Media Recording API. 

For Firefox OS - This API is targeted for Gecko 26 to support two partner app use cases. So it will eventually be hit by our users when these partner apps are submitted to marketplace & used, but not yet.

Comment 31

[Shelly Lin [:shelly]]

Either way the MediaRecorder should implement a way to force quit any running recording session.

Comment 32

[u459114]

(In reply to Shelly Lin [:shelly] from comment #31)
> Either way the MediaRecorder should implement a way to force quit any
> running recording session.
If we want to do it, I think we may add watch dog monitor at MediaRecorder. In MediaEncoder, considerate we can pause recording in MediaRecorder, we can't do this at this layer,

Comment 33

[u459114]

There are two ways to stop a record session. The first way is to call MediaRecorder::Stop(top-down). The second way is SourceMediaStream stop, and send TRACK_END notification to MediaEncoder(which is a Stream listener).

There are two bugs here.
1. audio destination node as source: in this case, the second path does not exist at all
2. audio tag as source: still tracking

Comment 34

[u459114]

(In reply to Shelly Lin [:shelly] from comment #31)
> Either way the MediaRecorder should implement a way to force quit any
> running recording session.

Here is my proposal:
1. Inherit session from nsIObserver.
2. In Session::Observe(), call Stop to make sure Recording thread exit.
3. In constructor of MediaRecorder::Session. registry shout down notification.
  nsContentUtils::RegisterShutdownObserver();
Any opinions are appreciate.

I think we may prevent(not fix) this bug. 
Bug 933695 is a bug that I found while debugging, which is another bug that we need to fix.

Comment 35

[Alex Keybl [:akeybl]]

This needs to be tracked all the way up to FF26 if we think this is a critical platform issue introduced in that Gecko version. Thanks for marking the bug properly with the regression keyword now.

Comment 36

[u459114]

Attachment: recieve_shutdown_notification

Had verified on test case in bug 920595 and Randy's testing web site.

Comment 37

[u459114]

Attachment: recieve_shutdown_notification

In this patch
1. Registry shutdown notification.
2. If the source media stream(or Audio node stream) does not send to TRACK_EVENT_END notification correctly to MediaEncoder, we still have a change to close Read Thread.

This is a solution to make MediaRecorder more robust. From correctness perspective, we still need to figure out 
1. Bug 933695: no TRACK_EVENT_END to MediaEncoder
2. AudioTag: no TRACK_EVENT_END to MediaEncoder

Comment 38

[u459114]

Attachment: recieve_shutdown_notification

Comment 39

[Jason Smith [:jsmith]]

C.J. - If we include the patch seen on this bug for 1.2, then do we need to continue to block on bug 932644 & bug 933365? Just wondering if bug 932644 & bug 933695 are required for 1.2 or not if we have the patch included in this bug.

Comment 40

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #39)
> C.J. - If we include the patch seen on this bug for 1.2, then do we need to
> continue to block on bug 932644 & bug 933365? Just wondering if bug 932644 &
> bug 933695 are required for 1.2 or not if we have the patch included in this
> bug.

The second bug reference is intended to be - bug 933695.

Comment 41

[u459114]

(In reply to Jason Smith [:jsmith] from comment #40)
> (In reply to Jason Smith [:jsmith] from comment #39)
> > C.J. - If we include the patch seen on this bug for 1.2, then do we need to
> > continue to block on bug 932644 & bug 933365? Just wondering if bug 932644 &
> > bug 933695 are required for 1.2 or not if we have the patch included in this
> > bug.
> 
> The second bug reference is intended to be - bug 933695.

bug 933695 - this bug still exist after apply patch here. I think this one is another 1.2 blocker.
bug 932644 - this one is vanished after patch apply.

Comment 42

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

Comment on attachment 826410 [details] [diff] [review]
recieve_shutdown_notification

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

::: content/media/MediaRecorder.cpp
@@ +127,5 @@
> +      // SourceMediaStream is ended, and send out TRACK_EVENT_END notification.
> +      // Read Thread will be terminate soon.
> +      // We need to switch MediaRecorder to "Stop" state first to make sure
> +      // MediaRecorder is not associated with this Session anymore, then, it's
> +      // safe to  delete this Session.

remove extra space

@@ +318,5 @@
>    // Hold a reference to MediaRecoder to make sure MediaRecoder be
>    // destroyed after all session object dead.
>    nsRefPtr<MediaRecorder> mRecorder;
>  
> +  // Recieve track data from source and dispatch to Encoder.

"Receive"

@@ +338,5 @@
>  };
>  
> +NS_IMPL_ISUPPORTS1(MediaRecorder::Session, nsIObserver)
> +
> +

Delete second blank line

Comment 43

[u459114]

Attachment: Receive shutdown notification to terminate Read Thread

Comment 44

[u459114]

TryServer result
https://tbpl.mozilla.org/?tree=Try&rev=8bf774a23401

Comment 46

[Jason Smith [:jsmith]]

(In reply to C.J. Ku[:CJKu] from comment #44)
> TryServer result
> https://tbpl.mozilla.org/?tree=Try&rev=8bf774a23401

Note - the try server run really needs to include the patch on bug 920595 as well to verify that this will cleanly pass in CI.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fdc953ce073e

Comment 48

[Jason Smith [:jsmith]]

Kicked off a try run with this patch + automated test patch

https://tbpl.mozilla.org/?tree=Try&rev=f4ea8141ed5f

Comment 49

[JW Wang [:jwwang] [:jw_wang]]

Follwed up in Bug 933695.

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fdc953ce073e

Comment 51

[Ryan VanderMeulen [:RyanVM]]

This depends heavily on bug 909670, which wasn't uplifted to b2g26. Please post a branch-specific patch or get approval to uplift that.

Comment 52

[Jason Smith [:jsmith]]

Marking verified per the try run - already confirmed the mochitest that triggers this bug originally is fixed by this patch.

Comment 53

[u459114]

need to uplift bug 919051, bug 817194, bug 909670.
koi? set on those bugs.

Comment 54

[Lukas Blakk [:lsblakk] use ?needinfo]

Ryan - why did this get wontfixed for 27 in comment 51?  Does this not need to get onto Aurora/Beta?

Comment 55

[Lukas Blakk [:lsblakk] use ?needinfo]

If this does need to be uplifted to mozilla-beta and mozilla-aurora (and not just for B2G) then please provide uplift nominations with risk assessment.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Sorry, I had assumed that this bug was B2G-only given the koi+ status and comment 13. Feel free to change the flags if I got that wrong.

Comment 57

[Jason Smith [:jsmith]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #56)
> Sorry, I had assumed that this bug was B2G-only given the koi+ status and
> comment 13. Feel free to change the flags if I got that wrong.

This feature technically works cross-platform, even though the team working on this happens to be a b2g-focused team.

Comment 58

[Jason Smith [:jsmith]]

(In reply to [e-mail] [:lsblakk] from comment #55)
> If this does need to be uplifted to mozilla-beta and mozilla-aurora (and not
> just for B2G) then please provide uplift nominations with risk assessment.

Actually - we first actually need to get a branch-specific patch here with a r+ and then nominate that patch for approval.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Not uplifting this to b2g26 until I know what the plan is for Aurora/Beta. Please set approval requests accordingly.

Comment 60

[u459114]

Attachment: firefox 26 - fix shutdown crash

Patch for FF 26

Comment 61

[u459114]

Attachment: firefox 27 - fix shutdown crash

Comment 62

[Ryan VanderMeulen [:RyanVM]]

(In reply to C.J. Ku[:CJKu] from comment #60)
> Created attachment 829195 [details] [diff] [review]
> firefox 26 - fix shutdown crash
> 
> Patch for FF 26

Did you mean to mark this obsolete? Also, please request approval for mozilla-beta on this patch and mozilla-aurora on the Fx27 patch.

Comment 63

[Maire Reavy [:mreavy]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #62)
> (In reply to C.J. Ku[:CJKu] from comment #60)
> > Created attachment 829195 [details] [diff] [review]
> > firefox 26 - fix shutdown crash
> > 
> > Patch for FF 26
> 
> Did you mean to mark this obsolete? Also, please request approval for
> mozilla-beta on this patch and mozilla-aurora on the Fx27 patch.

Thanks, Ryan.  I'm doing a needinfo to CJ because he's best qualified to write the requests for uplift.

Comment 64

[u459114]

Attachment: firefox 26 + b2g26 - fix shutdown crash

fix firefox 26(beta) and b2g26 shutdown crash

Comment 65

[u459114]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #62)
> (In reply to C.J. Ku[:CJKu] from comment #60)
> > Created attachment 829195 [details] [diff] [review]
> > firefox 26 - fix shutdown crash
> > 
> > Patch for FF 26
> 
> Did you mean to mark this obsolete? Also, please request approval for
> mozilla-beta on this patch and mozilla-aurora on the Fx27 patch.
Yes. 
I am currently work on patch for Fx27 and (Fx26/B2G26)

Comment 66

[u459114]

Attachment: firefox 27 - fix shutdown crash

Comment 67

[u459114]

Attachment: firefox 26 + b2g26 - fix shutdown crash

Comment 68

[u459114]

Comment on attachment 829432 [details] [diff] [review]
firefox 26 + b2g26 - fix shutdown crash

Hi Roc,
Please help to review this patch.
This patch is target on b2g16(B2G 1.2) and Fx26(mozilla_beta). To limit the size of code change, I didn't move in MediaRecorder::Session into this patch.

I created only a ShutdownObserver object to receive shutdown notification.

Comment 69

[u459114]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #59)
> Not uplifting this to b2g26 until I know what the plan is for Aurora/Beta.
> Please set approval requests accordingly.

- Fx27(mozilla_aurora)
We can uplift "Receive shutdown notification to terminate Read Thread" patch into aurora directly. Since the only difference, MediaRecoder.cpp, between mozilla_aurora and mozilla_central is this patch.

- b2g16(B2G 1.2) and Fx26(mozilla_beta)
Under reviewing.

Comment 70

[u459114]

Comment on attachment 826682 [details] [diff] [review]
Receive shutdown notification to terminate Read Thread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 896935. This bug exists after we enable media recorder feature.
User impact if declined: Can not shutdown browser while a page is recording a audio element or a webaudio context.
Testing completed (on m-c, etc.): on mozilla-central
Risk to taking this patch (and alternatives if risky): The code change is minor. Only construct a mechanism to receive shutdown notification to stop "read thread" while browser shutdown. Risk is low
String or IDL/UUID changes made by this patch: No

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Don't forget to request beta approval on the Fx26 patch as well :)

Comment 72

[u459114]

Comment on attachment 829432 [details] [diff] [review]
firefox 26 + b2g26 - fix shutdown crash

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  Bug 896935. This bug exists after we enable media recorder feature.
User impact if declined: Can not shutdown browser while a page is recording from a audio element or a webaudio context.
Testing completed (on m-c, etc.): local test media testing suite(content/media/test). All passed.
Risk to taking this patch (and alternatives if risky): Minor or none. This patch only changed MediaRecorder internal behavior while browser shutdown.
String or IDL/UUID changes made by this patch: none

mozilla-beta & b2g26 both need to have this patch to prevent Bug 924724

Comment 73

[u459114]

Comment on attachment 829432 [details] [diff] [review]
firefox 26 + b2g26 - fix shutdown crash

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

Remove approval‑mozilla‑beta?. Need to check test case again

Comment 74

[u459114]

Attachment: firefox 26 + b2g26 - fix shutdown crash

If MediaRecorder object destruct, in main thread, before shutdown notification, in main thread, we need to unregister ShutdownObserver first.

Comment 75

[u459114]

Attachment: firefox 26 + b2g26 - fix shutdown crash

TryServer result for "firefox 26 + b2g26 - fix shutdown crash" patch
https://tbpl.mozilla.org/?tree=Try&rev=546a4a81a366

Comment 76

[u459114]

Comment on attachment 830042 [details] [diff] [review]
firefox 26 + b2g26 - fix shutdown crash

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  Bug 896935. This bug exists after we enable media recorder feature.
User impact if declined: Can not shutdown browser while a page is recording from a audio element or a webaudio context.
Testing completed (on m-c, etc.): 
https://tbpl.mozilla.org/?tree=Try&rev=546a4a81a366
Risk to taking this patch (and alternatives if risky): 
Minor or none. This patch only changed MediaRecorder internal behavior while browser shutdown.
String or IDL/UUID changes made by this patch: none

mozilla-beta & b2g26 both need to have this patch to prevent Bug 924724

Comment 77

[u459114]

Label checkin-needed. 
mozilla-beta + mozilla-b2g26: "firefox 26 + b2g26 - fix shutdown crash"
mozilla-aurora: "Receive shutdown notification to terminate Read Thread"

Comment 78

[Ryan VanderMeulen [:RyanVM]]

Needs approval first. (You don't need to put checkin-needed on it anyway as I'm very much watching the bug anyway :)...)

Comment 79

[Jason Smith [:jsmith]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #78)
> Needs approval first. (You don't need to put checkin-needed on it anyway as
> I'm very much watching the bug anyway :)...)

Well technically for Fx26 landing you can land this on b2g26 w/o approval, as it already has a koi+.

Comment 80

[Ryan VanderMeulen [:RyanVM]]

I'm not double-landing this. I'll directly land it on b2g26 if beta approval is denied.

Comment 81

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/68e2cd102130
https://hg.mozilla.org/releases/mozilla-beta/rev/401bee952f9d

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/401bee952f9d