[Mac Updater] Add a staging-key-changed callback to the staging watcher.

Also do some cleanup of naming, and use CFRunLoop calls rather
than NSPorts to stop running NSRunLoops.

BUG=496298

Change-Id: Iaa862268ae2a0b40e2eedfbc482da2cf4c69e4e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1540099
Auto-Submit: Avi Drissman <avi@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644849}

chrome/browser/mac/relauncher.mm

@@ -365,8 +365,8 @@ int RelauncherMain(const content::MainFunctionParams& main_parameters) {
   // If an update is staged but not yet installed, wait for it to be installed.
   if (wait_for_staged_update) {
     base::scoped_nsobject<CrStagingKeyWatcher> watcher(
-        [[CrStagingKeyWatcher alloc] init]);
-    [watcher waitForStagingKey];
+        [[CrStagingKeyWatcher alloc] initWithPollingTime:0.5]);
+    [watcher waitForStagingKeyToClear];
   }
 
   NSString* path = base::SysUTF8ToNSString(relaunch_executable);

chrome/common/mac/staging_watcher.h

@@ -12,24 +12,50 @@
 // this state of "update pending" is indicated outside of Keystone by a key in
 // the CFPreferences.
 
+using StagingKeyChangedObserver = void (^)(BOOL stagingKeySet);
+
+// An object to observe the staging key. It can be used in either of two ways:
+// 1. To wait for the staging key to clear.
+// 2. To notify when the staging key changes state.
 @interface CrStagingKeyWatcher : NSObject
 
-- (instancetype)init;
+// On macOS 10.11 and earlier, polling is used, and |pollingTime| specifies the
+// frequency of the polling. On macOS 10.12 and later, no polling is performed
+// and |pollingTime| is ignored.
+- (instancetype)initWithPollingTime:(NSTimeInterval)pollingTime;
+
+// Returns a boolean indicating whether or not the staging key is set.
+- (BOOL)isStagingKeySet;
 
 // Sleeps until the staging key is clear. If there is no staging key set,
 // returns immediately.
-- (void)waitForStagingKey;
+- (void)waitForStagingKeyToClear;
+
+// Sets a block to be called when the staging key changes, and starts observing.
+// Only one observer may be set for a given CrStagingKeyWatcher; calling this
+// method again replaces the current observer.
+- (void)setStagingKeyChangedObserver:(StagingKeyChangedObserver)block;
 
 @end
 
 @interface CrStagingKeyWatcher (TestingInterface)
 
-- (instancetype)initWithUserDefaults:(NSUserDefaults*)defaults;
+// The designated initializer. Allows a non-default NSUserDefaults to be
+// specified.
+- (instancetype)initWithUserDefaults:(NSUserDefaults*)defaults
+                         pollingTime:(NSTimeInterval)pollingTime;
 
+// KVO works for NSUserDefaults on 10.12 and later. This method turns off the
+// use of KVO to allow the macOS 10.11 and earlier code path to be tested on
+// 10.12 and later.
 - (void)disableKVOForTesting;
 
+// Returns whether the last call to -waitForStagingKeyToClear blocked or
+// returned immediately.
 - (BOOL)lastWaitWasBlockedForTesting;
 
+// Returns the NSUserDefaults key that is used to indicate staging. The value to
+// be used is an array of strings, each string being a file path to the bundle.
 + (NSString*)stagingKeyForTesting;
 
 @end

chrome/common/mac/staging_watcher.mm

@@ -6,6 +6,7 @@
 
 #include "base/mac/bundle_locations.h"
 #include "base/mac/mac_util.h"
+#include "base/mac/scoped_block.h"
 #include "base/mac/scoped_nsobject.h"
 
 // Best documentation / Is unofficial documentation
