1111787 - "Forget This Site" command should clear EME salt values associated with the forgotten origin

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Peterson [:cpeterson]]

We should clear a salt if the user wants to forget about either of the origins in the origin pair associated with a salt.

Comment 1

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

We should clear storage associated with the salt too, since it can't be accessed anymore.

Comment 2

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

Start here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm#46

Comment 3

[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 4

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

JW: Can you take this bug?

I think you could do this by adding code to ForgetAboutSite.jsm to get the mozIGeckoMediaPluginService, and call a function on it (which you'll have to add and implement) that clears all storage and nodeIds for a given origin.

Comment 5

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

sure.

Comment 6

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

So all I need to do is delete these folders:
1. $profileDir/gmp/id/$hash/ where $hash is generated by the origin pair
2. $profileDir/gmp/storage/$nodeId/ where $nodeId is the salt generated for the origin pair

right?

Comment 7

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

Yes. You'll need to enumerate over $profileDir/gmp/id/* to check whether $profileDir/gmp/id/*/origin or $profileDir/gmp/id/*/topLevelOrigin match the origin being forgotten.

Comment 8

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

Attachment: 1111787_part1_forget_this_site-v1.patch

Patch part 1: delete the direcotry for the origin pair when "forget this site" is invoked.

More to come in next patch.

Comment 9

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

Comment on attachment 8539040 [details] [diff] [review]
1111787_part1_forget_this_site-v1.patch

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

::: dom/media/gmp/GMPService.cpp
@@ +933,5 @@
>  static nsresult
>  ReadFromFile(nsIFile* aPath,
>               const nsCString& aFileName,
>               nsCString& aOutData,
> +             int32_t aMaxLength = INT_MAX)

I don't think you should allow reading up to INT_MAX characters at once. If there happens to be a bad file being read, we could run out of memory.

i.e. don't specify "= INT_MAX" default value here.

@@ +1145,5 @@
> +MatchOrigin(nsIFile* aPath, const nsACString& aMatch)
> +{
> +  nsresult rv;
> +  nsCString str;
> +  rv = ReadFromFile(aPath, NS_LITERAL_CSTRING("origin"), str);

The max length for a valid textual representation of a domain name is 253 characters:

http://en.wikipedia.org/wiki/Domain_Name_System#Domain_name_syntax

I suggest you pass 253 here instead of implicitly passing INT_MAX.

@@ +1187,5 @@
> +
> +    // $profileDir/gmp/id/$hash
> +    nsCOMPtr<nsIFile> dirEntry(do_QueryInterface(supports, &rv));
> +    ERR_CONT(rv);
> +

You probably should `continue` here if !dirEntry->IsDirectory().

::: dom/media/gmp/mozIGeckoMediaPluginService.idl
@@ +98,5 @@
>  
>    /**
> +   * Clears storage data associated with the origin.
> +   */
> +  void forgetThisSite(in AString origin);

This changes the vtable on the mozIGeckoMediaPluginService interface. You need to run `./mach update-uuids mozIGeckoMediaPluginService` to ensure that the IID of the interface changes, else extensions and/or other code that was compiled under the old interface layout/ABI won't be foreced to recompile and will break in non-obvious ways.

Comment 10

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

Attachment: 1111787_part1_forget_this_site-v2.patch

Thanks for the review. Address comments in comment 9.

Comment 11

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

Attachment: 1111787_part2_add_IsAccessingStorageForNodeId_to_GMPParent-v1.patch

Part 2 - add ability to check whether GMPParent is accessing storage of a particular node ID.

This feature is needed by patch part 3 which needs to delete some of GMPParents which are accessing storage data for a particular node IDs.

Comment 12

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

Attachment: 1111787_part3_clear_plugins_storage-v1.patch

Part 3 - Clear plugis and storage data associated with the node IDs.

Comment 13

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

Comment on attachment 8540500 [details] [diff] [review]
1111787_part2_add_IsAccessingStorageForNodeId_to_GMPParent-v1.patch

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

I don't think we should only kill plugins that happen to have a record open at the time that we are running the "forget about this site" algorithm. The GMPs are probably unlikely to keep their records open; I'd expect them to open, read or write, then close. So we'd be likely to miss detecting GMPs that have accessed storage this way.

The reason why the code that you're changing here recorded whether a GMP had ever accessed storage was because I was worried that a GMP could write data to disk after we've cleared its storage that is dependent on state the GMP holds in memory, so the GMP's storage on disk count end up in an inconsistent, as far as it sees it anyway.

Also, if you clear the nodeId mapping (which you do in patch 1), but don't clear the stored records (which you do in patch 3), the stored records aren't accessible if a GMP for the same origins are re-created, as they'll have a different nodeId/salt. So it doesn't make sense to clear the nodeId/salt but not clear the records themselves, as the records themselves will then effectively "leak".

So, I think we should be more aggressive.

We should just kill all GMPs that match the nodeId, regardless of whether or not they have accessed storage or have records open. We should be able to rely on the existing shutdown code to ensure that no I/O can succeed while we're shutting down a GMP.

That means we can remove the HasAccessedStorage() logic.

So, please do the following:
* Remove the HasAccessedStorage() logic.
* Shutdown all plugins have nodeIds for the origin being cleared.

Comment 14

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

Comment on attachment 8540501 [details] [diff] [review]
1111787_part3_clear_plugins_storage-v1.patch

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

Just kill all plugins that match the nodeIds.

::: dom/media/gmp/GMPService.cpp
@@ +1276,5 @@
>    }
>  
> +  // Kill all plugins associated with the node IDs.
> +  KillPluginsForStorage(nodeIDsToClear);
> +

We also need to remove the NodeId from the mPersistentStorageAllowed hash table.

Comment 15

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

(In reply to Chris Pearce (:cpearce) (PTO until 5 Jan) from comment #13)
> I don't think we should only kill plugins that happen to have a record open
> at the time that we are running the "forget about this site" algorithm. The
> GMPs are probably unlikely to keep their records open; I'd expect them to
> open, read or write, then close. So we'd be likely to miss detecting GMPs
> that have accessed storage this way.

We want to delete GMPs that are accessing storage data to prevent inconsistency. We don't want GMPStorageParents stay alive while their underlying data is wiped out. ($profileDir/gmp/storage/$nodeId/ is deleted in patch 3) If a GMP opens, reads/writes and then closes before "Forget this site", we don't have the problem of inconsistency. All the storage data in $profileDir/gmp/storage/$nodeId/ is still cleared. I think that should be fine.

However, come to think about it again, I find GMPParent::mNodeId is still kept in the memory after "Forget this site". If we create a GMPStorageParent with that node ID later, we will create inconsistency since the node ID shouldn't be used anymore.

So, yes, you are right. We should delete GMPs that match the nodeId no matter they accessed the storage or not since mNodeId shouldn't be used anymore.

Comment 16

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

Btw, in GeckoMediaPluginService::ClearStorage(), we should delete all GMPParents which return false for CanBeSharedCrossNodeIds(), right? A GMPParent without node ID will not access storage data. It is fine not kill it.

Comment 17

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

Attachment: 1111787_part2_kill_plugins_with_valid_nodeIDs-v2.patch

1. Remove the HasAccessedStorage() logic.
2. Kill any plugin that has a valid nodeId when ClearStorage() is invoked.

Comment 18

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

Attachment: 1111787_part3_clear_plugins_storage-v2.patch

Clear plugis and storage data associated with the node IDs.

Comment 19

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

Comment on attachment 8542385 [details] [diff] [review]
1111787_part3_clear_plugins_storage-v2.patch

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

::: dom/media/gmp/GMPService.cpp
@@ +1163,5 @@
> +template<typename T> static void
> +KillPlugins(const nsTArray<nsRefPtr<GMPParent>>& aPlugins,
> +            Mutex& aMutex, T&& aFilter)
> +{
> +  // Shutdown all plugins that have matching node IDs as those IDs will become

The comment mentions matching nodeIds, but reality is that this function kills plugins that pass the filter function; the comment is inaccurate.

Comment 20

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

Attachment: 1111787_part3_clear_plugins_storage-v3.patch

Adress nits.

Comment 21

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

jw: is this ready to land?

Comment 22

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

Yes. I will open another bug for the test case.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=033cc285fcb0

Comment 23

[Carsten Book [:Tomcat]]

Hi Jw, this patch failed to apply (tried to land part1) and got:

Which patches do you want to import? [Default is '1', use '1-3' for all] 1
adding 1111787 to series file
renamed 1111787 -> 1111787_part1_forget_this_site-v2.patch
applying 1111787_part1_forget_this_site-v2.patch
patching file toolkit/forgetaboutsite/ForgetAboutSite.jsm
Hunk #1 FAILED at 75
1 out of 1 hunks FAILED -- saving rejects to file toolkit/forgetaboutsite/ForgetAboutSite.jsm.rej

could you take a look, thanks!

Comment 24

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

Attachment: 1111787_part1_forget_this_site-v3.patch

rebase part1. Please try again. Thanks.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff735f32aac
https://hg.mozilla.org/integration/mozilla-inbound/rev/20298762b610
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c7e9a00af8

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/7ff735f32aac
https://hg.mozilla.org/mozilla-central/rev/20298762b610
https://hg.mozilla.org/mozilla-central/rev/f3c7e9a00af8

Comment 27

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

Mass update firefox-status to track EME uplift.