PluginProvider doesn't keep IDs of plugins consistent between restarts

VERIFIED FIXED in mozilla6

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
5 months ago

People

(Reporter: Unfocused, Assigned: mossop)

Tracking

Trunk
mozilla6
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
blocking2.0: --- → ?
(Assignee)

Comment 1

7 years ago
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?
blocking2.0: ? → -
(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.
(Assignee)

Comment 3

7 years ago
Blocking on either stopping restoring the details from session restore or making the plugin ID's consistent (the latter is probably easiest)
blocking2.0: - → final+
(Assignee)

Updated

7 years ago
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Whiteboard: [has patch][waiting on try]
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][waiting on try] → [has patch]
(Assignee)

Comment 4

7 years ago
Created attachment 496187 [details] [diff] [review]
patch rev 1

Hashing the path to the plugin's file will give us a consistent ID for the same plugin across restarts.
Attachment #496187 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Whiteboard: [has patch] → [has patch][needs review rs]
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.
Attachment #496187 - Flags: review?(robert.bugzilla) → review-
Whiteboard: [has patch][needs review rs] → [needs new patch]
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.
(Assignee)

Updated

7 years ago
Whiteboard: [needs new patch] → [has patch][waiting on try]
(Assignee)

Comment 7

7 years ago
Created attachment 496845 [details] [diff] [review]
patch rev 2

This corrects and tests the issue with coalesced plugins
Attachment #496187 - Attachment is obsolete: true
Attachment #496845 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][waiting on try] → [has patch][needs review rs]
Whiteboard: [has patch][needs review rs] → [has new patch]
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
Attachment #496845 - Flags: review?(robert.bugzilla) → review-

Comment 9

7 years ago
Not blocking on this, not serious enough.
blocking2.0: final+ → -
(Assignee)

Updated

7 years ago
Whiteboard: [has new patch]
(Assignee)

Comment 10

7 years ago
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.
(Assignee)

Comment 11

7 years ago
Created attachment 516037 [details] [diff] [review]
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.
Attachment #496845 - Attachment is obsolete: true
Attachment #516037 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review rs]
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
Attachment #516037 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 13

7 years ago
Created attachment 526322 [details] [diff] [review]
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".
Attachment #516037 - Attachment is obsolete: true
Attachment #526322 - Flags: review?(robert.bugzilla)
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
Attachment #526322 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][needs review rs] → [has patch]
(Assignee)

Comment 15

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/95ece7200b56
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla6
(Assignee)

Updated

7 years ago
Flags: in-testsuite+
(Assignee)

Updated

7 years ago
Flags: in-litmus-
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110501 Firefox/6.0a1
Status: RESOLVED → VERIFIED

Updated

5 months ago
Depends on: 1370832
You need to log in before you can comment on or make changes to this bug.