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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e723334490

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Implement exclusivity checking for MediaPromises. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Don't reset request status in MediaDecoderStateMachine::FlushDecoding. v1

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 5

[Bobby Holley (:bholley)]

(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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e898e57b1374
https://hg.mozilla.org/mozilla-central/rev/340e18cb2cdc

Comment 7

[Ralph Giles (:rillian)]

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.

Comment 8

[Bobby Holley (:bholley)]

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.

Comment 9

[Ralph Giles (:rillian)]

Ok. As suggested, we'll land only part two until we can uplift fixes for bug 1124952 alongside part 1.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 11

[Ralph Giles (:rillian)]

Landed part 1. https://hg.mozilla.org/releases/mozilla-aurora/rev/ff571f6ff271