818664 - Report plugin version in plugin crash reports

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Benjamin Smedberg]

We stubbed this out and never implemented it: http://hg.mozilla.org/mozilla-central/annotate/277998cf11cd/dom/plugins/ipc/PluginModuleParent.cpp#l170

Comment 1

[Benjamin Smedberg]

Attachment: Report the version and name, rev. 1

Comment 2

[Josh Aas]

Comment on attachment 690954 [details] [diff] [review]
Report the version and name, rev. 1

Review of attachment 690954 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +174,5 @@
> +    if (ph) {
> +        nsPluginTag* tag = ph->TagForPlugin(mPlugin);
> +        if (tag) {
> +            pluginName = tag->mName;
> +            pluginVersion = tag->mVersion;

Isn't this copying the strings? If so, why not just use them without copying?

@@ +177,5 @@
> +            pluginName = tag->mName;
> +            pluginVersion = tag->mVersion;
> +        }
> +    }
> +        

Extra whitespace here.

Comment 3

[Benjamin Smedberg]

The strings use refcounted copy-on-write, so there shouldn't be any noticeable penalty.

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/acb351f0c50a

Comment 5

[Robert Kaiser]

We should file a bug on the Socorro side to do something useful with that data. What do you want to get out of it there?

Comment 6

[Benjamin Smedberg]

Socorro already reports the plugin version on the individual report pages. Once we validate the data I think we should replace the "flash version" detection system with this system.

Comment 7

[Benjamin Smedberg]

And other than that, I don't know what we want yet.

Comment 8

[[e-mail]]

Comment on attachment 690954 [details] [diff] [review]
Report the version and name, rev. 1

>+    nsRefPtr<nsPluginHost> ph = already_AddRefed<nsPluginHost>(nsPluginHost::GetInst());
(I can't tell from the patch in bug 781310 whether you just copied this mistake [it should use dont_AddRef] from somewhere else but fortunately that bug cleans things up anyway.)