Closed Bug 1174939 Opened 5 years ago Closed 5 years ago

Eagerly perform tail dispatch for a given task queue when shutting it down

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

Currently we assert against this case, and bug 1163223 got backed out it (though jya says he's seen the same assertion stack without bug 1163223). I'm not sure exactly what task is in the dispatcher, but this is a reasonable situation to be in (especially with mirror updates etc), and I think we should handle it in MediaTaskQueue rather than expecting the caller to deal with it.

I'll attach some patches.
Attachment #8622765 - Flags: review?(jwwang) → review+
Attachment #8622766 - Flags: review?(jwwang) → review+
Comment on attachment 8622766 [details] [diff] [review]
Part 2 - Handle non-empty tail dispatchers in BeginShutdown. v1

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

::: dom/media/MediaTaskQueue.cpp
@@ +158,5 @@
>  
>  nsRefPtr<ShutdownPromise>
>  MediaTaskQueue::BeginShutdown()
>  {
>    // Make sure there are no tasks for this queue waiting in the caller's tail

Btw, you need to update the comment.

@@ -161,5 @@
>  {
>    // Make sure there are no tasks for this queue waiting in the caller's tail
>    // dispatcher.
> -  MOZ_ASSERT_IF(AbstractThread::GetCurrent(),
> -                !AbstractThread::GetCurrent()->TailDispatcher().HasTasksFor(this));

Btw, is it possible to just remove the assertion since tail dispatcher will kick in anyway at the end of event cycle? Do we still need to do tail dispatching explicitly?
(In reply to JW Wang [:jwwang] from comment #4)
> >    // Make sure there are no tasks for this queue waiting in the caller's tail
> 
> Btw, you need to update the comment.

The comment is still valid I think, but I can make it clearer.

> 
> @@ -161,5 @@
> >  {
> >    // Make sure there are no tasks for this queue waiting in the caller's tail
> >    // dispatcher.
> > -  MOZ_ASSERT_IF(AbstractThread::GetCurrent(),
> > -                !AbstractThread::GetCurrent()->TailDispatcher().HasTasksFor(this));
> 
> Btw, is it possible to just remove the assertion since tail dispatcher will
> kick in anyway at the end of event cycle? Do we still need to do tail
> dispatching explicitly?

No - the problem is that as soon as BeginShutdown is called, the task queue starts rejecting new tasks. That's why the assertion was there, and why we need to dispatch any existing tasks explicitly to preserve the expected semantics (Dispatch() calls that occur before BeginShutdown() should succeed).
Blocks: 1173656
https://hg.mozilla.org/mozilla-central/rev/c95ecd760bf3
https://hg.mozilla.org/mozilla-central/rev/5d48c36674e5
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.