Closed Bug 581335 Opened 10 years ago Closed 10 years ago

Hook up crash reporting for content processes


(Core :: IPC, defect)

Not set



Tracking Status
fennec 2.0b3+ ---


(Reporter: ted, Assigned: mfinkle)



(Whiteboard: [strings])


(2 files, 3 obsolete files)

We'll need to do a little bit of work to hook up crash reporting for content processes. We should be able to reuse all of the Breakpad work that was done for OOPP, we just need the glue code in place to make content processes use it all. (This might involve some refactoring to share the code between the two child process types.)
Depends on: 592768
tracking-fennec: --- → ?
Whiteboard: [strings]
we really want this for Fennec.
tracking-fennec: ? → 2.0+
tracking-fennec: 2.0+ → 2.0b3+
Assignee: nobody → benjamin
Blocks: 605832
No longer blocks: 605832
Blocks: 605832
Attached patch Patch, WIP (obsolete) — Splinter Review
mfinkle, this should be enough for you or somebody to hook up the UI bits. I'm still trying to get the test to pass because in a Firefox tree the private-browsing service is causing a crash in the content process.
Attached patch mobile-browser changes, WIP (obsolete) — Splinter Review
Depends on: 614229
I moved the networking stuff to bug 614229. This passes tryserver with that.
Attachment #492460 - Attachment is obsolete: true
Attachment #492636 - Flags: review?(mark.finkle)
Comment on attachment 492636 [details] [diff] [review]
mozilla-central changes,  rev. 1

Tests look good.

I assume the dumpID sent via the observer notification is the one we'll need to pass the CrashSubmit.submit, so that looks nice and easy.

I don't really know how the PrivateNoteIntentionalCrash stuff will work, but I assume you do.  You should get cjones or ted to take a look at the other ContentParent changes.
Attachment #492636 - Flags: review?(mark.finkle) → review+
Attached patch mobile-browser rev 1 (obsolete) — Splinter Review
Adds most of what's needed by front-end. Need to test.
Attachment #492461 - Attachment is obsolete: true
Comment on attachment 492765 [details] [diff] [review]
mobile-browser rev 1

I was able to submit this crash report:

after I changed:
>+      CrashSubmit.submit(dumpID, Elements.stack, null, null);
this.CrashSubmit.submit(dumpID, Elements.stack, null, null);

there was an error on the console "strings is null" from CrashSubmit.jsm:180
This patch has Brad's fix for "this." and hides the "[ ] Submit crash report" UI if we have no crash report.

Brad was able to submit a report from Android. I have not been able to get a report on Linux Desktop.

The "strings is null" error is because we don't ship crashreporter.ini ion Android. We should file a followup bug for that.
Attachment #492765 - Attachment is obsolete: true
Attachment #493029 - Flags: review?(mbrubeck)
Attachment #493029 - Flags: review?(mbrubeck) → review?(21)
Comment on attachment 493029 [details] [diff] [review]
mobile-browser rev 2

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>-    this._waitingToClose = true;
>-    let reload = Services.prompt.confirmEx(window, title, message, buttons, closeText, reloadText, null, submitText, { value: true });
>+    // Only ahow the submit UI if we have a crash report we can submit
>+    if (!aSubject.hasKey("dumpID"))
>+      submitText = null;

Nit: submit UI is confusing to me, could you do a comment more understandable (from a french point of view)
Nit2: ahow == show
Attachment #493029 - Flags: review?(21) → review+

This bug should be finished now
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 614610
You need to log in before you can comment on or make changes to this bug.