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..15829290455ef95b04fcfd4a4080bdf87d60d6ba 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(!_numberOfPendingStashOrRestoreOperations);

[Eugene But (OOO till 7-30), 2015/05/29 21:14:26]

Optional NIT: 
s/_numberOfPendingStashOrRestoreOperations/self.numberOfPendingStashOrRestoreOperations

For consistency

[shreyasv1, 2015/05/29 22:22:56]

Done.

+  }
+  [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) {

[Eugene But (OOO till 7-30), 2015/05/29 21:14:26]

Can you rename setModeIfNoPendingStashOrRestoreOperations: to

- (void)finalizeChangeToMode:(CRWBrowsingDataStoreMode)mode 
    withCompletionHandler:(void(^)(BOOL))handler

and move completion handler call inside?

[shreyasv1, 2015/05/29 22:22:56]

I actually tried to consolidate the logic with a method like that.
My concern is that the |withCompletionHandler| semantically makes this an async
API. With the BOOL return way, there is no confusion.

Another way to consolidate this is have
|finalizeChangeToMode:callCompletionHandler:|.
Then completionHandler can be called in the same runloop and we will need an API
comment to make this behavior very clear. 

I found what I have done in this patch set easy from an API declaration
standpoint. Let me know if what I proposed is not too confusing and I will be
happy to change it.

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

You proposal (finalizeChangeToMode:callCompletionHandler:) sounds good.

I would add "and" word to the name
(finalizeChangeToMode:andCallCompletionHandler:) however it's up to you.

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

Done.

-                   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();
+    };
+  }
+
   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) {