Closed Bug 1239873 Opened 5 years ago Closed 5 years ago

Fix fragile shutdown of MediaStreamGraph using AsyncShutdown

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(2 files, 1 obsolete file)

From bug 1235968 comment 14: rework the shutdown code for MediaStreamGraph -- much like for MediaManager (using AsyncShutdown).
See Also: → 1234114
Rank: 10
Priority: -- → P1
Assignee: nobody → jib
Blocks: 1239570
Note that I couldn’t block past the final LIFECYCLE_WAITING_FOR_STREAM_DESTRUCTION stage, since completion of that stage seems to require shutdown to proceed.
Comment on attachment 8710865 [details]
MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31889/diff/1-2/
Rebase. try seems to have crapped up?
Try is *almost* perma-orange. More to do.
Attachment #8710865 - Flags: review?(rjesup)
Comment on attachment 8710865 [details]
MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

https://reviewboard.mozilla.org/r/31889/#review28685

Overall looks good; a few questions and one real (I think) issue

::: dom/media/MediaStreamGraph.cpp:1356
(Diff revision 2)
> +      gMediaStreamGraphShutdownBlocker = nullptr;

So I'm wondering... I think MediaStreamGraphShutDownRunnables exist for *each* extant graph, so don't we want to remove the blocker only when all of the ones we marked for mForceShutdown have actually shut down, not just one of them?

::: dom/media/MediaStreamGraph.cpp:1359
(Diff revision 2)
> +      // stage, since completion of that stage requires shutdown to proceed.

Elaborate on this (I presume other things not yet shut down are holding streams still)

::: dom/media/MediaStreamGraph.cpp:2728
(Diff revision 2)
> +        {

Should we assert MainThread here?  Or is it well-guaranteed for ShutdownBlocker?  (Likely we don't need to assert it.)
https://reviewboard.mozilla.org/r/31889/#review28703

::: dom/media/MediaStreamGraph.cpp:1356
(Diff revision 2)
> +      gMediaStreamGraphShutdownBlocker = nullptr;

Yes, and I also need to handle when there are zero open graphs at shutdown (i.e. remove blocker), which seems to be what's biting me on try.

Tip: you can drag-highlight several lines in mozReview. ;)

::: dom/media/MediaStreamGraph.cpp:1359
(Diff revision 2)
> +      // stage, since completion of that stage requires shutdown to proceed.

I presume so as well. Here's what I observed in an earlier version where I held the blocker all the way till MediaStreamGraph::Destroy():

> 2016-01-22 03:34:14.810041 UTC - [0x116873070]: D/MediaStreamGraph MediaStreamGraph 1731e6500 ForceShutdown
> 2016-01-22 03:34:14.810212 UTC - [0x17526fb30]: V/MediaStreamGraph MediaStream 172f44d00 bufferStartTime=0.043537 blockedTime=0.031927
> 2016-01-22 03:34:14.810240 UTC - [0x17526fb30]: D/MediaStreamGraph MediaStreamGraph 1731e6500 waiting for main thread cleanup
> 2016-01-22 03:34:14.843263 UTC - [0x116873070]: D/MediaStreamGraph Stopping threads for MediaStreamGraph 172de11c0
> 2016-01-22 03:34:22.345408 UTC - [0x116873070]: D/MediaStreamGraph Removing media stream 0 from the graph
> 2016-01-22 03:34:22.345432 UTC - [0x116873070]: D/MediaStreamGraph Removing media stream 0 from the graph
> 2016-01-22 03:34:22.345444 UTC - [0x116873070]: D/MediaStreamGraph Removing media stream 0 from the graph
> 2016-01-22 03:34:22.347174 UTC - [0x116873070]: D/MediaStreamGraph MediaStreamGraph 1731e6500 destroyed

I believe the ~8 idle seconds after "Stopping threads for MediaStreamGraph" is the AsyncShutdown blocker timing out after 10 seconds. Streams seem held up by other stuff.

::: dom/media/MediaStreamGraph.cpp:2728
(Diff revision 2)
> +        {

Seems well-guaranteed by API.
Comment on attachment 8710865 [details]
MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31889/diff/2-3/
Attachment #8710865 - Flags: review?(rjesup)
Would ideally like another pass to clean this up a bit, but seems green on try, so I'm putting it up.
Comment on attachment 8710865 [details]
MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

https://reviewboard.mozilla.org/r/31889/#review28799

::: dom/media/MediaStreamGraph.cpp:1455
(Diff revisions 2 - 3)
> -    }
> +    // which requires shutdown to proceed.

Should we re-block at a later stage?  This might be tricky, since some streams might continue to exist until the final CC pass, perhaps - which is after xpcom-shutdown-threads.  We have shut down the driver threads, however, so this should be ok.

::: dom/media/MediaStreamGraph.cpp:2834
(Diff revisions 2 - 3)
> -            MediaStreamGraphImpl* graph = iter.UserData();
> +            iter.UserData()->ForceShutDown(ticket);

This looks good!
Attachment #8710865 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #10)
> Should we re-block at a later stage?

That's definitely something we could look at as a followup, though I suspect you're right that we'd need to wait until final GC, which doesn't seem doable.

Try looks green and almost complete, I also have a green try from yesterday.
https://hg.mozilla.org/mozilla-central/rev/f31612f8f3bb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Duplicate of this bug: 1235968
Duplicate of this bug: 1234114
Depends on: 1255737
See Also: → 1270308
MozReview-Commit-ID: J39kgU76VwL
Assignee: jib → rjesup
Comment on attachment 8755520 [details] [diff] [review]
Backout f31612f8f3bb () from beta 47

wrong bug...
Attachment #8755520 - Attachment is obsolete: true
Assignee: rjesup → jib
We've made progress on this (two patches, fixing a lot of intermittents and
probably shutdown crashes in the wild as well), but we're not quite ready yet,
best to back it out again.
Attachment #8763606 - Flags: review?(jib)
Assignee: jib → padenot
Attachment #8763606 - Flags: review?(jib) → review+
Assignee: padenot → jib
Blocks: 1238542
You need to log in before you can comment on or make changes to this bug.