Make NetworkContextConfigurationBrowserTests run for SafeBrowsing.

Currently, they only run when the network service is disabled.  This CL
will both make it easier to enable them for SafeBrowsing once
SafeBrowsing correctly sets up its tests with the network service
enabled, and protects against regressions in the legacy in-process path
in the meantime.

Bug: 789640
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I5d33acbe7c3ee918a2fe5335c60c7c2b7bb0721a
Reviewed-on: https://chromium-review.googlesource.com/1042888Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556887}

chrome/browser/net/network_context_configuration_browsertest.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/net/profile_network_context_service_factory.h"
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_switches.h"
@@ -80,6 +81,7 @@ enum class NetworkServiceState {
 
 enum class NetworkContextType {
   kSystem,
+  kSafeBrowsing,
   kProfile,
   kIncognitoProfile,
 };
@@ -171,6 +173,10 @@ class NetworkContextConfigurationBrowserTest
       case NetworkContextType::kSystem:
         return g_browser_process->system_network_context_manager()
             ->GetURLLoaderFactory();
+      case NetworkContextType::kSafeBrowsing:
+        return g_browser_process->safe_browsing_service()
+            ->GetURLLoaderFactory()
+            .get();
       case NetworkContextType::kProfile:
         return content::BrowserContext::GetDefaultStoragePartition(
                    browser()->profile())
@@ -192,6 +198,8 @@ class NetworkContextConfigurationBrowserTest
       case NetworkContextType::kSystem:
         return g_browser_process->system_network_context_manager()
             ->GetContext();
+      case NetworkContextType::kSafeBrowsing:
+        return g_browser_process->safe_browsing_service()->GetNetworkContext();
       case NetworkContextType::kProfile:
         return content::BrowserContext::GetDefaultStoragePartition(
                    browser()->profile())
@@ -209,6 +217,7 @@ class NetworkContextConfigurationBrowserTest
   StorageType GetHttpCacheType() const {
     switch (GetParam().network_context_type) {
       case NetworkContextType::kSystem:
+      case NetworkContextType::kSafeBrowsing:
         return StorageType::kNone;
       case NetworkContextType::kProfile:
         return StorageType::kDisk;
@@ -226,6 +235,7 @@ class NetworkContextConfigurationBrowserTest
     PrefService* pref_service = nullptr;
     switch (GetParam().network_context_type) {
       case NetworkContextType::kSystem:
+      case NetworkContextType::kSafeBrowsing:
         pref_service = g_browser_process->local_state();
         break;
       case NetworkContextType::kProfile:
@@ -247,6 +257,7 @@ class NetworkContextConfigurationBrowserTest
     // requests are sent on a separate pipe from ProxyConfigs.
     switch (GetParam().network_context_type) {
       case NetworkContextType::kSystem:
+      case NetworkContextType::kSafeBrowsing:
         g_browser_process->system_network_context_manager()
             ->FlushProxyConfigMonitorForTesting();
         break;
@@ -334,6 +345,10 @@ class NetworkContextConfigurationBrowserTest
         g_browser_process->system_network_context_manager()
             ->FlushNetworkInterfaceForTesting();
         break;
+      case NetworkContextType::kSafeBrowsing:
+        g_browser_process->safe_browsing_service()
+            ->FlushNetworkInterfaceForTesting();
+        break;
       case NetworkContextType::kProfile:
         content::BrowserContext::GetDefaultStoragePartition(
             browser()->profile())
@@ -604,10 +619,12 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest,
 
 IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest,
                        UserAgentAndLanguagePrefs) {
-  // System network context isn't associated with any profile, so changing the
-  // language settings in the default one doesn't affect what it sends.
+  // The system and SafeBrowsing network contexts aren't associated with any
+  // profile, so changing the language settings for the profile's main network
+  // context won't affect what they send.
   bool system =
-      (GetParam().network_context_type == NetworkContextType::kSystem);
+      (GetParam().network_context_type == NetworkContextType::kSystem ||
+       GetParam().network_context_type == NetworkContextType::kSafeBrowsing);
   const char kDefaultAcceptLanguage[] = "en-us,en";
 
   std::string accept_language, user_agent;
@@ -963,6 +980,11 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
                         TestCase({NetworkServiceState::kEnabled,           \
                                   NetworkContextType::kSystem})));         \
                                                                            \
+  INSTANTIATE_TEST_CASE_P(                                                 \
+      SafeBrowsingNetworkContext, TestFixture,                             \
+      ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
+                                  NetworkContextType::kSafeBrowsing})));   \
+                                                                           \
   INSTANTIATE_TEST_CASE_P(                                                 \
       ProfileMainNetworkContext, TestFixture,                              \
       ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
@@ -981,6 +1003,9 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
 // Instiates tests with a prefix indicating which NetworkContext is being
 // tested, and a suffix of "/0" if the network service is disabled, "/1" if it's
 // enabled, and "/2" if it's enabled and restarted.
+//
+// TODO(mmenke): Enabled tests for the SafeBrowsing NetworkContext, once it
+// works with the network service enabled.
 #define INSTANTIATE_TEST_CASES_FOR_TEST_FIXTURE(TestFixture)               \
   INSTANTIATE_TEST_CASE_P(                                                 \
       SystemNetworkContext, TestFixture,                                   \
@@ -991,6 +1016,11 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
                         TestCase({NetworkServiceState::kRestarted,         \
                                   NetworkContextType::kSystem})));         \
                                                                            \
