Fixing an edge case in CRWBrowsingDataStore

ios/web/crw_browsing_data_store.mm

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/web/public/crw_browsing_data_store.h"
 
 #import <Foundation/Foundation.h>
 
 #import "base/ios/weak_nsobject.h"
 #include "base/logging.h"
@@ skipping 32 matching lines @@
 // |browsingDataType|.
 - (id<CRWBrowsingDataManager>)browsingDataManagerForBrowsingDataType:
         (NSString*)browsingDataType;
 // Returns an array of browsing data managers for all the |browsingDataType|.
 - (NSArray*)browsingDataManagersForBrowsingDataTypes:(NSSet*)browsingDataTypes;
 
 // Redefined to be read-write. Must be called from the main thread.
 @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|.
 // The kind of operations are:
 // 1) STASH: Stash operation involves stashing browsing data that web views
 // (UIWebViews and WKWebViews) create to the associated BrowserState's state
 // path.
 // 2) RESTORE: Restore operation involves restoring browsing data from the
 // associated BrowserState's state path so that web views (UIWebViews and
 // WKWebViews) can read from them.
@@ skipping 21 matching lines @@
 @implementation CRWBrowsingDataStore {
   web::BrowserState* _browserState;  // Weak, owns this object.
   // The mode of the CRWBrowsingDataStore.
   CRWBrowsingDataStoreMode _mode;
   // The dictionary that maps a browsing data type to its
   // CRWBrowsingDataManager.
   base::scoped_nsobject<NSDictionary> _browsingDataTypeMap;
   // 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 {
   static dispatch_once_t onceToken = 0;
   static NSOperationQueue* operationQueueForStashAndRestoreOperations = nil;
   dispatch_once(&onceToken, ^{
     operationQueueForStashAndRestoreOperations =
         [[NSOperationQueue alloc] init];
     [operationQueueForStashAndRestoreOperations
         setMaxConcurrentOperationCount:1U];
@@ skipping 60 matching lines @@
 - (id<CRWBrowsingDataManager>)browsingDataManagerForBrowsingDataType:
         (NSString*)browsingDataType {
   return [_browsingDataTypeMap objectForKey:browsingDataType];
 }
 
 - (NSArray*)browsingDataManagersForBrowsingDataTypes:(NSSet*)browsingDataTypes {
   return [_browsingDataTypeMap objectsForKeys:[browsingDataTypes allObjects]
                                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;
 }
 
 - (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;
 }
 
-- (void)makeActiveWithCompletionHandler:(ProceduralBlock)completionHandler {
+- (NSUInteger)numberOfPendingStashOrRestoreOperations {
+  DCHECK([NSThread isMainThread]);
+  return _numberOfPendingStashOrRestoreOperations;
+}
+
+- (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);
                  }
                }];
 }
 
 - (void)removeDataOfTypes:(NSSet*)browsingDataTypes
         completionHandler:(ProceduralBlock)completionHandler {
   DCHECK([NSThread isMainThread]);
 
   NSArray* browsingDataManagers =
       [self browsingDataManagersForBrowsingDataTypes:browsingDataTypes];
@@ skipping 28 matching lines @@
       selector = @selector(restoreData);
       break;
     case REMOVE:
       selector = @selector(removeData);
       break;
     default:
       NOTREACHED();
       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();
     });
   };
   base::scoped_nsobject<NSOperation> operation([self
       newOperationWithBrowsingDataManagers:browsingDataManagers
                                   selector:selector
                          completionHandler:callCompletionHandlerOnMainThread]);
 
-  if (operationType == RESTORE || operationType == STASH) {
-    _mode = SYNCHRONIZING;
-    _lastDispatchedStashOrRestoreOperation.reset([operation retain]);
-  }
 
   NSOperationQueue* queue = nil;
   switch (operationType) {
     case STASH:
     case RESTORE:
       queue = [CRWBrowsingDataStore operationQueueForStashAndRestoreOperations];
       break;
     case REMOVE:
       queue = [CRWBrowsingDataStore operationQueueForRemoveOperations];
       break;
@@ skipping 27 matching lines @@
   DCHECK(queue);
 
   if (_lastDispatchedOperation) {
     [operation addDependency:_lastDispatchedOperation];
   }
   _lastDispatchedOperation.reset([operation retain]);
   [queue addOperation:operation];
 }
 
 @end