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

RESOLVED FIXED in Firefox 41

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8622765 [details] [diff] [review]
Part 1 - Implement TaskDispatcher::DispatchTasksFor. v1
Attachment #8622765 - Flags: review?(jwwang)
(Assignee)

Comment 3

3 years ago
Created attachment 8622766 [details] [diff] [review]
Part 2 - Handle non-empty tail dispatchers in BeginShutdown. v1
Attachment #8622766 - Flags: review?(jwwang)
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?
(Assignee)

Comment 5

3 years ago
(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).
(Assignee)

Updated

3 years ago
Blocks: 1173656
https://hg.mozilla.org/mozilla-central/rev/c95ecd760bf3
https://hg.mozilla.org/mozilla-central/rev/5d48c36674e5
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.