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

VERIFIED FIXED in Firefox 26

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jsmith, Assigned: u459114)

Tracking

(4 keywords)

26 Branch
1.2 C5(Nov22)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 unaffected, firefox26+ fixed, firefox27+ fixed, firefox28+ fixed, b2g-v1.2 fixed)

Details

Attachments

(2 attachments, 10 obsolete attachments)

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.
Blocks: MediaEncoder
Severity: normal → critical
blocking-b2g: --- → koi?
Keywords: hang, reproducible
Blocks: 920595
Assignee: nobody → rlin
QA Contact: rlin
Plus it since it is the hang issue.
blocking-b2g: koi? → koi+
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.
Posted patch patch v1 (obsolete) — Splinter Review
Try to remove encoder from media stream listener to avoid this problem.
Attachment #816427 - Flags: review?(roc)
Test pasa of Running the mochitest in bug 920595 locally.
Keywords: checkin-needed
Hi Ryan, 
Please help to backout this in inbound due to bug 920595 can't pass on try server.
Sorry for the inconvenience.
Flags: needinfo?(ryanvm)
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...
May refer the AudioContext to Shutdown the instance while close the window, let me try...
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.
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.
blocking-b2g: koi+ → -
(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.
blocking-b2g: - → koi?
Duplicate of this bug: 931972
Reduced test case in the dupe.
Keywords: testcase
Keywords: regression
Apparently the dupe confirmed this was a regression during the 1.2 timeframe.
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
Blocks: 896353
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
We found the same call stack of those hang prlblem and still try to find solution for this object release problem.
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.
Flags: needinfo?(cku)
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.
Assignee: rlin → cku
Flags: needinfo?(cku)
Depends on: 932644
(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.
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
Flags: needinfo?(jwwang)
(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.
(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
Flags: needinfo?(jsmith)
sorry, please ignore #2 in previous comment
If you didn't run test_mediarecorder_record_audiocontext.html, and browser still has problem to shutdown, then we have another root cause here.
(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.
Flags: needinfo?(jsmith)
Blocking+ again in triage - can't shutdown an active process if media recording is active is really bad.
blocking-b2g: koi? → koi+
roc: Do we have any examples of this in the wild and how likely is it to impact our desktop users?
Flags: needinfo?(roc)
(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.
Either way the MediaRecorder should implement a way to force quit any running recording session.
(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,
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
Target Milestone: --- → 1.2 C5(Nov22)
Depends on: 933695
(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.
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.
Posted patch recieve_shutdown_notification (obsolete) — Splinter Review
Had verified on test case in bug 920595 and Randy's testing web site.
Attachment #826358 - Flags: feedback?(slin)
Attachment #826358 - Flags: feedback?(rlin)
Posted file recieve_shutdown_notification (obsolete) —
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
Attachment #826358 - Attachment is obsolete: true
Attachment #826358 - Flags: feedback?(slin)
Attachment #826358 - Flags: feedback?(rlin)
Attachment #826409 - Flags: review?(roc)
Posted patch recieve_shutdown_notification (obsolete) — Splinter Review
Attachment #826409 - Attachment is obsolete: true
Attachment #826409 - Flags: review?(roc)
Attachment #826410 - Flags: review?(roc)
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.
(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.
(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 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
Attachment #826410 - Flags: review?(roc) → review+
Attachment #816427 - Attachment is obsolete: true
Attachment #826410 - Attachment is obsolete: true
No longer depends on: 933695
Duplicate of this bug: 932644
No longer depends on: 932644
(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.
Blocks: MediaRecording
No longer blocks: MediaEncoder
Kicked off a try run with this patch + automated test patch

https://tbpl.mozilla.org/?tree=Try&rev=f4ea8141ed5f
Follwed up in Bug 933695.
Flags: needinfo?(jwwang)
https://hg.mozilla.org/mozilla-central/rev/fdc953ce073e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
This depends heavily on bug 909670, which wasn't uplifted to b2g26. Please post a branch-specific patch or get approval to uplift that.
Flags: needinfo?(roc) → needinfo?(cku)
Marking verified per the try run - already confirmed the mochitest that triggers this bug originally is fixed by this patch.
Status: RESOLVED → VERIFIED
need to uplift bug 919051, bug 817194, bug 909670.
koi? set on those bugs.
Flags: needinfo?(cku)
Ryan - why did this get wontfixed for 27 in comment 51?  Does this not need to get onto Aurora/Beta?
Flags: needinfo?(ryanvm)
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.
Flags: needinfo?(cku)
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.
Flags: needinfo?(ryanvm)
(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.
(In reply to lsblakk@mozilla.com [: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.
Not uplifting this to b2g26 until I know what the plan is for Aurora/Beta. Please set approval requests accordingly.
Patch for FF 26
Attachment #829195 - Attachment is obsolete: true
Flags: needinfo?(cku)
(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.
(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.
Flags: needinfo?(cku)
fix firefox 26(beta) and b2g26 shutdown crash
(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)
Flags: needinfo?(cku)
Attachment #829201 - Attachment is obsolete: true
Attachment #829385 - Attachment is obsolete: true
Attachment #829401 - Attachment is obsolete: true
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.
Attachment #829432 - Flags: review?(roc)
(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 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
Attachment #826682 - Flags: approval-mozilla-aurora?
Don't forget to request beta approval on the Fx26 patch as well :)
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
Attachment #829432 - Flags: approval-mozilla-beta?
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
Attachment #829432 - Flags: approval-mozilla-beta?
If MediaRecorder object destruct, in main thread, before shutdown notification, in main thread, we need to unregister ShutdownObserver first.
Attachment #829432 - Attachment is obsolete: true
TryServer result for "firefox 26 + b2g26 - fix shutdown crash" patch
https://tbpl.mozilla.org/?tree=Try&rev=546a4a81a366
Attachment #829999 - Attachment is obsolete: true
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
Attachment #830042 - Flags: approval-mozilla-beta?
Label checkin-needed. 
mozilla-beta + mozilla-b2g26: "firefox 26 + b2g26 - fix shutdown crash"
mozilla-aurora: "Receive shutdown notification to terminate Read Thread"
Keywords: checkin-needed
Needs approval first. (You don't need to put checkin-needed on it anyway as I'm very much watching the bug anyway :)...)
Keywords: checkin-needed
(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+.
I'm not double-landing this. I'll directly land it on b2g26 if beta approval is denied.
Attachment #826682 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #830042 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 900431
Component: Video/Audio → Video/Audio: Recording
No longer blocks: MediaRecording
You need to log in before you can comment on or make changes to this bug.