hsivonen writes: We should at least clear salts (and the origin pair mappings to those salts and the stored data associated with the salts) that were created during the time frame to be cleared. As for whether we should zap the storage for salts that were created further back in the past than the time frame to be cleared but whose associated storage was written to in the time frame to be cleared, that's less obvious. The assumption is that the CDM wants the storage to be opaque and encrypts it so that no one examining the computer can make sense of it anyway. However, we don't enforce this in any way. Also, file system time stamps might reveal that the storage was written to at a particular time, so if the user isn't worried that someone examining local browser state finds out they accessed service Foo in general but is worried about learning that they accessed it at a particular time, this might still be relevant. What do we do if IndexedDB for a given site existed prior to the time frame being cleared but was written into during the time frame being cleared?

See also: W3 EME Bug 27166 - All identifiers associated with a user should be clearable in the same way cookies are https://www.w3.org/Bugs/Public/show_bug.cgi?id=27166

Gecko listens for the "gmp-clear-storage" observer service notification, and will clear all it's identifiers and storage upon receiving it. Firefox doesn't yet dispatch this notification.

JW: Can you please take this bug? One way we could do this is to change the code path that runs in the "gmp-clear-storage" observer, and have it optionally take a timestamp. Then when we have a timestamp supplied, we must iterate through all records' files in storage, and check the modification timestamp of the file. If the storage for a nodeId has any records modified after the timestamp passed in, we'll delete all storage and state for that nodeId. You'll also need to figure out how this gets triggered from Firefox javascript, and ensure we receive notification that we need to "clear recent history". I don't yet know how that happens.

Hi Ehsan, Do you know where is the javascript code that does "clear recent history"? We need to also clean EME data when "Clear Recent History" command is invoked. Thanks.

Yes, the code lives in browser/base/content/sanitize.js. Note that there is already a browser:purge-session-history notification that gets sent out from that code. It seems like gmp-clear-storage should be replaced with it. There is also webapps-clear-data that gets sent if an app's data is being removed, we should probably handle that as well. (But browser:purge-session-history doesn't include a timestamp. Usually things that respect the timestamp for clear recent history are handled in sanitize.js directly.)

Part 1 - include timestamps for "browser:purge-session-history" notification. We need this timestamp to know which nodeId/record to clear that is modified since the timestamp.

Part 2 - have GeckoMediaPluginService listen to "browser:purge-session-history" event. More to come in next patches.

Comment on attachment 8545044 [details] [diff] [review] 1111788_part1_include_timestamp_for_purge_session_history-v1.patch Review of attachment 8545044 [details] [diff] [review]: ----------------------------------------------------------------- Fine with me, but I'm not the person who needs to r+ changes here. Gavin: Can you review this? ::: browser/base/content/sanitize.js @@ +248,5 @@ > > try { > var os = Components.classes["@mozilla.org/observer-service;1"] > .getService(Components.interfaces.nsIObserverService); > + os.notifyObservers(null, "browser:purge-session-history", JSON.stringify(this.range[0])); Looks like this.range can be undefined (line 244), so this will probably crash if that's the case. A null this.range I think implies we should clear everything?

Comment on attachment 8545046 [details] [diff] [review] 1111788_part2_listen_to_purge_session_history_event-v1.patch Review of attachment 8545046 [details] [diff] [review]: ----------------------------------------------------------------- Based on what ehsan is saying, we should remove "gmp-clear-storage" and just use "browser:purge-session-history". ::: dom/media/gmp/GMPService.cpp @@ +330,5 @@ > NS_NewRunnableMethod(this, &GeckoMediaPluginService::ClearStorage)); > NS_ENSURE_SUCCESS(rv, rv); > + } else if (!strcmp("browser:purge-session-history", aTopic)) { > + LOGD(("clear recent history since %s", NS_ConvertUTF16toUTF8(aSomeData).Data())); > + nsresult rv; You need to handle the case where aSomeData represents the empty range, i.e. "clear everything".

Comment on attachment 8545044 [details] [diff] [review] 1111788_part1_include_timestamp_for_purge_session_history-v1.patch Review of attachment 8545044 [details] [diff] [review]: ----------------------------------------------------------------- Coming to think about it again, it might be better to pass both this.range[0] and this.range[1] in case we need both timestamps in the future.

Comment on attachment 8545044 [details] [diff] [review] 1111788_part1_include_timestamp_for_purge_session_history-v1.patch I was going to make the same suggestion as comment 10, but the reality is that many other sanitize.js data types don't support a non-now "end time" anyways, so it's probably better for us to try to drop support for that anyways. So I guess only passing this.range[0] is fine. You need to handle this.range being null, though, and JSON.stringify is overkill for converting a number to a string. I would use this: let clearStartingTime = this.range ? String(this.range[0]) : ""; os.notifyObservers(null, "browser:purge-session-history", clearStartingTime); r=me with that change.

Address nits in comment 11. Thanks for the review!

Address comment 9.

Comment on attachment 8546328 [details] [diff] [review] 1111788_part2_listen_to_purge_session_history_event-v2.patch Review of attachment 8546328 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gmp/GMPService.cpp @@ +331,5 @@ > + return GMPDispatch(NS_NewRunnableMethod( > + this, &GeckoMediaPluginService::ClearStorage)); > + } > + > + // Clear nodeIds/records modified after |t|. You should probably put this "Clear nodeIds/records modified after" block in the next patch, when you actually have code to clear records after time |t|.

