Bug 740304 - Fix platform-dependent inconsistencies in the crash reporter client r=KrisWright
authorGabriele Svelto <gsvelto@mozilla.com>
Wed, 15 Jun 2022 09:12:38 +0000
changeset 620941 a9cc52f8793b58eb71ad702887fc81d0837b86c0
parent 620940 49223a431cd47064e5d1c045ff108f55a806bf05
child 620942 d1412c88244bab30e682331b935183d1e64370e5
push id39854
push userimoraru@mozilla.com
push dateWed, 15 Jun 2022 15:46:59 +0000
treeherdermozilla-central@5093bcd97c9c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersKrisWright
bugs740304
milestone103.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 740304 - Fix platform-dependent inconsistencies in the crash reporter client r=KrisWright The crash reporter client should behave in the same way across all platforms. Crucially the first time it's opened it needs to have the "submit this report" and "include the URL" checkboxes checked by default. It should also preserve the value of those checkboxes across runs. This patch does the following to achieve this goal * Set the submit and include URL checkboxes by default if the crash reporter preferences INI file is not available or cannot be read on Linux * Remove the unused preference keys from the Windows implementation * Preserve the value of the include URL checkbox across runs on macOS The names of the options used for both preferences are inconsistent across the different platforms and I'll harmonize them in a follow-up. Differential Revision: https://phabricator.services.mozilla.com/D148959
toolkit/crashreporter/client/crashreporter_linux.cpp
toolkit/crashreporter/client/crashreporter_osx.mm
toolkit/crashreporter/client/crashreporter_win.cpp
--- a/toolkit/crashreporter/client/crashreporter_linux.cpp
+++ b/toolkit/crashreporter/client/crashreporter_linux.cpp
@@ -34,29 +34,34 @@ static void* gnomeLib = nullptr;
 static void* gnomeuiLib = nullptr;
 
 static void LoadSettings() {
   /*
    * NOTE! This code needs to stay in sync with the preference checking
    *       code in in nsExceptionHandler.cpp.
    */
 
+  bool includeURL = true;
+  bool submitReport = true;
   StringTable settings;
   if (ReadStringsFromFile(gSettingsPath + "/" + kIniFile, settings, true)) {
-    if (settings.find("IncludeURL") != settings.end() &&
-        gIncludeURLCheck != 0) {
-      gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(gIncludeURLCheck),
-                                   settings["IncludeURL"][0] != '0');
+    if (settings.find("IncludeURL") != settings.end()) {
+      includeURL = settings["IncludeURL"][0] != '0';
+    }
+    if (settings.find("SubmitReport") != settings.end()) {
+      submitReport = settings["SubmitReport"][0] != '0';
     }
-    bool enabled = true;
-    if (settings.find("SubmitReport") != settings.end())
-      enabled = settings["SubmitReport"][0] != '0';
-    gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(gSubmitReportCheck),
-                                 enabled);
   }
