Move on-demand update checks from the FILE thread to the blocking pool.

chrome/browser/google/google_update_win_unittest.cc

Index: chrome/browser/google/google_update_win_unittest.cc
diff --git a/chrome/browser/google/google_update_win_unittest.cc b/chrome/browser/google/google_update_win_unittest.cc
index 325a634f0ccf4eaef9dc21b9105b6a7f5e3cb543..19ba32728c68157853a639add2efca20200d1081 100644
--- a/chrome/browser/google/google_update_win_unittest.cc
+++ b/chrome/browser/google/google_update_win_unittest.cc
@@ -8,6 +8,7 @@
 #include <atlbase.h>
 #include <atlcom.h>
 
+#include <map>
 #include <memory>
 #include <queue>
 
@@ -24,6 +25,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/version.h"
 #include "base/win/registry.h"
+#include "base/win/scoped_com_initializer.h"
 #include "base/win/scoped_comptr.h"
 #include "chrome/common/chrome_version.h"
 #include "chrome/installer/util/browser_distribution.h"
@@ -37,6 +39,7 @@
 using ::testing::DoAll;
 using ::testing::HasSubstr;
 using ::testing::InSequence;
+using ::testing::InvokeWithoutArgs;
 using ::testing::IsEmpty;
 using ::testing::Return;
 using ::testing::Sequence;
@@ -69,6 +72,49 @@ class MockUpdateCheckDelegate : public UpdateCheckDelegate {
   DISALLOW_COPY_AND_ASSIGN(MockUpdateCheckDelegate);
 };
 
+// A fake implementation of the COM IGlobalInterfaceTable that holds and hands
+// out object pointers.
+class FakeGit : public CComObjectRootEx<CComSingleThreadModel>,
+                public IGlobalInterfaceTable {
+ public:
+  BEGIN_COM_MAP(FakeGit)
+    COM_INTERFACE_ENTRY(IGlobalInterfaceTable)
+  END_COM_MAP()
+
+  FakeGit() = default;
+
+  HRESULT STDMETHODCALLTYPE RegisterInterfaceInGlobal(IUnknown* object,
+                                                      REFIID,
+                                                      DWORD* cookie) override {
+    objects_[++last_cookie_] = object;
+    *cookie = last_cookie_;
+    return S_OK;
+  }
+
+  HRESULT STDMETHODCALLTYPE RevokeInterfaceFromGlobal(DWORD cookie) override {
+    return objects_.erase(cookie) ? S_OK : E_INVALIDARG;
+  }
+
+  HRESULT STDMETHODCALLTYPE GetInterfaceFromGlobal(DWORD cookie,
+                                                   REFIID,
+                                                   void** object) override {
+    auto it = objects_.find(cookie);
+    if (it == objects_.end())
+      return E_INVALIDARG;
+    it->second.get()->AddRef();
+    *object = it->second.get();
+    return S_OK;
+  }
+
+  size_t size() const { return objects_.size(); }
+
+ private:
+  using CookieToObjectMap = std::map<DWORD, base::win::ScopedComPtr<IUnknown>>;
+
+  CookieToObjectMap objects_;
+  DWORD last_cookie_ = 0;
+};

[Peter Kasting, 2016/08/03 01:24:34]

Nit: DISALLOW_COPY_AND_ASSIGN?

[grt (UTC plus 2), 2016/08/03 05:36:31]

Done.

+
 // An interface that exposes a factory method for creating an IGoogleUpdate3Web
 // instance.
 class GoogleUpdateFactory {
@@ -418,18 +464,20 @@ class MockAppBundle : public CComObjectRootEx<CComSingleThreadModel>,
     CComObject<MockApp>* mock_app = nullptr;
     EXPECT_EQ(S_OK, CComObject<MockApp>::CreateInstance(&mock_app));
 
-    // Give mock_app_bundle a ref to the app which it will return when asked.
     // Note: to support multiple apps, get_appWeb expectations should use
     // successive indices.
-    mock_app->AddRef();
+    app_ = mock_app;
     EXPECT_CALL(*this, get_appWeb(0, _))
-        .WillOnce(DoAll(SetArgPointee<1>(mock_app),
-                        Return(S_OK)));
+        .WillRepeatedly(
+            DoAll(SetArgPointee<1>(mock_app),
+                  InvokeWithoutArgs(mock_app, &CComObject<MockApp>::AddRef)));
 
     return mock_app;
   }
 
  private:
+  base::win::ScopedComPtr<IAppWeb> app_;
+
   DISALLOW_COPY_AND_ASSIGN(MockAppBundle);
 };
 
@@ -477,11 +525,14 @@ class MockGoogleUpdate : public CComObjectRootEx<CComSingleThreadModel>,
     // Give this instance a ref to the bundle which it will return when created.
     mock_app_bundle->AddRef();
     EXPECT_CALL(*this, createAppBundleWeb(_))
+        .InSequence(sequence_)
         .WillOnce(DoAll(SetArgPointee<0>(mock_app_bundle), Return(S_OK)));
     return mock_app_bundle;
   }
 
  private:
+  Sequence sequence_;
+
   DISALLOW_COPY_AND_ASSIGN(MockGoogleUpdate);
 };
 
