[EME] clear private data when directed

RESOLVED FIXED in Firefox 37

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla36
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We need to plumb up the code to clear private data, and actually clear the private data.
Note: this bug covers plumbing up C++ code to flush data, not the UI to clear data.
Summary: [EME] Allow user to clear private data → [EME] clear private data when directed
"last-pb-context-exited" observer service notification fires when the last private browsing window closes. Need to clear data then.
Depends on: 1080068
Assignee: nobody → cpearce
Blocks: 1055393
We'll also need to kill running plugins that have touched storage when we clear storage, as they could end up in inconsistent state if we don't.
Created attachment 8507618 [details] [diff] [review]
Patch: Clear GMPStorage on "gmp-clear-storage notification

* GMPService observes the gmp-clear-storage notification, and delete the directory on disk when the notification is sent.
* GMPParent tracks whether the GMP instance has accessed storage, and when we clear storage we shutdown the GMPs that have accessed storage. We don't want them to write state after we've cleared state that depends on other data that we've cleared, as the GMPs may not work correctly subsequently.
* Unit tests to follow in bug 1055393.
Attachment #8507618 - Flags: review?(rjesup)
Comment on attachment 8507618 [details] [diff] [review]
Patch: Clear GMPStorage on "gmp-clear-storage notification

Review of attachment 8507618 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/gmp/GMPService.cpp
@@ +333,5 @@
> +      return rv;
> +    }
> +    thread->Dispatch(
> +      NS_NewRunnableMethod(this, &GeckoMediaPluginService::ClearStorage),
> +      NS_DISPATCH_SYNC);

I'm a little concerned about throwing a DISPATCH_SYNC from an observer, which must be MainThread.  A grep in the tree found no obvious instances of this being done, unless it was indirect.

Mostly I'm concerned if it's possible for JS to go reentrant due to this, since DISPATCH_SYNC spins the event loop.  If Observe() can be called from within a JS stack, then doing DISPATCH_SYNC from within it could cause JS reentrancy.

In general, the safe answer on MainThread is "don't use DISPATCH_SYNC".

I'm open to being convinced it's safe in this instance and hard to avoid... or find a way to not need it.
Attachment #8507618 - Flags: review?(rjesup) → review-
Basically, I spin the event loop here because when we re-add the GMPs after shutting them down, we need to sync dispatch from GMP thread to the main thread to get their parent process objects re-created on the main thread. So we can't do a SyncRunnable::DispatchToThread() here, as then we'd deadlock.

We could make the operation async, and have the GMPService fire a observer service notification when it's done.
SyncRunnable is almost always the wrong choice if you're on MainThread; it was really created for non-nsThreads to avoid promoting them (and then randomly leaking events to shutdown).

Let's make it async and then we don't have to worry.
Created attachment 8507723 [details] [diff] [review]
Patch v2: Async gmp-clear-storage-complete notification

Similar to previous patch, but sends a "gmp-clear-storage-complete" observer service notification once the storage is cleared, rather than spinning the main event loop.
Attachment #8507618 - Attachment is obsolete: true
Attachment #8507723 - Flags: review?(rjesup)

Updated

3 years ago
Attachment #8507723 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/187b1aea9615
https://hg.mozilla.org/mozilla-central/rev/187b1aea9615
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Mass update firefox-status to track EME uplift.
status-firefox37: --- → fixed
status-firefox38: --- → fixed
status-firefox39: --- → fixed
You need to log in before you can comment on or make changes to this bug.