539841 - "Plugin Crashed" UI needs to know name of plugin that crashed

Status: RESOLVED DUPLICATE of bug 541076

(Core Graveyard :: Plug-ins, defect)

Description

[Justin Dolske [:Dolske]]

When a plugin crashes, the UI needs to know the name of the plugin that crashed so it's clear to the user what's going on.

Currently, AFIAK, this isn't exposed. At least not easily to chrome, and def. not to content. I think bug 538910 will require it to be available in both places. Exposing it on the PluginCrashed event added in bug 519541 would probably be sufficient.

Not sure if there are potential security/privacy issues with exposing this to content... I suspect not, especially if it's just when the plugin crashes, and just has the plugin's name (although that sometimes exposes a version). The name is just wanted for UI, so we'd probably want to filter such details off anyway. The only other potentially interesting situation that comes to mind is having this expose the specific plugin being used to handle a type (eg Gnash vs Flash). I'd suspect a lot of this can already be derived by content.

Comment 1

[(not currently active) Ted Mielczarek]

I was thinking we'd sort this out over in bug 539552, but I'm not 100% sure.

Comment 2

[Benjamin Smedberg]

Josh thought that this event was not exposed to content, only visible from chrome.

Comment 3

[(not currently active) Ted Mielczarek]

Testing shows it's exposed to content:
http://mavra.perilith.com/~luser/test-plugin-crash.html

Comment 4

[Justin Dolske [:Dolske]]

I guess the security details I was worried about in comment 0 are not likely to be a problem, because I noticed that these details are already exposed to content via navigator.plugins.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Seems easy enough to expose the plugin name to chrome this way. We were already doing the work as part of blocklisting, just needed to expose it. One thing I'm not entirely sure about is if this needs some more code to clear mPluginTag if, say, the object is transformed to a non-plugin object.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 422450 [details] [diff] [review]
Patch v.2

>diff --git a/content/base/src/nsObjectLoadingContent.cpp b/content/base/src/nsObjectLoadingContent.cpp

>+nsObjectLoadingContent::GetPluginInfo(nsIPluginTag **aResult)
>+{
>+  NS_ADDREF(*aResult = mPluginTag);

I think you want NS_IF_ADDREF here, to avoid crashes for non-plugins.

Comment 8

[(not currently active) Ted Mielczarek]

Alternately, you could piggyback on my patch in bug 541076 and put this data on the PluginCrashed event directly.

Comment 9

[Justin Dolske [:Dolske]]

Seems like it might be useful for other purposes to be able to get at this info directly from the DOM node, though...

Comment 10

[Justin Dolske [:Dolske]]

(In reply to comment #7)
> I think you want NS_IF_ADDREF here, to avoid crashes for non-plugins.

*sigh* Indeed, I'm crashing here (nsObjectLoadingContent::GetPluginInfo) because mPluginTag can be null after killing a mozilla-runtime. But these are indeed plugins, which means when this happens I can't get the plugin's name.

I can reproduce this almost 100% of the time on http://www.worldofwarcraft.com/cataclysm and http://www.starcraft2.com/ (other random sites seem to be more hit-and-miss, notably I've never hit this when crashing the test plugin).

Comment 11

[Benjamin Smedberg]

Yes, we need to store the plugin name (I would think on the IPDL actor) during initialization and pass it to you later. You certainly can't get any data off it once you get the crashed event.

Comment 12

[Justin Dolske [:Dolske]]

I'm going to take ted's' advice from comment 8 and just roll this into the bug 541076 patch. It's easier to add this into the event, since all the info is easily at hand there.