1145336 - [EME] Uninstall old GMPs during updates

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Stephen A Pohl [:spohl]]

We currently leave old GMPs on disk and simply remove them from the addons manager. We should remove them from disk during an update.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: wip

This is a completely untested patch. I'm curious if this is at all the right approach (by putting this in GMPService), or if we need to tackle this differently. Chris or Anthony, can one of you answer this question, or is there someone else familiar with GMPService?

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'll let cpearce answer this.

Comment 3

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

Looks about right. :jwwang can review the GMPService changes.

Comment 4

[Stephen A Pohl [:spohl]]

Attachment: wip

:jwwang, would you be able to give me feedback on this approach? Do the changes to GMPService seem acceptable to you? I don't expect a full review just yet, I'm merely looking for initial feedback whether or not I should pursue this potential solution. Thank you!

Comment 5

[Stephen A Pohl [:spohl]]

FYI: I finally got around to testing this patch and it as far as I can tell, it works exactly as advertised. The old GMP is correctly removed from disk during an update.

Comment 6

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

Comment on attachment 8580473 [details] [diff] [review]
wip

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

::: dom/media/gmp/GMPService.cpp
@@ +649,3 @@
>      mService->AddOnGMPThread(mPath);
>    } else {
> +    mService->RemoveOnGMPThread(mPath, mOperation == REMOVE ? false : true);

(mPath, mOperation == REMOVE_AND_DELETE_FROM_DISK)

::: dom/media/gmp/mozIGeckoMediaPluginService.idl
@@ +100,5 @@
>    /**
> +   * Remove a directory for gecko media plugins and delete it from disk.
> +   * @note Main-thread API.
> +   */
> +  void removeAndDeletePluginDirectory(in AString directory);

I am fine with the naming. But it occurs to me there is apt-get on Linux which has a "purge" command.

Comment 7

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

Comment on attachment 8580473 [details] [diff] [review]
wip

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

::: dom/media/gmp/GMPService.cpp
@@ +923,5 @@
>    for (size_t i = 0; i < mPlugins.Length(); ++i) {
>      nsCOMPtr<nsIFile> pluginpath = mPlugins[i]->GetDirectory();
>      bool equals;
>      if (NS_SUCCEEDED(directory->Equals(pluginpath, &equals)) && equals) {
>        mPlugins[i]->CloseActive(true);

Btw, I think we also need to call AbortAsyncShutdown() to ensure the plugin is closed/shutdown immediately.

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thank you for the helpful feedback! I'm setting this back to r? to have you look over one additional change that I made to the patch (see details below).

(In reply to JW Wang [:jwwang] from comment #7)
> ::: dom/media/gmp/GMPService.cpp
> @@ +923,5 @@
> >    for (size_t i = 0; i < mPlugins.Length(); ++i) {
> >      nsCOMPtr<nsIFile> pluginpath = mPlugins[i]->GetDirectory();
> >      bool equals;
> >      if (NS_SUCCEEDED(directory->Equals(pluginpath, &equals)) && equals) {
> >        mPlugins[i]->CloseActive(true);
> 
> Btw, I think we also need to call AbortAsyncShutdown() to ensure the plugin
> is closed/shutdown immediately.

I believe you're right, but now that I'm reading this again I also believe that we need to call CloseActive(false) when we're removing the files from disk. Do my changes in this patch seem correct to you?

Thanks again for your help!

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Test changes

This simply adds a removeAndDeletePluginDirectory method to our MockGMPService in our tests to avoid JS errors saying that it isn't a function.

Comment 10

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

Comment on attachment 8580860 [details] [diff] [review]
Patch

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

::: dom/media/gmp/GMPService.cpp
@@ +930,5 @@
> +        mPlugins[i]->AbortAsyncShutdown();
> +        pluginpath->Remove(true);
> +      } else {
> +        mPlugins[i]->CloseActive(true);
> +      }

|aDeleteFromDisk == true| should do all things that |aDeleteFromDisk == false| would do in addition to |pluginpath->Remove(true)|, right?

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback as discussed on irc.

Comment 12

[Stephen A Pohl [:spohl]]

Comment on attachment 8581391 [details] [diff] [review]
Patch

Sorry, wrong patch :-/

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated for current trunk. Carrying over r+.

Comment 15

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a23240479b9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/181e52c05fe0

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a23240479b9b
https://hg.mozilla.org/mozilla-central/rev/181e52c05fe0

Comment 17

[Stephen A Pohl [:spohl]]

[Tracking Requested - why for this release]:
This is part of EME, which is slated for first release in Firefox 38.

Comment 18

[Stephen A Pohl [:spohl]]

Attachment: Combined patch for aurora

Note: this patch requires the combined aurora patch for bug 1140263 to be applied first!

Approval Request Comment
[Feature/regressing bug #]: Adobe EME
[User impact if declined]: When GMPs are updated, we would not delete the old files and leave them on disk.
[Describe test coverage new/current, TreeHerder]: We have mochitest and xpcshell regression tests. Local testing confirms that old GMPs are properly removed from disk during updates.
[Risks and why]: minimal
[String/UUID change made/needed]: The UUID for mozIGeckoMediaPluginService had to be updated due to the addition of the new |removeAndDeletePluginDirectory| method.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/595d5143f8e1