1111788 - "Clear Recent History" command should clear salts and associated origin pair mappings created during cleared history

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Peterson [:cpeterson]]

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?

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 5

[(no longer active)]

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.)

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1111788_part1_include_timestamp_for_purge_session_history-v1.patch

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.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1111788_part2_listen_to_purge_session_history_event-v1.patch

Part 2 - have GeckoMediaPluginService listen to "browser:purge-session-history" event.

More to come in next patches.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 10

[JW Wang [:jwwang] [:jw_wang]]

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 11

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1111788_part1_include_timestamp_for_purge_session_history-v2.patch

Address nits in comment 11. Thanks for the review!

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1111788_part2_listen_to_purge_session_history_event-v2.patch

Address comment 9.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

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|.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1111788_part2_listen_to_purge_session_history_event-v3.patch

Thanks for the quick review. Address comment 14.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1111788_part2_listen_to_purge_session_history_event-v4.patch

oops! fix build errors.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1111788_part3_clear_nodeIds_records_modified_after-v1.patch

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

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 19

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1111788_part3_clear_nodeIds_records_modified_after-v2.patch

address nits in comment 18.

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1 - Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2 - Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3 - Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8572288 [details] [diff] [review]
Patch 1 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8572289 [details] [diff] [review]
Patch 2 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8572290 [details] [diff] [review]
Patch 3 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8572288 [details] [diff] [review]
Patch 1 - Beta patch

Previously approved as part of the EME platform landing on Beta.