Refactor task runners in ControlServiceProxy.

This is done as part of splitting IDL and renaming ControlService
to UpdateServiceInternal.

This change:
* removes a sequence checker on the object destructor
* replaces a thread checker with a sequence checker
* replaces a couple of posted callbacks with direct Run().
* renames com_task_runner to STA_task_runner to
make it clear it is the task runner running the COM code in the
COM apartment and not some other task runner, and for acronym
capitalization
* uses a SingleThreadTaskRunner instead of a SequencedTaskRunner.

The intention is to declutter the code a little and possibly
simplify the sequence checker management on destruction.

Bug: 1140562, 1143011
Change-Id: I6db022483c72d340da3cd053e6a900db4c5702ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2510912Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822822}

chrome/updater/win/control_service_proxy.cc

@@ -14,23 +14,19 @@
 #include "base/single_thread_task_runner.h"
 #include "base/task/task_traits.h"
 #include "base/threading/sequenced_task_runner_handle.h"
-#include "base/threading/thread_checker.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/updater/app/server/win/updater_idl.h"
 #include "chrome/updater/win/constants.h"
 
 namespace updater {
 namespace {
 
-
 static constexpr base::TaskTraits kComClientTraits = {
     base::TaskPriority::BEST_EFFORT,
     base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN};
 
 // This class implements the IUpdaterControlCallback interface and exposes it as
-// a COM object. The class has thread-affinity for the STA thread. However, its
-// functions are invoked directly by COM RPC, and they are not sequenced through
-// the thread task runner. This means that sequence checkers can't be used in
-// this class.
+// a COM object. The class has thread-affinity for the STA thread.
 class UpdaterControlCallback
     : public Microsoft::WRL::RuntimeClass<
           Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::ClassicCom>,
@@ -39,24 +35,16 @@ class UpdaterControlCallback
   UpdaterControlCallback(
       Microsoft::WRL::ComPtr<IUpdaterControl> updater_control,
       base::OnceClosure callback)
-      : com_task_runner_(base::SequencedTaskRunnerHandle::Get()),
-        updater_control_(updater_control),
-        callback_(std::move(callback)) {}
+      : updater_control_(updater_control), callback_(std::move(callback)) {}
 
   UpdaterControlCallback(const UpdaterControlCallback&) = delete;
   UpdaterControlCallback& operator=(const UpdaterControlCallback&) = delete;
 
   // Overrides for IUpdaterControlCallback.
   //
-  // Invoked by COM RPC on the apartment thread when the call to any of the
-  // non-blocking `ControlServiceProxy` functions completes.
-  IFACEMETHODIMP Run(LONG result) override {
-    DVLOG(2) << __func__ << " result " << result << ".";
-    com_task_runner_->PostTask(FROM_HERE,
-                               base::BindOnce(&UpdaterControlCallback::RunOnSTA,
-                                              base::WrapRefCounted(this)));
-    return S_OK;
-  }
+  // Invoked by COM RPC on the apartment thread (STA) when the call to any of
+  // the non-blocking `ControlServiceProxy` functions completes.
+  IFACEMETHODIMP Run(LONG result) override;
 
   // Disconnects this callback from its subject and ensures the callbacks are
   // not posted after this function is called. Returns the completion callback
@@ -66,14 +54,18 @@ class UpdaterControlCallback
  private:
   ~UpdaterControlCallback() override = default;
 
-  // Called in sequence on the |com_task_runner_|.
   void RunOnSTA();
 
   // Bound to the STA thread.
-  THREAD_CHECKER(thread_checker_);
+  SEQUENCE_CHECKER(sequence_checker_);
 
-  // Bound to the STA thread.
-  scoped_refptr<base::SequencedTaskRunner> com_task_runner_;
+  // Sequences COM function calls.
+  scoped_refptr<base::SingleThreadTaskRunner> STA_task_runner_ =
+      base::ThreadTaskRunnerHandle::Get();
+
+  // The thread id of the STA thread.
+  const base::PlatformThreadId STA_thread_id_ =
+      base::PlatformThread::CurrentId();
 
   // Keeps a reference of the updater object alive, while this object is
   // owned by the COM RPC runtime.
@@ -83,15 +75,29 @@ class UpdaterControlCallback
   base::OnceClosure callback_;
 };
 
+IFACEMETHODIMP UpdaterControlCallback::Run(LONG result) {
+  DVLOG(2) << __func__ << " result " << result << ".";
+
+  // Since this function is invoked directly by COM RPC, the code can only
+  // assert its OS thread-affinity but not its task runner sequence-affinity.
+  // For this reason, the implementation is delegated to a helper function,
+  // which is sequenced by `STA_task_runner`.
+  DCHECK_EQ(base::PlatformThread::CurrentId(), STA_thread_id_);
+  STA_task_runner_->PostTask(FROM_HERE,
+                             base::BindOnce(&UpdaterControlCallback::RunOnSTA,
+                                            base::WrapRefCounted(this)));
+  return S_OK;
+}
+
 base::OnceClosure UpdaterControlCallback::Disconnect() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DVLOG(2) << __func__;
   updater_control_ = nullptr;
   return std::move(callback_);
 }
 
 void UpdaterControlCallback::RunOnSTA() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   updater_control_ = nullptr;
 
@@ -99,18 +105,16 @@ void UpdaterControlCallback::RunOnSTA() {
     DVLOG(2) << "Skipping posting the completion callback.";
     return;
   }
-  com_task_runner_->PostTask(FROM_HERE, std::move(callback_));
+  STA_task_runner_->PostTask(FROM_HERE, std::move(callback_));
 }
 
 }  // namespace
 
 ControlServiceProxy::ControlServiceProxy(ServiceScope /*scope*/)