@@ -26,8 +27,11 @@ NSString* const kStagingKey = @"UpdatePending";
 
 @interface CrStagingKeyWatcher () {
   base::scoped_nsobject<NSUserDefaults> defaults_;
+  NSTimeInterval pollingTime_;
+  base::scoped_nsobject<NSTimer> pollingTimer_;
   BOOL observing_;
-  base::scoped_nsobject<NSPort> wakePort_;
+  base::mac::ScopedBlock<StagingKeyChangedObserver> callback_;
+  BOOL lastStagingKeyValue_;
 
   BOOL kvoDisabledForTesting_;
   BOOL lastWaitWasBlockedForTesting_;
@@ -37,14 +41,23 @@ NSString* const kStagingKey = @"UpdatePending";
 
 @implementation CrStagingKeyWatcher
 
-- (instancetype)init {
-  return [self initWithUserDefaults:[NSUserDefaults standardUserDefaults]];
+- (instancetype)initWithPollingTime:(NSTimeInterval)pollingTime {
+  return [self initWithUserDefaults:[NSUserDefaults standardUserDefaults]
+                        pollingTime:pollingTime];
 }
 
-- (instancetype)initWithUserDefaults:(NSUserDefaults*)defaults {
+- (instancetype)initWithUserDefaults:(NSUserDefaults*)defaults
+                         pollingTime:(NSTimeInterval)pollingTime {
   if ((self = [super init])) {
+    pollingTime_ = pollingTime;
     defaults_.reset(defaults, base::scoped_policy::RETAIN);
+    lastStagingKeyValue_ = [self isStagingKeySet];
     if (base::mac::IsAtLeastOS10_12() && !kvoDisabledForTesting_) {
+      // If a change is made in another process (which is the use case here),
+      // the prior value is never provided in the observation callback change
+      // dictionary, whether or not NSKeyValueObservingOptionPrior is specified.
+      // Therefore, pass in 0 for the NSKeyValueObservingOptions and rely on
+      // keeping the previous value in |lastStagingKeyValue_|.
       [defaults_ addObserver:self
                   forKeyPath:kStagingKey
                      options:0
@@ -55,7 +68,7 @@ NSString* const kStagingKey = @"UpdatePending";
   return self;
 }
 
-- (BOOL)shouldWait {
+- (BOOL)isStagingKeySet {
   NSArray<NSString*>* paths = [defaults_ stringArrayForKey:kStagingKey];
   if (!paths)
     return NO;
@@ -65,28 +78,29 @@ NSString* const kStagingKey = @"UpdatePending";
   return [paths containsObject:appPath];
 }
 
-- (void)waitForStagingKey {
-  if (![self shouldWait]) {
+- (void)waitForStagingKeyToClear {
+  if (![self isStagingKeySet]) {
     lastWaitWasBlockedForTesting_ = NO;
     return;
   }
 
   NSRunLoop* runloop = [NSRunLoop currentRunLoop];
   if (observing_) {
-    wakePort_.reset([NSPort port], base::scoped_policy::RETAIN);
-    [runloop addPort:wakePort_ forMode:NSDefaultRunLoopMode];
-
-    while ([self shouldWait] && [runloop runMode:NSDefaultRunLoopMode
-                                      beforeDate:[NSDate distantFuture]]) {
+    callback_.reset(
+        ^(BOOL stagingKeySet) {
+          CFRunLoopStop([runloop getCFRunLoop]);
+        },
+        base::scoped_policy::RETAIN);
+
+    while ([self isStagingKeySet] && [runloop runMode:NSDefaultRunLoopMode
+                                           beforeDate:[NSDate distantFuture]]) {
       /* run! */
     }
   } else {
-    const NSTimeInterval kPollingTime = 0.5;
-
-    while ([self shouldWait] &&
+    while ([self isStagingKeySet] &&
            [runloop
                   runMode:NSDefaultRunLoopMode
-               beforeDate:[NSDate dateWithTimeIntervalSinceNow:kPollingTime]]) {
+               beforeDate:[NSDate dateWithTimeIntervalSinceNow:pollingTime_]]) {
       /* run! */
     }
   }
@@ -94,11 +108,31 @@ NSString* const kStagingKey = @"UpdatePending";
   lastWaitWasBlockedForTesting_ = YES;
 }
 
+- (void)setStagingKeyChangedObserver:(StagingKeyChangedObserver)block {
+  callback_.reset(block, base::scoped_policy::RETAIN);
+
+  if (observing_) {
+    // Nothing to be done; the observation is already started.
+  } else {
+    pollingTimer_.reset(
+        [NSTimer scheduledTimerWithTimeInterval:pollingTime_
+                                         target:self
+                                       selector:@selector(timerFired:)
+                                       userInfo:nil
+                                        repeats:YES],
+        base::scoped_policy::RETAIN);
+  }
+}
+
+- (void)timerFired:(NSTimer*)timer {
+  [self observeValueForKeyPath:nil ofObject:nil change:nil context:nil];
+}
+
 - (void)dealloc {
   if (observing_)
     [defaults_ removeObserver:self forKeyPath:kStagingKey context:nullptr];
-  if (wakePort_)
-    [wakePort_ invalidate];
+  if (pollingTimer_)
+    [pollingTimer_ invalidate];
 
   [super dealloc];
 }
@@ -119,7 +153,12 @@ NSString* const kStagingKey = @"UpdatePending";
                       ofObject:(id)object
                         change:(NSDictionary*)change
                        context:(void*)context {
-  [wakePort_ sendBeforeDate:[NSDate date] components:nil from:nil reserved:0];
+  BOOL isStagingKeySet = [self isStagingKeySet];
+  if (isStagingKeySet == lastStagingKeyValue_)
+    return;
+
+  lastStagingKeyValue_ = isStagingKeySet;
+  callback_.get()([self isStagingKeySet]);
 }
 
 @end

chrome/common/mac/staging_watcher_unittest.mm

@@ -34,13 +34,14 @@ class StagingKeyWatcherTest : public testing::TestWithParam<KVOOrNot> {
     [defaults_ removeObjectForKey:[CrStagingKeyWatcher stagingKeyForTesting]];
   }
 
-  CrStagingKeyWatcher* CreateKeyWatcher() {
-    keyWatcher_.reset(
-        [[CrStagingKeyWatcher alloc] initWithUserDefaults:defaults_]);
+  base::scoped_nsobject<CrStagingKeyWatcher> CreateKeyWatcher() {
+    base::scoped_nsobject<CrStagingKeyWatcher> keyWatcher(
+        [[CrStagingKeyWatcher alloc] initWithUserDefaults:defaults_
+                                              pollingTime:0.5]);
     if (GetParam() == KVOOrNot::kDontUseKVO)
-      [keyWatcher_ disableKVOForTesting];
+      [keyWatcher disableKVOForTesting];
 
-    return keyWatcher_;
+    return keyWatcher;
   }
 
   void SetDefaultsValue(id value) {
@@ -56,8 +57,18 @@ class StagingKeyWatcherTest : public testing::TestWithParam<KVOOrNot> {
                              ]];
   }
 
+  void SetDefaultsValueInSeparateProcess() {
+    NSString* appPath = [base::mac::OuterBundle() bundlePath];
+
+    [NSTask launchedTaskWithLaunchPath:@"/usr/bin/defaults"
+                             arguments:@[
+                               @"write", testingBundleID_.get(),
+                               [CrStagingKeyWatcher stagingKeyForTesting],
+                               @"-array", appPath
+                             ]];
+  }
+
  private:
-  base::scoped_nsobject<CrStagingKeyWatcher> keyWatcher_;
   base::scoped_nsobject<NSString> testingBundleID_;
   base::scoped_nsobject<NSUserDefaults> defaults_;
 };
@@ -70,32 +81,32 @@ INSTANTIATE_TEST_SUITE_P(KVOandNot,
 }  // namespace
 
 TEST_P(StagingKeyWatcherTest, NoBlockingWhenNoKey) {
-  CrStagingKeyWatcher* watcher = CreateKeyWatcher();
-  [watcher waitForStagingKey];
+  base::scoped_nsobject<CrStagingKeyWatcher> watcher = CreateKeyWatcher();
+  [watcher waitForStagingKeyToClear];
   ASSERT_FALSE([watcher lastWaitWasBlockedForTesting]);
 }
 
 TEST_P(StagingKeyWatcherTest, NoBlockingWhenWrongKeyType) {
   SetDefaultsValue(@"this is not an string array");
 
-  CrStagingKeyWatcher* watcher = CreateKeyWatcher();
-  [watcher waitForStagingKey];
+  base::scoped_nsobject<CrStagingKeyWatcher> watcher = CreateKeyWatcher();
+  [watcher waitForStagingKeyToClear];
   ASSERT_FALSE([watcher lastWaitWasBlockedForTesting]);
 }
 
 TEST_P(StagingKeyWatcherTest, NoBlockingWhenWrongArrayType) {
   SetDefaultsValue(@[ @3, @1, @4, @1, @5 ]);
 
-  CrStagingKeyWatcher* watcher = CreateKeyWatcher();
-  [watcher waitForStagingKey];
+  base::scoped_nsobject<CrStagingKeyWatcher> watcher = CreateKeyWatcher();
+  [watcher waitForStagingKeyToClear];
   ASSERT_FALSE([watcher lastWaitWasBlockedForTesting]);
 }
 
 TEST_P(StagingKeyWatcherTest, NoBlockingWhenEmptyArray) {
   SetDefaultsValue(@[]);
 
-  CrStagingKeyWatcher* watcher = CreateKeyWatcher();
-  [watcher waitForStagingKey];
+  base::scoped_nsobject<CrStagingKeyWatcher> watcher = CreateKeyWatcher();
+  [watcher waitForStagingKeyToClear];
   ASSERT_FALSE([watcher lastWaitWasBlockedForTesting]);
 }
 
@@ -111,7 +122,44 @@ TEST_P(StagingKeyWatcherTest, BlockFunctionality) {
     ClearDefaultsValueInSeparateProcess();
   });
 
-  CrStagingKeyWatcher* watcher = CreateKeyWatcher();
-  [watcher waitForStagingKey];
+  base::scoped_nsobject<CrStagingKeyWatcher> watcher = CreateKeyWatcher();
+  [watcher waitForStagingKeyToClear];
   ASSERT_TRUE([watcher lastWaitWasBlockedForTesting]);
 }
+
+TEST_P(StagingKeyWatcherTest, CallbackOnKeySet) {
+  // The staging key begins not set.
+
+  base::scoped_nsobject<CrStagingKeyWatcher> watcher = CreateKeyWatcher();
+  NSRunLoop* runloop = [NSRunLoop currentRunLoop];
+
+  [watcher setStagingKeyChangedObserver:^(BOOL stagingKeySet) {
+    CFRunLoopStop([runloop getCFRunLoop]);
+  }];
+
+  SetDefaultsValueInSeparateProcess();
+  while (![watcher isStagingKeySet] &&
+         [runloop runMode:NSDefaultRunLoopMode
+               beforeDate:[NSDate distantFuture]]) {
+    /* run! */
+  }
+}
+
+TEST_P(StagingKeyWatcherTest, CallbackOnKeyUnset) {
+  NSString* appPath = [base::mac::OuterBundle() bundlePath];
+  SetDefaultsValue(@[ appPath ]);
+
+  base::scoped_nsobject<CrStagingKeyWatcher> watcher = CreateKeyWatcher();
+  NSRunLoop* runloop = [NSRunLoop currentRunLoop];
+
+  [watcher setStagingKeyChangedObserver:^(BOOL stagingKeySet) {
+    CFRunLoopStop([runloop getCFRunLoop]);
+  }];
+
+  ClearDefaultsValueInSeparateProcess();
+  while ([watcher isStagingKeySet] &&
+         [runloop runMode:NSDefaultRunLoopMode
+               beforeDate:[NSDate distantFuture]]) {
+    /* run! */
+  }
+}