Flushing can cause the MDSM to invoke Request{Audio,Video}Decode multiple times

RESOLVED FIXED in Firefox 36

Status

()

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

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

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

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(2 attachments)

MediaDecoderStateMachine::FlushDecoding resets m{Audio,Video}RequestStatus to RequestStatus::Idle, which lets us call Request{Audio,Video}Data again. This causes two separate requests to share the same mVideoPromise, even though two requests will be dispatched to the underlying subdecoder. When the second request comes back, it will find that the promise holder is already empty, and crash.

This seems to be a holdover from the pre-MediaPromise days when RequestSampleCallback notifications would actually get flushed and never return. MediaPromises are guaranteed to either be resolved or rejected, so we should wait for them to be rejected before issuing new requests.

This would also be a good time to introduce better invariants against this kind of promise sharing (per bug 1107368). I'll include a patch to do that.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e723334490
Created attachment 8552123 [details] [diff] [review]
Part 1 - Implement exclusivity checking for MediaPromises. v1
Attachment #8552123 - Flags: review?(cpearce)
Created attachment 8552124 [details] [diff] [review]
Part 2 - Don't reset request status in MediaDecoderStateMachine::FlushDecoding. v1
Attachment #8552124 - Flags: review?(cpearce)
Attachment #8552123 - Flags: review?(cpearce) → review+
Attachment #8552124 - Flags: review?(cpearce) → review+
Comment on attachment 8552124 [details] [diff] [review]
Part 2 - Don't reset request status in MediaDecoderStateMachine::FlushDecoding. v1

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ -2924,5 @@
>  
> -  // These flags will be reset when the decoded data returned in OnAudioDecoded()
> -  // and OnVideoDecoded(). Because the decode tasks are flushed, these flags need
> -  // to be reset here.
> -  mAudioRequestStatus = RequestStatus::Idle;

It might be possible that another decoding task is enqueued before we acquire the lock and reset the status to idle and this patch just fix the problem?
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #1)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e723334490

Green.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e898e57b1374
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/340e18cb2cdc
https://hg.mozilla.org/mozilla-central/rev/e898e57b1374
https://hg.mozilla.org/mozilla-central/rev/340e18cb2cdc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8552124 [details] [diff] [review]
Part 2 - Don't reset request status in MediaDecoderStateMachine::FlushDecoding. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Crashes during video playback.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Both should be low risk.
[String/UUID change made/needed]: None.

Requesting uplift for both patches on this bug. Part 2 is more essential than Part 1 for beta, but I'd like to uplift both for consistency across branches. It may catch additional issues like this one.
Attachment #8552124 - Flags: approval-mozilla-beta?
Attachment #8552124 - Flags: approval-mozilla-aurora?
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
I think we should avoid uplifting part 1 (which adds extra release-mode assertions) until bug 1124952 is sorted out. Part 1 would be really useful on beta though (since it gives us much better and clearer crashes than we'd otherwise get), so I'll try to look at it today.
Depends on: 1124952
Ok. As suggested, we'll land only part two until we can uplift fixes for bug 1124952 alongside part 1.
Attachment #8552124 - Flags: approval-mozilla-beta?
Attachment #8552124 - Flags: approval-mozilla-beta+
Attachment #8552124 - Flags: approval-mozilla-aurora?
Attachment #8552124 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2555aadca9a4
https://hg.mozilla.org/releases/mozilla-beta/rev/e17127e00300

Leaving the statuses set to affected until Part 1 can land.
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Landed part 1. https://hg.mozilla.org/releases/mozilla-aurora/rev/ff571f6ff271
You need to log in before you can comment on or make changes to this bug.