Closed Bug 539841 Opened 15 years ago Closed 15 years ago

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


(Core Graveyard :: Plug-ins, defect)

Not set


(Not tracked)



(Reporter: Dolske, Assigned: Dolske)




(2 obsolete files)

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.
I was thinking we'd sort this out over in bug 539552, but I'm not 100% sure.
Josh thought that this event was not exposed to content, only visible from chrome.
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.
Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Attached patch Patch v.2 (obsolete) — Splinter Review
Attachment #421980 - Attachment is obsolete: true
Attachment #422450 - Flags: review?(jst)
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.
Alternately, you could piggyback on my patch in bug 541076 and put this data on the PluginCrashed event directly.
Seems like it might be useful for other purposes to be able to get at this info directly from the DOM node, though...
Attachment #422450 - Flags: review?(jst) → review+
(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 and (other random sites seem to be more hit-and-miss, notably I've never hit this when crashing the test plugin).
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.
Attachment #422450 - Attachment is obsolete: true
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.
Closed: 15 years ago
Resolution: --- → DUPLICATE
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.