1357688 - Crash pings in the crashreporter follow-up

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #1345153 +++

We should address the remaining issues described in bug 1345153 comment 23

Comment 1

[Georg Fritzsche [:gfritzsche]]

What is this bug about, can you name & describe this better?

Comment 2

[Gabriele Svelto [:gsvelto]]

It's about addressing the review comments/nits we hadn't addressed in the original patch before landing due to time constraints... and you made me realize this is in the wrong component because they're all changes that need to happen in the crash management code, not in Telemtry. Sorry for the confusion.

Comment 3

[Gabriele Svelto [:gsvelto]]

Adjusting the title to reflect the fact that this isn't about the pingsender

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1357688 - Clean up and document the crash reporter environment variables; .mielczarek

This cleans up the way the crash reporter client invokes the minidump analyzer
by removing the extra command-line parameter and replacing it with an
environment variable. Since I was at it I've also cleaned up other uses of env
variables in the code and added documentation for all of them.

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

Comment 5

[Gabriele Svelto [:gsvelto]]

So the patch here includes the cleanups for bug 1345153, plus further simplifications for the functionality added in bug 1401400 and bug 379290. I've also added documentation covering all the crash reporter environment variables, this should make life easier for people looking them up in search engines or directly in the documentation.

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8954294 [details]
Bug 1357688 - Clean up and document the crash reporter environment variables; .mielczarek

https://reviewboard.mozilla.org/r/223424/#review229496


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

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


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/crashreporter/nsExceptionHandler.cpp:812
(Diff revision 1)
> -    if (aAllThreads) {
> -      Unused << execl(aProgramPath,
> -                      aProgramPath, "--full", aMinidumpPath, (char*)0);
> -    } else {
> -      Unused << execl(aProgramPath,
> +    Unused << execl(aProgramPath,
> -                      aProgramPath, aMinidumpPath, (char*)0);
> +                    aProgramPath, aMinidumpPath, (char*)0);

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

                    aProgramPath, aMinidumpPath, (char*)0);
                                                        ^
                                                        nullptr

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8954294 [details]
Bug 1357688 - Clean up and document the crash reporter environment variables; .mielczarek

https://reviewboard.mozilla.org/r/223424/#review229582

Thanks! Once this lands we can make the old MDN page redirect to these new docs: https://developer.mozilla.org/en-US/docs/Archive/Misc_top_level/Environment_variables_affecting_crash_reporting (which is good because non-web documentation on MDN is being deprecated).