+  INSTANTIATE_TEST_CASE_P(                                                 \
+      SafeBrowsingNetworkContext, TestFixture,                             \
+      ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
+                                  NetworkContextType::kSafeBrowsing})));   \
+                                                                           \
   INSTANTIATE_TEST_CASE_P(                                                 \
       ProfileMainNetworkContext, TestFixture,                              \
       ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -240,6 +240,11 @@ SafeBrowsingService::GetURLLoaderFactory() {
   return network_context_->GetURLLoaderFactory();
 }
 
+void SafeBrowsingService::FlushNetworkInterfaceForTesting() {
+  if (network_context_)
+    network_context_->FlushForTesting();
+}
+
 scoped_refptr<network::SharedURLLoaderFactory>
 SafeBrowsingService::GetURLLoaderFactoryOnIOThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);

chrome/browser/safe_browsing/safe_browsing_service.h

@@ -156,6 +156,9 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   network::mojom::NetworkContext* GetNetworkContext();
   virtual scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory();
 
+  // Flushes above two interfaces to avoid races in tests.
+  void FlushNetworkInterfaceForTesting();
+
   // Called to get a SharedURLLoaderFactory that can be used on the IO thread.
   scoped_refptr<network::SharedURLLoaderFactory>
   GetURLLoaderFactoryOnIOThread();

components/safe_browsing/browser/safe_browsing_network_context.cc

@@ -41,6 +41,13 @@ class SafeBrowsingNetworkContext::SharedURLLoaderFactory
     return network_context_.get();
   }
 
+  void FlushForTesting() {
+    if (network_context_)
+      network_context_.FlushForTesting();
+    if (url_loader_factory_)
+      url_loader_factory_.FlushForTesting();
+  }
+
  protected:
   // network::URLLoaderFactory implementation:
   void CreateLoaderAndStart(network::mojom::URLLoaderRequest loader,
@@ -150,6 +157,11 @@ SafeBrowsingNetworkContext::GetNetworkContext() {
   return url_loader_factory_->GetNetworkContext();
 }
 
+void SafeBrowsingNetworkContext::FlushForTesting() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  url_loader_factory_->FlushForTesting();
+}
+
 void SafeBrowsingNetworkContext::ServiceShuttingDown() {
   url_loader_factory_->Reset();
 }

components/safe_browsing/browser/safe_browsing_network_context.h

@@ -39,6 +39,9 @@ class SafeBrowsingNetworkContext {
   // Returns a NetworkContext.
   network::mojom::NetworkContext* GetNetworkContext();
 
+  // Flushes NetworkContext and URLLoaderFactory pipes.
+  void FlushForTesting();
+
   // Called at shutdown to ensure that the URLRequestContextGetter reference is
   // destroyed..
   void ServiceShuttingDown();

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

@@ -250,8 +250,19 @@
 -CertificateTransparencyBrowserTest.ProfileRequest
 -CertificateTransparencyBrowserTest.SystemRequest
 
-# Fail on Windows only
--ConditionalCacheCountingHelperBrowserTest.Count
+# SafeBrowsing doesn't yet set up its NetworkContext correctly when the network
+# service is enabled.
+# https://crbug.com/789640
+-SafeBrowsingNetworkContext/NetworkContextConfigurationBrowserTest.DiskCache/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationBrowserTest.DataURL/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationBrowserTest.FileURL/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationBrowserTest.ProxyConfig/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationDataPacBrowserTest.DataPac/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationFilePacBrowserTest.FilePac/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationFtpPacBrowserTest.FtpPac/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationHttpPacBrowserTest.HttpPac/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationHttpsStrippingPacBrowserTest.PacHttpsUrlStripping/0
+-SafeBrowsingNetworkContext/NetworkContextConfigurationProxyOnStartBrowserTest.TestInitialProxyConfig/0
 
 # NOTE: if adding an exclusion for an existing failure (e.g. additional test for
 # feature X that is already not working), please add it beside the existing