1295934 - Add a crash report annotation when we hit DoneStartingUp

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

We don't have a good annotation for "startup crash" currently. We do have a piece of code that defines when we're "done starting up", however:
https://dxr.mozilla.org/mozilla-central/rev/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/toolkit/xre/nsAppRunner.cpp#4255

We should add a crash report annotation there to use for distinguishing startup crashes from other crashes.

Comment 1

[Nicholas Nethercote [inactive]]

It would be easy to add an annotation (e.g. "DoneStartingUp") to the crash report once we hit this point. The harder part is dealing with it on the Socorro side.

Ideally we'd want a "this is a startup crash" indicator rather than a "this is not a startup crash indicator". I guess it wouldn't be hard to turn !DoneStartingUp into StartupCrash during crash processing.

AFAICT, Socorro currently decides if a crash signature is a startup crash if the uptime is 0 in more than 50% of the crash reports: https://github.com/mozilla/socorro/blob/fb3c8dcb1f2a66cfc2a9ee08b07038c2f5c28c21/webapp-django/crashstats/topcrashers/views.py#L109-L113.  (peterbe, is that right?)

That does feel quite unscientific, and using DoneStartingUp instead seems like a clear improvement. If we change the meaning of "startup crash" in crash-stats we should give people plenty of warning.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Add a crash report annotation when we hit DoneStartingUp

Here's an attempt at it. I have a couple of things I'm unsure about marked with
"njn" comments.

I'm also not sure if it works. I tried triggering some deliberate crashes, well
after start-up, such as this one:
https://crash-stats.mozilla.com/report/index/b6732e8e-e7f3-4150-8a10-e71232160822
but there is no DoneStartingUp field visible in the metadata tab or the raw
dump field. Am I doing something wrong?

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8783412 [details] [diff] [review]
Add a crash report annotation when we hit DoneStartingUp

Review of attachment 8783412 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the patch I would have written.

I don't know why it doesn't show up in the Socorro UI, but if you load the raw JSON you can see it's present: https://crash-stats.mozilla.com/rawdumps/b6732e8e-e7f3-4150-8a10-e71232160822.json

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +177,5 @@
>  static xpstring *defaultMemoryReportPath = nullptr;
>  
>  // A whitelist of crash annotations which do not contain sensitive data
>  // and are saved in the crash record and sent with Firefox Health Report.
> +// njn: need to add DoneStartingUp here, commented out or not?

Adding DoneStartingUp here seems fine.

@@ +331,5 @@
>  #endif // MOZ_CRASHREPORTER_INJECTOR
>  
>  // Crashreporter annotations that we don't send along in subprocess
>  // reports
> +// njn: need DoneStartingUp here?

This is an interesting point--we probably want it listed here so we aren't sending along the chrome process "DoneStartingUp" for content process crashes, but do we want to define a similar startup point for content processes?

Comment 4

[(not currently active) Ted Mielczarek]

Oh, and I just looked and now it's present in the "Raw Metadata", so maybe we either overlooked it or the Socorro UI has some glitch?

Comment 5

[Nicholas Nethercote [inactive]]

I now see the field in the Metadata tab. I swear it wasn't there before. Hmm.

Comment 6

[(not currently active) Ted Mielczarek]

A thought while at the gym--should we annotate this to "0" immediately when we initialize the crash reporter, so that we always have a value for it?

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Add a "StartupCrash" crash report annotation

New version. Things of note:

- I took your suggestion of adding an annotation as soon as the crash reporter
  starts up. I'm not sure if I chose the right place for this.
  SetExceptionHandler() is another candidate.

- I renamed the field "StartupCrash", and flipped its meaning. That way we
  won't have to do any processing on the Socorro side, which is nice.

- I think *not* doing this in content processes is the right thing. At least,
  my personal interpretation of "start-up crash" is that it's the chrome
  process.

- I discovered why I sometimes couldn't see the "DoneStartingUp" field on
  crash-stats -- it depends on whether or not I'm logged in! When logged in I
  can see it, otherwise I can't. I did a test with "StartupCrash" and saw the
  same behaviour:
  https://crash-stats.mozilla.com/report/index/97c6a62a-3e5c-492c-badc-766132160823
  When logged in I see 35 fields in the Metadata tab; when not logged in I see
  29. The difference is these fields:
    dump_checksums
    Email
    StartupCrash
    TelemetryEnvironment
    type_tag
    uuid
  I think it might be the API_WHITELIST variable in Socorro that's the cause.

Comment 8

[Nicholas Nethercote [inactive]]

