Some plugins are listed multiple times in the add-ons manager

VERIFIED FIXED in mozilla1.9alpha8

Status

()

VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Peter6, Assigned: rstrong)

Tracking

Trunk
mozilla1.9alpha8
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite -
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

11 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081007 Minefield/3.0a8pre ID:2007081007

repro:
make sure java is enabled
look in the add-ons manager under plugins

result:
7 times Java, gee, how convenient if you wish to disable Java ;-)
This issue is also present for Quicktime, in that there are 5 or 6 entries in the plugins manager.
I see the same thing. Java version is 1.6.0_02.
Blocks: 391625
Summary: there are 7 parts of java plugin in the add-ons manager → Some plugins are listed multiple times in the add-ons manager

Updated

11 years ago
No longer blocks: 391625

Comment 3

11 years ago
see Bug#339056 comment #27-, too.
known and taking
Assignee: nobody → robert.bugzilla
Created attachment 276355 [details] [diff] [review]
patch - perfect is the enemy of good

Michael, I'd like to get your thoughts on this approach. Thanks
Attachment #276355 - Flags: review?(michael.wu)
Created attachment 276356 [details] [diff] [review]
patch - perfect is the enemy of good rev2
Attachment #276355 - Attachment is obsolete: true
Attachment #276356 - Flags: review?(michael.wu)
Attachment #276355 - Flags: review?(michael.wu)
Comment on attachment 276356 [details] [diff] [review]
patch - perfect is the enemy of good rev2

