Closed Bug 1567170 Opened 1 year ago Closed 1 year ago

media::Await in fuzzing target leaks threads

Categories

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

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: decoder, Assigned: decoder, NeedInfo)

References

Details

Attachments

(2 files)

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!

Flags: needinfo?(jyavenard)

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

Type: defect → task
Type: task → defect

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

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.

Flags: needinfo?(jyavenard)
See Also: → 1573072

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(drno)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30be24d99ba2
P1. Shutdown demuxer earlier. r=decoder
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13e2b7bfea3e
Avoid thread leak in media fuzzing target. r=jya

Reopening because second patch has not landed.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.