Incorrect labeling in DecodedStream.cpp

RESOLVED FIXED in Firefox 57

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
Created attachment 8905703 [details] [diff] [review]
patch

I found this bug during some testing. The problem is that we can have calls like this:

1. DecodedStream::Start()
2. DecodedStream::Stop()
3. DecodedStream::Start()

These happen on some media thread, but they post runnables to the main thread. Start() posts its runnable using SystemGroup. Stop() posts its runnable using mAbstractMainThread. We don't guarantee any ordering between how these are scheduled. So the DestroyData runnable posted in step 2 might actually run after the SystemGroup runnable posted in step 3.

If this happens, then we'll call OutputStreamManager::Connect in step 3 and assert because we're already connected. This is a bit similar to bug 1256520.

Ideally we would use mAbstractMainThread to post the sync runnable in steps 1 and 3. However, we can't do that because we can't use tail dispatch for sync runnables, and mAbstractMainThread uses tail dispatch.

Another option is to use SystemGroup when posting the DestroyData runnable in step 2. This seems to work as far as I can tell. However, I'm a little worried that there might be other problems where this SystemGroup runnable runs before some mAbstractMainThread runnable that was posted earlier.

Since I don't know this code well at all, I took a more conservative path and left the DestroyData runnable unlabeled. This way, it will run in the order it was posted to the main thread, regardless of which queue other runnables are posted to. I checked the ranking of common runnables, and I don't even see it there. So I don't think this will have much effect on performance.

JW, it's up to you whether we should leave the runnable unlabeled, or use SystemGroup to label it. Or we could do something else I haven't thought of.
Attachment #8905703 - Flags: review?(jwwang)
Consider the following flow (with your changes):
1. SetPlaying()
   -> UpdateStreamSuspended()
   -> mAbstractMainThread->Dispatch(r1)
2. Stop()
   DestroyData()
   -> NS_DispatchToMainThread(r2)

Is it possible for r2 to run before r1?
(Assignee)

Comment 2

3 months ago
No, r2 will never run before r1. Any unlabeled runnable:
  - will run after all runnables (labeled or unlabeled) that were dispatched before it and
  - will run before all runnables (labeled or unlabeled) that were dispatched after it.

I guess this does mean that using SystemGroup for DestroyData won't work though.
Thanks for the explanation.
Attachment #8905703 - Flags: review?(jwwang) → review+
Component: Audio/Video → Audio/Video: Playback

Comment 4

3 months ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f0ff3546ee
Fix DecodedStream labeling bug (r=jwwang)
https://hg.mozilla.org/mozilla-central/rev/f3f0ff3546ee
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.