::: toolkit/crashreporter/client/crashreporter.cpp:679
(Diff revision 1)
>    } else {
>      // Start by running minidump analyzer to gather stack traces.
>      string reporterDumpFile = gReporterDumpFile;
>      vector<string> args = { reporterDumpFile };
> -    if (minidumpAllThreads) {
> +    string dumpAllThreadsEnv = UIGetEnv("MOZ_CRASHREPORTER_DUMP_ALL_THREADS");
> +    if (!dumpAllThreadsEnv.empty() && (dumpAllThreadsEnv[0] = '1')) {

Seems like we ought to be consistent with how we handle environment variable values--either "set is true" or "set to 1 is true".

::: toolkit/crashreporter/client/crashreporter_unix_common.cpp:75
(Diff revision 1)
>  
>      dumpfiles.pop_back();
>    }
>  }
>  
> -bool UIRunProgram(const std::string& exename,
> +bool UIRunProgram(const string& exename, const vector<std::string>& args,

You missed a `std::` :)

::: toolkit/crashreporter/docs/index.rst:224
(Diff revision 1)
> +Environment variables affecting crash reporting
> +===============================================
> +
> +The exception handler and crash reporter client behavior can be altered by
> +setting certain environment variables, some of these variables are used for
> +testing but quite a few have only internal users.

I wonder if it'd be clearer to split these out into two lists:
* Things that users can usefully set
* Internal implementation details
*

::: toolkit/crashreporter/docs/index.rst:266
(Diff revision 1)
> +  non-debug builds, this prevents the exception handler from being installed
> +  thus disabling the crash reporter client too
> +- ``MOZ_CRASHREPORTER_FULLDUMP`` - Forces the exception handler to generate a
> +  full crash dump instead of a minidump
> +- ``MOZ_CRASHREPORTER_NO_REPORT`` - If set prevents the crash reporter client
> +  from being launched

This should probably mention that minidumps will still be written, we just won't display the crashreporter UI.

::: toolkit/crashreporter/nsExceptionHandler.cpp:1863
(Diff revision 1)
> -  delete directoryPath;
> -
>  #if defined(XP_WIN32)
> -  _wputenv(dirEnv.c_str());
> +  SetEnvironmentVariableW(aEnvVarName, directoryPath->c_str());
>  #else
> -  XP_CHAR* str = new XP_CHAR[dirEnv.size() + 1];
> +  setenv(aEnvVarName, directoryPath->c_str(), /* overwrite */ 1);

Huh, I have no idea why this code used `PR_SetEnv` in the first place.

::: toolkit/crashreporter/nsExceptionHandler.cpp:2624
(Diff revision 1)
> -  // Save the path in the environment for the crash reporter application.
> -  nsAutoString eventsDirEnv(NS_LITERAL_STRING("MOZ_CRASHREPORTER_EVENTS_DIRECTORY="));
> -  eventsDirEnv.Append(path);
> +#ifdef XP_WIN
> +  eventsDirectory = wcsdup(path->c_str());
> +  SetEnvironmentVariableW(eventsDirectoryEnv, path->c_str());
> -  _wputenv(eventsDirEnv.get());
>  #else
> -  nsCString path;
> +  eventsDirectory = strdup(path->c_str());

I think there's a bug on file for this somewhere, but at some point the C++ standard changed to explicitly say that `c_str` doesn't allocate, so all this code that goes to great lengths to call it in advance of crashing is no longer necessary and we could simply use `std::{string,wstring}` globals or whatever.

Comment 8

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8954294 [details]
Bug 1357688 - Clean up and document the crash reporter environment variables; .mielczarek

https://reviewboard.mozilla.org/r/223424/#review229920

::: toolkit/crashreporter/client/crashreporter.cpp:679
(Diff revision 1)
>    } else {
>      // Start by running minidump analyzer to gather stack traces.
>      string reporterDumpFile = gReporterDumpFile;
>      vector<string> args = { reporterDumpFile };
> -    if (minidumpAllThreads) {
> +    string dumpAllThreadsEnv = UIGetEnv("MOZ_CRASHREPORTER_DUMP_ALL_THREADS");
> +    if (!dumpAllThreadsEnv.empty() && (dumpAllThreadsEnv[0] = '1')) {

Yeah, I'll make it "set is true". The reason I wrote it this way is a leftover I forgot to remove. Since we don't have a proper way of unsetting an environment variable I thought to use "set to 1 is true" but then realized that when we set this variable we always crash right away so there's no interface that requires to unset it.

::: toolkit/crashreporter/client/crashreporter_unix_common.cpp:75
(Diff revision 1)
>  
>      dumpfiles.pop_back();
>    }
>  }
>  
> -bool UIRunProgram(const std::string& exename,
> +bool UIRunProgram(const string& exename, const vector<std::string>& args,

Good catch

::: toolkit/crashreporter/docs/index.rst:224
(Diff revision 1)
> +Environment variables affecting crash reporting
> +===============================================
> +
> +The exception handler and crash reporter client behavior can be altered by
> +setting certain environment variables, some of these variables are used for
> +testing but quite a few have only internal users.

Yeah, good point, especially because the ones that are implementation details are set on one side and read on the other so what they belong isn't really clear cut.

::: toolkit/crashreporter/nsExceptionHandler.cpp:2624
(Diff revision 1)
> -  // Save the path in the environment for the crash reporter application.
> -  nsAutoString eventsDirEnv(NS_LITERAL_STRING("MOZ_CRASHREPORTER_EVENTS_DIRECTORY="));
> -  eventsDirEnv.Append(path);
> +#ifdef XP_WIN
> +  eventsDirectory = wcsdup(path->c_str());
> +  SetEnvironmentVariableW(eventsDirectoryEnv, path->c_str());
> -  _wputenv(eventsDirEnv.get());
>  #else
> -  nsCString path;
> +  eventsDirectory = strdup(path->c_str());

I couldn't find one so it's worth filing it! It would cut out a lot of clutter from this file.

Comment 9

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8954294 [details]
Bug 1357688 - Clean up and document the crash reporter environment variables; .mielczarek

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

Comment 10

[Gabriele Svelto [:gsvelto]]

I've updated the patch, I'll do another round of testing before landing it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2d65d9d57ad
Clean up and document the crash reporter environment variables; r=ted.mielczarek

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d2d65d9d57ad