1228215 - [EME] Segregate GMP storage by GMP

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect)

Description

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

We should separate the GMP storage by plugin, so that GMPs can't read each others' records and so we expose different node Ids for the same origin pair to different GMPs.

Comment 1

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

Attachment: Patch 1: Add helper to do dir enumeration in GMPServiceParent.

Add a helper to do directory enumeration in GMPServiceParent, since we do it a bit there, and I'll need to add a few more calls.

Comment 2

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

Attachment: Patch 2: Add a 'gmpName' parameter to GMPService::GetNodeId()

Add a GMPName argument to GMPService::GetNodeId(). This is so that GetNodeId() can return a different nodeId for different GMPs for the same origin. GMPName is "gmp-eme-adobe", "gmp-clearkey", etc.

I implement GMPService::GetNodeId() in a later patch.

Comment 3

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

Attachment: Patch 3: Expose the GMP name on each GMPParent

The GMPParents know their name ("gmp-eme-adobe" etc), we'll need to access that when deciding where to store records, so expose it.

Comment 4

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

Attachment: Patch 4: Store each GMP's storage and nodeId salt in separate directories

* Instead of all GMP storage going in $profileDir/gmp/$platform/, store it in $profileDir/gmp/$platform/gmp-name/.
* Implement GMPService::GetNodeId() returning separate nodeIds for each GMP/origin-pair.
* Store the nodeId salt info in new per-GMP storage location.
* Store new GMP records in new new per-GMP storage location
* Update data clearing for new per-GMP storage location.
* Update comments.
* gTests still pass :)

Comment 5

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

Attachment: Patch 5: Migrate existing GMP storage to new per-GMP location

Migrate existing GMP storage records to new location, overwriting if new location already exists.

Comment 6

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

Comment on attachment 8692294 [details] [diff] [review]
Patch 1: Add helper to do dir enumeration in GMPServiceParent.

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

::: dom/media/gmp/GMPUtils.h
@@ +50,5 @@
> +    DirsOnly, // Enumeration only includes directories.
> +    FilesAndDirs // Enumeration includes directories and non-directory files.
> +  };
> +
> +  explicit DirectoryEnumerator(nsIFile* aPath, Mode aMode);

Do we need "explicit" for a constructor that takes more than one argument?

Comment 7

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

Comment on attachment 8692300 [details] [diff] [review]
Patch 4: Store each GMP's storage and nodeId salt in separate directories

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

::: dom/media/gmp/GMPServiceParent.cpp
@@ +1624,4 @@
>          return false;
>        }
>  
> +      // $profileDir/gmp/$platform/$pluginName/id/

Isn't it  $profileDir/gmp/$platform/$gmpName/id/? How is $gmpName different from $pluginName?

::: dom/media/gmp/GMPStorageParent.cpp
@@ +591,5 @@
>      return NS_ERROR_FAILURE;
>    }
>    if (persistent) {
> +    UniquePtr<GMPDiskStorage> storage =
> +      MakeUnique<GMPDiskStorage>(mNodeId, NS_LITERAL_STRING("gmp-") + mPlugin->GetPluginBaseName());

Can GetPluginBaseName() just return a name without "gmp-" prefix stripped?

Comment 8

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

Comment on attachment 8692301 [details] [diff] [review]
Patch 5: Migrate existing GMP storage to new per-GMP location

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

::: dom/media/gmp/GMPServiceParent.cpp
@@ +328,4 @@
>    return GeckoMediaPluginService::Init();
>  }
>  
> +NS_IMETHODIMP 

space.

Comment 9

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

(In reply to JW Wang [:jwwang] from comment #7)
> Comment on attachment 8692300 [details] [diff] [review]
> Patch 4: Store each GMP's storage and nodeId salt in separate directories
> 
> Review of attachment 8692300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/gmp/GMPServiceParent.cpp
> @@ +1624,4 @@
> >          return false;
> >        }
> >  
> > +      // $profileDir/gmp/$platform/$pluginName/id/
> 
> Isn't it  $profileDir/gmp/$platform/$gmpName/id/? How is $gmpName different
> from $pluginName?

For consistency with the GetNodeId() parameter I added earlier, yeah. I'll change it.

 
> ::: dom/media/gmp/GMPStorageParent.cpp
> @@ +591,5 @@
> >      return NS_ERROR_FAILURE;
> >    }
> >    if (persistent) {
> > +    UniquePtr<GMPDiskStorage> storage =
> > +      MakeUnique<GMPDiskStorage>(mNodeId, NS_LITERAL_STRING("gmp-") + mPlugin->GetPluginBaseName());
> 
> Can GetPluginBaseName() just return a name without "gmp-" prefix stripped?

Yes, if GetPluginBaseName() returns a new string object rather than a const reference.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11ede1cca006
https://hg.mozilla.org/integration/mozilla-inbound/rev/eabac8a5315b
https://hg.mozilla.org/integration/mozilla-inbound/rev/584678105c14
https://hg.mozilla.org/integration/mozilla-inbound/rev/37e82b51a404
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf6fd3421e6

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11ede1cca006
https://hg.mozilla.org/mozilla-central/rev/eabac8a5315b
https://hg.mozilla.org/mozilla-central/rev/584678105c14
https://hg.mozilla.org/mozilla-central/rev/37e82b51a404
https://hg.mozilla.org/mozilla-central/rev/1bf6fd3421e6