Fix extension updater browser tests with network service.

Bug: 871542
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Iab57f575186577532a7327c3fcf43266f91a1a43
Reviewed-on: https://chromium-review.googlesource.com/1172730
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Minh Nguyen <mxnguyen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582812}

chrome/browser/extensions/updater/extension_update_client_base_browsertest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/extensions/updater/extension_update_client_base_browsertest.h"
 
+#include "base/bind.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
@@ -13,7 +14,6 @@
 #include "chrome/browser/extensions/browsertest_util.h"
 #include "components/update_client/url_loader_post_interceptor.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/browser_thread.h"
 #include "extensions/browser/updater/update_service.h"
 #include "extensions/browser/updater/update_service_factory.h"
 #include "extensions/common/extension_features.h"
@@ -158,11 +158,11 @@ void ExtensionUpdateClientBaseTest::SetUpOnMainThread() {
   ASSERT_TRUE(update_service_);
 }
 
-void ExtensionUpdateClientBaseTest::SetUpNetworkInterceptors() {
-  auto io_thread = content::BrowserThread::GetTaskRunnerForThread(
-      content::BrowserThread::IO);
-  ASSERT_TRUE(io_thread);
+void ExtensionUpdateClientBaseTest::TearDownOnMainThread() {
+  get_interceptor_.reset();
+}
 
+void ExtensionUpdateClientBaseTest::SetUpNetworkInterceptors() {
   const auto update_urls = GetUpdateUrls();
   ASSERT_TRUE(!update_urls.empty());
   const GURL update_url = update_urls.front();
@@ -180,10 +180,9 @@ void ExtensionUpdateClientBaseTest::SetUpNetworkInterceptors() {
       ping_urls, &https_server_for_ping_);
   https_server_for_ping_.StartAcceptingConnections();
 
-  get_interceptor_ = std::make_unique<net::LocalHostTestURLRequestInterceptor>(
-      io_thread, base::CreateTaskRunnerWithTraits(
-                     {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-                      base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}));
+  get_interceptor_ =
+      std::make_unique<content::URLLoaderInterceptor>(base::BindRepeating(
+          &ExtensionUpdateClientBaseTest::OnRequest, base::Unretained(this)));
 }
 
 update_client::UpdateClient::Observer::Events
@@ -197,4 +196,13 @@ ExtensionUpdateClientBaseTest::WaitOnComponentUpdaterCompleteEvent(
   return event;
 }
 
+bool ExtensionUpdateClientBaseTest::OnRequest(
+    content::URLLoaderInterceptor::RequestParams* params) {
+  if (params->url_request.url.host() != "localhost")
+    return false;
+
+  get_interceptor_count_++;
+  return callback_ && callback_.Run(params);
+}
+
 }  // namespace extensions

chrome/browser/extensions/updater/extension_update_client_base_browsertest.h

@@ -9,6 +9,7 @@
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/updater/chrome_update_client_config.h"
 #include "components/update_client/update_client.h"
