Closed Bug 1123983 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

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.
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?
https://hg.mozilla.org/mozilla-central/rev/e898e57b1374
https://hg.mozilla.org/mozilla-central/rev/340e18cb2cdc
Status: NEW → RESOLVED
Closed: 5 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?
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+
You need to log in before you can comment on or make changes to this bug.