1567170 - media::Await in fuzzing target leaks threads

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

It looks like the latest version of the media fuzzing target (landed in bug 1465407) is spawning a new thread per iteration but these threads never exit. The code is

  void Run() {
    mBenchmark->Init();
    media::Await(
        GetMediaThreadPool(MediaThreadType::PLAYBACK), mBenchmark->Run(),
        [&](uint32_t aDecodeFps) {}, [&](const MediaResult& aError) {});
    return;
  }

If I use the old version of Run instead that used SpinEventLoopUntil it works:

  uint32_t Run()
  {
    bool done = false;
    uint32_t result = 0;

    mBenchmark->Init();
    mBenchmark->Run()->Then(
      // Non DocGroup-version of AbstractThread::MainThread() is fine for testing.
      AbstractThread::MainThread(), __func__,
      [&](uint32_t aDecodeFps) { result = aDecodeFps; done = true; },
      [&]() { done = true; });

   // Wait until benchmark completes.
    SpinEventLoopUntil([&]() { return done; });
    return result;
  }

:jya, I assume we need to use some kind of fixed thread pool with one worker thread that is being reused all the time with media::Await? Can you help me figure out how to do that? Or should I revert the code back to the old version? Thanks!

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1567170 - P1. Shutdown demuxer earlier. r?decoder

If for some reason mDecoder didn't exist, we would get an assertion inside FinalizeShutdown().

Comment 3

[Jean-Yves Avenard [:jya]]

So what is happening is that this code runs on the main thread and calls media::Await on the main thread.

media::Await will block until the promise is resolved.

the problem here is that media::Await is given a SharedThreadPool. A SharedThreadPool uses a nsThreadPool internally. Upon completion, a nsThreadPool deletes an unused thread by queuing a task on the main thread that will call nsThread::ShutdownAsync.

As the main thread is used in a loop, those pending tasks are never going to be run and the thread are never going to be deleted.
Each loop in the fuzzer will create a new SharedThreadPool which will in turn create 4 threads. It doesn't take long to fill 2GB of RAM.

the reason the other method worked fine is as an EventLoop gets created, the tasks deleting the unused threads get to run.

Ideally, nsThread shouldn't need to fire a task on the main thread when used from a nsThreadPool as the thread only gets to be shutdown when there are no tasks left pending. They are free to be deleted right away.

For now, the solution is to either:
1- Don't call media::Await on the main thread, and we should assert as such.
2- Take a strong reference to the SharedThreadPool so that it doesn't have to be re-created every single loop. A SharedThreadPool only gets re-created if no-one is using it anymore.

I'll open a follow-up bug to improve nsThreadPool/nsThread interaction.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:drno, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Christian Holler (:decoder)]

Attachment: Bug 1567170 - Avoid thread leak in media fuzzing target. r?jya

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30be24d99ba2
P1. Shutdown demuxer earlier. r=decoder

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/30be24d99ba2

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13e2b7bfea3e
Avoid thread leak in media fuzzing target. r=jya

Comment 9

[Christian Holler (:decoder)]

Reopening because second patch has not landed.

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/13e2b7bfea3e