[ios] Change SessionServiceIOS API to avoid useless work.

If multiple call to -saveSession:directory:immediately: are made
in quick succession, the SessionIOS objects are created to be
immediately destroyed when the scheduled save is cancelled.

Change the SessionServiceIOS API to instead take a block creating
the SessionIOS instance and invoke it on the main thread, before
requesting that the data be serialised to the disk (by sending a
task on IO thread).

BUG=725539

Change-Id: I82b42d2b46baf03a7280e60185973c635dc164a2
Reviewed-on: https://chromium-review.googlesource.com/513929
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476254}

ios/chrome/browser/sessions/session_service_ios.h

@@ -11,6 +11,8 @@
 
 @class SessionIOS;
 
+using SessionIOSFactory = SessionIOS* (^)();
+
 // A singleton service for saving the current session. Can either save on a
 // delay or immediately. Saving is always performed on a separate thread.
 @interface SessionServiceIOS : NSObject
@@ -24,12 +26,12 @@
     (const scoped_refptr<base::SequencedTaskRunner>&)taskRunner
     NS_DESIGNATED_INITIALIZER;
 
-// Saves the session represented by |session| to |directory|. If |immediately|
+// Saves the session returned by |factory| to |directory|. If |immediately|
 // is NO, the save is done after a delay. If another call is pending, this one
 // is ignored. If YES, the save is done now, cancelling any pending calls.
 // Either way, the save is done on a separate thread to avoid blocking the UI
 // thread.
-- (void)saveSession:(SessionIOS*)sessionWindow
+- (void)saveSession:(SessionIOSFactory)factory
           directory:(NSString*)directory
         immediately:(BOOL)immediately;
 

ios/chrome/browser/sessions/session_service_ios.mm

@@ -74,7 +74,7 @@ NSString* const kRootObjectKey = @"root";  // Key for the root object.
   scoped_refptr<base::SequencedTaskRunner> _taskRunner;
 
   // Maps session path to the pending session for the delayed save behaviour.
-  NSMutableDictionary<NSString*, SessionIOS*>* _pendingSessions;
+  NSMutableDictionary<NSString*, SessionIOSFactory>* _pendingSessions;
 }
 
 #pragma mark - NSObject overrides
@@ -108,12 +108,12 @@ NSString* const kRootObjectKey = @"root";  // Key for the root object.
   return self;
 }
 
