Closed Bug 526623 Opened 11 years ago Closed 11 years ago

Send channel with crash data

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
blocking2.0 --- alpha1+
status1.9.2 --- beta5-fixed

People

(Reporter: samuel.sidler+old, Assigned: ddahl)

References

Details

(Whiteboard: [crashkill-metrics])

Attachments

(2 files, 2 obsolete files)

We should send the channel a user is one with the crash data in an easy format for Socorro to read.

Doing so would allow us to easily process 100% of crash reports for users during the maintenance release beta periods we have. Otherwise, we'll have to unthrottle then re-throttle all the time.
Flags: blocking1.9.2?
Whiteboard: [crashkill-metrics]
This wants a toolkit or browser peer to fix it.
Would take fix, would not block.
blocking2.0: --- → ?
Flags: blocking1.9.2? → blocking1.9.2-
Notes for where to do this and how, from irc:

<Mossop> ddahl: Here is where we push some information to the crash reporter from the extension manager http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#2349
<ddahl> thanks
<Mossop> You'd want to do the same in some other component during startup, maybe browser glue, maybe something more toolkity
<Mossop> ddahl: And here is some example code to read the update channel: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/src/nsUpdateService.js.in#573
<ddahl> ok cool
<Mossop> Ok, gonna see if a change of scenery kicks my brain back on again
<ddahl> Mossop: where do you think would be a good place to insert this call, so it runs once on startup or delayed startup or 2 hours after startup or something like that? I wish dietrich was online:)
<Mossop> ddahl: Depends how important it is. If you need it for al crashes, even early startup ones then you need it as soon as possible which would be a potential perf hit (albeit a small one)
<Mossop> ddahl: Actually in the app updater code might be a good place since it is update related, depends what rs thinks
Assignee: nobody → ddahl
I am working on a patch that annotates the CrashReporter right after xpcom startup:

http://pastebin.mozilla.org/682971

It seems to be crashing fx after line 12.
Attached patch v 1.0 Working Trunk Patch (obsolete) — Splinter Review
Working patch, deep in XRE_Main
Attachment #411805 - Flags: review?
Attachment #411805 - Flags: review? → review?(dtownsend)
Attached patch v 1.1 Working Trunk Patch (obsolete) — Splinter Review
Forgot to #ifdef everything on MOZ_CRASHREPORTER
Attachment #411805 - Attachment is obsolete: true
Attachment #411809 - Flags: review?(dtownsend)
Attachment #411805 - Flags: review?(dtownsend)
Comment on attachment 411809 [details] [diff] [review]
v 1.1 Working Trunk Patch

>+#ifdef MOZ_CRASHREPORTER
>+      // tell the crash reporter to also send the release channel
>+      nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv);
>+      if (NS_SUCCEEDED(rv)) {
>+        nsCOMPtr<nsIPrefBranch> defaultPrefBranch;
>+        rv = prefs->GetDefaultBranch(nsnull, getter_AddRefs(defaultPrefBranch));
>+
>+        if (NS_SUCCEEDED(rv)){

Nit: space before the curly brace

>+          nsXPIDLCString sval;
>+          rv = defaultPrefBranch->GetCharPref("app.update.channel", getter_Copies(sval));
>+          if (NS_SUCCEEDED(rv)) {
>+            CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ReleaseChannel"),
>+                                               nsDependentCString(sval));

Can you not just pass sval here rather than wrapping in an nsDependentCString?

>+          }
>+        }
>+      }
>+#endif
Seems to work without the nsDependantString
Attachment #411809 - Attachment is obsolete: true
Attachment #412012 - Flags: review?(dtownsend)
Attachment #411809 - Flags: review?(dtownsend)
Comment on attachment 412012 [details] [diff] [review]
v 1.2 Working Trunk Patch

Looks good. Please keep a close eye on Ts after landing though to make sure this doesn't do any harm.
Attachment #412012 - Flags: review?(dtownsend) → review+
thanks, will do. 

Sam:

Perhaps we should get a confirmation on the server side of this before checkin? I can send crash reports when you are ready.
No, you can just land it, and the server will ignore it. That's fine. File a new bug in Webtools:Socorro to get the server to collect and display the data. If it doesn't need to be queryable (in the db), then it should be pretty easy.
Keywords: checkin-needed
Blocks: 528271
Attachment #412035 - Flags: review?(dtownsend)
Comment on attachment 412035 [details] [diff] [review]
v 1.0 Working 1.9.2 Patch

When there is basically no change in the patch like this, just different line numbers or slightly different context you generally don't need to re-request review.
Attachment #412035 - Flags: review?(dtownsend) → review+
Pushed to trunk: http://hg.mozilla.org/mozilla-central/rev/87417297cb3c
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 412035 [details] [diff] [review]
v 1.0 Working 1.9.2 Patch

a192=beltzner
Attachment #412035 - Flags: approval1.9.2+
Marking the 14 bugs that are both:
 * nominated for blocking1.9.3:?
 * fixed on the 1.9.2 branch (according to status1.9.2)
as blocking1.9.3:alpha1, so that we don't have to go through the nominations individually.  They're all fixed already (so there's no work to do), and being fixed on 1.9.2 means they probably do block 1.9.3.
blocking2.0: ? → alpha1
Blocks: 649432
Duplicate of this bug: 649419
You need to log in before you can comment on or make changes to this bug.