Closed Bug 1725470 Opened 4 months ago Closed 4 months ago

Thread model violation in EMEMediaDataDecoderProxy

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
93 Branch
Tracking Status
firefox92 --- wontfix
firefox93 --- fixed

People

(Reporter: jhlin, Assigned: jhlin)

References

Details

Attachments

(2 files)

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

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

Pushed by jolin@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

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:
Attachment #9236232 - Flags: approval-mozilla-beta?
Attachment #9236233 - Flags: approval-mozilla-beta?

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.

Flags: needinfo?(ryanvm)
Attachment #9236232 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Attachment #9236233 - Flags: approval-mozilla-beta? → approval-mozilla-release?

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

Flags: needinfo?(ryanvm) → needinfo?(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.

Flags: needinfo?(jolin)

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.

Blocks: 1720532

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.

Attachment #9236232 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #9236233 - Flags: approval-mozilla-release? → approval-mozilla-release-
Blocks: 1689329
You need to log in before you can comment on or make changes to this bug.