541076 - add minidump id and plugin name to PluginCrashed event

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(not currently active) Ted Mielczarek]

The plugin crash reporting UI will need the ID of the minidump in order to submit it. This patch changes the PluginCrashed event into a "datacontainerevent" so that we can attach a crash ID field to it.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: propogate minidump id up to PluginCrashed event

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 422724 [details] [diff] [review]
propogate minidump id up to PluginCrashed event

>+  nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(mContent));
You don't need target for anything

>+  privateEvent->SetTarget(target);
You don't need this because you dispatch to target.

>+  target->DispatchEvent(event, &dummy);
You could use nsEventDispatcher::DispatchDOMEvent(mContent, nsnull, event, nsnull, nsnull);

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: propogate minidump id up to PluginCrashed event, updated with smaug's suggestions

Thanks for the hints!

Comment 4

[Justin Dolske [:Dolske]]

Can untrusted content get at the data set on a nsIDOMDataContainerEvent?

Comment 5

[Justin Dolske [:Dolske]]

Also, might it be better to call this "minidumpID" instead if "crashID"? IIRC, this is completely different from the crash report ID assigned after submitting the report, so clearly differentiating the two would help prevent confusion.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #4)
> Can untrusted content get at the data set on a nsIDOMDataContainerEvent?
Depends on how the event is consumed on chrome side. If chrome has listener
for capturing phase and calls .stopPropagation() then content doesn't see the event.

But still, might be perhaps better to dispatch the event to chrome event 
handler.
In that case, set the target using ->SetTarget(), but dispatch the event
to the chrome event handler of the window 
Or, even simpler is to just do
privateEvent->GetInternalNSEvent()->flags |= NS_EVENT_FLAG_ONLY_CHROME_DISPATCH;

Comment 7

[(not currently active) Ted Mielczarek]

Yeah, we should do that. I wasn't sure if content could get at this data, since it seemed like you had to QI to a different interface to expose it anyway, which content can't do.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 422733 [details] [diff] [review]
propogate minidump id up to PluginCrashed event, updated with smaug's suggestions

This looks good fwiw, but I'll do a proper review once there's a new patch that dispatches to chrome only.

Comment 9

[Justin Dolske [:Dolske]]

ted: mind if I steal this patch from you?

As we discussed on IRC, I actually want to send the minidump ID to the browser *once* via observerServer, and the plugin instances just need to know if the observer decided to submit a crash report or not.

Net effect is I'm changing this patch's |pluginCrashed(in AString dumpID);| to |pluginCrashed(in boolean submittedReport);|, so I might as well do that here.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

*yoink*

Comment 11

[Justin Dolske [:Dolske]]

Bugzilla's intradiff seems to be failing again, this should be ted's patch + NS_EVENT_FLAG_ONLY_CHROME_DISPATCH + add observer notification with dumpid + change domevent to use a boolean.

Comment 12

[(not currently active) Ted Mielczarek]

Feel free, of course. I got hung up trying to write a test that proved that content couldn't access the event, but maybe it's not worth it.

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Rolling what I _was_ trying to fix in bug 539841 into this patch (just like ted suggested. I should listen to him more often ;). Adds the name of the plugin which crashed to the PluginCrashed event.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 424931 [details] [diff] [review]
Patch v.4

>diff --git a/content/base/src/nsObjectLoadingContent.cpp b/content/base/src/nsObjectLoadingContent.cpp
>--- a/content/base/src/nsObjectLoadingContent.cpp
>+++ b/content/base/src/nsObjectLoadingContent.cpp
>+  // add a "crashid" property to this event

Need to change the comment here.

>+  variant = do_CreateInstance("@mozilla.org/variant;1");
>+  if (!variant) {
>+    NS_WARNING("Couldn't create crashSubmit variant for PluginCrashed event!");
>+    return NS_OK;
>+  }
>+  variant->SetAsBool(mSubmittedCrashReport);

>diff --git a/modules/plugin/test/mochitest/test_crash_notify.xul b/modules/plugin/test/mochitest/test_crash_notify.xul
>--- a/modules/plugin/test/mochitest/test_crash_notify.xul
>+++ b/modules/plugin/test/mochitest/test_crash_notify.xul
>@@ -14,35 +14,94 @@
> <embed id="plugin1" type="application/x-test" width="200" height="200"></embed>
> </body>
> <script class="testbody" type="application/javascript">
> <![CDATA[
> SimpleTest.waitForExplicitFinish();
> 
> var success = false;
> 
>+var observerFired = false;
>+
>+var testObserver = {
>+  observe: function(subject, topic, data) {
>+    observerFired = true;
>+    ok(true, "Observer fired");
>+    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+    is(topic, "plugin-crashed", "Checking correct topic");
>+    is(data,  null, "Checking null data");
>+    ok((subject instanceof Components.interfaces.nsIPropertyBag2), "got Propbag");
>+    ok((subject instanceof Components.interfaces.nsIWritablePropertyBag2),
>+"got writable Propbag");
>+    subject.setPropertyAsBool("submittedCrashReport", true);

I don't think this is necessary, you're already testing that you're getting the notification, and that the subject is correct, I don't think you need to fake the crash submission, it doesn't really test anything.

> function onPluginCrashed(aEvent) {
>   ok(true, "Plugin crashed notification received");
>+  ok(observerFired, "Observer should have fired first");
>+  is(aEvent.type, "PluginCrashed", "event is correct type");
> 
>   var pluginElement = document.getElementById("plugin1");
>   is (pluginElement, aEvent.target, "Plugin crashed event target is plugin element");
> 
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+  
>+  ok(aEvent instanceof Components.interfaces.nsIDOMDataContainerEvent,
>+     "plugin crashed event has the right interface");
>+  var pluginName = aEvent.getData("pluginName");
>+  is(pluginName, "Test Plug-in");
>+  var didReport = aEvent.getData("submittedCrashReport");
>+  // XXX Could fail if app's own observer gets notified last and set to false?
>+  ok(didReport, "event said crash report was submitted");

Further proof that you should probably just drop this part of the test. :)

Also, you should probably have the observer unregister itself from the observer service. Otherwise presumably it may get notifications from other tests that crash plugins.

Looks fine otherwise (although obviously I can't review the parts I wrote).

Comment 16

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Updated with review comments.

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch v.6

Added removeObserver() to test because it was getting triggered again in the test added by bug 541446.

Comment 18

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/c8a2d638f537

Comment 19

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100216 Minefield/3.7a2pre.  still need to doublecheck on linux builds.

Comment 20

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/58646bc93264

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 22

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430