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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Peter van der Woude [:Peter6]]

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 ;-)

Comment 1

[Steve England [:stevee]]

This issue is also present for Quicktime, in that there are 5 or 6 entries in the plugins manager.

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

I see the same thing. Java version is 1.6.0_02.

Comment 3

[pal-moz]

see Bug#339056 comment #27-, too.

Comment 4

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

known and taking

Comment 5

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

Attachment: patch - perfect is the enemy of good

Michael, I'd like to get your thoughts on this approach. Thanks

Comment 6

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

Attachment: patch - perfect is the enemy of good rev2

Comment 7

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

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/

Comment 8

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

Attachment: 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.

Comment 9

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

note: this also takes care of sorting (bug 391754) and  QuickTime Plugin description formating error shown in error console (bug 391666).

Comment 10

[Michael Wu [:mwu]]

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.

Comment 11

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

Attachment: 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?

Comment 12

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

Filed Bug 391972 proposing the addition of a homepage url to the plugin's metadata

Comment 13

[Michael Wu [:mwu]]

(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 14

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

Comment on attachment 276422 [details] [diff] [review]
patch - cleanup all html and account for quoted href's

Thanks Michael!

Comment 15

[Michael Wu [:mwu]]

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.

Comment 16

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

Attachment: 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.

Comment 17

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

Attachment: 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).

Comment 18

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

Attachment: patch - slightly happier

sidenote: turns out that VLC also uses an anchor like QuickTime

Comment 19

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

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

[Michael Wu [:mwu]]

(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; ?

Comment 21

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

(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.

Comment 22

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

Attachment: patch - bah

Comment 23

[Elmar Ludwig]

> +      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 24

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

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.

Comment 25

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

Attachment: patch rev 8

Comment 26

[(not reading, please use [e-mail] instead)]

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?

Comment 27

[(not reading, please use [e-mail] instead)]

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?

Comment 28

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

(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.

Comment 29

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

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

Comment 30

[(not reading, please use [e-mail] instead)]

in-testsuite- per rob strong's comment #28, but this is something we could add to litmus.

Comment 31

[Stephen Donner [:stephend] Not actively reading bugmail]

http://litmus.mozilla.org/show_test.cgi?id=4583

in-litmus+

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 33

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

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

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 35

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

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.

Comment 36

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

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.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 38

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

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.

Comment 39

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

Nevermind... there is an example in bug 199952. Thanks!

Comment 40

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

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

Comment 41

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

Filed bug 394978 for grouping by name and description