617289 - PluginProvider doesn't keep IDs of plugins consistent between restarts

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The PluginProvider uses nsIUUIDGenerator to generate an IDs for plugins each time its plugin list is built (once per application session). So plugin IDs aren't consistent between application restarts. 

This causes troubles, as UI will remember an addon's ID if its closed when viewing the details pane. When the application starts next and the UI is opened, it will attempt to show the details of an addon with an ID that no longer exists. This results in gDetailsView.show() hitting a situation that has the following comment:

  // This case should never happen in normal operation

And the details pane will be stuck on its loading screen, until you manually switch to a different view.

I think this could be solved by using the plugin's path+name as the ID (and possibly hashing the string) - which should make the ID consistent between restarts.

Comment 1

[Dave Townsend [:mossop]]

Damn, thought I had corrected that, I was planning on just using md5 to generate a ID based on the path.

Don't think this blocks however. We only persist the selected category not the actual view so re-opening the manager shouldn't show the details, are you seeing this?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #1)
> Don't think this blocks however. We only persist the selected category not the
> actual view so re-opening the manager shouldn't show the details, are you
> seeing this?

Correct, I can second Blair. Reopening the Add-ons Manager with the detail view open before, opens the detail view.

Comment 3

[Dave Townsend [:mossop]]

Blocking on either stopping restoring the details from session restore or making the plugin ID's consistent (the latter is probably easiest)

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Hashing the path to the plugin's file will give us a consistent ID for the same plugin across restarts.

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 496187 [details] [diff] [review]
patch rev 1

This patch is pretty slick but I don't think there is a guarantee that plugins will be returned in the same order as previously so using the filename for the hash won't give consistent results. You can probably get away with sorting by filename before creating the hash / id.

Comment 6

[Robert Strong (they/them - no direct email)]

fyi: I didn't find a bug to add a plugin id though there is Bug 391972 for adding an optional homepage URL to the plugin metadata. Probably be a good thing to either morph bug 391972 to add all metadata in one fell swoop or file a new bug and dupe bug 391972 to it.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This corrects and tests the issue with coalesced plugins

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 496845 [details] [diff] [review]
patch rev 2

Talked with Dave about how this could be optimized and he said he would resubmit

Comment 9

[Benjamin Smedberg]

Not blocking on this, not serious enough.

Comment 10

[Dave Townsend [:mossop]]

Note, I think what we talked about was just hashing the name and description rather than all the installed file paths. This should give us a more unique ID per plugin no matter how many times it is installed on the system.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Not sure if I'll bother even trying to land this for Firefox 4 so no rush to review but this should do the job.

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 516037 [details] [diff] [review]
patch rev 3

>diff --git a/toolkit/mozapps/extensions/PluginProvider.jsm b/toolkit/mozapps/extensions/PluginProvider.jsm
>--- a/toolkit/mozapps/extensions/PluginProvider.jsm
>+++ b/toolkit/mozapps/extensions/PluginProvider.jsm
>...
>@@ -143,29 +153,46 @@ var PluginProvider = {
>   },
> 
>   buildPluginList: function PL_buildPluginList() {
>     let tags = Cc["@mozilla.org/plugin/host;1"].
>                getService(Ci.nsIPluginHost).
>                getPluginTags({});
> 
>     this.plugins = {};
>-    let seen = {};
>+    let plugins = {};
>     tags.forEach(function(aTag) {
>-      if (!(aTag.name in seen))
>-        seen[aTag.name] = {};
>-      if (!(aTag.description in seen[aTag.name])) {
>-        let id = Cc["@mozilla.org/uuid-generator;1"].
>-                 getService(Ci.nsIUUIDGenerator).
>-                 generateUUID();
>-        this.plugins[id] = {
>+      if (!(aTag.name in plugins))
>+        plugins[aTag.name] = {};
>+      if (!(aTag.description in plugins[aTag.name])) {
>+        let plugin = {
>           name: aTag.name,
>-          description: aTag.description
>+          description: aTag.description,
>+          tags: [aTag]
>         };
>-        seen[aTag.name][aTag.description] = true;
>+
>+        let hasher = Cc["@mozilla.org/security/hash;1"].
>+                     createInstance(Ci.nsICryptoHash);
>+        hasher.init(Ci.nsICryptoHash.MD5);
>+        let stringStream = Cc["@mozilla.org/io/string-input-stream;1"].
>+                           createInstance(Ci.nsIStringInputStream);
>+        stringStream.data = aTag.name + "|" + aTag.description;
The separator doesn't provide any value here afaict.

>+        hasher.updateFromStream(stringStream, -1);
>+        let id = getHashStringForCrypto(hasher);
Instead of passing the hasher it would be cleaner to move it to getHashStringForCrypto.

I'd like to take another look at this with those changes

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Addressed the comments. Using the separator was disguising something odd, on my system I have a plugin with an empty name and description (I think because I am compiling 64-bit build but have a 32-bit plugin or something) and that was causing failures so in that circumstance I just hash the string "null".

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 526322 [details] [diff] [review]
patch rev 4

>diff --git a/toolkit/mozapps/extensions/PluginProvider.jsm b/toolkit/mozapps/extensions/PluginProvider.jsm
>--- a/toolkit/mozapps/extensions/PluginProvider.jsm
>+++ b/toolkit/mozapps/extensions/PluginProvider.jsm
>...
>@@ -143,29 +161,39 @@ var PluginProvider = {
>   },
> 
>   buildPluginList: function PL_buildPluginList() {
>     let tags = Cc["@mozilla.org/plugin/host;1"].
>                getService(Ci.nsIPluginHost).
>                getPluginTags({});
> 
>     this.plugins = {};
>-    let seen = {};
>+    let plugins = {};
>     tags.forEach(function(aTag) {
>-      if (!(aTag.name in seen))
>-        seen[aTag.name] = {};
>-      if (!(aTag.description in seen[aTag.name])) {
>-        let id = Cc["@mozilla.org/uuid-generator;1"].
>-                 getService(Ci.nsIUUIDGenerator).
>-                 generateUUID();
>-        this.plugins[id] = {
>+      if (!(aTag.name in plugins))
>+        plugins[aTag.name] = {};
>+      if (!(aTag.description in plugins[aTag.name])) {
>+        let plugin = {
>           name: aTag.name,
>-          description: aTag.description
>+          description: aTag.description,
>+          tags: [aTag]
>         };
>-        seen[aTag.name][aTag.description] = true;
>+
>+        let id = getHashForString(aTag.name + aTag.description);
>+        id = "{" + id.substr(0, 8) + "-" +
>+                   id.substr(8, 4) + "-" +
>+                   id.substr(12, 4) + "-" +
>+                   id.substr(16, 4) + "-" +
>+                   id.substr(20) + "}";
Since you are redefining the value of id here would it make sense to just return the value that actually you want in getHashForString()?

r=me either way

Comment 15

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/95ece7200b56

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110501 Firefox/6.0a1