Closed Bug 1145336 Opened 5 years ago Closed 5 years ago

[EME] Uninstall old GMPs during updates

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox37 --- disabled
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: spohl, Assigned: spohl)

References

Details

(Keywords: qawanted)

Attachments

(3 files, 5 obsolete files)

We currently leave old GMPs on disk and simply remove them from the addons manager. We should remove them from disk during an update.
Attached patch wip (obsolete) — Splinter Review
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?
Flags: needinfo?(cpearce)
Flags: needinfo?(ajones)
I'll let cpearce answer this.
Flags: needinfo?(ajones)
Looks about right. :jwwang can review the GMPService changes.
Flags: needinfo?(cpearce)
Attached patch wip (obsolete) — Splinter Review
: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!
Attachment #8580258 - Attachment is obsolete: true
Flags: needinfo?(jwwang)
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 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.
Attachment #8580473 - Flags: review+
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.
Blocks: 1145694
Attached patch Patch (obsolete) — Splinter Review
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!
Attachment #8580473 - Attachment is obsolete: true
Flags: needinfo?(jwwang)
Attachment #8580860 - Flags: review?(jwwang)
Attached patch Test changesSplinter Review
This simply adds a removeAndDeletePluginDirectory method to our MockGMPService in our tests to avoid JS errors saying that it isn't a function.
Attachment #8580862 - Flags: review?(jwwang)
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?
Attachment #8580860 - Flags: review?(jwwang) → review-
Attachment #8580862 - Flags: review?(jwwang) → review+
Attached patch Patch (obsolete) — Splinter Review
Addressed feedback as discussed on irc.
Attachment #8580860 - Attachment is obsolete: true
Attachment #8581391 - Flags: review?(jwwang)
Comment on attachment 8581391 [details] [diff] [review]
Patch

Sorry, wrong patch :-/
Attachment #8581391 - Flags: review?(jwwang)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8581391 - Attachment is obsolete: true
Attachment #8581392 - Flags: review?(jwwang)
Attachment #8581392 - Flags: review?(jwwang) → review+
Attached patch PatchSplinter Review
Updated for current trunk. Carrying over r+.
Attachment #8581392 - Attachment is obsolete: true
Attachment #8581400 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a23240479b9b
https://hg.mozilla.org/mozilla-central/rev/181e52c05fe0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
[Tracking Requested - why for this release]:
This is part of EME, which is slated for first release in Firefox 38.
Keywords: qawanted
QA Contact: spolk
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.
Attachment #8581636 - Flags: review+
Attachment #8581636 - Flags: approval-mozilla-aurora?
Attachment #8581636 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.