+#include "content/public/test/url_loader_interceptor.h"
 
 namespace base {
 namespace test {
@@ -20,10 +21,6 @@ namespace content {
 class BrowserMainParts;
 }  // namespace content
 
-namespace net {
-class TestURLRequestInterceptor;
-}  // namespace net
-
 namespace update_client {
 class URLLoaderPostInterceptor;
 }  // namespace update_client
@@ -45,6 +42,7 @@ class ExtensionUpdateClientBaseTest : public ExtensionBrowserTest {
   void SetUp() override;
   void SetUpOnMainThread() override;
   void CreatedBrowserMainParts(content::BrowserMainParts* parts) final;
+  void TearDownOnMainThread() override;
 
   // Injects a test configurator to the main extension browser client.
   // Override this function to inject your own custom configurator to the
@@ -65,9 +63,18 @@ class ExtensionUpdateClientBaseTest : public ExtensionBrowserTest {
   virtual std::vector<GURL> GetUpdateUrls() const;
   virtual std::vector<GURL> GetPingUrls() const;
 
+  void set_interceptor_hook(
+      content::URLLoaderInterceptor::InterceptCallback callback) {
+    callback_ = std::move(callback);
+  }
+
+  int get_interceptor_count() { return get_interceptor_count_; }
+
  protected:
   extensions::UpdateService* update_service_ = nullptr;
-  std::unique_ptr<net::TestURLRequestInterceptor> get_interceptor_;
+  std::unique_ptr<content::URLLoaderInterceptor> get_interceptor_;
+  int get_interceptor_count_ = 0;
+  content::URLLoaderInterceptor::InterceptCallback callback_;
 
   std::unique_ptr<update_client::URLLoaderPostInterceptor> update_interceptor_;
   std::unique_ptr<update_client::URLLoaderPostInterceptor> ping_interceptor_;
@@ -76,6 +83,8 @@ class ExtensionUpdateClientBaseTest : public ExtensionBrowserTest {
   net::EmbeddedTestServer https_server_for_ping_;
 
  private:
+  bool OnRequest(content::URLLoaderInterceptor::RequestParams* params);
+
   base::test::ScopedFeatureList scoped_feature_list_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionUpdateClientBaseTest);

chrome/browser/extensions/updater/update_service_browsertest.cc

@@ -4,6 +4,7 @@
 
 #include "base/command_line.h"
 #include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "chrome/browser/extensions/content_verifier_test_utils.h"
 #include "chrome/browser/extensions/extension_management_test_util.h"
@@ -97,7 +98,7 @@ IN_PROC_BROWSER_TEST_F(UpdateServiceTest, NoUpdate) {
       << update_interceptor_->GetRequestsAsString();
 
   // No update, thus no download nor ping activities.
-  EXPECT_EQ(0, get_interceptor_->GetHitCount());
+  EXPECT_EQ(0, get_interceptor_count());
   EXPECT_EQ(0, ping_interceptor_->GetCount())
       << ping_interceptor_->GetRequestsAsString();
 
@@ -155,7 +156,7 @@ IN_PROC_BROWSER_TEST_F(UpdateServiceTest, UpdateCheckError) {
       << update_interceptor_->GetRequestsAsString();
 
   // Error, thus no download nor ping activities.
-  EXPECT_EQ(0, get_interceptor_->GetHitCount());
+  EXPECT_EQ(0, get_interceptor_count());
   EXPECT_EQ(0, ping_interceptor_->GetCount())
       << ping_interceptor_->GetRequestsAsString();
 
@@ -224,7 +225,7 @@ IN_PROC_BROWSER_TEST_F(UpdateServiceTest, TwoUpdateCheckErrors) {
       << update_interceptor_->GetRequestsAsString();
 
   // Error, thus no download nor ping activities.
-  EXPECT_EQ(0, get_interceptor_->GetHitCount());
+  EXPECT_EQ(0, get_interceptor_count());
   EXPECT_EQ(0, ping_interceptor_->GetCount())
       << ping_interceptor_->GetRequestsAsString();
 }
@@ -246,8 +247,15 @@ IN_PROC_BROWSER_TEST_F(UpdateServiceTest, SuccessfulUpdate) {
   ASSERT_TRUE(ping_interceptor_->ExpectRequest(
       std::make_unique<update_client::PartialMatch>("eventtype"),
       ping_response));
-  get_interceptor_->SetResponseIgnoreQuery(
-      GURL("http://localhost/download/v1.crx"), crx_path);
+  set_interceptor_hook(base::BindLambdaForTesting(
+      [&](content::URLLoaderInterceptor::RequestParams* params) {
+        if (params->url_request.url.path() != "/download/v1.crx")
+          return false;
+
+        content::URLLoaderInterceptor::WriteResponse(crx_path,
+                                                     params->client.get());
+        return true;
+      }));
 
   const Extension* extension =
       InstallExtension(crx_path, 1, Manifest::EXTERNAL_POLICY_DOWNLOAD);
@@ -283,7 +291,7 @@ IN_PROC_BROWSER_TEST_F(UpdateServiceTest, SuccessfulUpdate) {
 
   ASSERT_EQ(1, update_interceptor_->GetCount())
       << update_interceptor_->GetRequestsAsString();
-  EXPECT_EQ(1, get_interceptor_->GetHitCount());
+  EXPECT_EQ(1, get_interceptor_count());
 
   const std::string update_request =
       std::get<0>(update_interceptor_->GetRequests()[0]);
@@ -311,8 +319,15 @@ IN_PROC_BROWSER_TEST_F(UpdateServiceTest, PolicyCorrupted) {
   ASSERT_TRUE(ping_interceptor_->ExpectRequest(
       std::make_unique<update_client::PartialMatch>("eventtype"),
       ping_response));
-  get_interceptor_->SetResponseIgnoreQuery(
-      GURL("http://localhost/download/v1.crx"), crx_path);
+  set_interceptor_hook(base::BindLambdaForTesting(
+      [&](content::URLLoaderInterceptor::RequestParams* params) {
+        if (params->url_request.url.path() != "/download/v1.crx")
+          return false;
+
+        content::URLLoaderInterceptor::WriteResponse(crx_path,
+                                                     params->client.get());
+        return true;
+      }));
 
   // Setup fake policy and update check objects.
   content_verifier_test::ForceInstallProvider policy(kExtensionId);
@@ -353,7 +368,7 @@ IN_PROC_BROWSER_TEST_F(UpdateServiceTest, PolicyCorrupted) {
 
   ASSERT_EQ(1, update_interceptor_->GetCount())
       << update_interceptor_->GetRequestsAsString();
-  EXPECT_EQ(1, get_interceptor_->GetHitCount());
+  EXPECT_EQ(1, get_interceptor_count());
 
   // Make sure that the update check request is formed correctly when the
   // extension is corrupted:
@@ -409,7 +424,7 @@ IN_PROC_BROWSER_TEST_F(UpdateServiceTest, UninstallExtensionWhileUpdating) {
 
   EXPECT_EQ(0, update_interceptor_->GetCount())
       << update_interceptor_->GetRequestsAsString();
-  EXPECT_EQ(0, get_interceptor_->GetHitCount());
+  EXPECT_EQ(0, get_interceptor_count());
 }
 
 class PolicyUpdateServiceTest : public ExtensionUpdateClientBaseTest {
@@ -455,8 +470,15 @@ class PolicyUpdateServiceTest : public ExtensionUpdateClientBaseTest {
     const base::FilePath ping_response =
         test_data_dir_.AppendASCII("updater/ping_reply_1.xml");
 
-    get_interceptor_->SetResponseIgnoreQuery(
-        GURL("http://localhost/download/v1.crx"), crx_path);
+    set_interceptor_hook(base::BindLambdaForTesting(
+        [=](content::URLLoaderInterceptor::RequestParams* params) {
+          if (params->url_request.url.path() != "/download/v1.crx")
+            return false;
+
+          content::URLLoaderInterceptor::WriteResponse(crx_path,
+                                                       params->client.get());
+          return true;
+        }));
     ASSERT_TRUE(update_interceptor_->ExpectRequest(
         std::make_unique<update_client::PartialMatch>("<updatecheck/>"),
         update_response));
@@ -540,7 +562,7 @@ IN_PROC_BROWSER_TEST_F(PolicyUpdateServiceTest, FailedUpdateRetries) {
 
   ASSERT_EQ(1, update_interceptor_->GetCount())
       << update_interceptor_->GetRequestsAsString();
-  EXPECT_EQ(1, get_interceptor_->GetHitCount());
+  EXPECT_EQ(1, get_interceptor_count());
 
   // Make sure that the update check request is formed correctly when the
   // extension is corrupted:
@@ -594,7 +616,7 @@ IN_PROC_BROWSER_TEST_F(PolicyUpdateServiceTest, Backoff) {
 
   ASSERT_EQ(4, update_interceptor_->GetCount())
       << update_interceptor_->GetRequestsAsString();
-  EXPECT_EQ(4, get_interceptor_->GetHitCount());
+  EXPECT_EQ(4, get_interceptor_count());
 
   const std::vector<base::TimeDelta>& calls = delay_tracker.calls();
 
@@ -640,7 +662,7 @@ IN_PROC_BROWSER_TEST_F(PolicyUpdateServiceTest, PRE_PolicyCorruptedOnStartup) {
 
   EXPECT_EQ(0, update_interceptor_->GetCount())
       << update_interceptor_->GetRequestsAsString();
-  EXPECT_EQ(0, get_interceptor_->GetHitCount());
+  EXPECT_EQ(0, get_interceptor_count());
 }
 
 // Now actually test what happens on the next startup after the PRE test above.
@@ -665,7 +687,7 @@ IN_PROC_BROWSER_TEST_F(PolicyUpdateServiceTest, PolicyCorruptedOnStartup) {
 
   ASSERT_EQ(1, update_interceptor_->GetCount())
       << update_interceptor_->GetRequestsAsString();
-  EXPECT_EQ(1, get_interceptor_->GetHitCount());
+  EXPECT_EQ(1, get_interceptor_count());
 
   const std::string update_request =
       std::get<0>(update_interceptor_->GetRequests()[0]);

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -14,15 +14,6 @@
 -SubresourceFilterBrowserTest.FailedProvisionalLoadInMainframe
 -WebViewTest.WebViewInBackgroundPage
 
-
-# These tests uses net::URLRequestFilter (through TestRequestInterceptor) to
-# mock out network responses, and use URLFetcherDownloader to issue requests,
-# which has yet to be switched from URLFetcher.
-# https://crbug.com/871542
--PolicyUpdateServiceTest.Backoff
--PolicyUpdateServiceTest.FailedUpdateRetries
--PolicyUpdateServiceTest.PolicyCorruptedOnStartup
-
 # Domain Reliability has yet to be hookup to the NetworkService.
 # https://crbug.com/853251
 -DomainReliabilityBrowserTest.Upload
@@ -40,11 +31,6 @@
 -ProfileBrowserTest.URLFetcherUsingMediaContextDuringShutdown
 -ProfileWithoutMediaCacheBrowserTest.NoSeparateMediaCache
 
-# components/update_client/ still uses URLFetcher.
-# After bug 844973 was fixed, these two still fail/crash:
--UpdateServiceTest.PolicyCorrupted
--UpdateServiceTest.SuccessfulUpdate
-
 # about:net-internals should be largely removed before shipping the network
 # service.
 # https://crbug.com/678391