[ios] Refactor CrashRestoreHelper for multi-window and multi-profile

The code to deal with multi-window enabled/disabled was duplicated in
CrashRestoreHelper where calling the multi-window enabled code with a
session with an empty name was the same as calling code for single
window. Refactor the code to avoid the duplication.

Change the backup directory used for the saved session when restore
is in progress as the single directory approach would not work with
multi-profile.

Bug: 1165798
Change-Id: Ifa6efe051999b77688a8a6ffdb8b9ed3b185e60e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632677
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845174}

ios/chrome/app/main_controller.mm

@@ -485,16 +485,13 @@ void MainControllerAuthenticationServiceDelegate::ClearBrowsingData(
   // browser state.
   BOOL needRestoration = NO;
   if (isPostCrashLaunch) {
-    if (IsMultiwindowSupported()) {
-      NSSet<NSString*>* sessions =
-          [[PreviousSessionInfo sharedInstance] connectedSceneSessionsIDs];
-      needRestoration =
-          [CrashRestoreHelper moveAsideSessions:sessions
-                                forBrowserState:chromeBrowserState];
-    } else {
-      needRestoration = [CrashRestoreHelper
-          moveAsideSessionInformationForBrowserState:chromeBrowserState];
-    }
+    NSSet<NSString*>* sessions =
+        IsMultiwindowSupported()
+            ? [[PreviousSessionInfo sharedInstance] connectedSceneSessionsIDs]
+            : [NSSet setWithArray:@[ @"" ]];
+
+    needRestoration = [CrashRestoreHelper moveAsideSessions:sessions
+                                            forBrowserState:chromeBrowserState];
   }
   if (!IsMultipleScenesSupported() && IsMultiwindowSupported()) {
     NSSet<NSString*>* previousSessions =

ios/chrome/browser/crash_report/crash_restore_helper.h

@@ -16,7 +16,8 @@ class ChromeBrowserState;
 - (instancetype)initWithBrowser:(Browser*)browser;
 
 // Returns YES if a backup file for sessionID can be found on disk.
-+ (BOOL)isBackedUpSessionID:(NSString*)sessionID;
++ (BOOL)isBackedUpSessionID:(NSString*)sessionID
+               browserState:(ChromeBrowserState*)browserState;
 
 // Saves the session information stored on disk for sessions with |sessionIDs|
 // in temporary files and will then delete those from their default location.
@@ -29,17 +30,18 @@ class ChromeBrowserState;
 + (BOOL)moveAsideSessions:(NSSet<NSString*>*)sessionIDs
           forBrowserState:(ChromeBrowserState*)browserState;
 
-// Move the session information for Legacy non multiwindow supported OS.
-// This method deletes the session from its default location, while
-// allowing restoring it back later.
-// Returns |YES| if the delettion and backup was successful.
-+ (BOOL)moveAsideSessionInformationForBrowserState:
-    (ChromeBrowserState*)browserState;
-
 // Shows an infobar on the currently active tab of the browser. This infobar
 // lets the user restore its session after a crash.
 - (void)showRestorePrompt;
 
 @end
 
+@interface CrashRestoreHelper (Testing)
+
+// Returns the path for back of session |sessionID| relative in |directory|.
++ (NSString*)backupPathForSessionID:(NSString*)sessionID
+                          directory:(NSString*)directory;
+
+@end
+
 #endif  // IOS_CHROME_BROWSER_CRASH_REPORT_CRASH_RESTORE_HELPER_H_

ios/chrome/browser/crash_report/crash_restore_helper.mm

@@ -54,18 +54,10 @@
 
 // Private methods.
 @interface CrashRestoreHelper ()<InfoBarManagerObserverBridgeProtocol>
-// Deletes the session file for the given browser state, optionally backing it
-// up beforehand to |backupFile| if it is not nil.  This method returns YES in
-// case of success, NO otherwise.
-+ (BOOL)deleteSessionForBrowserState:(ChromeBrowserState*)browserState
-                          backupFile:(NSString*)file;
-
-// Returns the path where the sessions with |sessionID| for the main browser
-// state are backed up.
-+ (NSString*)backupPathForSessionID:(NSString*)sessionID;
 
 // Returns a list of IDs for all backed up sessions.
-+ (NSArray<NSString*>*)backedupSessionIDs;
++ (NSArray<NSString*>*)backedupSessionIDsForBrowserState:
+    (ChromeBrowserState*)browserState;
 
 // Restores the sessions after a crash. It should only be called if
 // |moveAsideSessions:forBrowserState| for the browser state of the current
@@ -79,11 +71,12 @@
 
 namespace {
 
+NSString* const kSessionBackupDirectory =
+    @"Backups";  // The name for directory which contains all session backup
+                 // subdirectories for multiple sessions.
+
 NSString* const kSessionBackupFileName =
-    @"session.bak";  // The session file name on disk.
-NSString* const kSessionBackupDirectoryName =
-    @"Sessions";  // The name for directory which contains all session backup
-                  // subdirectories for multiple sessions.
+    @"session.backup.plist";  // The session file name on disk.
 
 class InfoBarManagerObserverBridge : infobars::InfoBarManager::Observer {
  public:
@@ -235,13 +228,12 @@ int SessionCrashedInfoBarDelegate::GetIconId() const {
 
 @implementation CrashRestoreHelper {
   Browser* _browser;
+  // Indicate that the session has been restored to tabs or to recently closed
+  // and should not be re-restored.
+  BOOL _sessionRestored;
   std::unique_ptr<InfoBarManagerObserverBridge> _infoBarBridge;
 }
 
-// Indicate that the session has been restored to tabs or to recently closed
-// and should not be rerestored.
-static BOOL _sessionRestored = NO;
-
 - (instancetype)initWithBrowser:(Browser*)browser {
   if (self = [super init]) {
     _browser = browser;
@@ -271,17 +263,18 @@ static BOOL _sessionRestored = NO;
   BOOL partialSuccess = NO;
   NSString* stashPath =
       base::SysUTF8ToNSString(browserState->GetStatePath().value());
-  NSString* backupPath = nil;
 
   for (NSString* sessionID in sessionIDs) {
     NSString* sessionPath =
         [SessionServiceIOS sessionPathForSessionID:sessionID
                                          directory:stashPath];
-    if (shouldBackup)
-      backupPath = [self backupPathForSessionID:sessionID];
+    NSString* backupPath = nil;
+    if (shouldBackup) {
+      backupPath = [self backupPathForSessionID:sessionID directory:stashPath];
+    }
 
-    partialSuccess |= [[self class] deleteSessionFromPath:sessionPath
-                                               backupFile:backupPath];
+    partialSuccess |= [self deleteSessionFromPath:sessionPath
+                                       backupFile:backupPath];
   }
   return partialSuccess;
 }
@@ -331,102 +324,35 @@ static BOOL _sessionRestored = NO;
   return YES;
 }
 
-+ (BOOL)deleteSessionForBrowserState:(ChromeBrowserState*)browserState
-                          backupFile:(NSString*)file {
-  NSString* stashPath =
-      base::SysUTF8ToNSString(browserState->GetStatePath().value());
-  NSString* sessionPath = [SessionServiceIOS sessionPathForDirectory:stashPath];
-  NSFileManager* fileManager = [NSFileManager defaultManager];
-  if (![fileManager fileExistsAtPath:sessionPath])
-    return NO;
-  if (file) {
-    NSError* error = nil;
-    BOOL fileOperationSuccess =
-        [fileManager removeItemAtPath:file error:&error];
-    NSInteger errorCode = fileOperationSuccess ? 0 : [error code];
-    base::UmaHistogramSparse("TabRestore.error_remove_backup_at_path",
-                             errorCode);
-    if (!fileOperationSuccess && errorCode != NSFileNoSuchFileError) {
-      return NO;
-    }
++ (NSString*)backupPathForSessionID:(NSString*)sessionID
+                          directory:(NSString*)directory {
+  if (!sessionID.length)
+    return [directory stringByAppendingPathComponent:kSessionBackupFileName];
 
-    fileOperationSuccess =
-        [fileManager moveItemAtPath:sessionPath toPath:file error:&error];
-    errorCode = fileOperationSuccess ? 0 : [error code];
-    base::UmaHistogramSparse("TabRestore.error_move_session_at_path_to_backup",
-                             errorCode);
-    if (!fileOperationSuccess) {
-      return NO;
-    }
-  } else {
-    NSError* error;
-    BOOL fileOperationSuccess =
-        [fileManager removeItemAtPath:sessionPath error:&error];
-    NSInteger errorCode = fileOperationSuccess ? 0 : [error code];
-    base::UmaHistogramSparse("TabRestore.error_remove_session_at_path",
-                             errorCode);
-    if (!fileOperationSuccess) {
-      return NO;
-    }
-  }
-  return YES;
-}
-
-+ (NSString*)backupPathForSessionID:(NSString*)sessionID {
-  NSString* tmpDirectory = NSTemporaryDirectory();
-  if (!sessionID || !sessionID.length)
-    return [tmpDirectory stringByAppendingPathComponent:kSessionBackupFileName];
   return [NSString pathWithComponents:@[
-    tmpDirectory, kSessionBackupDirectoryName, sessionID, kSessionBackupFileName
+    directory,
+    kSessionBackupDirectory,
+    sessionID,
+    kSessionBackupFileName,
   ]];
 }
 
-+ (NSString*)backupSessionsDirectoryPath {
-  NSString* tmpDirectory = NSTemporaryDirectory();
-  return
-      [tmpDirectory stringByAppendingPathComponent:kSessionBackupDirectoryName];
-}
-
-+ (NSArray<NSString*>*)backedupSessionPaths {
-  if (!IsMultiwindowSupported())
-    return @[ [[self class] backupPathForSessionID:nil] ];
-  NSString* sessionsDirectoryPath = [[self class] backupSessionsDirectoryPath];
-  NSArray<NSString*>* sessionsIDs = [[NSFileManager defaultManager]
-      contentsOfDirectoryAtPath:sessionsDirectoryPath
-                          error:nil];
-  NSMutableArray<NSString*>* sessionFilePaths =
-      [[NSMutableArray alloc] initWithCapacity:sessionsIDs.count];
-  for (NSString* sessionID in sessionsIDs) {
-    [sessionFilePaths
-        addObject:[[self class] backupPathForSessionID:sessionID]];
-  }
-  return sessionFilePaths;
-}
-
-+ (NSArray<NSString*>*)backedupSessionIDs {
++ (NSArray<NSString*>*)backedupSessionIDsForBrowserState:
+    (ChromeBrowserState*)browserState {
   if (!IsMultiwindowSupported())
     return @[ @"" ];
-  NSString* sessionsDirectoryPath = [[self class] backupSessionsDirectoryPath];
+  NSString* stashPath =
+      base::SysUTF8ToNSString(browserState->GetStatePath().AsUTF8Unsafe());
   return [[NSFileManager defaultManager]
-      contentsOfDirectoryAtPath:sessionsDirectoryPath
+      contentsOfDirectoryAtPath:
+          [stashPath stringByAppendingPathComponent:kSessionBackupDirectory]
                           error:nil];
 }
 
-+ (BOOL)isBackedUpSessionID:(NSString*)sessionID {
-  return [[[self class] backedupSessionIDs] containsObject:sessionID];
-}
-
-+ (BOOL)moveAsideSessionInformationForBrowserState:
-    (ChromeBrowserState*)browserState {
-  DCHECK(!IsMultiwindowSupported());
-  // This may be the first time that the OTR browser state is being accessed, so
-  // ensure that the OTR ChromeBrowserState is created first.
-  ChromeBrowserState* otrBrowserState =
-      browserState->GetOffTheRecordChromeBrowserState();
-
-  [self deleteSessionForBrowserState:otrBrowserState backupFile:nil];
-  return [self deleteSessionForBrowserState:browserState
-                                 backupFile:[self backupPathForSessionID:nil]];
++ (BOOL)isBackedUpSessionID:(NSString*)sessionID
+               browserState:(ChromeBrowserState*)browserState {
+  return [[self backedupSessionIDsForBrowserState:browserState]
+      containsObject:sessionID];
 }
 
 + (BOOL)moveAsideSessions:(NSSet<NSString*>*)sessionIDs
@@ -438,43 +364,65 @@ static BOOL _sessionRestored = NO;
   [self deleteSessions:sessionIDs
        forBrowserState:otrBrowserState
           shouldBackup:NO];
+
   return [self deleteSessions:sessionIDs
               forBrowserState:browserState
                  shouldBackup:YES];
 }
 
 - (BOOL)restoreSessionsAfterCrash {
-  CrashRestoreHelper* strongSelf = self;
   DCHECK(!_sessionRestored);
   _sessionRestored = YES;
+
+  // Deleting _infoBarBridge will release the owning reference it has to self
+  // which may be the last reference existing. Thus it is unsafe to access to
+  // the current instance after _infoBarBridge.reset(). Use a local variable
+  // with precise lifetime to ensure the code self is valid till the end of the
+  // current method.
+  // TODO(crbug.com/1168480): fix ownership of CrashRestoreHelper.
+  __attribute__((objc_precise_lifetime)) CrashRestoreHelper* keepAlive = self;
   _infoBarBridge.reset();
-  BrowserList* browserList = BrowserListFactory::GetForBrowserState(
-      strongSelf.browser->GetBrowserState());
+
+  return [CrashRestoreHelper
+      restoreSessionsAfterCrashForBrowserState:_browser->GetBrowserState()];
+}
+
++ (BOOL)restoreSessionsAfterCrashForBrowserState:
+    (ChromeBrowserState*)browserState {
+  NSString* stashPath =
+      base::SysUTF8ToNSString(browserState->GetStatePath().AsUTF8Unsafe());
+
+  BrowserList* browserList =
+      BrowserListFactory::GetForBrowserState(browserState);
   breakpad_helper::WillStartCrashRestoration();
   BOOL success = NO;
   // First restore all conected sessions.
   NSFileManager* fileManager = [NSFileManager defaultManager];
   NSError* error = nil;
-  std::set<Browser*> regularBrowsers = browserList->AllRegularBrowsers();
 
+  std::set<Browser*> regularBrowsers = browserList->AllRegularBrowsers();
   for (Browser* browser : regularBrowsers) {
     NSString* sessionID = SceneStateBrowserAgent::FromBrowser(browser)
                               ->GetSceneState()
                               .sceneSessionID;
-    NSString* sessionPath =
-        [[strongSelf class] backupPathForSessionID:sessionID];
+
+    NSString* backupPath =
+        [CrashRestoreHelper backupPathForSessionID:sessionID
+                                         directory:stashPath];
+
     SessionIOS* session =
-        [[SessionServiceIOS sharedService] loadSessionFromPath:sessionPath];
+        [[SessionServiceIOS sharedService] loadSessionFromPath:backupPath];
 
     if (!session)
       continue;
     success |= SessionRestorationBrowserAgent::FromBrowser(browser)
                    ->RestoreSessionWindow(session.sessionWindows[0]);
-    // remove the backup directory for this session as it will not be moved
-    // back to its original browser state direcotry.
+
+    // Remove the backup directory for this session as it will not be moved
+    // back to its original browser state directory.
     if (IsMultiwindowSupported()) {
       [fileManager
-          removeItemAtPath:[sessionPath stringByDeletingLastPathComponent]
+          removeItemAtPath:[backupPath stringByDeletingLastPathComponent]
                      error:&error];
     }
   }
@@ -486,21 +434,21 @@ static BOOL _sessionRestored = NO;
 
   // Now put non restored sessions files to its original location in the browser
   // state directory.
-  Browser* anyBrowser = *regularBrowsers.begin();
-  NSString* stashPath = base::SysUTF8ToNSString(
-      anyBrowser->GetBrowserState()->GetStatePath().value());
-
   NSArray<NSString*>* backedupSessionIDs =
-      [[strongSelf class] backedupSessionIDs];
+      [CrashRestoreHelper backedupSessionIDsForBrowserState:browserState];
   for (NSString* sessionID in backedupSessionIDs) {
     NSString* originalSessionPath =
         [SessionServiceIOS sessionPathForSessionID:sessionID
                                          directory:stashPath];
+
     NSString* backupPath =
-        [[strongSelf class] backupPathForSessionID:sessionID];
+        [CrashRestoreHelper backupPathForSessionID:sessionID
+                                         directory:stashPath];
+
     [fileManager moveItemAtPath:backupPath
                          toPath:originalSessionPath
                           error:&error];
+
     // Remove Parent directory for the backup path, so it doesn't show restore
     // prompt again.
     [fileManager removeItemAtPath:[backupPath stringByDeletingLastPathComponent]
@@ -523,24 +471,31 @@ static BOOL _sessionRestored = NO;
   // the recently closed tabs.
   _sessionRestored = YES;
 
-  NSArray<NSString*>* sessionsIDs = [[self class] backedupSessionIDs];
+  ChromeBrowserState* browserState = _browser->GetBrowserState();
+  NSString* stashPath =
+      base::SysUTF8ToNSString(browserState->GetStatePath().AsUTF8Unsafe());
+
+  NSArray<NSString*>* sessionsIDs =
+      [CrashRestoreHelper backedupSessionIDsForBrowserState:browserState];
   NSFileManager* fileManager = [NSFileManager defaultManager];
   NSError* error = nil;
   for (NSString* sessionID in sessionsIDs) {
-    NSString* sessionPath = [[self class] backupPathForSessionID:sessionID];
+    NSString* backupPath =
+        [CrashRestoreHelper backupPathForSessionID:sessionID
+                                         directory:stashPath];
+
     SessionIOS* session =
-        [[SessionServiceIOS sharedService] loadSessionFromPath:sessionPath];
+        [[SessionServiceIOS sharedService] loadSessionFromPath:backupPath];
 
     NSArray<CRWSessionStorage*>* sessions = session.sessionWindows[0].sessions;
     if (!sessions.count)
       continue;
 
     sessions::TabRestoreService* const tabRestoreService =
-        IOSChromeTabRestoreServiceFactory::GetForBrowserState(
-            _browser->GetBrowserState());
+        IOSChromeTabRestoreServiceFactory::GetForBrowserState(browserState);
     tabRestoreService->LoadTabsFromLastSession();
 
-    web::WebState::CreateParams params(_browser->GetBrowserState());
+    web::WebState::CreateParams params(browserState);
     for (CRWSessionStorage* session in sessions) {
       auto live_tab = std::make_unique<sessions::RestoreIOSLiveTab>(session);
       // Add all tabs at the 0 position as the position is relative to an old
@@ -549,7 +504,7 @@ static BOOL _sessionRestored = NO;
     }
     if (IsMultiwindowSupported()) {
       [fileManager
-          removeItemAtPath:[sessionPath stringByDeletingLastPathComponent]
+          removeItemAtPath:[backupPath stringByDeletingLastPathComponent]
                      error:&error];
     }
   }

ios/chrome/browser/crash_report/crash_restore_helper_unittest.mm

@@ -28,10 +28,6 @@
 
 using testing::Return;
 
-@interface CrashRestoreHelper (Test)
-+ (NSString*)backupPathForSessionID:(NSString*)sessionID;
-@end
-
 namespace {
 
 class CrashRestoreHelperTest : public PlatformTest {
@@ -55,13 +51,14 @@ class CrashRestoreHelperTest : public PlatformTest {
         chrome_browser_state_.get(),
         off_the_record_chrome_browser_state_,
     };
-    NSString* backup_path =
-        [CrashRestoreHelper backupPathForSessionID:session_id];
-    [file_manager removeItemAtPath:backup_path error:nil];
     NSData* data = [NSData dataWithBytes:"hello" length:5];
     for (size_t index = 0; index < base::size(browser_states); ++index) {
       NSString* state_path = base::SysUTF8ToNSString(
           browser_states[index]->GetStatePath().value());
+      NSString* backup_path =
+          [CrashRestoreHelper backupPathForSessionID:session_id
+                                           directory:state_path];
+      [file_manager removeItemAtPath:backup_path error:nil];
       NSString* session_path =
           [SessionServiceIOS sessionPathForSessionID:session_id
                                            directory:state_path];
@@ -103,10 +100,14 @@ class CrashRestoreHelperTest : public PlatformTest {
   // Returns |true| if the session with |session_id| was backed up correctly,
   // and deletes the backup file. if |session_id| is nil, the default backup
   // session location is used.
-  bool CheckAndDeleteSessionBackedUp(NSString* session_id) {
+  bool CheckAndDeleteSessionBackedUp(NSString* session_id,
+                                     ChromeBrowserState* browser_state) {
     NSFileManager* file_manager = [NSFileManager defaultManager];
-    NSString* backup_path =
-        [CrashRestoreHelper backupPathForSessionID:session_id];
+    NSString* backup_path = [CrashRestoreHelper
+        backupPathForSessionID:session_id
+                     directory:base::SysUTF8ToNSString(
+                                   browser_state->GetStatePath()
+                                       .AsUTF8Unsafe())];
     if (![file_manager fileExistsAtPath:backup_path])
       return false;
     [file_manager removeItemAtPath:backup_path error:nil];
@@ -123,14 +124,12 @@ class CrashRestoreHelperTest : public PlatformTest {
 // Tests that moving session work correctly when multiple windows are not
 // supported.
 TEST_F(CrashRestoreHelperTest, MoveAsideSingleSession) {
-  // This test is only enabled when multi-window is disabled.
-  if (IsMultiwindowSupported())
-    return;
   ASSERT_TRUE(CreateSession(nil));
-  [CrashRestoreHelper
-      moveAsideSessionInformationForBrowserState:chrome_browser_state_.get()];
+  [CrashRestoreHelper moveAsideSessions:[NSSet setWithArray:@[ @"" ]]
+                        forBrowserState:chrome_browser_state_.get()];
   EXPECT_TRUE(IsSessionErased(nil));
-  EXPECT_EQ(YES, CheckAndDeleteSessionBackedUp(nil));
+  EXPECT_EQ(YES,
+            CheckAndDeleteSessionBackedUp(nil, chrome_browser_state_.get()));
 }
 
 // Tests that moving session work correctly when multiple windows are supported.
@@ -145,7 +144,8 @@ TEST_F(CrashRestoreHelperTest, MoveAsideMultipleSessions) {
                         forBrowserState:chrome_browser_state_.get()];
   for (NSString* session_id in session_ids) {
     EXPECT_TRUE(IsSessionErased(session_id));
-    EXPECT_EQ(YES, CheckAndDeleteSessionBackedUp(session_id));
+    EXPECT_EQ(YES, CheckAndDeleteSessionBackedUp(session_id,
+                                                 chrome_browser_state_.get()));
   }
 }
 

ios/chrome/browser/sessions/session_service_ios.mm

@@ -215,7 +215,7 @@ NSString* const kSessionFileName =
 
 + (NSString*)sessionPathForSessionID:(NSString*)sessionID
                            directory:(NSString*)directory {
-  if (!sessionID)
+  if (!sessionID.length)
     return [[self class] sessionPathForDirectory:directory];
   return [NSString pathWithComponents:@[
     directory, kSessionDirectory, sessionID, kSessionFileName

ios/chrome/browser/ui/main/scene_controller.mm

@@ -677,12 +677,13 @@ const char kMultiWindowOpenInNewWindowHistogram[] =
   // Only create the restoration helper if the session with the current session
   // id was backed up successfully.
   if (self.sceneState.appState.sessionRestorationRequired) {
+    Browser* mainBrowser = self.mainInterface.browser;
     if (!IsMultiwindowSupported() ||
         [CrashRestoreHelper
-            isBackedUpSessionID:self.sceneState.sceneSessionID]) {
+            isBackedUpSessionID:self.sceneState.sceneSessionID
+                   browserState:mainBrowser->GetBrowserState()]) {
       self.sceneState.appState.startupInformation.restoreHelper =
-          [[CrashRestoreHelper alloc]
-              initWithBrowser:self.mainInterface.browser];
+          [[CrashRestoreHelper alloc] initWithBrowser:mainBrowser];
     }
   }