-    : com_task_runner_(
+    : STA_task_runner_(
           base::ThreadPool::CreateCOMSTATaskRunner(kComClientTraits)) {}
 
-ControlServiceProxy::~ControlServiceProxy() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-}
+ControlServiceProxy::~ControlServiceProxy() = default;
 
 void ControlServiceProxy::Uninitialize() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -119,23 +123,20 @@ void ControlServiceProxy::Uninitialize() {
 void ControlServiceProxy::Run(base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // Reposts the call to the COM task runner. Adapts |callback| so that
-  // the callback runs on the main sequence.
-  com_task_runner_->PostTask(
+  STA_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(
           &ControlServiceProxy::RunOnSTA, this,
           base::BindOnce(
-              [](scoped_refptr<base::SequencedTaskRunner> taskrunner,
+              [](scoped_refptr<base::SequencedTaskRunner> task_runner,
                  base::OnceClosure callback) {
-                taskrunner->PostTask(FROM_HERE,
-                                     base::BindOnce(std::move(callback)));
+                task_runner->PostTask(FROM_HERE, std::move(callback));
               },
               base::SequencedTaskRunnerHandle::Get(), std::move(callback))));
 }
 
 void ControlServiceProxy::RunOnSTA(base::OnceClosure callback) {
-  DCHECK(com_task_runner_->BelongsToCurrentThread());
+  DCHECK(STA_task_runner_->BelongsToCurrentThread());
 
   Microsoft::WRL::ComPtr<IUnknown> server;
   HRESULT hr = ::CoCreateInstance(CLSID_UpdaterControlClass, nullptr,
@@ -143,7 +144,7 @@ void ControlServiceProxy::RunOnSTA(base::OnceClosure callback) {
   if (FAILED(hr)) {
     DVLOG(2) << "Failed to instantiate the updater control server. " << std::hex
              << hr;
-    com_task_runner_->PostTask(FROM_HERE, std::move(callback));
+    std::move(callback).Run();
     return;
   }
 
@@ -152,7 +153,7 @@ void ControlServiceProxy::RunOnSTA(base::OnceClosure callback) {
   if (FAILED(hr)) {
     DVLOG(2) << "Failed to query the updater_control interface. " << std::hex
              << hr;
-    com_task_runner_->PostTask(FROM_HERE, std::move(callback));
+    std::move(callback).Run();
     return;
   }
 
@@ -174,7 +175,7 @@ void ControlServiceProxy::RunOnSTA(base::OnceClosure callback) {
     // state of the update server is. The RPC callback may or may not have run.
     // Disconnecting the object resolves this ambiguity and transfers the
     // ownership of the callback back to the caller.
-    com_task_runner_->PostTask(FROM_HERE, rpc_callback->Disconnect());
+    rpc_callback->Disconnect().Run();
     return;
   }
 }
@@ -182,22 +183,22 @@ void ControlServiceProxy::RunOnSTA(base::OnceClosure callback) {
 void ControlServiceProxy::InitializeUpdateService(base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  com_task_runner_->PostTask(
+  STA_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(
           &ControlServiceProxy::InitializeUpdateServiceOnSTA, this,
           base::BindOnce(
-              [](scoped_refptr<base::SequencedTaskRunner> taskrunner,
+              [](scoped_refptr<base::SingleThreadTaskRunner> task_runner,
                  base::OnceClosure callback) {
-                taskrunner->PostTask(FROM_HERE,
-                                     base::BindOnce(std::move(callback)));
+                task_runner->PostTask(FROM_HERE,
+                                      base::BindOnce(std::move(callback)));
               },
-              base::SequencedTaskRunnerHandle::Get(), std::move(callback))));
+              STA_task_runner_, std::move(callback))));
 }
 
 void ControlServiceProxy::InitializeUpdateServiceOnSTA(
     base::OnceClosure callback) {
-  DCHECK(com_task_runner_->BelongsToCurrentThread());
+  DCHECK(STA_task_runner_->BelongsToCurrentThread());
 
   Microsoft::WRL::ComPtr<IUnknown> server;
   HRESULT hr = ::CoCreateInstance(CLSID_UpdaterControlClass, nullptr,
@@ -205,7 +206,7 @@ void ControlServiceProxy::InitializeUpdateServiceOnSTA(
   if (FAILED(hr)) {
     DVLOG(2) << "Failed to instantiate the updater control server. " << std::hex
              << hr;
-    com_task_runner_->PostTask(FROM_HERE, std::move(callback));
+    std::move(callback).Run();
     return;
   }
 
@@ -214,7 +215,7 @@ void ControlServiceProxy::InitializeUpdateServiceOnSTA(
   if (FAILED(hr)) {
     DVLOG(2) << "Failed to query the updater_control interface. " << std::hex
              << hr;
-    com_task_runner_->PostTask(FROM_HERE, std::move(callback));
+    std::move(callback).Run();
     return;
   }
 
@@ -224,8 +225,9 @@ void ControlServiceProxy::InitializeUpdateServiceOnSTA(
   if (FAILED(hr)) {
     DVLOG(2) << "Failed to call IUpdaterControl::InitializeUpdateService"
              << std::hex << hr;
-    com_task_runner_->PostTask(FROM_HERE, rpc_callback->Disconnect());
+    rpc_callback->Disconnect().Run();
     return;
   }
 }
+
 }  // namespace updater

chrome/updater/win/control_service_proxy.h

@@ -39,7 +39,7 @@ class ControlServiceProxy : public ControlService {
 
   // Runs the tasks which involve outbound COM calls and inbound COM callbacks.
   // This task runner is thread-affine with the COM STA.
-  scoped_refptr<base::SingleThreadTaskRunner> com_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> STA_task_runner_;
 };
 
 }  // namespace updater