+
+  if (gIncludeURLCheck) {
+    gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(gIncludeURLCheck),
+                                 includeURL);
+  }
+  gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(gSubmitReportCheck),
+                               submitReport);
 }
 
 static string Escape(const string& str) {
   string ret;
   for (auto c : str) {
     if (c == '\\') {
       ret += "\\\\";
     } else if (c == '\n') {
--- a/toolkit/crashreporter/client/crashreporter_osx.mm
+++ b/toolkit/crashreporter/client/crashreporter_osx.mm
@@ -164,16 +164,25 @@ static bool RestartApplication() {
   NSUserDefaults* userDefaults = [NSUserDefaults standardUserDefaults];
   if (nil != [userDefaults objectForKey:@"submitReport"]) {
     submitChecked = [userDefaults boolForKey:@"submitReport"];
   } else {
     [userDefaults setBool:submitChecked forKey:@"submitReport"];
   }
   [mSubmitReportButton setState:(submitChecked ? NSOnState : NSOffState)];
 
+  // load default state of include URL checkbox
+  BOOL includeChecked = YES;
+  if (nil != [userDefaults objectForKey:@"IncludeURL"]) {
+    includeChecked = [userDefaults boolForKey:@"IncludeURL"];
+  } else {
+    [userDefaults setBool:includeChecked forKey:@"IncludeURL"];
+  }
+  [mIncludeURLButton setState:(includeChecked ? NSOnState : NSOffState)];
+
   [self updateSubmit];
   [self updateURL];
   [self updateEmail];
 
   [mWindow makeKeyAndOrderFront:nil];
 }
 
 - (void)showErrorUI:(const string&)message {
@@ -273,16 +282,19 @@ static bool RestartApplication() {
 
 - (IBAction)restartClicked:(id)sender {
   RestartApplication();
   [self maybeSubmitReport];
 }
 
 - (IBAction)includeURLClicked:(id)sender {
   [self updateURL];
+  NSUserDefaults* userDefaults = [NSUserDefaults standardUserDefaults];
+  [userDefaults setBool:([mIncludeURLButton state] == NSOnState) forKey:@"IncludeURL"];
+  [userDefaults synchronize];
 }
 
 - (void)textDidChange:(NSNotification*)aNotification {
   // update comment parameter
   if ([[[mCommentText textStorage] mutableString] length] > 0)
     gQueryParameters["Comments"] = [[[mCommentText textStorage] mutableString] UTF8String];
   else
     gQueryParameters.removeMember("Comments");
--- a/toolkit/crashreporter/client/crashreporter_win.cpp
+++ b/toolkit/crashreporter/client/crashreporter_win.cpp
@@ -18,19 +18,17 @@
 #include <shlwapi.h>
 #include <math.h>
 #include <set>
 #include <algorithm>
 #include "resource.h"
 #include "windows/sender/crash_report_sender.h"
 #include "common/windows/string_utils-inl.h"
 
-#define CRASH_REPORTER_VALUE L"Enabled"
 #define SUBMIT_REPORT_VALUE L"SubmitCrashReport"
-#define SUBMIT_REPORT_OLD L"SubmitReport"
 #define INCLUDE_URL_VALUE L"IncludeURL"
 
 #define SENDURL_ORIGINAL L"https://crash-reports.mozilla.com/submit"
 #define SENDURL_XPSP2 L"https://crash-reports-xpsp2.mozilla.com/submit"
 
 #define WM_UPLOADCOMPLETE WM_APP
 
 // Thanks, Windows.h :(
@@ -117,33 +115,16 @@ static bool GetBoolValue(HKEY hRegKey, L
   if (RegQueryValueEx(hRegKey, valueName, nullptr, &type, (LPBYTE)value,
                       &dataSize) == ERROR_SUCCESS &&
       type == REG_DWORD)
     return true;
 
   return false;
 }
 
-// Removes a value from HKEY_LOCAL_MACHINE and HKEY_CURRENT_USER, if it exists.
-static void RemoveUnusedValues(const wchar_t* key, LPCTSTR valueName) {
-  HKEY hRegKey;
-
-  if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, key, 0, KEY_SET_VALUE, &hRegKey) ==
-      ERROR_SUCCESS) {
-    RegDeleteValue(hRegKey, valueName);
-    RegCloseKey(hRegKey);
-  }
-
-  if (RegOpenKeyEx(HKEY_CURRENT_USER, key, 0, KEY_SET_VALUE, &hRegKey) ==
-      ERROR_SUCCESS) {
-    RegDeleteValue(hRegKey, valueName);
-    RegCloseKey(hRegKey);
-  }
-}
-
 static bool CheckBoolKey(const wchar_t* key, const wchar_t* valueName,
                          bool* enabled) {
   /*
    * NOTE! This code needs to stay in sync with the preference checking
    *       code in in nsExceptionHandler.cpp.
    */
   *enabled = false;
   bool found = false;
@@ -172,19 +153,16 @@ static bool CheckBoolKey(const wchar_t* 
 
 static void SetBoolKey(const wchar_t* key, const wchar_t* value, bool enabled) {
   /*
    * NOTE! This code needs to stay in sync with the preference setting
    *       code in in nsExceptionHandler.cpp.
    */
   HKEY hRegKey;
 
-  // remove the old value from the registry if it exists
-  RemoveUnusedValues(key, SUBMIT_REPORT_OLD);
-
   if (RegCreateKey(HKEY_CURRENT_USER, key, &hRegKey) == ERROR_SUCCESS) {
     DWORD data = (enabled ? 1 : 0);
     RegSetValueEx(hRegKey, value, 0, REG_DWORD, (LPBYTE)&data, sizeof(data));
     RegCloseKey(hRegKey);
   }
 }
 
 static string FormatLastError() {