Closed Bug 1173641 Opened 5 years ago Closed 5 years ago

MediaTaskQueue should drop its reference to the threadpool when it finishes shutting down


(Core :: Audio/Video, defect)

Not set



Tracking Status
firefox41 --- fixed


(Reporter: bholley, Assigned: bholley)




(3 files)

I just spent several hours debugging a really weird hang. It turned out to be the result of having MediaDecoderReader holding onto a reference to its task queue (indirectly, by virtue of having a mirror value).

The task queue keeps the thread pool alive, which in turn causes us to spin indefinitely at shutdown in SharedThreadPool::SpinUntilShutdown. And because that happens _before_ the final cycle collection, the MediaDecoderReader may still be held alive by script, causing the deadlock.

The solution would seem to be what I've written up in the bug title. Let's see if it works.
As soon as a class has a mirror or canonical, it's going to hold onto the task
queue until destruction anyway.
Attachment #8620760 - Flags: review?(jwwang)
Comment on attachment 8620757 [details] [diff] [review]
Part 1 - Hoist shutdown promise resolution into a helper. v1

Review of attachment 8620757 [details] [diff] [review]:

::: dom/media/MediaTaskQueue.cpp
@@ -237,5 @@
>      MonitorAutoLock mon(mQueue->mQueueMonitor);
>      MOZ_ASSERT(mQueue->mIsRunning);
>      if (mQueue->mTasks.size() == 0) {
>        mQueue->mIsRunning = false;
> -      mQueue->mShutdownPromise.ResolveIfExists(true, __func__);

I was confused by why we resolve the shutdown promise here for mIsShutdown could be still false. Now you make it clear.
Attachment #8620757 - Flags: review?(jwwang) → review+
Attachment #8620759 - Flags: review?(jwwang) → review+
Attachment #8620760 - Flags: review?(jwwang) → review+
You need to log in before you can comment on or make changes to this bug.