Closed Bug 1060192 Opened 10 years ago Closed 10 years ago

[EME] clear private data when directed

Categories

(Core :: Audio/Video, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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
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.
* 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.
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)
Attachment #8507723 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/187b1aea9615
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.