Fixing an edge case in CRWBrowsingDataStore

ios/web/crw_browsing_data_store.mm

Index: ios/web/crw_browsing_data_store.mm
diff --git a/ios/web/crw_browsing_data_store.mm b/ios/web/crw_browsing_data_store.mm
index 8a96d4a64aa1d1a81332102999d0750bdbc99e2e..a988838a77da89176ad0b0606089173699234bca 100644
--- a/ios/web/crw_browsing_data_store.mm
+++ b/ios/web/crw_browsing_data_store.mm
@@ -50,7 +50,12 @@ enum OperationType {
 @property(nonatomic, assign) CRWBrowsingDataStoreMode mode;
 // Sets the mode iff there are no more stash or restore operations that are
 // still pending. |mode| can only be either |ACTIVE| or |INACTIVE|.
-- (void)setModeIfNotStashingOrRestoring:(CRWBrowsingDataStoreMode)mode;
+// Returns YES if the mode change was successful.
+- (BOOL)setModeIfNoPendingStashOrRestoreOperations:
+        (CRWBrowsingDataStoreMode)mode;
+
+// The number of stash or restore operations that are still pending.
+@property(nonatomic, assign) NSUInteger numberOfPendingStashOrRestoreOperations;
 
 // Performs operations of type |operationType| on each of the
 // |browsingDataManagers|.
@@ -92,8 +97,10 @@ enum OperationType {
   // The last operation that was enqueued to be run. Can be stash, restore or a
   // delete operation.
   base::scoped_nsobject<NSOperation> _lastDispatchedOperation;
-  // The last stash or restore operations that was dispatched to be run.
-  base::scoped_nsobject<NSOperation> _lastDispatchedStashOrRestoreOperation;
+  // The number of stash or restore operations that are still pending. If this
+  // value > 0 the mode of the CRWBrowsingDataStore is SYNCHRONIZING. The mode
+  // can be made ACTIVE or INACTIVE only be set when this value is 0.
+  NSUInteger _numberOfPendingStashOrRestoreOperations;
 }
 
 + (NSOperationQueue*)operationQueueForStashAndRestoreOperations {
@@ -174,6 +181,18 @@ enum OperationType {
                                notFoundMarker:[NSNull null]];
 }
 
++ (BOOL)automaticallyNotifiesObserversForKey:(NSString*)key {
+  // It is necessary to override this for |mode| because the default KVO
+  // behavior in NSObject is to fire a notification irrespective of if an actual
+  // change was made to the ivar or not. The |mode| property needs fine grained
+  // control over the actual notifications being fired since observers need to
+  // be notified iff the |mode| actually changed.
+  if ([key isEqual:@"mode"]) {
+    return NO;
+  }
+  return [super automaticallyNotifiesObserversForKey:(NSString*)key];
+}
+
 - (CRWBrowsingDataStoreMode)mode {
   DCHECK([NSThread isMainThread]);
   return _mode;
@@ -181,41 +200,69 @@ enum OperationType {
 
 - (void)setMode:(CRWBrowsingDataStoreMode)mode {
   DCHECK([NSThread isMainThread]);
+  if (_mode == mode) {
+    return;
+  }
+  if (mode == ACTIVE || mode == INACTIVE) {
+    DCHECK(!self.numberOfPendingStashOrRestoreOperations);
+  }
+  [self willChangeValueForKey:@"mode"];
   _mode = mode;
+  [self didChangeValueForKey:@"mode"];
 }
 
-- (void)setModeIfNotStashingOrRestoring:(CRWBrowsingDataStoreMode)mode {
+- (BOOL)setModeIfNoPendingStashOrRestoreOperations:
+        (CRWBrowsingDataStoreMode)mode {
   DCHECK([NSThread isMainThread]);
   DCHECK_NE(SYNCHRONIZING, mode);
-  if ([_lastDispatchedStashOrRestoreOperation isFinished]) {
-    _mode = mode;
+
+  if (!self.numberOfPendingStashOrRestoreOperations) {
+    [self setMode:mode];
+    return YES;
   }
+  return NO;
+}
+
+- (NSUInteger)numberOfPendingStashOrRestoreOperations {
+  DCHECK([NSThread isMainThread]);
+  return _numberOfPendingStashOrRestoreOperations;
 }
 
-- (void)makeActiveWithCompletionHandler:(ProceduralBlock)completionHandler {
+- (void)setNumberOfPendingStashOrRestoreOperations:
+        (NSUInteger)numberOfPendingStashOrRestoreOperations {
+  DCHECK([NSThread isMainThread]);
+  _numberOfPendingStashOrRestoreOperations =
+      numberOfPendingStashOrRestoreOperations;
+}
+
+- (void)makeActiveWithCompletionHandler:
+        (void (^)(BOOL success))completionHandler {
   DCHECK([NSThread isMainThread]);
 
   base::WeakNSObject<CRWBrowsingDataStore> weakSelf(self);
   [self performOperationWithType:RESTORE
             browsingDataManagers:[self allBrowsingDataManagers]
                completionHandler:^{
-                 [weakSelf setModeIfNotStashingOrRestoring:ACTIVE];
+                 BOOL modeUpdateWasSuccessful = [weakSelf
+                     setModeIfNoPendingStashOrRestoreOperations:ACTIVE];
                  if (completionHandler) {
-                   completionHandler();
+                   completionHandler(modeUpdateWasSuccessful);
                  }
                }];
 }
 
-- (void)makeInactiveWithCompletionHandler:(ProceduralBlock)completionHandler {
+- (void)makeInactiveWithCompletionHandler:
+        (void (^)(BOOL success))completionHandler {
   DCHECK([NSThread isMainThread]);
 
   base::WeakNSObject<CRWBrowsingDataStore> weakSelf(self);
   [self performOperationWithType:STASH
             browsingDataManagers:[self allBrowsingDataManagers]
                completionHandler:^{
-                 [weakSelf setModeIfNotStashingOrRestoring:INACTIVE];
+                 BOOL modeUpdateWasSuccessful = [weakSelf
+                     setModeIfNoPendingStashOrRestoreOperations:INACTIVE];
                  if (completionHandler) {
-                   completionHandler();
+                   completionHandler(modeUpdateWasSuccessful);
                  }
                }];
 }
@@ -264,9 +311,18 @@ enum OperationType {
       break;
   };
 
+  if (operationType == RESTORE || operationType == STASH) {
+    [self setMode:SYNCHRONIZING];
+    ++self.numberOfPendingStashOrRestoreOperations;
+    completionHandler = ^{
+      --self.numberOfPendingStashOrRestoreOperations;
+      completionHandler();

[Eugene But (OOO till 7-30), 2015/05/29 22:38:28]

Is this (retaining self) ok from memory management perspective? Could you please
check if the block and self are not leaking.

[shreyasv1, 2015/06/01 18:31:54]

Yes. That is right thing to do here, There is no need for a weak ref dance.
self if not leaking because it's just like using self in any other block.
completionHandler's memory management is handled by the compiler -- since we are
not wrapping it in a scoped_nsobject.
completionHandler is not retaining itself, added a small note as per offline
discussion.

+    };
+  }
+
   id callCompletionHandlerOnMainThread = ^{
-    // This can be called on a background thread, hence the need to bounce to
-    // the main thread.
+    // This is called on a background thread, hence the need to bounce to the
+    // main thread.
     dispatch_async(dispatch_get_main_queue(), ^{
       completionHandler();
     });
@@ -276,10 +332,6 @@ enum OperationType {
                                   selector:selector
                          completionHandler:callCompletionHandlerOnMainThread]);
 
-  if (operationType == RESTORE || operationType == STASH) {
-    _mode = SYNCHRONIZING;
-    _lastDispatchedStashOrRestoreOperation.reset([operation retain]);
-  }
 
   NSOperationQueue* queue = nil;
   switch (operationType) {