379290 - MOZ_CRASHREPORTER_AUTO_SUBMIT for crash reporter client

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect, P2)

Description

[(not currently active) Ted Mielczarek]

It would be nice to use Breakpad on the unit test boxes, but we need at least two new settings:
MOZ_AIRBAG_SILENT    : don't show any reporter UI
MOZ_AIRBAG_NO_REPORT : don't send the report to a server

Names are open to discussion.

Comment 1

[Benjamin Smedberg]

SILENT means "send automatically"?

I think we should settle on generic names for these: how does MOZ_CRASHREPORTER_* sound?

Comment 2

[(not currently active) Ted Mielczarek]

That was my intent, so maybe we should call it _AUTO or something?  And yeah, we should stop using airbag.  :)  So how about

MOZ_CRASHREPORTER_SILENT (or _AUTO),
MOZ_CRASHREPORTER_NO_REPORT

Comment 3

[Rob Campbell [:rc] (:robcee)]

Can we include an option for local processing?

MOZ_CRASHREPORTER_LOCAL or similar? Or is that the intention of _NO_REPORT?

Comment 4

[Benjamin Smedberg]

MOZ_CRASHREPORTER_AUTO sounds more descriptive to me.

The effect of MOZ_CRASHREPORTER_NO_REPORT is to create the minidump but not launch the reporter process, correct? Or does it launch the reporter process but it quits immediately?

Local processing is hard and should be punted to another bug... it involves another binary, plus settings for the local symbol store.

Comment 5

[(not currently active) Ted Mielczarek]

I intended to do all the env variable handling in the crash reporter app, but we could probably handle _NO_REPORT in the main app instead, and it'd be easier.

Now that I think about this, for robcee's purposes he only really needs _NO_REPORT, but I'd like to do _AUTO as well because I think it will be useful for other things, like the perftest machines.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: add MOZ_CRASHREPORTER_NO_REPORT

If MOZ_CRASHREPORTER_NO_REPORT is set to a non-zero value, then we won't launch the crash reporter client.  The minidump and extra data will still be written to $profile/minidumps.

Comment 7

[Benjamin Smedberg]

Comment on attachment 263594 [details] [diff] [review]
add MOZ_CRASHREPORTER_NO_REPORT

The convention with other two-state envvars we read is that empty means off, and any value means on. So

if (noReportEnv && *noReportEnv) {
  doReport = false;
}

Are you sure all of our compilers support bool/false? Using the moz-standard PRBool/PR_FALSE would be safer.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: with bsmedberg's comment, + rename MOZ_AIRBAG to MOZ_CRASHREPORTER

We're already using |bool| and |false| in that file, since Breakpad uses them.  Since we're really only targeting tier 1 platforms with this, I think it's ok.

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: add MOZ_CRASHREPORTER_NO_REPORT [checked in]

Ok, we're not going to rename that other env var, but I did change the test there to be consistent.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 264038 [details] [diff] [review]
add MOZ_CRASHREPORTER_NO_REPORT [checked in]

Checked in.  
I'm leaving this open to add MOZ_CRASHREPORTER_AUTO.

Comment 11

[(not currently active) Ted Mielczarek]

Attachment: add MOZ_CRASHREPORTER_AUTO_SUBMIT

This doesn't work on OSX yet.  Setting MOZ_CRASHREPORTER_AUTO_SUBMIT causes the crash reporter client to submit without prompting, and exit immediately after submitting.

Comment 12

[(not currently active) Ted Mielczarek]

Dave said a while back that he'd take care of AUTO_SUBMIT, which is the only thing left for this bug, afaik.  It should be very useful for the perftest machines, all of which currently have MOZ_CRASHREPORTER_DISABLE set.

Comment 13

[Adam Guthrie]

We really need to get this in soon so we can get crash data from the unit test tinderboxen.

Comment 14

[(not currently active) Ted Mielczarek]

This won't help the unit test boxes, since those don't use nightly builds.  I did file bug 387555 on them though.  This would help the perftest boxes if we fixed bug 385785.

Comment 15

[Carsten Book [:Tomcat]]

cc'ing chofmann, having this would help a lot the topsite tests

Comment 16

[Kristian Klausen]

Any update on this? This could be really useful, we have ~ 100 Debian Jessie boxes with Firefox stable, and it would be really nice if they could auto submit crash and then could report the crash-stats.mozilla.com to our backend.

Comment 17

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 379290 - Add env var to auto submit crashes

Review commit: https://reviewboard.mozilla.org/r/205950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205950/

Comment 18

[Code Review Bot [:reviewbot]]

Comment on attachment 8935109 [details]
Bug 379290 - Add env var to auto submit crashes

