427743 - Expose File Version Number of Plugins

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Carsten Book [:Tomcat]]

For Debugging of crashes and for blocklist Plugins we should expose File Versions of Plugins. I will also file a bug for displaying this information in about:plugins

<timelyx>	Tomcat, bug 423176 comment 16 has the hints for getting file version info
<timelyx>	Tomcat: see if you can find someone to write code for that
>timelyx>	it should be somewhere sharable
<timelyx>	since both plugins and accessible need it

Comment 1

[Dave Townsend [:mossop]]

This is available on windows onl as I understand it

Comment 2

[Damon Sicore (:damons)]

Wouldn't block the release for something like this this late in the release cycle.

Comment 3

[Dave Townsend [:mossop]]

This will require interface changes so I'm not sure how we could add it in a point release. Question is is it wanted enough for 1.9.1 to take the interface change (nsIPluginTag)?

Could quite easily be done with an nsIPluginTag2 I guess.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This exposes the dll file version on nsIPluginTag as fileversion

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 2

bsmedberg says the interface change is ok on 1.9.1 as it is only additive.

This version adds support for reading the plugin bundle version on OSX when possible

Comment 6

[Dave Townsend [:mossop]]

I should add that other platforms can add versions where available easily, I don't think Linux has any concept of it though and I haven't a clue about any other platforms.

Comment 7

[Johnny Stenback (:jst)]

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

Looks good, but should we while we're here also go ahead an add a NP_GetPluginVersion() method for Unix plugins? Much like NP_GetMIMEDescription()?

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> (From update of attachment 327974 [details] [diff] [review])
> Looks good, but should we while we're here also go ahead an add a
> NP_GetPluginVersion() method for Unix plugins? Much like
> NP_GetMIMEDescription()?

Been looking at this, but I think in order to do that I would have to update nsIPlugin to add a method to retrieve a version, and wouldn't that break any plugins that were compiled as proper xpcom components? I'm not quite sure how many of those there are but it seems suggested that that is the recommended style now.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 3 (test)

This is a test patch that adds version lookups for OS/2 and BeOS. I have neither OS though so can't compile to test, will try to find someone who can.

Comment 10

[Peter Weilbacher]

Thanks, Dave, for digging up the OS/2 APIs for this kind of stuff. Will give your patch a try on OS/2. (I guess you will be out of luck to find someone to test BeOS as that only compiles on 1.8 branch.)

Comment 11

[Johnny Stenback (:jst)]

(In reply to comment #8)
> Been looking at this, but I think in order to do that I would have to update
> nsIPlugin to add a method to retrieve a version, and wouldn't that break any
> plugins that were compiled as proper xpcom components? I'm not quite sure how
> many of those there are but it seems suggested that that is the recommended
> style now.

nsIPlugin is going away in Mozilla 2. There's only one plugin that we know of that is an XPCOM plugin, and that's the Sun Java plugin, versions other than the newest one that ships with an NPRuntime enabled plugin. That new plugin is a pure NPAPI plugin as far as us talking to it goes (it internally still uses some XPCOM APIs, but that's unrelated to nsIPlugin).

So in other words, we should be able to add an NPP_GetPluginVersion() w/o touching nsIPlugin, as we don't care about plugin version information from plugins that are XPCOM plugins.

Comment 12

[Peter Weilbacher]

(In reply to comment #9)
> This is a test patch that adds version lookups for OS/2 and BeOS. I have
> neither OS though so can't compile to test, will try to find someone who can.

For OS/2 this has a few problems:
- it still contains |readOnlyString| instead of |version|
- NS_INFO_ProductVersion is actually saved with a length of 8 bytes
- version should be declared as a pointer of some kind, so that feeding its
  address to DosQueryResourceSize makes it a double pointer
With that it compiles, but the bit shifting to get the numbers doesn't work right. I haven't managed to find out how to correct it, though. Will let you know once I do.

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch rev 4

This covers the main 3 platforms, adding Unix by attempting to find a NP_GetPluginVersion entry in the library. I've also renamed the property from fileversion to just version which seems to make more sense now it is no longer just a dll version.

I've been trying to figure out how to come up with a test for this. I could try to add a plugin that is only built when tests are enabled that a mochitest could them query a version of, but not sure if that goes against bug 443324. Any thoughts?

Comment 14

[Johnny Stenback (:jst)]

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

- In a couple of places:

+ * NP_GetPluginVersion [optional]
+ *  - Netscape uses the return value to indicate to the user what version of
+ *    this plugin is installed.

No, Netscape doesn't use this version, but Mozilla does :) I know this just follows the same pattern in this code, but I'd deviate in this case and say either Mozilla, or "the browser".

Comment 15

[Dave Townsend [:mossop]]

Landed in changeset 0c080bbdb464

Comment 16

[Dave Townsend [:mossop]]

Bug 391728 has tests for this

Comment 17

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080909020334 Minefield/3.1b1pre.   

Also added litmus test: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7031

Comment 18

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1

Comment 19

[Daniel Veditz [:dveditz]]

It's probably not realistic to get this into a 3.0.x release, unfortunately.