-- (void)saveSession:(SessionIOS*)session
+- (void)saveSession:(SessionIOSFactory)factory
           directory:(NSString*)directory
         immediately:(BOOL)immediately {
   NSString* sessionPath = [[self class] sessionPathForDirectory:directory];
   BOOL hadPendingSession = [_pendingSessions objectForKey:sessionPath] != nil;
-  [_pendingSessions setObject:session forKey:sessionPath];
+  [_pendingSessions setObject:factory forKey:sessionPath];
   if (immediately) {
     [NSObject cancelPreviousPerformRequestsWithTarget:self];
     [self performSaveToPathInBackground:sessionPath];
@@ -194,8 +194,9 @@ NSString* const kRootObjectKey = @"root";  // Key for the root object.
 
   // Serialize to NSData on the main thread to avoid accessing potentially
   // non-threadsafe objects on a background thread.
-  SessionIOS* session = [_pendingSessions objectForKey:sessionPath];
+  SessionIOSFactory factory = [_pendingSessions objectForKey:sessionPath];
   [_pendingSessions removeObjectForKey:sessionPath];
+  SessionIOS* session = factory();
 
   @try {
     NSData* sessionData = [NSKeyedArchiver archivedDataWithRootObject:session];

ios/chrome/browser/sessions/session_service_ios_unittest.mm

@@ -70,21 +70,24 @@ class SessionServiceTest : public PlatformTest {
     return base::SysUTF8ToNSString(session_path.AsUTF8Unsafe());
   }
 
-  // Create a SessionIOS corresponding to |window_count| windows each with
-  // |tab_count| tabs.
-  SessionIOS* CreateSession(NSUInteger window_count, NSUInteger tab_count) {
-    NSMutableArray<SessionWindowIOS*>* windows = [NSMutableArray array];
-    while (windows.count < window_count) {
-      NSMutableArray<CRWSessionStorage*>* tabs = [NSMutableArray array];
-      while (tabs.count < tab_count) {
-        [tabs addObject:[[CRWSessionStorage alloc] init]];
+  // Create a SessionIOSFactory creating a SessionIOS with |window_count|
+  // windows each with |tab_count| tabs.
+  SessionIOSFactory CreateSessionFactory(NSUInteger window_count,
+                                         NSUInteger tab_count) {
+    return ^{
+      NSMutableArray<SessionWindowIOS*>* windows = [NSMutableArray array];
+      while (windows.count < window_count) {
+        NSMutableArray<CRWSessionStorage*>* tabs = [NSMutableArray array];
+        while (tabs.count < tab_count) {
+          [tabs addObject:[[CRWSessionStorage alloc] init]];
+        }
+        [windows addObject:[[SessionWindowIOS alloc]
+                               initWithSessions:[tabs copy]
+                                  selectedIndex:(tabs.count ? tabs.count - 1
+                                                            : NSNotFound)]];
       }
-      [windows addObject:[[SessionWindowIOS alloc]
-                             initWithSessions:[tabs copy]
-                                selectedIndex:(tabs.count ? tabs.count - 1
-                                                          : NSNotFound)]];
-    }
-    return [[SessionIOS alloc] initWithWindows:[windows copy]];
+      return [[SessionIOS alloc] initWithWindows:[windows copy]];
+    };
   }
 
   SessionServiceIOS* session_service() { return session_service_; }
@@ -106,7 +109,7 @@ TEST_F(SessionServiceTest, SessionPathForDirectory) {
 }
 
 TEST_F(SessionServiceTest, SaveSessionWindowToPath) {
-  [session_service() saveSession:CreateSession(0u, 0u)
+  [session_service() saveSession:CreateSessionFactory(0u, 0u)
                        directory:directory()
                      immediately:YES];
 
@@ -125,7 +128,7 @@ TEST_F(SessionServiceTest, SaveSessionWindowToPathDirectoryExists) {
                                                          attributes:nil
                                                               error:nullptr]);
 
-  [session_service() saveSession:CreateSession(0u, 0u)
+  [session_service() saveSession:CreateSessionFactory(0u, 0u)
                        directory:directory()
                      immediately:YES];
 
@@ -145,7 +148,7 @@ TEST_F(SessionServiceTest, LoadSessionFromDirectoryNoFile) {
 }
 
 TEST_F(SessionServiceTest, LoadSessionFromDirectory) {
-  [session_service() saveSession:CreateSession(2u, 1u)
+  [session_service() saveSession:CreateSessionFactory(2u, 1u)
                        directory:directory()
                      immediately:YES];
 
@@ -164,7 +167,7 @@ TEST_F(SessionServiceTest, LoadSessionFromDirectory) {
 }
 
 TEST_F(SessionServiceTest, LoadSessionFromPath) {
-  [session_service() saveSession:CreateSession(2u, 1u)
+  [session_service() saveSession:CreateSessionFactory(2u, 1u)
                        directory:directory()
                      immediately:YES];
 

ios/chrome/browser/sessions/test_session_service.mm

@@ -19,11 +19,11 @@
   return [super initWithTaskRunner:base::ThreadTaskRunnerHandle::Get()];
 }
 
-- (void)saveSession:(SessionIOS*)session
+- (void)saveSession:(SessionIOSFactory)factory
           directory:(NSString*)directory
         immediately:(BOOL)immediately {
   NSString* sessionPath = [[self class] sessionPathForDirectory:directory];
-  NSData* data = [NSKeyedArchiver archivedDataWithRootObject:session];
+  NSData* data = [NSKeyedArchiver archivedDataWithRootObject:factory()];
   if (self.performIO)
     [self performSaveSessionData:data sessionPath:sessionPath];
 }

ios/chrome/browser/tabs/tab_model.mm

@@ -376,7 +376,11 @@ std::unique_ptr<web::WebState> CreateWebState(
     return;
   NSString* statePath =
       base::SysUTF8ToNSString(_browserState->GetStatePath().AsUTF8Unsafe());
-  [_sessionService saveSession:self.sessionForSaving
+  __weak TabModel* weakSelf = self;
+  SessionIOSFactory sessionFactory = ^{
+    return weakSelf.sessionForSaving;
+  };
+  [_sessionService saveSession:sessionFactory
                      directory:statePath
                    immediately:immediately];
 }

ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.h

@@ -11,6 +11,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #import "ios/shared/chrome/browser/ui/browser_list/browser_list_session_service.h"
 #import "ios/web/public/web_state/web_state.h"
 
@@ -54,6 +55,10 @@ class BrowserListSessionServiceImpl : public BrowserListSessionService {
   CreateWebStateCallback create_web_state_;
   std::unique_ptr<BrowserListObserver> observer_;
 
+  // Used to ensure that the block passed to SessionServiceIOS does not access
+  // this object once it has been destroyed.
+  base::WeakPtrFactory<BrowserListSessionServiceImpl> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(BrowserListSessionServiceImpl);
 };
 

ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm

@@ -197,7 +197,8 @@ BrowserListSessionServiceImpl::BrowserListSessionServiceImpl(
     : browser_list_(browser_list),
       session_directory_([session_directory copy]),
       session_service_(session_service),
-      create_web_state_(create_web_state) {
+      create_web_state_(create_web_state),
+      weak_factory_(this) {
   DCHECK(browser_list_);
   DCHECK(session_directory_);
   DCHECK(session_service_);
@@ -269,15 +270,25 @@ void BrowserListSessionServiceImpl::ScheduleLastSessionDeletion() {
 void BrowserListSessionServiceImpl::ScheduleSaveSession(bool immediately) {
   DCHECK(browser_list_) << "ScheduleSaveSession called after Shutdown.";
   DCHECK_GE(browser_list_->count(), 0);
-  const int count = browser_list_->count();
-  NSMutableArray<SessionWindowIOS*>* windows =
-      [NSMutableArray arrayWithCapacity:static_cast<NSUInteger>(count)];
-  for (int index = 0; index < count; ++index) {
-    Browser* browser = browser_list_->GetBrowserAtIndex(index);
-    [windows addObject:SerializeWebStateList(&browser->web_state_list())];
-  }
 
-  [session_service_ saveSession:[[SessionIOS alloc] initWithWindows:windows]
+  base::WeakPtr<BrowserListSessionServiceImpl> weak_ptr =
+      weak_factory_.GetWeakPtr();
+  SessionIOSFactory session_factory = ^SessionIOS*() {
+    BrowserListSessionServiceImpl* service = weak_ptr.get();
+    if (!weak_ptr)
+      return nil;
+
+    const int count = service->browser_list_->count();
+    NSMutableArray<SessionWindowIOS*>* windows =
+        [NSMutableArray arrayWithCapacity:static_cast<NSUInteger>(count)];
+    for (int index = 0; index < count; ++index) {
+      Browser* browser = service->browser_list_->GetBrowserAtIndex(index);
+      [windows addObject:SerializeWebStateList(&browser->web_state_list())];
+    }
+    return [[SessionIOS alloc] initWithWindows:windows];
+  };
+
+  [session_service_ saveSession:session_factory
                       directory:session_directory_
                     immediately:immediately];
 }