1041327 - Record submission event for plugin crashes [notifications]

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

(From Bug 994708 comment #0)
> Once bug 983313 is finished, we will be recording plugin and content
> crashes/hangs in FHR. As a followup step we should also record the
> submission rate for those crashes.
> 
> This bug tracks only the subprocess crashes because they are submitted using
> a different mechanism than main-process crashes and hangs. Main-process
> crash submission rate is tracked in bug

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed fix

I might have over engineered this.

> -                  updateLink = Services.urlFormatter.formatURLPref("plugins.update.url");
> +                  updateLink = this._urlFormatter.formatURLPref("plugins.update.url");

Drive by fix: Removed an (unnecessary) dependency on Services

> +@BINPATH@/components/toolkit_crashservice.xpt
Somewhere in notification.xml we do:

>          if ("nsICrashReporter" in Components.interfaces)
>             Components.utils.import("resource://gre/modules/CrashSubmit.jsm", this);

("nsICrashReporter" in Components.interfaces) returns false. Package oops!

Comment 2

[[e-mail]]

Comment on attachment 8459322 [details] [diff] [review]
Patch v1.0 Proposed fix

[Wow, this is a longwinded way of saying "plugin"...]
>+      <field name="_ppt">null</field>
>+
>+      <property name="pluginProcessType" readonly="true">
>+        <getter>
>+          <![CDATA[
>+            if (!this._ppt) {
>+              let tmp = {};
>+              Components.utils.import("resource://gre/modules/CrashManager.jsm", tmp);
>+              let _ppt = tmp.CrashManager.Singleton.PROCESS_TYPE_PLUGIN;
>+            }
>+            return this._ppt;
>+          ]]>
>+        </getter>
>+      </property>
Fields are already lazily evaluated, so you can just write an expression for the type instead of bothering with a property. Or you can just import the crash manager in the submission code, since modules are only ever loaded once.

>-                  updateLink = Services.urlFormatter.formatURLPref("plugins.update.url");
>+                  updateLink = this._urlFormatter.formatURLPref("plugins.update.url");
Nice catch.

>+@BINPATH@/components/toolkit_crashservice.xpt
D'oh!

Comment 3

[Philip Chee]

Attachment: Patch v2.0 address review issues.

> Fields are already lazily evaluated, so you can just write an expression 
> for the type instead of bothering with a property. Or you can just import 
> the crash manager in the submission code, since modules are only ever 
> loaded once.
Imported CrashManager.jsm into the submission code.

Comment 4

[[e-mail]]

Comment on attachment 8463022 [details] [diff] [review]
Patch v2.0 address review issues.

>-            this.CrashSubmit.submit(pluginDumpID, { extraExtraKeyVals: keyVals });
>+
>+            var tmp = {};
>+            Components.utils.import("resource://gre/modules/CrashManager.jsm", tmp);
>+            var pluginProcessType = tmp.CrashManager.Singleton.PROCESS_TYPE_PLUGIN;
>+
>+            this.CrashSubmit.submit(pluginDumpID, { extraExtraKeyVals: keyVals,
>+                                                    processType: pluginProcessType });
Rather conveniently, this got simplified by bug 1024672.

Comment 5

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/ca595f397309

Comment 6

[Philip Chee]

Comment on attachment 8463022 [details] [diff] [review]
Patch v2.0 address review issues.

[Approval Request Comment]
Dependencies (bug #): mozilla-central Bug 994708 - Part 2 and Bug 1024672 - Part 3 : These Firefox bugs landed on mozilla-aurora (Gecko31)
User impact if declined: Crash reports might not be sent properly.
Testing completed (on m-c, etc.): This has been in Firefox since July/August.
Risk to taking this patch (and alternatives if risky): Low. Correctness fix.
String changes made by this patch: None.

Comment 7

[rsx11m]

Comment on attachment 8463022 [details] [diff] [review]
Patch v2.0 address review issues.

Clearing flags as this has reached 2.32 with the regular release train and 2.31 is now obsolete.