> https://crash-stats.mozilla.com/report/index/97c6a62a-3e5c-492c-badc-
> 766132160823
>   When logged in I see 35 fields in the Metadata tab; when not logged in I
> see
>   29. [...]
>   I think it might be the API_WHITELIST variable in Socorro that's the cause.

Kan-Ru explained this to me: it needs to be added to the Super Search fields, similar to bug 1264244.

Comment 9

[Peter Bengtsson [:peterbe]]

Yep, the Metadata tab loops over all keys in the raw crash, as stored in S3, and if you're not signed in to view PII, it filters by API_WHITELIST:

https://github.com/mozilla/socorro/blob/f6a02e45015b4d502d31c2fb20d34d7941071200/webapp-django/crashstats/crashstats/views.py#L1445-L1452

Just file away on the new fields. Perhaps even better would be if you file AND implement them yourselves. Perhaps we should make you a superuser :) 
The reason why the socorro team has been filling in the new SuperSearch fields was for deliberate inertia (i.e. a second pair of eyes).


* Email should obviously never be added. 
* Can TelemetryEnvironment ever contain PII?
* Why isn't uuid already part of the report index page? 

In the SuperSearch fields admin page, there's a special report (https://crash-stats.mozilla.com/admin/supersearch-fields/missing/) that compares ALL available fields noticed in the last 3 weeks that don't exist as a SuperSearch field. The list is huge and mostly crazy Unicode encoding problems. In it, I see there's `raw_crash.StartupCrash`. (and there's `raw_crash.StartupTimePluginVersion` too by the way)

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8783778 [details] [diff] [review]
Add a "StartupCrash" crash report annotation

Review of attachment 8783778 [details] [diff] [review]:
-----------------------------------------------------------------

This makes more sense this way, thanks!

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8783778 [details] [diff] [review]
Add a "StartupCrash" crash report annotation

bsmedberg, does this change seem ok to you? The idea is to have a crash annotation that clearly indicates if we are starting up or not. It is only used in the chrome process. The idea is that once this patch lands we would modify Socorro to use it to identify startup crashes, rather than the existing "do more than half the crashes with this signature have an uptime of 0?" heuristic.

(As well as having a clearer meaning, this also means we can tell if an individual crash is a startup crash, whereas the current heuristic only applies to groups of crashes.)

Comment 12

[Benjamin Smedberg]

Comment on attachment 8783778 [details] [diff] [review]
Add a "StartupCrash" crash report annotation

I believe that annotations from the main process also end up in subprocess crash reports, so this is likely to show up (but be meaningless) in content and plugin crash reports.

Comment 13

[Nicholas Nethercote [inactive]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #12)
> 
> I believe that annotations from the main process also end up in subprocess
> crash reports, so this is likely to show up (but be meaningless) in content
> and plugin crash reports.

I added some debugging printf statements and AFAICT those annotation points (within XRE_mainInit() and XRE_mainRun()) aren't hit in content processes. Even if that's wrong, adding "StartupCrash" to kSubprocessBlacklist[] should prevent them being submitted. At least, that's my understanding!

Comment 14

[(not currently active) Ted Mielczarek]

You can easily test this by submitting a content process crash using https://addons.mozilla.org/en-US/firefox/addon/crash-me-now-simple/ (crash content process button is available in the customize dialog)

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> You can easily test this by submitting a content process crash using
> https://addons.mozilla.org/en-US/firefox/addon/crash-me-now-simple/ (crash
> content process button is available in the customize dialog)

Thank you for the tip. Very useful!

I did some experimenting. My hypothesis about this annotation not being attached to content process crash reports was wrong. When I removed "StartupCrash" from kSubprocessBlacklist[] I got a content crash with "StartupCrash" in it:

  https://crash-stats.mozilla.com/report/index/9c485aad-96aa-47cd-b480-b93f02160825

Then I put "StartupCrash" back into kSubprocessBlacklist[] and now I'm not getting it for content crashes, which is good:

  https://crash-stats.mozilla.com/report/index/66099a52-a336-4779-a722-d9c752160825

I am getting it for chrome crashes though, as intended:

  https://crash-stats.mozilla.com/report/index/a526b2ca-4c9b-4db6-8bd4-3696c2160825

I'm a little confused about kSubprocessBlacklist[] -- it appears to work for StartupCrash, FramePoisonBase, FramePoisonSize. But it doesn't work for URL and StartupTime -- I get them in content crashes. I don't know why.

Anyway, the patch as reviewed appears to work in the desired way.

Comment 16

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/813bce8554d1861608a89825aacce59d865c8923
Bug 1295934 - Add a "StartupCrash" crash report annotation. r=ted.

Comment 17

[Nicholas Nethercote [inactive]]

I filed bug 1297966 for fully hooking up this new annotation in Socorro.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/813bce8554d1