Closed Bug 1357688 Opened 2 years ago Closed 2 years ago

Crash pings in the crashreporter follow-up

Categories

(Toolkit :: Crash Reporting, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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

We should address the remaining issues described in bug 1345153 comment 23
What is this bug about, can you name & describe this better?
Flags: needinfo?(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.
Flags: needinfo?(gsvelto)
Component: Telemetry → Breakpad Integration
Depends on: 1416078
Blocks: 1416078
No longer depends on: 1416078
Adjusting the title to reflect the fact that this isn't about the pingsender
Summary: pingsender follow-up → Crash pings in the crashreporter follow-up
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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 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 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.
Attachment #8954294 - Flags: review?(ted) → review+
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.
I've updated the patch, I'll do another round of testing before landing it.
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2d65d9d57ad
Clean up and document the crash reporter environment variables; r=ted.mielczarek
https://hg.mozilla.org/mozilla-central/rev/d2d65d9d57ad
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.