526623 - Send channel with crash data

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Samuel Sidler (old account; do not CC)]

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.

Comment 1

[(not currently active) Ted Mielczarek]

This wants a toolkit or browser peer to fix it.

Comment 2

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

Would take fix, would not block.

Comment 3

[David Dahl :ddahl]

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

Comment 4

[David Dahl :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.

Comment 5

[David Dahl :ddahl]

Attachment: v 1.0 Working Trunk Patch

Working patch, deep in XRE_Main

Comment 6

[David Dahl :ddahl]

Attachment: v 1.1 Working Trunk Patch

Forgot to #ifdef everything on MOZ_CRASHREPORTER

Comment 7

[Dave Townsend [:mossop]]

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

Comment 8

[David Dahl :ddahl]

Attachment: v 1.2 Working Trunk Patch

Seems to work without the nsDependantString

Comment 9

[Dave Townsend [:mossop]]

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.

Comment 10

[David Dahl :ddahl]

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.

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[David Dahl :ddahl]

Attachment: v 1.0 Working 1.9.2 Patch

Comment 13

[Dave Townsend [:mossop]]

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.

Comment 14

[Dave Townsend [:mossop]]

Pushed to trunk: http://hg.mozilla.org/mozilla-central/rev/87417297cb3c

Comment 15

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

Comment on attachment 412035 [details] [diff] [review]
v 1.0 Working 1.9.2 Patch

a192=beltzner

Comment 16

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a7348573b6f8

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.