@@ -531,7 +582,8 @@ class GoogleUpdateWinTest : public ::testing::TestWithParam<bool> {
   GoogleUpdateWinTest()
       : task_runner_(new base::TestSimpleTaskRunner()),
         task_runner_handle_(task_runner_),
-        system_level_install_(GetParam()) {}
+        system_level_install_(GetParam()),
+        fake_git_(nullptr) {}
 
   void SetUp() override {
     ::testing::TestWithParam<bool>::SetUp();
@@ -582,7 +634,8 @@ class GoogleUpdateWinTest : public ::testing::TestWithParam<bool> {
 
     // Provide an IGoogleUpdate3Web class factory so that this test can provide
     // a mocked-out instance.
-    SetGoogleUpdateFactoryForTesting(
+    SetUpdateCheckFactoriesForTesting(
+        base::Bind(&GoogleUpdateWinTest::GetFakeGit, base::Unretained(this)),
         base::Bind(&GoogleUpdateFactory::Create,
                    base::Unretained(&mock_google_update_factory_)));
 
@@ -611,16 +664,35 @@ class GoogleUpdateWinTest : public ::testing::TestWithParam<bool> {
   }
 
   void TearDown() override {
+    // Be sure all objects were removed from the Global Interface Table.
+    if (fake_git_) {
+      ASSERT_EQ(0U, fake_git_->size());

[Peter Kasting, 2016/08/03 01:24:34]

Nit: You could also elect to provide .empty() in place of .size(), which would
make this assert slightly shorter, at the cost of omitting the precise count
from the logs in cases where the assert fails.  (I'd probably
ASSERT_TRUE(fake_git_->empty()) if it were an STL class.)

[grt (UTC plus 2), 2016/08/03 05:36:31]

Done.

+      git_.Release();
+      fake_git_ = nullptr;
+    }
+
     // Remove the test's IGoogleUpdate on-demand update class factory.
-    SetGoogleUpdateFactoryForTesting(GoogleUpdate3ClassFactory());
+    SetUpdateCheckFactoriesForTesting(GlobalInterfaceTableClassFactory(),
+                                      GoogleUpdate3ClassFactory());
     ::testing::TestWithParam<bool>::TearDown();
   }
 
+  HRESULT GetFakeGit(base::win::ScopedComPtr<IGlobalInterfaceTable>* git) {

[Peter Kasting, 2016/08/03 01:24:34]

Seems like this should return void.

[grt (UTC plus 2), 2016/08/03 05:36:31]

This is bound into a GlobalInterfaceTableClassFactory callback, which returns
HRESULT.

+    if (!fake_git_) {
+      EXPECT_EQ(S_OK, CComObject<FakeGit>::CreateInstance(&fake_git_));
+      EXPECT_NE(nullptr, fake_git_);
+      git_ = fake_git_;
+    }
+    *git = git_;
+    return S_OK;
+  }
+
   static const base::char16 kClients[];
   static const base::char16 kClientState[];
   static const base::char16 kChromeGuid[];
   static const base::char16 kChromeBinariesGuid[];
 
+  base::win::ScopedCOMInitializer com_initializer_;
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
   bool system_level_install_;
@@ -630,6 +702,9 @@ class GoogleUpdateWinTest : public ::testing::TestWithParam<bool> {
   std::unique_ptr<base::ScopedPathOverride> local_app_data_override_;
   registry_util::RegistryOverrideManager registry_override_manager_;
 
+  CComObject<FakeGit>* fake_git_;
+  base::win::ScopedComPtr<IGlobalInterfaceTable> git_;
+
   // A mock object, the OnUpdateCheckCallback method of which will be invoked
   // each time the update check machinery invokes the given UpdateCheckCallback.
   StrictMock<MockUpdateCheckDelegate> mock_update_check_delegate_;
@@ -914,27 +989,18 @@ TEST_P(GoogleUpdateWinTest, UpdateFailed) {
 TEST_P(GoogleUpdateWinTest, RetryAfterExternalUpdaterError) {
   static const HRESULT GOOPDATE_E_APP_USING_EXTERNAL_UPDATER = 0xa043081d;
 
-  CComObject<MockAppBundle>* mock_app_bundle =
-      mock_google_update_factory_.MakeServerMock()->MakeAppBundle();
+  CComObject<MockGoogleUpdate>* google_update =
+      mock_google_update_factory_.MakeServerMock();
+  CComObject<MockAppBundle>* mock_app_bundle = google_update->MakeAppBundle();
 
   // The first attempt will fail in createInstalledApp indicating that an update
   // is already in progress.
-  Sequence bundle_seq;
   EXPECT_CALL(*mock_app_bundle, createInstalledApp(StrEq(kChromeBinariesGuid)))
-      .InSequence(bundle_seq)
       .WillOnce(Return(GOOPDATE_E_APP_USING_EXTERNAL_UPDATER));
 
-  // Expect a retry on the same instance.
-  EXPECT_CALL(*mock_app_bundle, createInstalledApp(StrEq(kChromeBinariesGuid)))
-      .InSequence(bundle_seq)
-      .WillOnce(Return(S_OK));
-
-  // See MakeApp() for an explanation of this:
-  CComObject<MockApp>* mock_app = nullptr;
-  EXPECT_EQ(S_OK, CComObject<MockApp>::CreateInstance(&mock_app));
-  mock_app->AddRef();
-  EXPECT_CALL(*mock_app_bundle, get_appWeb(0, _))
-      .WillOnce(DoAll(SetArgPointee<1>(mock_app), Return(S_OK)));
+  // Expect a retry on a new bundle.
+  mock_app_bundle = google_update->MakeAppBundle();
+  CComObject<MockApp>* mock_app = mock_app_bundle->MakeApp(kChromeBinariesGuid);
 
   // Expect the bundle to be called on to start the update.
   EXPECT_CALL(*mock_app_bundle, checkForUpdate()).WillOnce(Return(S_OK));