Updater: Fix XPC. Correctly dial XPC service. Repeating callbacks fixed.

With this CL we are properly dialing the XPC connection and sending messages.
This also fixes the issues with the StateChangeCallback being sent over XPC.

Bug: 1055876
Change-Id: I81077476e49be486620ae4e2eb8cba022c3ae4de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2124933
Commit-Queue: Mila Green <milagreen@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754971}

chrome/updater/BUILD.gn

@@ -97,6 +97,7 @@ if (is_win || is_mac) {
         "server/mac/server.mm",
         "server/mac/service_delegate.h",
         "server/mac/service_delegate.mm",
+        "server/mac/service_protocol.mm",
         "server/mac/update_service_wrappers.h",
         "server/mac/update_service_wrappers.mm",
         "update_apps_mac.mm",

chrome/updater/mac/update_service_out_of_process.mm

@@ -34,9 +34,10 @@ using base::SysUTF8ToNSString;
 - (instancetype)init {
   if ((self = [super init])) {
     _xpcConnection.reset([[NSXPCConnection alloc]
-        initWithServiceName:updater::GetGoogleUpdateServiceMachName().get()]);
-    _xpcConnection.get().remoteObjectInterface =
-        [NSXPCInterface interfaceWithProtocol:@protocol(CRUUpdateChecking)];
+        initWithMachServiceName:updater::GetGoogleUpdateServiceMachName().get()
+                        options:0]);
+
+    _xpcConnection.get().remoteObjectInterface = updater::GetXpcInterface();
 
     _xpcConnection.get().interruptionHandler = ^{
       LOG(WARNING) << "CRUUpdateCheckingService: XPC connection interrupted.";
@@ -73,7 +74,9 @@ using base::SysUTF8ToNSString;
                             reply:reply];
 }
 
-- (void)checkForUpdatesWithReply:(void (^_Nullable)(int rc))reply {
+- (void)checkForUpdatesWithUpdateState:
+            (id<CRUUpdateStateObserving> _Nonnull)updateState
+                                 reply:(void (^_Nullable)(int rc))reply {
   auto errorHandler = ^(NSError* xpcError) {
     LOG(ERROR) << "XPC connection failed: "
                << base::SysNSStringToUTF8([xpcError description]);
@@ -81,12 +84,14 @@ using base::SysUTF8ToNSString;
   };
 
   [[_xpcConnection remoteObjectProxyWithErrorHandler:errorHandler]
-      checkForUpdatesWithReply:reply];
+      checkForUpdatesWithUpdateState:updateState
+                               reply:reply];
 }
 
 - (void)checkForUpdateWithAppID:(NSString* _Nonnull)appID
                        priority:(CRUPriorityWrapper* _Nonnull)priority
-                    updateState:(CRUUpdateStateObserver* _Nonnull)updateState
+                    updateState:
+                        (id<CRUUpdateStateObserving> _Nonnull)updateState
                           reply:(void (^_Nullable)(int rc))reply {
   auto errorHandler = ^(NSError* xpcError) {
     LOG(ERROR) << "XPC connection failed: "
@@ -147,7 +152,11 @@ void UpdateServiceOutOfProcess::UpdateAll(StateChangeCallback state_update,
                                   static_cast<update_client::Error>(error)));
   };
 