Thanks for the quick review. Address comment 14.

oops! fix build errors.

Part 3 - clear nodeIds/records which are modified after the time of "clear recent history".

Comment on attachment 8546465 [details] [diff] [review] 1111788_part3_clear_nodeIds_records_modified_after-v1.patch Review of attachment 8546465 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. ::: dom/media/gmp/GMPService.cpp @@ +333,5 @@ > + > + // Clear nodeIds/records modified after |t|. > + nsresult rv; > + PRTime t = nsDependentString(aSomeData).ToInteger64(&rv, 10); > + NS_ENSURE_SUCCESS(rv, rv); jesup r-'d me for using NS_ENSURE_SUCCESS(rv,rv) in this file before, so we may as well keep using the verbose style mandated by the style guide of: if (NS_FAILED(rv)) { return rv; } We may as well be consistent, in this file at least. @@ +1327,5 @@ > > +void > +GeckoMediaPluginService::ClearRecentHistoryOnGMPThread(PRTime aSince) > +{ > +#define ERR_BREAK(x) if (NS_FAILED(x)) { break; } I'm not really enjoying reading the ERR_BREAK etc macros, and mentally expanding them every time I read them. If we consistently used these macros across the code base, it wouldn't be as taxing, but since this is the only file that uses this specific style, I think we should just expand out the code in these cases, and not use the macros. Code is read much more than it is written, so code should be optimized for readability, not writability (within reason ;). I know I r+'d them before, but they're starting to grate on me... Please remove them here and the existing ones above. Thanks. @@ +1340,5 @@ > + nsCOMPtr<nsIFile> path; > + ERR_BREAK(GetStorageDir(getter_AddRefs(path))); > + ERR_BREAK(path->AppendNative(NS_LITERAL_CSTRING("storage"))); > + storagePath = path.forget(); > + } while (false); It's a bit unfortunate having a do{...}while(false); loop here just to effectively get a goto. Can't this be: nsCOMPtr<nsIFile> storagePath; nsCOMPtr<nsIFile> temp; if (NS_SUCCEEDED(GetStorageDir(getter_AddRefs(temp))) && NS_SUCCEEDED(temp->AppendNative(NS_LITERAL_CSTRING("storage")))) { storagePath = temp.forget(); } @@ +1369,5 @@ > + } > + return false; > + } > + > + virtual bool operator()(nsIFile* aPath) { Might want to either comment that aPath is the dirEntry for $profileDir/gmp/id/$hash, or rename aPath so it's clear.

Comment on attachment 8546465 [details] [diff] [review] 1111788_part3_clear_nodeIds_records_modified_after-v1.patch Review of attachment 8546465 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gmp/GMPService.cpp @@ +333,5 @@ > + > + // Clear nodeIds/records modified after |t|. > + nsresult rv; > + PRTime t = nsDependentString(aSomeData).ToInteger64(&rv, 10); > + NS_ENSURE_SUCCESS(rv, rv); Is there a reason for not using NS_ENSURE_SUCCESS in this file?

The style guide discourages using NS_ENSURE_* macros now. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros It was enforced by jesup, the other reviewer here, so we may as well continue to enforce it. Personally I prefer the NS_ENSURE macros, but after having to mentally expand the ERR_* macros you were adding I am more sympathetic to the argument that the NS_ENSURE macros make it harder for newbies to understand our code.

address nits in comment 18.

try: https://tbpl.mozilla.org/?tree=Try&rev=7d2165225e94

https://hg.mozilla.org/integration/mozilla-inbound/rev/caf330700380 https://hg.mozilla.org/integration/mozilla-inbound/rev/f699fe109870 https://hg.mozilla.org/integration/mozilla-inbound/rev/140424a59bd7

https://hg.mozilla.org/mozilla-central/rev/caf330700380 https://hg.mozilla.org/mozilla-central/rev/f699fe109870 https://hg.mozilla.org/mozilla-central/rev/140424a59bd7

Patch for beta branch as part of EME platform uplift.

Patch for beta branch as part of EME platform uplift.

Patch for beta branch as part of EME platform uplift.

https://hg.mozilla.org/releases/mozilla-beta/rev/6195599f25e0 https://hg.mozilla.org/releases/mozilla-beta/rev/67145bce29be https://hg.mozilla.org/releases/mozilla-beta/rev/74d72da474f9

Comment on attachment 8572288 [details] [diff] [review] Patch 1 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572289 [details] [diff] [review] Patch 2 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572290 [details] [diff] [review] Patch 3 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572288 [details] [diff] [review] Patch 1 - Beta patch Previously approved as part of the EME platform landing on Beta.