1239873 - Fix fragile shutdown of MediaStreamGraph using AsyncShutdown

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

From bug 1235968 comment 14: rework the shutdown code for MediaStreamGraph -- much like for MediaManager (using AsyncShutdown).

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

Review commit: https://reviewboard.mozilla.org/r/31889/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31889/

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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/

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Rebase. try seems to have crapped up?

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try is *almost* perma-orange. More to do.

Comment 6

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

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.)

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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/

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Would ideally like another pass to clean this up a bit, but seems green on try, so I'm putting it up.

Comment 10

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

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!

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f31612f8f3bb

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 16

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

Attachment: Backout f31612f8f3bb () from beta 47

MozReview-Commit-ID: J39kgU76VwL

Comment 17

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

Comment on attachment 8755520 [details] [diff] [review]
Backout f31612f8f3bb () from beta 47

wrong bug...

Comment 18

[Paul Adenot (:padenot)]

Attachment: Backout f31612f8f3bb () from beta 48

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.