Closed Bug 1041327 Opened 6 years ago Closed 5 years ago

Record submission event for plugin crashes [notifications]

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.31 affected, seamonkey2.32 fixed)

RESOLVED FIXED
seamonkey2.32
Tracking Status
seamonkey2.31 --- affected
seamonkey2.32 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 1 obsolete file)

(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
Attached patch Patch v1.0 Proposed fix (obsolete) — Splinter Review
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!
Attachment #8459322 - Flags: review?(neil)
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!
> 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.
Attachment #8459322 - Attachment is obsolete: true
Attachment #8459322 - Flags: review?(neil)
Attachment #8463022 - Flags: review?(neil)
Attachment #8463022 - Flags: review?(neil) → review+
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.
Depends on: 1024672
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/ca595f397309
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.32
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.
Attachment #8463022 - Flags: approval-comm-aurora?
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.
Attachment #8463022 - Flags: approval-comm-aurora?
You need to log in before you can comment on or make changes to this bug.