1267933 - The shutdown process of MediaShutdownManager is flawed

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

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

https://hg.mozilla.org/mozilla-central/file/fc15477ce628599519cb0055f52cc195d640dc94/dom/media/MediaDecoder.cpp#l661

MediaDecoder::Shutdown() deregisters itself from MediaShutdownManager synchronously. It is possible for MediaDecoder::Shutdown() to happen just a moment before MediaShutdownManager::Shutdown(), and MediaShutdownManager::Shutdown() will return without waiting for the MediaDecoder to complete shutdown.

Comment 1

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

Will kinda rewrite the shutdown flow to amend the flaw.

Comment 2

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e866bc7f4a

Comment 3

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

Attachment: MozReview Request: Bug 1267933 - rewrite the shutdown sequence of MediaShutdownManager. r=jya,gerald.

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

Comment 4

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

This pretty much rewrite the whole shutdown process of MediaShutdownManager. Will need more eyes to ensure this is done correctly.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8748453 [details]
MozReview Request: Bug 1267933 - rewrite the shutdown sequence of MediaShutdownManager. r=jya,gerald.

https://reviewboard.mozilla.org/r/50301/#review47149

Looking good to me.
Optional nit:

::: dom/media/MediaDecoder.cpp:649
(Diff revision 1)
> -    shutdown = mDecoderStateMachine->BeginShutdown()
> +    mDecoderStateMachine->BeginShutdown()
> -        ->Then(AbstractThread::MainThread(), __func__, this,
> +      ->Then(AbstractThread::MainThread(), __func__, this,
> -               &MediaDecoder::FinishShutdown,
> +             &MediaDecoder::FinishShutdown,
> -               &MediaDecoder::FinishShutdown)
> -        ->CompletionPromise();
> +             &MediaDecoder::FinishShutdown);
> +  } else {
> +    // Ensure we always unregister MediaShutdownManager asynchronously.

Maybe explain why you need to be asynchronous (in this case because you're iterating over the decoders list, right?)

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8748453 [details]
MozReview Request: Bug 1267933 - rewrite the shutdown sequence of MediaShutdownManager. r=jya,gerald.

https://reviewboard.mozilla.org/r/50301/#review47157

::: dom/media/MediaDecoder.cpp:649
(Diff revision 1)
> -    shutdown = mDecoderStateMachine->BeginShutdown()
> +    mDecoderStateMachine->BeginShutdown()
> -        ->Then(AbstractThread::MainThread(), __func__, this,
> +      ->Then(AbstractThread::MainThread(), __func__, this,
> -               &MediaDecoder::FinishShutdown,
> +             &MediaDecoder::FinishShutdown,
> -               &MediaDecoder::FinishShutdown)
> -        ->CompletionPromise();
> +             &MediaDecoder::FinishShutdown);
> +  } else {
> +    // Ensure we always unregister MediaShutdownManager asynchronously.

This few lines are really the only thing that were needed to fix this bug : ensuring the MediaDecoder doesn't unregister while the shutdown manager is going

::: dom/media/MediaShutdownManager.cpp
(Diff revision 1)
>    for (auto iter = mDecoders.Iter(); !iter.Done(); iter.Next()) {
> -    promises.AppendElement(iter.Get()->GetKey()->Shutdown()->Then(
> -      // We want to ensure that all shutdowns have completed, regardless
> -      // of the ShutdownPromise being resolved or rejected. At this stage,
> -      // a MediaDecoder's ShutdownPromise is only ever resolved, but as this may
> +    iter.Get()->GetKey()->Shutdown();
> +    // Check MediaDecoder::Shutdown doesn't call Unregister() synchronously in
> +    // order not to corrupt our hashtable traversal.
> +    MOZ_ASSERT(mDecoders.Count() == oldCount);
> -      // change in the future we want to avoid nasty surprises, so we wrap the

You are now certainly regressing why we need to have MediaShutdownManager::Shutdown() to be blocking ; which was to ensure that gfx is always shutdown *after* all media related objects are shutdown (that include MediaDecoderReader, MediaDataDecoder etc): bug 1207220

Now you are all asynchronous, and you immediately exit. So gfx can be shutdown before the last reader shutdown.

Comment 7

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

https://reviewboard.mozilla.org/r/50301/#review47157

> You are now certainly regressing why we need to have MediaShutdownManager::Shutdown() to be blocking ; which was to ensure that gfx is always shutdown *after* all media related objects are shutdown (that include MediaDecoderReader, MediaDataDecoder etc): bug 1207220
> 
> Now you are all asynchronous, and you immediately exit. So gfx can be shutdown before the last reader shutdown.

We have |while (sInstance) {| to spin the loop so MediaShutdownManager::Shutdown() doesn't return until all MediaDecoders complete shutdown.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8748453 [details]
MozReview Request: Bug 1267933 - rewrite the shutdown sequence of MediaShutdownManager. r=jya,gerald.

https://reviewboard.mozilla.org/r/50301/#review47163

::: dom/media/MediaShutdownManager.cpp:124
(Diff revision 1)
> -    }
>    }
>  
> -  // Remove the MediaShutdownManager instance from the shutdown observer
> -  // list.
> -  nsContentUtils::UnregisterShutdownObserver(this);
> +  // Spin the loop until all decoders are unregistered
> +  // which will then clear |sInstance|.
> +  while (sInstance) {

Shouldn't you hold a reference to yourself here to be clearer? otherwise when sInstance is cleared, "this" will be deleted.

Or keep the comment that this likely got deleted once you exit the loop.

Comment 9

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/50301/#review47157

> We have |while (sInstance) {| to spin the loop so MediaShutdownManager::Shutdown() doesn't return until all MediaDecoders complete shutdown.

oh I had missed that...

Comment 10

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/50301/#review47167

Comment 11

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

https://reviewboard.mozilla.org/r/50301/#review47163

> Shouldn't you hold a reference to yourself here to be clearer? otherwise when sInstance is cleared, "this" will be deleted.
> 
> Or keep the comment that this likely got deleted once you exit the loop.

https://hg.mozilla.org/mozilla-central/file/0a25833062a880f369e6f9f622413a94cc671bf4/xpcom/ds/nsObserverList.cpp#l109
The publisher keeps subscribers alive during notification. However, it is a good practice not to access |this| after the while loop. I will add comments for that.

Comment 12

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

Thanks for the reviews!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19216295bd20

Comment 14

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

Attachment: 1267933_part2_add_comments.patch

Part 2 - add comments per comment 5. r=gerald.

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8748975 [details] [diff] [review]
1267933_part2_add_comments.patch

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

There was no need to request a review for that. :-)

Comment 16

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

Thanks for the super fast review! :)

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Gerald Squelart [:gerald] from comment #15)
> Comment on attachment 8748975 [details] [diff] [review]
> 1267933_part2_add_comments.patch
> 
> Review of attachment 8748975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There was no need to request a review for that. :-)

Oh, I see it's a separate patch, I suppose r+ is required before landing.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/945893b62336

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19216295bd20
https://hg.mozilla.org/mozilla-central/rev/945893b62336