-  [client_ checkForUpdatesWithReply:reply];
+  base::scoped_nsprotocol<id<CRUUpdateStateObserving>> stateObserver(
+      [[CRUUpdateStateObserver alloc]
+          initWithRepeatingCallback:state_update
+                     callbackRunner:callback_runner_]);
+  [client_ checkForUpdatesWithUpdateState:stateObserver.get() reply:reply];
 }
 
 void UpdateServiceOutOfProcess::Update(const std::string& app_id,
@@ -166,7 +175,7 @@ void UpdateServiceOutOfProcess::Update(const std::string& app_id,
 
   base::scoped_nsobject<CRUPriorityWrapper> priorityWrapper(
       [[CRUPriorityWrapper alloc] initWithPriority:priority]);
-  base::scoped_nsobject<CRUUpdateStateObserver> stateObserver(
+  base::scoped_nsprotocol<id<CRUUpdateStateObserving>> stateObserver(
       [[CRUUpdateStateObserver alloc]
           initWithRepeatingCallback:state_update
                      callbackRunner:callback_runner_]);

chrome/updater/registration_data.cc

@@ -7,6 +7,7 @@
 namespace updater {
 
 RegistrationRequest::RegistrationRequest() = default;
+RegistrationRequest::RegistrationRequest(const RegistrationRequest&) = default;
 RegistrationRequest::~RegistrationRequest() = default;
 
 }  // namespace updater

chrome/updater/registration_data.h

@@ -15,10 +15,9 @@ namespace updater {
 
 struct RegistrationRequest {
   RegistrationRequest();
+  RegistrationRequest(const RegistrationRequest&);
+  RegistrationRequest& operator=(const RegistrationRequest& other) = default;
   ~RegistrationRequest();
-  RegistrationRequest(const RegistrationRequest&) = delete;
-  RegistrationRequest& operator=(const RegistrationRequest&) = delete;
-
   // Application ID of the app.
   std::string app_id;
 

chrome/updater/server/mac/service_delegate.mm

@@ -11,6 +11,7 @@
 #include "base/mac/scoped_block.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #import "chrome/updater/server/mac/service_protocol.h"
 #import "chrome/updater/server/mac/update_service_wrappers.h"
 #include "chrome/updater/update_service.h"
@@ -19,17 +20,23 @@
 
 // Designated initializer.
 - (instancetype)initWithUpdateService:(updater::UpdateService*)service
+                       callbackRunner:(scoped_refptr<base::SequencedTaskRunner>)
+                                          callbackRunner
     NS_DESIGNATED_INITIALIZER;
 
 @end
 
 @implementation CRUUpdateCheckXPCServiceImpl {
   updater::UpdateService* _service;
+  scoped_refptr<base::SequencedTaskRunner> _callbackRunner;
 }
 
-- (instancetype)initWithUpdateService:(updater::UpdateService*)service {
+- (instancetype)initWithUpdateService:(updater::UpdateService*)service
+                       callbackRunner:(scoped_refptr<base::SequencedTaskRunner>)
+                                          callbackRunner {
   if (self = [super init]) {
     _service = service;
+    _callbackRunner = callbackRunner;
   }
   return self;
 }
@@ -38,31 +45,51 @@
   // Unsupported, but we must override NSObject's designated initializer.
   DLOG(ERROR)
       << "Plain init method not supported for CRUUpdateCheckXPCServiceImpl.";
-  return [self initWithUpdateService:nullptr];
+  return [self initWithUpdateService:nullptr callbackRunner:nullptr];
 }
 
-- (void)checkForUpdatesWithReply:(void (^_Nullable)(int rc))reply {
-  base::OnceCallback<void(updater::UpdateService::Result)> cb =
+- (void)checkForUpdatesWithUpdateState:(id<CRUUpdateStateObserving>)updateState
+                                 reply:(void (^_Nullable)(int rc))reply {
+  auto cb =
       base::BindOnce(base::RetainBlock(^(updater::UpdateService::Result error) {
         VLOG(0) << "UpdateAll complete: error = " << static_cast<int>(error);
         reply(static_cast<int>(error));
       }));
 
-  _service->UpdateAll({}, base::BindOnce(std::move(cb)));
+  auto sccb = base::BindRepeating(
+      base::RetainBlock(^(updater::UpdateService::UpdateState state) {
+        base::scoped_nsobject<CRUUpdateStateWrapper> updateStateWrapper(
+            [[CRUUpdateStateWrapper alloc] initWithUpdateState:state]);
+        [updateState observeUpdateState:updateStateWrapper.get()];
+      }));
+
+  _callbackRunner->PostTask(
+      FROM_HERE, base::BindOnce(&updater::UpdateService::UpdateAll, _service,
+                                std::move(sccb), std::move(cb)));
 }
 
 - (void)checkForUpdateWithAppID:(NSString* _Nonnull)appID
                        priority:(CRUPriorityWrapper* _Nonnull)priority
-                    updateState:(CRUUpdateStateObserver* _Nonnull)updateState
+                    updateState:(id<CRUUpdateStateObserving>)updateState
                           reply:(void (^_Nullable)(int rc))reply {
-  base::OnceCallback<void(updater::UpdateService::Result)> cb =
+  auto cb =
       base::BindOnce(base::RetainBlock(^(updater::UpdateService::Result error) {
         VLOG(0) << "Update complete: error = " << static_cast<int>(error);
         reply(static_cast<int>(error));
       }));
 
-  _service->Update(base::SysNSStringToUTF8(appID), [priority priority],
-                   [updateState callback], base::BindOnce(std::move(cb)));
+  auto sccb = base::BindRepeating(
+      base::RetainBlock(^(updater::UpdateService::UpdateState state) {
+        base::scoped_nsobject<CRUUpdateStateWrapper> updateStateWrapper(
+            [[CRUUpdateStateWrapper alloc] initWithUpdateState:state]);
+        [updateState observeUpdateState:updateStateWrapper.get()];
+      }));
+
+  _callbackRunner->PostTask(
+      FROM_HERE,
+      base::BindOnce(&updater::UpdateService::Update, _service,
+                     base::SysNSStringToUTF8(appID), [priority priority],
+                     std::move(sccb), std::move(cb)));
 }
 
 - (void)registerForUpdatesWithAppId:(NSString* _Nullable)appId
@@ -79,27 +106,30 @@
   request.existence_checker_path =
       base::FilePath(base::SysNSStringToUTF8(existenceCheckerPath));
 
-  base::OnceCallback<void(const updater::RegistrationResponse&)> cb =
-      base::BindOnce(
-          base::RetainBlock(^(const updater::RegistrationResponse& response) {
-            VLOG(0) << "Registration complete: status code = "
-                    << response.status_code;
-            reply(response.status_code);
-          }));
+  auto cb = base::BindOnce(
+      base::RetainBlock(^(const updater::RegistrationResponse& response) {
+        VLOG(0) << "Registration complete: status code = "
+                << response.status_code;
+        reply(response.status_code);
+      }));
 
-  _service->RegisterApp(request, base::BindOnce(std::move(cb)));
+  _callbackRunner->PostTask(
+      FROM_HERE, base::BindOnce(&updater::UpdateService::RegisterApp, _service,
+                                request, std::move(cb)));
 }
 
 @end
 
 @implementation CRUUpdateCheckXPCServiceDelegate {
   scoped_refptr<updater::UpdateService> _service;
+  scoped_refptr<base::SequencedTaskRunner> _callbackRunner;
 }
 
 - (instancetype)initWithUpdateService:
     (scoped_refptr<updater::UpdateService>)service {
   if (self = [super init]) {
     _service = service;
+    _callbackRunner = base::SequencedTaskRunnerHandle::Get();
   }
   return self;
 }
@@ -115,22 +145,13 @@
     shouldAcceptNewConnection:(NSXPCConnection*)newConnection {
   // Check to see if the other side of the connection is "okay";
   // if not, invalidate newConnection and return NO.
-  NSXPCInterface* updateCheckingInterface =
-      [NSXPCInterface interfaceWithProtocol:@protocol(CRUUpdateChecking)];
-  NSXPCInterface* updateStateObservingInterface =
-      [NSXPCInterface interfaceWithProtocol:@protocol(CRUUpdateStateObserving)];
-  [updateCheckingInterface
-       setInterface:updateStateObservingInterface
-        forSelector:@selector(checkForUpdateWithAppID:
-                                             priority:updateState:reply:)
-      argumentIndex:2
-            ofReply:NO];
-
-  newConnection.exportedInterface = updateCheckingInterface;
+
+  newConnection.exportedInterface = updater::GetXpcInterface();
 
   base::scoped_nsobject<CRUUpdateCheckXPCServiceImpl> object(
       [[CRUUpdateCheckXPCServiceImpl alloc]
-          initWithUpdateService:_service.get()]);
+          initWithUpdateService:_service.get()
+                 callbackRunner:_callbackRunner.get()]);
   newConnection.exportedObject = object.get();
   [newConnection resume];
   return YES;

chrome/updater/server/mac/service_protocol.h

@@ -26,7 +26,9 @@
 @protocol CRUUpdateChecking <NSObject>
 
 // Checks for updates and returns the result in the reply block.
-- (void)checkForUpdatesWithReply:(void (^_Nullable)(int rc))reply;
+- (void)checkForUpdatesWithUpdateState:
+            (CRUUpdateStateObserver* _Nonnull)updateState
+                                 reply:(void (^_Nullable)(int rc))reply;
 
 // Checks for update of a given app, with specified priority. Sends repeated
 // updates of progress and returns the result in the reply block.
@@ -45,4 +47,12 @@
 
 @end
 
+namespace updater {
+
+// Constructs an NSXPCInterface for a connection using CRUUpdateChecking and
+// CRUUpdateStateObserving protocols.
+NSXPCInterface* _Nonnull GetXpcInterface();
+
+}  // namespace updater
+
 #endif  // CHROME_UPDATER_SERVER_MAC_SERVICE_PROTOCOL_H_

chrome/updater/server/mac/service_protocol.mm

+// Copyright 2020 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 <Foundation/Foundation.h>
+
+#import "chrome/updater/server/mac/service_protocol.h"
+
+namespace updater {
+
+NSXPCInterface* GetXpcInterface() {
+  NSXPCInterface* updateCheckingInterface =
+      [NSXPCInterface interfaceWithProtocol:@protocol(CRUUpdateChecking)];
+  NSXPCInterface* updateStateObservingInterface =
+      [NSXPCInterface interfaceWithProtocol:@protocol(CRUUpdateStateObserving)];
+
+  [updateCheckingInterface
+       setInterface:updateStateObservingInterface
+        forSelector:@selector(checkForUpdatesWithUpdateState:reply:)
+      argumentIndex:0
+            ofReply:NO];
+
+  [updateCheckingInterface
+       setInterface:updateStateObservingInterface
+        forSelector:@selector(checkForUpdateWithAppID:
+                                             priority:updateState:reply:)
+      argumentIndex:2
+            ofReply:NO];
+
+  return updateCheckingInterface;
+}
+
+}  // namespace updater