Fix Safe mode breakpad preferences check

The previous implementation of CacheUploadingEnabled was really
ambiguous in multiple ways:
- it is set to match the current status of the breakpad upload
(which for example is disable on background) and it is used as
a cache of the user preference.
- it sets a UserDefault to be reused in the next startup early
part but it relies on the startup sequence anyway to initialize
it.

The result is that there are multiple buggy scenarios, like
killing the app in the background (while breakpad is not running)
then crash looping.

This CL simplifies the logic by caching the user preference only,
independently to the actual status of breakpad_upload (which is
not relevant on next startup).

Bug: 948178
Tbr: rohitrao
Change-Id: Ib43dac8e9de4a6fe4c663714bb32fefcdce0a354
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1561121
Commit-Queue: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652182}

ios/chrome/app/application_delegate/metrics_mediator.mm

@@ -265,6 +265,7 @@ using metrics_mediator::kAppEnteredBackgroundDateKey;
 }
 
 - (void)setBreakpadEnabled:(BOOL)enabled withUploading:(BOOL)allowUploading {
+  breakpad_helper::SetUserEnabledUploading(enabled);
   if (enabled) {
     breakpad_helper::SetEnabled(true);
 

ios/chrome/browser/crash_report/breakpad_helper.h

@@ -21,8 +21,12 @@ void SetEnabled(bool enabled);
 // Enable/Disable uploading crash reports.
 void SetUploadingEnabled(bool enabled);
 
-// Returns true if uploading crash reports is enabled.
-bool IsUploadingEnabled();
+// Sets the user preferences related to Breakpad and cache them to be used on
+// next startup to check if safe mode must be started.
+void SetUserEnabledUploading(bool enabled);
+
+// Returns true if uploading crash reports is enabled in the settings.
+bool UserEnabledUploading();
 
 // Cleans up all stored crash reports.
 void CleanupCrashReports();

ios/chrome/browser/crash_report/breakpad_helper.mm

@@ -112,14 +112,6 @@ bool FatalMessageHandler(int severity,
   return false;
 }
 
-// Caches the uploading flag in NSUserDefaults, so that we can access the value
-// in safe mode.
-void CacheUploadingEnabled(bool uploading_enabled) {
-  NSUserDefaults* user_defaults = [NSUserDefaults standardUserDefaults];
-  [user_defaults setBool:uploading_enabled ? YES : NO
-                  forKey:kCrashReportsUploadingEnabledKey];
-}
-
 }  // namespace
 
 void Start(const std::string& channel_name) {
@@ -155,18 +147,23 @@ void SetEnabled(bool enabled) {
     [[BreakpadController sharedInstance] start:NO];
   } else {
     [[BreakpadController sharedInstance] stop];
-    CacheUploadingEnabled(false);
   }
 }
 
 void SetBreakpadUploadingEnabled(bool enabled) {
-  CacheUploadingEnabled(g_crash_reporter_enabled && enabled);
-
   if (!g_crash_reporter_enabled)
     return;
   [[BreakpadController sharedInstance] setUploadingEnabled:enabled];
 }
 
+// Caches the uploading flag in NSUserDefaults, so that we can access the value
+// in safe mode.
+void SetUserEnabledUploading(bool uploading_enabled) {
+  [[NSUserDefaults standardUserDefaults]
+      setBool:uploading_enabled ? YES : NO
+       forKey:kCrashReportsUploadingEnabledKey];
+}
+
 void SetUploadingEnabled(bool enabled) {
   if (enabled &&
       [UIApplication sharedApplication].applicationState ==
@@ -185,8 +182,7 @@ void SetUploadingEnabled(bool enabled) {
   }
 }
 
-bool IsUploadingEnabled() {
-  // Return the value cached by CacheUploadingEnabled().
+bool UserEnabledUploading() {
   return [[NSUserDefaults standardUserDefaults]
       boolForKey:kCrashReportsUploadingEnabledKey];
 }

ios/chrome/browser/crash_report/breakpad_helper_unittest.mm

@@ -19,19 +19,6 @@
 
 namespace {
 
-// Wait for the UTE crash processing. This is needed the first time
-// |breakpad_helper::SetUploadingEnabled| is called.
-void WaitMainThreadFreezeCrashProcessingIfNeeded() {
-  if (!
-      [MainThreadFreezeDetector sharedInstance].canUploadBreakpadCrashReports) {
-    EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout(
-        base::test::ios::kWaitForActionTimeout, ^bool {
-          return [MainThreadFreezeDetector sharedInstance]
-              .canUploadBreakpadCrashReports;
-        }));
-  }
-}
-
 const int kCrashReportCount = 3;
 NSString* const kUploadedInRecoveryMode = @"uploaded_in_recovery_mode";
 
@@ -102,33 +89,22 @@ TEST_F(BreakpadHelperTest, HasReportToUpload) {
 }
 
 TEST_F(BreakpadHelperTest, IsUploadingEnabled) {
-  // Test when crash reporter is disabled.
+  breakpad_helper::SetUserEnabledUploading(true);
+  EXPECT_TRUE(breakpad_helper::UserEnabledUploading());
   breakpad_helper::SetEnabled(false);
-
-  EXPECT_FALSE(breakpad_helper::IsUploadingEnabled());
-
-  breakpad_helper::SetUploadingEnabled(false);
-  WaitMainThreadFreezeCrashProcessingIfNeeded();
-  EXPECT_FALSE(breakpad_helper::IsUploadingEnabled());
-
-  breakpad_helper::SetUploadingEnabled(true);
-  EXPECT_FALSE(breakpad_helper::IsUploadingEnabled());
-
-  // Test when crash reporter is enabled.
+  EXPECT_TRUE(breakpad_helper::UserEnabledUploading());
   [[mock_breakpad_controller_ expect] start:NO];
   breakpad_helper::SetEnabled(true);
-  EXPECT_FALSE(breakpad_helper::IsUploadingEnabled());
-  EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
+  EXPECT_TRUE(breakpad_helper::UserEnabledUploading());
 
-  [[mock_breakpad_controller_ expect] setUploadingEnabled:NO];
-  breakpad_helper::SetUploadingEnabled(false);
-  EXPECT_FALSE(breakpad_helper::IsUploadingEnabled());
-  EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
-
-  [[mock_breakpad_controller_ expect] setUploadingEnabled:YES];
-  breakpad_helper::SetUploadingEnabled(true);
-  EXPECT_TRUE(breakpad_helper::IsUploadingEnabled());
-  EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
+  breakpad_helper::SetUserEnabledUploading(false);
+  EXPECT_FALSE(breakpad_helper::UserEnabledUploading());
+  [[mock_breakpad_controller_ expect] stop];
+  breakpad_helper::SetEnabled(false);
+  EXPECT_FALSE(breakpad_helper::UserEnabledUploading());
+  [[mock_breakpad_controller_ expect] start:NO];
+  breakpad_helper::SetEnabled(true);
+  EXPECT_FALSE(breakpad_helper::UserEnabledUploading());
 }
 
 TEST_F(BreakpadHelperTest, StartUploadingReportsInRecoveryMode) {

ios/chrome/browser/ui/safe_mode/safe_mode_view_controller.mm

@@ -79,7 +79,7 @@ const NSTimeInterval kUploadTotalTime = 5;
   // If uploading is enabled and more than one report has stacked up, then we
   // assume that the app may be in a state that is preventing crash report
   // uploads before crashing again.
-  return breakpad_helper::IsUploadingEnabled() &&
+  return breakpad_helper::UserEnabledUploading() &&
          breakpad_helper::GetCrashReportCount() > 1;
 }
 

ios/chrome/browser/ui/safe_mode/safe_mode_view_controller_unittest.mm

@@ -54,12 +54,13 @@ class SafeModeViewControllerTest : public PlatformTest {
 };
 
 // Verify that +[SafeModeViewController hasSuggestions] returns YES if and only
-// if crash reporter and crash report uploading are enabled and there are
-// multiple crash reports to upload.
+// if crash reporting is enabled by the user and there are multiple crash
+// reports to upload. +[SafeModeViewController hasSuggestions] does not depend
+// on the value of breakpad_helper::IsEnabled or
+// breakpad_helper::IsUploadingEnabled.
 TEST_F(SafeModeViewControllerTest, HasSuggestions) {
   // Test when crash reporter is disabled.
-  breakpad_helper::SetEnabled(false);
-  EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
+  breakpad_helper::SetUserEnabledUploading(false);
   EXPECT_FALSE([SafeModeViewController hasSuggestions]);
 
   breakpad_helper::SetUploadingEnabled(false);
@@ -85,15 +86,29 @@ TEST_F(SafeModeViewControllerTest, HasSuggestions) {
   EXPECT_FALSE([SafeModeViewController hasSuggestions]);
 
   // Test when crash reporter is enabled.
-  [[mock_breakpad_controller_ expect] start:NO];
-  breakpad_helper::SetEnabled(true);
+  breakpad_helper::SetUserEnabledUploading(true);
   EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
+  [mock_breakpad_controller_ cr_expectGetCrashReportCount:0];
   EXPECT_FALSE([SafeModeViewController hasSuggestions]);
+  EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
+
+  [mock_breakpad_controller_ cr_expectGetCrashReportCount:kCrashReportCount];
+  EXPECT_TRUE([SafeModeViewController hasSuggestions]);
+  EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
+
+  [[mock_breakpad_controller_ expect] start:NO];
+  breakpad_helper::SetEnabled(true);
 
   [[mock_breakpad_controller_ expect] setUploadingEnabled:NO];
   breakpad_helper::SetUploadingEnabled(false);
   EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
+  [mock_breakpad_controller_ cr_expectGetCrashReportCount:0];
   EXPECT_FALSE([SafeModeViewController hasSuggestions]);
+  EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
+
+  [mock_breakpad_controller_ cr_expectGetCrashReportCount:kCrashReportCount];
+  EXPECT_TRUE([SafeModeViewController hasSuggestions]);
+  EXPECT_OCMOCK_VERIFY(mock_breakpad_controller_);
 
   // Test when crash reporter and crash report uploading are enabled.
   [[mock_breakpad_controller_ expect] setUploadingEnabled:YES];