1446931 - dom/media/Benchmark.cpp lacks error handling prior to decoder initialization

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

[Christian Holler (:decoder)]

The Benchmark class lacks handling errors in all stages prior to mDecoder being assigned. Most of the error handling paths call MainThreadShutdown()

https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/dom/media/Benchmark.cpp#243

but that function doesn't do anything if mDecoder is not assigned yet. This causes the promise to be never rejected and any code waiting for the promise hangs forever.

My first suggestion was to fix it by duplicating the inner (non-decoder related) code to the else branch, e.g. insert this after line 279:

> +  } else {
> +    RefPtr<Benchmark> ref(mMainThreadState);
> +    Thread()->AsTaskQueue()->BeginShutdown()->Then(
> +      ref->Thread(), __func__,
> +      [ref]() { ref->Dispose(); },
> +      []() { MOZ_CRASH("not reached"); });

This causes the promise to be rejected properly. However, I'm still seeing threads being leaked sometimes, so I am unsure if this is sufficient as a fix. Could mDecoderTaskQueue be populated already if mDecoder == nullptr? If so, we would probably have to shut that down as well in the else case.

Also, there is a call to MainThreadShutdown() missing here:

https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/dom/media/Benchmark.cpp#312

Comment 1

[Christian Holler (:decoder)]

Needinfo for :jya to help with this, as it seems more complicated to get this right without leaking any threads or memory.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1446931 - P1. Handle errors in Benchmark.

Benchmark had never been intended to be able to process invalid content. However over time Benchmark class has been used extensively by the fuzzing team.
As such, it became necessary to handle errors of all kind.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8963755 [details]
Bug 1446931 - P1. Handle errors in Benchmark.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232634/diff/1-2/

Comment 4

[Christian Holler (:decoder)]

Comment on attachment 8963755 [details]
Bug 1446931 - P1. Handle errors in Benchmark.

https://reviewboard.mozilla.org/r/232634/#review238102

::: dom/media/Benchmark.cpp:346
(Diff revision 2)
> +void
>  BenchmarkPlayback::InputExhausted()
>  {
>    MOZ_ASSERT(OnThread());
>    if (mFinished || mSampleIndex >= mSamples.Length()) {
>      return;

I don't think we can simply return here, right? Do we need to split this if() up and return an Error() in the second clause? Previously, I had to add a MainThreadShutdown() call here to prevent further hangs.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Christian Holler (:decoder) from comment #4)
> Comment on attachment 8963755 [details]
> Bug 1446931 - Handle errors in Benchmark.
> 
> https://reviewboard.mozilla.org/r/232634/#review238102
> 
> ::: dom/media/Benchmark.cpp:346
> (Diff revision 2)
> > +void
> >  BenchmarkPlayback::InputExhausted()
> >  {
> >    MOZ_ASSERT(OnThread());
> >    if (mFinished || mSampleIndex >= mSamples.Length()) {
> >      return;
> 
> I don't think we can simply return here, right? Do we need to split this
> if() up and return an Error() in the second clause? Previously, I had to add
> a MainThreadShutdown() call here to prevent further hangs.

That condition should never be entered, it's a remnant from where the decoders were callback based and we could have received a new callback while we were shutting down.

With the new promise based API, you can't get there...

should remove that code, or just assert on it...

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1446931 - P2. Refactor code flow in Benchmark.

The code relied on a flag to be set to simply abort. However, that made the code workflow hard to read.

We split each runs so that there's no ambiguity.

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

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8963755 [details]
Bug 1446931 - P1. Handle errors in Benchmark.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232634/diff/2-3/

Comment 8

[Christian Holler (:decoder)]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> 
> That condition should never be entered, it's a remnant from where the
> decoders were callback based and we could have received a new callback while
> we were shutting down.
> 
> With the new promise based API, you can't get there...
> 
> should remove that code, or just assert on it...

Prior to the patch in this bug, I was hitting this branch (hence I fixed it). I will add a MOZ_CRASH there and see if I can still hit this with out patch applied.

Comment 9

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

Comment on attachment 8963755 [details]
Bug 1446931 - P1. Handle errors in Benchmark.

https://reviewboard.mozilla.org/r/232634/#review238604

Comment 10

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

Comment on attachment 8963884 [details]
Bug 1446931 - P2. Refactor code flow in Benchmark.

https://reviewboard.mozilla.org/r/232740/#review238610

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13b8bb0d5cc6
P1. Handle errors in Benchmark. r=gerald
https://hg.mozilla.org/integration/autoland/rev/8e0de009a63f
P2. Refactor code flow in Benchmark. r=gerald

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/13b8bb0d5cc6
https://hg.mozilla.org/mozilla-central/rev/8e0de009a63f