1725470 - Thread model violation in EMEMediaDataDecoderProxy

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[John Lin [:jhlin][:jolin]]

When playing encrypted content on Android, EMEMediaDataDecoderProxy is constructed on one event target and its Init() runs on another. This will cause assertion crash like [1] in debug builds.

[1] https://searchfox.org/mozilla-central/rev/8f08c21f093be1c1c42438697f8bca67af94fc77/dom/media/platforms/android/RemoteDataDecoder.cpp#721

Comment 1

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1725470 - p1: pass proxy thread to the base class. r?bryce

EMEMediaDataDecoderProxy doesn't override all inherited methods. Without
providing the same proxy thread, it's possible to break to the thread model.

Comment 2

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1725470 - p2: call proxy methods instead of dispatch when already on proxy thread. r?bryce

Depends on D122627

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/888dfd0a90d2
p1: pass proxy thread to the base class. r=bryce
https://hg.mozilla.org/integration/autoland/rev/e5f8ba43ac8f
p2: call proxy methods instead of dispatch when already on proxy thread. r=bryce

Comment 4

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/888dfd0a90d2
https://hg.mozilla.org/mozilla-central/rev/e5f8ba43ac8f

Comment 5

[John Lin [:jhlin][:jolin]]

Comment on attachment 9236232 [details]
Bug 1725470 - p1: pass proxy thread to the base class. r?bryce

Beta/Release Uplift Approval Request

• User impact if declined: Potential content tab crash when playing protected media.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It is a simple change that ensures tasks will be dispatched to a specific task queue.
• String changes made/needed:

Comment 6

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 9236232 [details]
Bug 1725470 - p1: pass proxy thread to the base class. r?bryce

We are past betas and already built our Release Candidate, morphing the request into a release uplift request. Ryan, fyi.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Looking at the blame, this appears to be a longstanding issue? Is there a particular urgency for getting this into Fx92 during RC week?

Comment 8

[John Lin [:jhlin][:jolin]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

Looking at the blame, this appears to be a longstanding issue? Is there a particular urgency for getting this into Fx92 during RC week?

I suspected it's the cause of certain crash reports that showed mostly in release versions and would like to see if applying the patch fixes them.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

John pointed me to some of the bugs which may be fixed by this change. Looking through the crash volumes, I don't think this rises to the level of a last-minute RC respin driver, especially since the crashes are very rare on Nightly and we won't get a good feel for the impact of this bug until Beta. Given that, I'm going to leave the approval request pending for now so we can see some data for Beta93 and then consider this as a dot release ride-along should the opportunity arise.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9236232 [details]
Bug 1725470 - p1: pass proxy thread to the base class. r?bryce

No other good drivers for a mobile release and the crash volume is pretty low. Let's let this ride with 93.