If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement tail dispatch on the main thread

RESOLVED FIXED in Firefox 40

Status

()

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

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla40
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(4 attachments)

We're going to need to do this at some point if we want to use the state-mirroring machinery to mirror canonical main-thread values like mPlayState. I've also determined that TailDispatchers would nicely solve the races I'm seeing in bug 1144486 (given a few modifications).
Depends on: 1154805
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a79c466c48
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d1ad0e1a329
Created attachment 8593590 [details] [diff] [review]
Part 1 - Fix racey test_error_in_video_document.html. v1

This test assumes that the decoding machinery will have completed its work one
event-loop-round-trip after the document loads (see the executeSoon call), which
is a totally bogus assumption.
Attachment #8593590 - Flags: review?(jwwang)
Created attachment 8593591 [details] [diff] [review]
Part 2 - Assert against potential deadlocks between synchronous MediaTaskQueue operations and tail dispatchers. v1
Attachment #8593591 - Flags: review?(jwwang)
Created attachment 8593592 [details] [diff] [review]
Part 3 - Tighten up some existing assertions. v1

This is a followup from bug 1154805 comment 13.
Attachment #8593592 - Flags: review?(jwwang)
Created attachment 8593593 [details] [diff] [review]
Part 4 - Implement tail dispatch for the main thread. v1
Attachment #8593593 - Flags: review?(jwwang)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=382b0bae547d
Attachment #8593591 - Flags: review?(jwwang) → review+
Attachment #8593592 - Flags: review?(jwwang) → review+
Comment on attachment 8593593 [details] [diff] [review]
Part 4 - Implement tail dispatch for the main thread. v1

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

::: dom/media/AbstractThread.cpp
@@ +67,5 @@
>      MOZ_ASSERT(in == (GetCurrent() == this));
>      return in;
>    }
>  
> +  void FireTailDispatcher() { MOZ_ASSERT(mTailDispatcher.isSome()); mTailDispatcher.reset(); }

I am afraid this is not consistent with |appShell->RunInStableState|. We should run the runnables contained in TaskGroupRunnable immediately here instead of dispatch it because FireTailDispatcher() is already executed in the synchronous section.
Attachment #8593593 - Flags: review?(jwwang) → review-
Comment on attachment 8593593 [details] [diff] [review]
Part 4 - Implement tail dispatch for the main thread. v1

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

::: dom/media/AbstractThread.cpp
@@ +67,5 @@
>      MOZ_ASSERT(in == (GetCurrent() == this));
>      return in;
>    }
>  
> +  void FireTailDispatcher() { MOZ_ASSERT(mTailDispatcher.isSome()); mTailDispatcher.reset(); }

Thinking about it again, I don't think we need to make TailDispatcher interchangeable with |appShell->RunInStableState|.
Attachment #8593593 - Flags: review- → review+
(In reply to JW Wang [:jwwang] from comment #9)
> Thinking about it again, I don't think we need to make TailDispatcher
> interchangeable with |appShell->RunInStableState|.

Yeah - we use RunInStableState to implement TailDispatcher, but their semantics are fundamentally different.

Would you mind stamping the test change (part 1) as well? Then I can land it when inbound reopens. :-)
Flags: needinfo?(jwwang)
Comment on attachment 8593590 [details] [diff] [review]
Part 1 - Fix racey test_error_in_video_document.html. v1

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

This is indeed bug 608634. The delay-load-flag of the media element will prevent 'load' event of the page from being fired. Therefore, the error event of the media element should always be fired before the page load event. However, my last attempt to fix bug 608634 didn't succeed and the bug was pending until now. In order not to block your patch, I suggest to put a SimpleTest.todo to note we have to fix it in the future.
Attachment #8593590 - Flags: review?(jwwang) → review+
Flags: needinfo?(jwwang)
Thanks for the reviews!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b48c3a29d25
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5005606be182
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/af419f5b5ef9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/82b1e0300d4f
https://hg.mozilla.org/mozilla-central/rev/7b48c3a29d25
https://hg.mozilla.org/mozilla-central/rev/5005606be182
https://hg.mozilla.org/mozilla-central/rev/af419f5b5ef9
https://hg.mozilla.org/mozilla-central/rev/82b1e0300d4f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.