Bug 1357688 - Clean up and document the crash reporter environment variables; r?ted.mielczarek draft
authorGabriele Svelto <gsvelto@mozilla.com>
Mon, 26 Feb 2018 15:00:58 +0100
changeset 761218 05a762d6cb5e5fd9856743b4db38287d7c58a487
parent 759477 bfe62272d2a21f9d10d45e5aaa680f5c735604ae
push id100925
push usergsvelto@mozilla.com
push dateWed, 28 Feb 2018 21:50:28 +0000
reviewersted.mielczarek
bugs1357688
milestone60.0a1
Bug 1357688 - Clean up and document the crash reporter environment variables; r?ted.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. MozReview-Commit-ID: ATkgsI3L2Md
toolkit/crashreporter/client/crashreporter.cpp
toolkit/crashreporter/client/crashreporter_unix_common.cpp
toolkit/crashreporter/client/ping.cpp
toolkit/crashreporter/docs/index.rst
toolkit/crashreporter/nsExceptionHandler.cpp
--- a/toolkit/crashreporter/client/crashreporter.cpp
+++ b/toolkit/crashreporter/client/crashreporter.cpp
@@ -642,54 +642,46 @@ GetProgramPath(const string& exename)
   path.erase(pos);
   path.append(exename + BIN_SUFFIX);
 
   return path;
 }
 
 int main(int argc, char** argv)
 {
-  bool minidumpAllThreads = false;
-
   gArgc = argc;
   gArgv = argv;
 
   string autoSubmitEnv = UIGetEnv("MOZ_CRASHREPORTER_AUTO_SUBMIT");
-  if (!autoSubmitEnv.empty()) {
-    gAutoSubmit = true;
-  }
+  gAutoSubmit = !autoSubmitEnv.empty();
 
   if (!ReadConfig()) {
     UIError("Couldn't read configuration.");
     return 0;
   }
 
   if (!UIInit()) {
     return 0;
   }
 
-  if (argc == 3) {
-    if (!strcmp(argv[1], "--full")) {
-      minidumpAllThreads = true;
-    }
-    gReporterDumpFile = argv[2];
-  } else if (argc == 2) {
+  if (argc > 1) {
     gReporterDumpFile = argv[1];
   }
 
   if (gReporterDumpFile.empty()) {
     // no dump file specified, run the default UI
     if (!gAutoSubmit) {
       UIShowDefaultUI();
     }
   } 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()) {
       args.insert(args.begin(), "--full");
     }
     UIRunProgram(GetProgramPath(UI_MINIDUMP_ANALYZER_FILENAME),
                  args, /* wait */ true);
 
     // go ahead with the crash reporter
     gExtraFile = GetAdditionalFilename(gReporterDumpFile, kExtraDataExtension);
     if (gExtraFile.empty()) {
--- a/toolkit/crashreporter/client/crashreporter_unix_common.cpp
+++ b/toolkit/crashreporter/client/crashreporter_unix_common.cpp
@@ -22,17 +22,17 @@ struct FileData
   string path;
 };
 
 static bool CompareFDTime(const FileData& fd1, const FileData& fd2)
 {
   return fd1.timestamp > fd2.timestamp;
 }
 
-void UIPruneSavedDumps(const std::string& directory)
+void UIPruneSavedDumps(const string& directory)
 {
   DIR *dirfd = opendir(directory.c_str());
   if (!dirfd)
     return;
 
   vector<FileData> dumpfiles;
 
   while (dirent *dir = readdir(dirfd)) {
@@ -67,40 +67,37 @@ void UIPruneSavedDumps(const std::string
     // s/.dmp/.extra/
     path.replace(path.size() - 4, 4, ".extra");
     UIDeleteFile(path.c_str());
 
     dumpfiles.pop_back();
   }
 }
 
-bool UIRunProgram(const std::string& exename,
-                  const std::vector<std::string>& args,
-                  bool wait)
+bool UIRunProgram(const string& exename, const vector<string>& args, bool wait)
 {
   pid_t pid = fork();
 
   if (pid == -1) {
     return false;
   } else if (pid == 0) {
     // Child
     size_t argvLen = args.size() + 2;
-    char** argv = new char*[argvLen];
+    vector<char*> argv(argvLen);
 
     argv[0] = const_cast<char*>(exename.c_str());
 
     for (size_t i = 0; i < args.size(); i++) {
       argv[i + 1] = const_cast<char*>(args[i].c_str());
     }
 
     argv[argvLen - 1] = nullptr;
 
     // Run the program
-    int rv = execv(exename.c_str(), argv);
-    delete[] argv;
+    int rv = execv(exename.c_str(), argv.data());
 
     if (rv == -1) {
       exit(EXIT_FAILURE);
     }
   } else {
     // Parent
     if (wait) {
       waitpid(pid, nullptr, 0);
--- a/toolkit/crashreporter/client/ping.cpp
+++ b/toolkit/crashreporter/client/ping.cpp
@@ -281,23 +281,18 @@ GenerateSubmissionUrl(const string& aUrl
 static bool
 WritePing(const string& aPath, const string& aPing)
 {
   ofstream* f = UIOpenWrite(aPath.c_str());
   bool success = false;
 
   if (f->is_open()) {
     *f << aPing;
-    f->flush();
-
-    if (f->good()) {
-      success = true;
-    }
-
     f->close();
+    success = f->good();
   }
 
   delete f;
   return success;
 }
 
 // Assembles the crash ping using the strings extracted from the .extra file
 // and sends it using the crash sender. All the telemetry specific data but the
--- a/toolkit/crashreporter/docs/index.rst
+++ b/toolkit/crashreporter/docs/index.rst
@@ -210,8 +210,64 @@ are annotated with ``Hang=1``.
 about:crashes
 =============
 
 If the crash reporter subsystem is enabled, the *about:crashes*
 page will be registered with the application. This page provides
 information about previous and submitted crashes.
 
 It is also possible to submit crashes from *about:crashes*.
+
+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.
+
+User-specified environment variables
+------------------------------------
+
+- ``MOZ_CRASHREPORTER`` - The opposite of MOZ_CRASHREPORTER_DISABLE, force
+  crash reporting on even if disabled in application.ini. You must use this to
+  enable crash reporting on debug builds.
+- ``MOZ_CRASHREPORTER_DISABLE`` - Disable Breakpad crash reporting completely
+  in non-debug builds. You can use this if you would rather use the JIT
+  debugger on Windows with the symbol server, for example.
+- ``MOZ_CRASHREPORTER_FULLDUMP`` - Store full application memory in the
+  minidump, so you can open it in a Microsoft debugger. Don't submit it to the
+  server. (Windows only.)
+- ``MOZ_CRASHREPORTER_NO_DELETE_DUMP`` - Don't delete the crash report dump
+  file after submitting it to the server. Minidumps will still be moved to the
+  "Crash Reports/pending" directory.
+- ``MOZ_CRASHREPORTER_NO_REPORT`` - Save the minidump file but don't launch the
+  crash reporting UI or send the report to the server. Minidumps will be stored
+  in the user's profile directory, in a subdirectory named "minidumps".
+- ``MOZ_CRASHREPORTER_SHUTDOWN`` - Save the minidump and then force the
+  application to close. This is useful for content crashes that don't normally
+  close the chrome (main application) processes. This variable would cause the
+  application to close as well.
+- ``MOZ_CRASHREPORTER_URL`` - Sets the URL that the crash reporter will submit
+  reports to.
+
+Environment variables used internally
+-------------------------------------
+
+- ``MOZ_CRASHREPORTER_AUTO_SUBMIT`` - When set causes the crash reporter client
+  to skip the UI flow and submit the crash report directly.
+- ``MOZ_CRASHREPORTER_DATA_DIRECTORY`` - Platform dependent data directory, the
+  pending crash reports will be stored in a subdirectory of this path. This
+  overrides the default one generated by the client's code.
+- ``MOZ_CRASHREPORTER_DUMP_ALL_THREADS`` - When set to 1 stack traces for
+  all threads are generated and sent in the crash ping, when not set only the
+  trace for the crashing thread will be generated instead.
+- ``MOZ_CRASHREPORTER_EVENTS_DIRECTORY`` - Path of the directory holding the
+  crash event files.
+- ``MOZ_CRASHREPORTER_PING_DIRECTORY`` - Path of the directory holding the
+  pending crash ping files.
+- ``MOZ_CRASHREPORTER_RESTART_ARG_<n>`` - Each of these variable specifies one
+  of the arguments that had been passed to the application, the crash reporter
+  client uses them for restarting it.
+- ``MOZ_CRASHREPORTER_RESTART_XUL_APP_FILE`` - If a XUL app file was specified
+  when starting the app it has to be stored in this variable so that the crash
+  reporter client can restart the application.
+- ``MOZ_CRASHREPORTER_STRINGS_OVERRIDE`` - Overrides the path used to load the
+  .ini file holding the strings used in the crash reporter client UI.
--- a/toolkit/crashreporter/nsExceptionHandler.cpp
+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
@@ -195,17 +195,16 @@ static XP_CHAR* pendingDirectory;
 static XP_CHAR* crashReporterPath;
 static XP_CHAR* memoryReportPath;
 #ifdef XP_MACOSX
 static XP_CHAR* libraryPath; // Path where the NSS library is
 #endif // XP_MACOSX
 
 // Where crash events should go.
 static XP_CHAR* eventsDirectory;
-static char* eventsEnv = nullptr;
 
 // The current telemetry session ID to write to the event file
 static char* currentSessionId = nullptr;
 
 // If this is false, we don't launch the crash reporter
 static bool doReport = true;
 
 // if this is true, we pass the exception on to the OS crash reporter
@@ -227,17 +226,16 @@ static char* androidUserSerial = nullptr
 static Mutex* crashReporterAPILock;
 static Mutex* notesFieldLock;
 static AnnotationTable* crashReporterAPIData_Hash;
 static nsCString* crashReporterAPIData = nullptr;
 static nsCString* crashEventAPIData = nullptr;
 static nsCString* notesField = nullptr;
 static bool isGarbageCollecting;
 static uint32_t eventloopNestingLevel = 0;
-static bool minidumpAnalysisAllThreads = false;
 
 // Avoid a race during application termination.
 static Mutex* dumpSafetyLock;
 static bool isSafeToDump = false;
 
 // Whether to include heap regions of the crash context.
 static bool sIncludeContextHeap = false;
 
@@ -749,31 +747,26 @@ WriteGlobalMemoryStatus(PlatformWriter* 
  * Launches the program specified in aProgramPath with aMinidumpPath as its
  * sole argument.
  *
  * @param aProgramPath The path of the program to be launched
  * @param aMinidumpPath The path of the minidump file, passed as an argument
  *        to the launched program
  */
 static bool
-LaunchProgram(const XP_CHAR* aProgramPath, const XP_CHAR* aMinidumpPath,
-              bool aAllThreads)
+LaunchProgram(const XP_CHAR* aProgramPath, const XP_CHAR* aMinidumpPath)
 {
 #ifdef XP_WIN
   XP_CHAR cmdLine[CMDLINE_SIZE];
   XP_CHAR* p;
 
   size_t size = CMDLINE_SIZE;
   p = Concat(cmdLine, L"\"", &size);
   p = Concat(p, aProgramPath, &size);
-  p = Concat(p, L"\" ", &size);
-  if (aAllThreads) {
-    p = Concat(p, L"--full ", &size);
-  }
-  p = Concat(p, L"\"", &size);
+  p = Concat(p, L"\" \"", &size);
   p = Concat(p, aMinidumpPath, &size);
   Concat(p, L"\"", &size);
 
   PROCESS_INFORMATION pi = {};
   STARTUPINFO si = {};
   si.cb = sizeof(si);
 
   // If CreateProcess() fails don't do anything
@@ -783,30 +776,22 @@ LaunchProgram(const XP_CHAR* aProgramPat
     CloseHandle(pi.hProcess);
     CloseHandle(pi.hThread);
   }
 #elif defined(XP_MACOSX)
   // Needed to locate NSS and its dependencies
   setenv("DYLD_LIBRARY_PATH", libraryPath, /* overwrite */ 1);
 
   pid_t pid = 0;
-  char* my_argv[] = {
+  char* const my_argv[] = {
     const_cast<char*>(aProgramPath),
     const_cast<char*>(aMinidumpPath),
-    nullptr,
     nullptr
   };
 
-  char fullArg[] = "--full";
-
-  if (aAllThreads) {
-    my_argv[2] = my_argv[1];
-    my_argv[1] = fullArg;
-  }
-
   char **env = nullptr;
   char ***nsEnv = _NSGetEnviron();
   if (nsEnv) {
     env = *nsEnv;
   }
 
   int rv = posix_spawnp(&pid, my_argv[0], nullptr, nullptr, my_argv, env);
 
@@ -818,23 +803,18 @@ LaunchProgram(const XP_CHAR* aProgramPat
 
   if (pid == -1) {
     return false;
   } else if (pid == 0) {
     // need to clobber this, as libcurl might load NSS,
     // and we want it to load the system NSS.
     unsetenv("LD_LIBRARY_PATH");
 
-    if (aAllThreads) {
-      Unused << execl(aProgramPath,
-                      aProgramPath, "--full", aMinidumpPath, (char*)0);
-    } else {
-      Unused << execl(aProgramPath,
-                      aProgramPath, aMinidumpPath, (char*)0);
-    }
+    Unused << execl(aProgramPath,
+                    aProgramPath, aMinidumpPath, nullptr);
     _exit(1);
   }
 #endif // XP_MACOSX
 
   return true;
 }
 
 #else
@@ -1140,18 +1120,17 @@ MinidumpCallback(
 #endif // XP_WIN
     return returnValue;
   }
 
 #if defined(MOZ_WIDGET_ANDROID) // Android
   returnValue = LaunchCrashReporterActivity(crashReporterPath, minidumpPath,
                                             succeeded);
 #else // Windows, Mac, Linux, etc...
-  returnValue = LaunchProgram(crashReporterPath, minidumpPath,
-                              minidumpAnalysisAllThreads);
+  returnValue = LaunchProgram(crashReporterPath, minidumpPath);
 #ifdef XP_WIN
   TerminateProcess(GetCurrentProcess(), 1);
 #endif
 #endif
 
   return returnValue;
 }
 
@@ -1867,38 +1846,30 @@ SetupCrashReporterDirectory(nsIFile* aAp
   nsCOMPtr<nsIFile> directory;
   nsresult rv = aAppDataDirectory->Clone(getter_AddRefs(directory));
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = directory->AppendNative(nsDependentCString(aDirName));
   NS_ENSURE_SUCCESS(rv, rv);
 
   EnsureDirectoryExists(directory);
-
-  xpstring dirEnv(aEnvVarName);
-  dirEnv.append(XP_TEXT("="));
-
   xpstring* directoryPath = CreatePathFromFile(directory);
 
   if (!directoryPath) {
     return NS_ERROR_FAILURE;
   }
 
-  dirEnv.append(*directoryPath);
-  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];
-  strncpy(str, dirEnv.c_str(), dirEnv.size() + 1);
-  // |PR_SetEnv| requires str to leak.
-  PR_SetEnv(str);
+  setenv(aEnvVarName, directoryPath->c_str(), /* overwrite */ 1);
 #endif
 
+  delete directoryPath;
+
   if (aDirectory) {
     directory.forget(aDirectory);
   }
 
   return NS_OK;
 }
 
 // Annotate the crash report with a Unique User ID and time
@@ -2209,17 +2180,18 @@ nsresult SetGarbageCollecting(bool colle
 
 void SetEventloopNestingLevel(uint32_t level)
 {
   eventloopNestingLevel = level;
 }
 
 void SetMinidumpAnalysisAllThreads()
 {
-  minidumpAnalysisAllThreads = true;
+  char* env = strdup("MOZ_CRASHREPORTER_DUMP_ALL_THREADS=1");
+  PR_SetEnv(env);
 }
 
 nsresult AppendAppNotesToCrashReport(const nsACString& data)
 {
   if (!GetEnabled())
     return NS_ERROR_NOT_INITIALIZED;
 
   if (FindInReadable(NS_LITERAL_CSTRING("\0"), data))
@@ -2619,57 +2591,46 @@ nsresult SetSubmitReports(bool aSubmitRe
 
     obsServ->NotifyObservers(nullptr, "submit-reports-pref-changed", nullptr);
     return NS_OK;
 }
 
 static void
 SetCrashEventsDir(nsIFile* aDir)
 {
+  static const XP_CHAR eventsDirectoryEnv[] =
+    XP_TEXT("MOZ_CRASHREPORTER_EVENTS_DIRECTORY");
+
   nsCOMPtr<nsIFile> eventsDir = aDir;
 
   const char *env = PR_GetEnv("CRASHES_EVENTS_DIR");
   if (env && *env) {
     NS_NewNativeLocalFile(nsDependentCString(env),
                           false, getter_AddRefs(eventsDir));
     EnsureDirectoryExists(eventsDir);
   }
 
   if (eventsDirectory) {
     free(eventsDirectory);
   }
 
+  xpstring* path = CreatePathFromFile(eventsDir);
+  if (!path) {
+    return; // There's no clean failure from this
+  }
+
 #ifdef XP_WIN
-  nsString path;
-  eventsDir->GetPath(path);
-  eventsDirectory = reinterpret_cast<wchar_t*>(ToNewUnicode(path));
-
-  // Save the path in the environment for the crash reporter application.
-  nsAutoString eventsDirEnv(NS_LITERAL_STRING("MOZ_CRASHREPORTER_EVENTS_DIRECTORY="));
-  eventsDirEnv.Append(path);
-  _wputenv(eventsDirEnv.get());
+  eventsDirectory = wcsdup(path->c_str());
+  SetEnvironmentVariableW(eventsDirectoryEnv, path->c_str());
 #else
-  nsCString path;
-  eventsDir->GetNativePath(path);
-  eventsDirectory = ToNewCString(path);
-
-  // Save the path in the environment for the crash reporter application.
-  nsAutoCString eventsDirEnv("MOZ_CRASHREPORTER_EVENTS_DIRECTORY=");
-  eventsDirEnv.Append(path);
-
-  // PR_SetEnv() wants the string to be available for the lifetime
-  // of the app, so dup it here.
-  char* oldEventsEnv = eventsEnv;
-  eventsEnv = ToNewCString(eventsDirEnv);
-  PR_SetEnv(eventsEnv);
-
-  if (oldEventsEnv) {
-    free(oldEventsEnv);
-  }
+  eventsDirectory = strdup(path->c_str());
+  setenv(eventsDirectoryEnv, path->c_str(), /* overwrite */ 1);
 #endif
+
+  delete path;
 }
 
 void
 SetProfileDirectory(nsIFile* aDir)
 {
   nsCOMPtr<nsIFile> dir;
   aDir->Clone(getter_AddRefs(dir));