Closed
Bug 1060192
Opened 10 years ago
Closed 10 years ago
[EME] clear private data when directed
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
8.38 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
We need to plumb up the code to clear private data, and actually clear the private data.
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
"last-pb-context-exited" observer service notification fires when the last private browsing window closes. Need to clear data then.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → cpearce
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
* 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 5•10 years ago
|
||
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-
Assignee | ||
Comment 6•10 years ago
|
||
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.
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
Attachment #8507723 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/187b1aea9615
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/187b1aea9615
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 11•9 years ago
|
||
Mass update firefox-status to track EME uplift.
You need to log in
before you can comment on or make changes to this bug.
Description
•