Closed
Bug 617289
Opened 14 years ago
Closed 13 years ago
PluginProvider doesn't keep IDs of plugins consistent between restarts
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla6
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: Unfocused, Assigned: mossop)
References
Details
Attachments
(1 file, 3 obsolete files)
10.27 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 1•14 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: ? → -
Comment 2•14 years ago
|
||
(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•14 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•14 years ago
|
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Whiteboard: [has patch][waiting on try]
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch][waiting on try] → [has patch]
Assignee | ||
Comment 4•14 years ago
|
||
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•14 years ago
|
Whiteboard: [has patch] → [has patch][needs review rs]
Comment 5•14 years ago
|
||
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-
Updated•14 years ago
|
Whiteboard: [has patch][needs review rs] → [needs new patch]
Comment 6•14 years ago
|
||
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•14 years ago
|
Whiteboard: [needs new patch] → [has patch][waiting on try]
Assignee | ||
Comment 7•14 years ago
|
||
This corrects and tests the issue with coalesced plugins
Attachment #496187 -
Attachment is obsolete: true
Attachment #496845 -
Flags: review?(robert.bugzilla)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch][waiting on try] → [has patch][needs review rs]
Updated•14 years ago
|
Whiteboard: [has patch][needs review rs] → [has new patch]
Comment 8•14 years ago
|
||
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-
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has new patch]
Assignee | ||
Comment 10•13 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•13 years ago
|
||
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•13 years ago
|
Whiteboard: [has patch][needs review rs]
Comment 12•13 years ago
|
||
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•13 years ago
|
||
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 14•13 years ago
|
||
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+
Updated•13 years ago
|
Whiteboard: [has patch][needs review rs] → [has patch]
Assignee | ||
Comment 15•13 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/95ece7200b56
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla6
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite+
Assignee | ||
Updated•13 years ago
|
Flags: in-litmus-
Comment 16•13 years ago
|
||
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110501 Firefox/6.0a1
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•