[go: up one dir, main page]

Page MenuHomePhabricator

Bug 2079863 - Synchronize GeckoMediaDrmBridge's threads with release(). r?#geckoview-reviewers,jolin
ClosedPublic

Authored by RyanVM on Thu, Oct 15, 3:58 AM.

Details

Summary

The provisioning response and the pending session operations run on the handler thread and MediaDrm events
arrive on the main thread, while RemoteMediaDrmBridgeStub calls release() and the other entry points on a
binder thread. A release() in the middle of one of them could use the released MediaDrm and crash the process,
or start a handler thread that nothing quits. The entry points and those three paths now hold the bridge's
lock; the provisioning request itself stays outside it.

A second release() now returns early. The bridge releases itself after some MediaDrm failures, and the stub's
later release() used to throw before it finished its own teardown.

RemoteMediaDrmBridgeStub now updates its static list of stubs under the class lock Codec reads it with. Code
that read this state without a lock and has no callers is removed instead: isSecureDecoderComponentRequired(),
and MediaDrmProxy's getMediaCrypto() lookup with its proxy list, which takes getMediaCrypto() off the
GeckoMediaDrm interface.

A failed provisioning response now also rejects the queued session operations, which used to wait for a later
provisioning to succeed. A session that started the provisioning is rejected once, in release() as well.

Diff Detail

Event Timeline

RyanVM created this revision.
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Thu, Oct 15, 3:59 AM
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.

(Reviewing Diff 1407044)
The locking looks sound. Lock order is always stub, then bridge, never reversed. The callbacks made under the bridge lock are all oneway binder calls, so the main-thread listener can't stall on the remote side. Leaving the network fetch outside the lock keeps release() from waiting on a slow provisioning server. The early return in release() is safe because only release() clears mDrm. One gap remains: KeyStatusChangeListener and getMediaCrypto() still run without the lock, so a key-status callback can still be forwarded after teardown.


Automated review completed with no comments.
RyanVM updated this revision to Diff 1407924.
RyanVM edited the summary of this revision. (Show Details)
mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoMediaDrmBridge.java
742

This patch bundles three changes that can each land and be reviewed on their own:

  1. Dead-code removal. Drop isSecureDecoderComponentRequired(), MediaDrmProxy.sProxyList, the @WrapForJNI static getMediaCrypto(stubId) and getMediaCryptoFromBridge(), and getMediaCrypto() from the GeckoMediaDrm interface and RemoteMediaDrmBridge. Removing a JNI-exposed method changes the generated bindings, so it deserves its own review and its own revert path.
  2. Behavior change on failed provisioning. Add rejectPendingOperations and call it from onProvisionResponse so queued sessions are rejected (and the triggering promise is rejected only once). Include the serverErrorRejectsProvisioning addition and failedProvisioningForASessionRejectsItOnce. This changes what Gecko observes for queued sessions, unlike the rest of the patch, which only adds synchronization.
  3. Threading fix. Synchronize the public entry points, the listeners, the provisioning delivery and runPendingOperations. Make release() idempotent, lock the stub list, and include the tearDown, release-waits and key-status-after-release tests.

Splitting matters here because larger patches get less thorough review and empirically introduce more regressions. The locking change in particular needs careful deadlock analysis, and the large re-indentation of MediaDrmListener.onEvent already hides much of the diff. Separating these keeps each review focused and makes any regression easier to bisect and back out.

RyanVM added inline comments.
mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoMediaDrmBridge.java
742

Keeping this as one patch. The removed methods were deleted instead of being locked, so they go with the locking change, and nothing in C++ calls the removed JNI method. The rejection fix is small and has its own tests.

RyanVM updated this revision to Diff 1407988.
RyanVM retitled this revision from WIP: Bug 2079863 - Synchronize GeckoMediaDrmBridge's threads with release(). r?#geckoview-reviewers,jolin to Bug 2079863 - Synchronize GeckoMediaDrmBridge's threads with release(). r?#geckoview-reviewers,jolin.
RyanVM added reviewers: jolin, geckoview-reviewers.
jonalmeida added a subscriber: jonalmeida.

Rubber stamping.

This revision is now accepted and ready to land.Fri, Oct 16, 7:02 PM