I'll fix this in the final patch
>     gPluginsDS.Assert(pluginNode,
>                       rdf.GetResource(PREFIX_NS_EM + "description"),
>                       rdf.GetLiteral(desc),
s/desc/plugin.description/
Created attachment 276361 [details]
Add-ons Mgr. Plugins view

There are still a couple we might consider using a regexp to group... I think this is adequate considering the data provided by plugins.
note: this also takes care of sorting (bug 391754) and  QuickTime Plugin description formating error shown in error console (bug 391666).

Comment 10

11 years ago
Comment on attachment 276356 [details] [diff] [review]
patch - perfect is the enemy of good rev2

>+  var urlGet = new RegExp(".*<A HREF=([^>]*)>[^<]*</A>.*", "gi");
>+  var hrefReplace = new RegExp("<A HREF=[^>]*>([^<]*)</A>", "gi");
>+  var brReplace = new RegExp("<br>", "gi");
I kinda prefer the other regexp syntax.

>   for (var i = 0; i < plugins.length; i++) {
>     var plugin = plugins[i];
>-    var pluginNode = rdf.GetResource(PREFIX_ITEM_URI + plugin.filename);
>-    var desc = plugin.description.replace(/<br>/g, "<br/> ");
>-    try {
>-      div.innerHTML = desc;
>-      desc = div.textContent;
>-    } catch (e) {
>-      desc = plugin.description;
>+    var name = plugin.name;
>+    var desc = plugin.description.replace(brReplace, " ").replace(hrefReplace, "$1");
>+    var homepageURL = null;
>+    if (urlGet.test(plugin.description))
>+      homepageURL = plugin.description.replace(urlGet, "$1");
>+    if (!(name in gPlugins)) {
>+      gPlugins[name] = { name        : name,
>+                         filename    : plugin.filename,
>+                         description : desc,
>+                         homepageURL : homepageURL,
>+                         disabled    : plugin.disabled,
>+                         blocklisted : plugin.blocklisted,
>+                         plugins     : [] };
>     }
>+    gPlugins[name].plugins.push(plugin);
>+  }
>+
>+  for (var pluginName in gPlugins) {
>+    var plugin = gPlugins[pluginName];
plugin was already defined previously.

Patch looks good to me. Only possible problem I see is that the first plugin of a particular name defines whether it is blocklisted or disabled.. but I'm not sure if there's any good solutions to that other than combining plugins according to their blocklisted and enable/disabled state in addition to name.
Attachment #276356 - Flags: review?(michael.wu) → review+
Created attachment 276422 [details] [diff] [review]
patch - cleanup all html and account for quoted href's

Michael, is there any reason not to perform a greedy cleanup like this? What's the reason you prefer the regexp to be the way it was originally?
Attachment #276356 - Attachment is obsolete: true
Attachment #276422 - Flags: review?(benjamin)
Filed Bug 391972 proposing the addition of a homepage url to the plugin's metadata
Attachment #276422 - Flags: review?(benjamin) → review?(michael.wu)

Comment 13

11 years ago
(In reply to comment #11)
> Created an attachment (id=276422) [details]
> patch - cleanup all html and account for quoted href's
> 
> Michael, is there any reason not to perform a greedy cleanup like this?
Don't think so.. should be fine.

> What's the reason you prefer the regexp to be the way it was originally?
> 
It looks better to me and is a bit more efficient. The page at <http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Guide:Regular_Expressions> recommends new RegExp when runtime compilation of the regexp is necessary.. we don't need that here.
Comment on attachment 276422 [details] [diff] [review]
patch - cleanup all html and account for quoted href's

Thanks Michael!
Attachment #276422 - Flags: review?(michael.wu)

Comment 15

11 years ago
Er, review flag got cleared while I was commenting on the patch, so I'll just post my comments here.

>+  // Matches an anchor's href value which some plugins add to their description
>+  // to provide a link to the plugin's homepage in about:plugins
>+  var urlGet = new RegExp(".*<A HREF=[\"|']?([^>\"']*)[\"|']?>.*", "gi");
A few comments about this regexp:
1. A matched pair of single quotes or double quotes isn't required. (eg. href="asdf' works)
2. The href attribute must come exactly one space after the tag name. If the url is quoted, there cannot be any whitespace between the ending quote and the end of the tag.
3. Any other attributes will trip up the regexp.. what if someone uses <a href=something.com target=_blank>?

Probably not worth worrying about these edge cases until we can do proper html parsing in JS..

>+  // Case insensitive sort
>+  function compare(a, b) {
>+    if (a.name.toLowerCase() < b.name.toLowerCase()) return (-1);
>+    if (a.name.toLowerCase() > b.name.toLowerCase()) return (1);
The parenthesis for -1 and 1 look strange.
Created attachment 276439 [details] [diff] [review]
patch - address comments

Sorry about that Michael... I thought comment #13 was the review. I checked and it appears that the quotes need to be stripped for this to work with the current cmd_homepage. I'm going to do some more testing on handling additional attributes, etc. before requesting review.
Attachment #276422 - Attachment is obsolete: true
Created attachment 276440 [details] [diff] [review]
patch

I think I'm about as happy with this as I'm going to get. Michael, feel free to review this (I'm not too concerned about unmatched quotes).
Attachment #276439 - Attachment is obsolete: true
Attachment #276440 - Flags: review?(benjamin)
Created attachment 276442 [details] [diff] [review]
patch - slightly happier

sidenote: turns out that VLC also uses an anchor like QuickTime
Attachment #276440 - Attachment is obsolete: true
Attachment #276442 - Flags: review?(benjamin)
Attachment #276440 - Flags: review?(benjamin)
Comment on attachment 276442 [details] [diff] [review]
patch - slightly happier

Probably should use beginUpdateBatch and endUpdateBatch

>+  gPluginsDS.beginUpdateBatch();
>+  for (var pluginName in gPlugins) {
>+    plugin = gPlugins[pluginName];
>+    var pluginNode = rdf.GetResource(PREFIX_ITEM_URI + plugin.filename);
>     rootctr.AppendElement(pluginNode);
>     gPluginsDS.Assert(pluginNode,
>                       rdf.GetResource(PREFIX_NS_EM + "name"),
>                       rdf.GetLiteral(plugin.name),
>                       true);
>...snip
>-    gPlugins[plugin.filename] = plugin;
>   }
>+  gPluginsDS.endUpdateBatch();

Comment 20

11 years ago
(In reply to comment #19)
> (From update of attachment 276442 [details] [diff] [review])
> Probably should use beginUpdateBatch and endUpdateBatch
> 
I'll do that in bug 391625 if necessary. This code only runs once right now and on an RDF datasource that isn't attached to anything.

Is there a particular reason for using return (-1); instead of just return -1; ?
(In reply to comment #20)
>...
> Is there a particular reason for using return (-1); instead of just return -1;
> ?
That's just me being stupid... will fix.

Created attachment 276443 [details] [diff] [review]
patch - bah
Attachment #276442 - Attachment is obsolete: true
Attachment #276443 - Flags: review?
Attachment #276442 - Flags: review?(benjamin)
Attachment #276443 - Flags: review? → review?(benjamin)

Comment 23

11 years ago
> +      if (/\<A\s+HREF=[^>]*\>/i.test(plugin.description))

Just wondering: Why do you put backslashes before "<" and ">" here?

> +        homepageURL = plugin.description.replace
> +                (/.*\<A\s+HREF=[\"|']?([^>"'\s]*).*\>.*/i, "$1");

Did you really intend to put the "|" inside the character class?

The two occurrences of ".*" near the end will probably cause a lot of unnecessary backtracking. I would probably use something like this:

homepageURL = /<A\s+HREF=["']?([^>"'\s]*)/i.exec(plugin.description)[1]

This should be slightly faster and give the same result.
Comment on attachment 276443 [details] [diff] [review]
patch - bah

Thanks Elmar

I've also had a serializer pointed out to me that may be of help.
Attachment #276443 - Flags: review?(benjamin)
Created attachment 277119 [details] [diff] [review]
patch rev 8
Attachment #276443 - Attachment is obsolete: true
Attachment #277119 - Flags: review?(sspitzer)
Comment on attachment 277119 [details] [diff] [review]
patch rev 8

robert can you elaborate (for others), why you're using regex instead of the html -> text serialzer?
Attachment #277119 - Flags: review?(sspitzer) → review+
one more request for you to consider, since this in toolkit:

can move those regex's into functions, have the existing code call them, butwrite a unit tests to verify the do what you expect?
(In reply to comment #26)
> (From update of attachment 277119 [details] [diff] [review])
> robert can you elaborate (for others), why you're using regex instead of the
> html -> text serialzer?
Will do... I'm going to check the perf comparison between the two methods prior to doing so. 

(In reply to comment #27)
> one more request for you to consider, since this in toolkit:
> 
> can move those regex's into functions, have the existing code call them,
> butwrite a unit tests to verify the do what you expect?
After thinking about this a bit I would prefer not to. Basically, we would only test the regexp against known text where we already know it works and if that regressed it would be more appropriate IMO if such a test was done in the JavaScript tests and not as part of the EM.
Checked in to trunk

Checking in mozilla/toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.135; previous revision: 1.134
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M8
in-testsuite- per rob strong's comment #28, but this is something we could add to litmus.
Flags: in-testsuite-
Flags: in-litmus?
Flags: in-litmus? → in-litmus+
When there are multiple copies of the Java plugin or Flash plugin on the system (and shown in about:plugins), does this patch ensure that the version displayed is actually the version that's used?  (That could be pretty important if a user uses the version number displayed to determine if they're up-to-date on security updates.)
It simply groups them using the name provided by the plugin so if there are more than one plugin with the same name no matter the version it groups them together and enabling / disabling will enable / disable all of the plugins with the same name. A crappy solution at best but our plugin code would need to provide version info if we were to do it by version and the plugin vendors would need to update their plugins to provide the version.

http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsPluginHostImpl.h#140
But for many plugins (Acrobat, Flash), the version is part of the description but not part of the name.  So it seems pretty bad if we might show the description for the plugin that's not the one we're actually using.
Acrobat on Windows doesn't have the version in the description or name. Flash on Windows puts 9.0 r47 among the text in the description.

I guess we could key off of description but it will most likely have similar problems as with keying off of name. Until we get plugins to provide more consistent and well formed metadata associated with the plugin I suspect there will be similar issues to the one you brought up.
For example, I tell some non techie person to disable Java in the add-ons mgr... they do but there is another one listed. The current ui disables all instances which seems like the right thing to do for the non techie user. This obviously is not sufficient for the techie user that wants to disable one version and leave another enabled but the ui isn't geared for techie users.

Another option would be to have a hidden pref that is not present by default to not group them at all. This would give the techie user that would likely be comfortable with adding a hidden pref the ability to disable individual plugins though the plugin themselves would need to provide unique information in their description which many of them don't.
In some sense there are two different things going on that cause multiple plugins to show up in the display:
 * some plugins (Java, Quicktime) are actually multiple plugins
 * users sometimes have >1 version of a plugin installed (see also bug 199952)
We definitely want to merge the first case, and may not want to merge the second case, especially where the name or description differs.

Could you key off the combination of the description and the name (i.e., all the displayed information), so that at least we're not displaying potentially misleading information?
I believe so and it should be better. Can you provide an example where keying off of the name and description would provide a different result than just keying off of the description. I ask because I don't know of such a case and would like to use it to test when / if I change the current behavior.
Nevermind... there is an example in bug 199952. Thanks!
Actually, the descriptions are only unique in bug 199952 but since there could be plugins out there where the name is unique and the description is not it is probably best to key off of both the name and description. I won't be able to get to changing this until after M8
Filed bug 394978 for grouping by name and description
(Reporter)

Updated

11 years ago
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.