Closed Bug 1228215 Opened 4 years ago Closed 4 years ago

[EME] Segregate GMP storage by GMP

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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.
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.
Attachment #8692294 - Flags: review?(jwwang)
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.
Attachment #8692296 - Flags: review?(jwwang)
The GMPParents know their name ("gmp-eme-adobe" etc), we'll need to access that when deciding where to store records, so expose it.
Attachment #8692297 - Flags: review?(jwwang)
* 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 :)
Attachment #8692300 - Flags: review?(jwwang)
Migrate existing GMP storage records to new location, overwriting if new location already exists.
Attachment #8692301 - Flags: review?(jwwang)
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?
Attachment #8692294 - Flags: review?(jwwang) → review+
Attachment #8692296 - Flags: review?(jwwang) → review+
Attachment #8692297 - Flags: review?(jwwang) → review+
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?
Attachment #8692300 - Flags: review?(jwwang) → review+
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.
Attachment #8692301 - Flags: review?(jwwang) → review+
(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.
You need to log in before you can comment on or make changes to this bug.