MOZ_CRASHREPORTER_AUTO_SUBMIT for crash reporter client

RESOLVED FIXED in Firefox 59

Status

()

P2
normal
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: ted, Assigned: agashlin)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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

12 years ago
SILENT means "send automatically"?

I think we should settle on generic names for these: how does MOZ_CRASHREPORTER_* sound?
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
Can we include an option for local processing?

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

Comment 4

12 years ago
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.
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.
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.
Attachment #263594 - Flags: review?(benjamin)

Comment 7

12 years ago
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.
Attachment #263594 - Flags: review?(benjamin) → review-
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.
Attachment #263594 - Attachment is obsolete: true
Attachment #264020 - Flags: review?(benjamin)
Ok, we're not going to rename that other env var, but I did change the test there to be consistent.
Attachment #264020 - Attachment is obsolete: true
Attachment #264038 - Flags: review?(benjamin)
Attachment #264020 - Flags: review?(benjamin)

Updated

12 years ago
Attachment #264038 - Flags: review?(benjamin) → review+
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.
Attachment #264038 - Attachment description: without the MOZ_AIRBAG rename → add MOZ_CRASHREPORTER_NO_REPORT [checked in]
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.
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.
Assignee: ted.mielczarek → dcamp

Comment 13

12 years ago
We really need to get this in soon so we can get crash data from the unit test tinderboxen.
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.
(Reporter)

Updated

11 years ago
Assignee: dcamp → ted.mielczarek
Summary: add some environment variables to support use in automated testing → MOZ_CRASHREPORTER_AUTO_SUBMIT for crash reporter client
Blocks: 458903

Updated

10 years ago
No longer blocks: 458903
cc'ing chofmann, having this would help a lot the topsite tests
Assignee: ted.mielczarek → nobody
Blocks: 1169230

Comment 16

3 years ago
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.
Depends on: 1416078
Blocks: 1416078
No longer depends on: 1416078
Priority: -- → P2
(Assignee)

Updated

a year ago
Assignee: nobody → agashlin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
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

a year ago
mozreview-review
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
Attachment #8935109 - Flags: review?(gsvelto) → review+
(Assignee)

Comment 20

a year ago
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.
(Assignee)

Comment 21

a year ago
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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #265826 - Attachment is obsolete: true
(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 hidden (mozreview-request)
(Assignee)

Comment 25

a year ago
Good point Gabriele, thanks again.

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

Comment 26

a year ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70e6e9c9efd1
Add env var to auto submit crashes r=gsvelto
Keywords: checkin-needed

Comment 27

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70e6e9c9efd1
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.