https://reviewboard.mozilla.org/r/205950/#review211574


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: toolkit/crashreporter/client/crashreporter_linux.cpp:401
(Diff revision 1)
>    if (gQueryParameters.find("URL") != gQueryParameters.end())
>      gURLParameter = gQueryParameters["URL"];
>  
> +  if (gAutoSubmit) {
> +    SendReport();
> +    CloseApp(0);

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

    CloseApp(0);
             ^
             nullptr

Comment 19

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8935109 [details]
Bug 379290 - Add env var to auto submit crashes

https://reviewboard.mozilla.org/r/205950/#review211576

This is looking good, I've just noted a bunch of minor nits to address. Have you already tested on all platforms? Fortunately this change will allow us to finally write integratio tests for this code so that we won't have to rely on manual testing so much for this piece of code.

::: toolkit/crashreporter/client/crashreporter.cpp:62
(Diff revision 1)
>  static const char kMemoryReportExtension[] = ".memory.json.gz";
>  
>  void UIError(const string& message)
>  {
> +  if (gAutoSubmit)
> +    return;

nit: missing braces

::: toolkit/crashreporter/client/crashreporter.cpp:654
(Diff revision 1)
>    bool minidumpAllThreads = false;
>  
>    gArgc = argc;
>    gArgv = argv;
>  
> +  char* autoSubmitEnv = getenv("MOZ_CRASHREPORTER_AUTO_SUBMIT");

Use UIGetEnv() instead, it returns a string which is empty if the variable is not defined or empty.

::: toolkit/crashreporter/client/crashreporter.cpp:656
(Diff revision 1)
>    gArgc = argc;
>    gArgv = argv;
>  
> +  char* autoSubmitEnv = getenv("MOZ_CRASHREPORTER_AUTO_SUBMIT");
> +  if (autoSubmitEnv && *autoSubmitEnv)
> +    gAutoSubmit = true;

nit: missing braces

::: toolkit/crashreporter/client/crashreporter.cpp:677
(Diff revision 1)
>      gReporterDumpFile = argv[1];
>    }
>  
>    if (gReporterDumpFile.empty()) {
>      // no dump file specified, run the default UI
> +    if (!gAutoSubmit)

nit: braces

::: toolkit/crashreporter/client/crashreporter_gtk_common.cpp:61
(Diff revision 1)
>  vector<string> gRestartArgs;
>  GThread* gSendThreadID;
>  
>  // From crashreporter_linux.cpp
>  void SaveSettings();
> -void SendReport();
> +void DisableGUIAndSendReport();

nit: Add back the SendReport() declaration since the function is still there

::: toolkit/crashreporter/client/crashreporter_gtk_common.cpp:96
(Diff revision 1)
>  }
>  
>  // Quit the app, used as a timeout callback
>  static gboolean CloseApp(gpointer data)
>  {
> +  if (!gAutoSubmit)

nit: braces

::: toolkit/crashreporter/client/crashreporter_linux.cpp:106
(Diff revision 1)
> +  LoadProxyinfo();
> +#endif
> +
> +  // spawn a thread to do the sending
> +  GError* err;
> +  gSendThreadID = g_thread_create(SendThread, nullptr, TRUE, &err);

nit: Remove the error parameter since we don't use it anyway

::: toolkit/crashreporter/client/crashreporter_osx.mm:547
(Diff revision 1)
>  -(void)sendReport
>  {
>    if (![self setupPost]) {
>      LogMessage("Crash report submission failed: could not set up POST data");
> +
> +    if (gAutoSubmit)

nit: braces

::: toolkit/crashreporter/client/crashreporter_osx.mm:640
(Diff revision 1)
>      [r release];
>    }
>  
>    SendCompleted(success, reply);
>  
> +  if (gAutoSubmit)

nit: braces

Comment 20

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Thanks for the review, I'll get those and the nullptr nit from clangbot fixed.

Regarding braces, though: I would prefer to use them on one-line if statements, but throughout the crash reporter they are not used (sometimes even with if/else), so I thought it would be more consistent to leave them out. Let me know if you disagree.

Comment 21

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

And yes, this was tested on Mac (OS X 10.11.6), Linux (Lubuntu 17.10), and Windows (10-something), with both a fake server (to verify what was sent) and the real server (to get the right response for post-submit behavior).

Comment 22

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8935109 [details]
Bug 379290 - Add env var to auto submit crashes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205950/diff/1-2/

Comment 23

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Adam Gashlin [:agashlin] from comment #20)
> Thanks for the review, I'll get those and the nullptr nit from clangbot
> fixed.

Excellent, thanks.

> Regarding braces, though: I would prefer to use them on one-line if
> statements, but throughout the crash reporter they are not used (sometimes
> even with if/else), so I thought it would be more consistent to leave them
> out. Let me know if you disagree.

A lot of the code in the crashreporter client is so old it predates our coding guidelines. As we update it and modify the code we should adapt it to follow the guidelines even if it means having some inconsistencies. There's been talks of using clang-format to re-format old code so eventually this is going to happen anyway.

Comment 24

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8935109 [details]
Bug 379290 - Add env var to auto submit crashes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205950/diff/2-3/

Comment 25

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Good point Gabriele, thanks again.

Try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bf3e8f889503d29b82ef382ea009ec17ce2760b

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70e6e9c9efd1
Add env var to auto submit crashes r=gsvelto

Comment 27

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/70e6e9c9efd1