[Mac] Really make sure browser tests do not use a bundled child process executable.

The change in be983c8b was incomplete
because calls into SetUpBundleOverrides() would re-override the
CHILD_PROCESS_EXE path. This moves the SetUpBundleOverrides() into ChromeMain
rather than in various delegate methods, so that it is not called in tests.

This requires allowing the crashpad_handler to be started from the Framework
that is not bundled in a .app, so the rpath is adjusted in the component build.

Finally this enables network service browser_tests, which were previously
disabled on Mac, to verify the fix.

Bug: 877992
Test: browser_tests --gtest_filter=*NetworkContextConfigurationBrowserTest*
Change-Id: If88a8dd13b9b7b0500d08a180658597ae6f5997d
Reviewed-on: https://chromium-review.googlesource.com/1198306
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588516}

chrome/app/chrome_main.cc

@@ -88,12 +88,13 @@ int ChromeMain(int argc, const char** argv) {
   const base::CommandLine* command_line(base::CommandLine::ForCurrentProcess());
   ALLOW_UNUSED_LOCAL(command_line);
 
+#if defined(OS_MACOSX)
+  SetUpBundleOverrides();
+#endif
+
   // Chrome-specific process modes.
 #if defined(OS_LINUX) || defined(OS_MACOSX) || defined(OS_WIN)
   if (command_line->HasSwitch(switches::kHeadless)) {
-#if defined(OS_MACOSX)
-    SetUpBundleOverrides();
-#endif
     return headless::HeadlessShellMain(params);
   }
 #endif  // defined(OS_LINUX) || defined(OS_MACOSX) || defined(OS_WIN)

chrome/app/chrome_main_delegate.cc

@@ -535,7 +535,6 @@ bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {
   const bool is_browser = !command_line.HasSwitch(switches::kProcessType);
   ObjcEvilDoers::ZombieEnable(true, is_browser ? 10000 : 1000);
 
-  SetUpBundleOverrides();
   chrome::common::mac::EnableCFBundleBlocker();
 #endif
 
@@ -734,7 +733,7 @@ void ChromeMainDelegate::InitMacCrashReporter(
     CHECK(command_line.HasSwitch(switches::kProcessType) &&
           !process_type.empty())
         << "Helper application requires --type.";
-  } else {
+  } else if (base::mac::AmIBundled()) {
     CHECK(!command_line.HasSwitch(switches::kProcessType) &&
           process_type.empty())
         << "Main application forbids --type, saw " << process_type;
@@ -1107,10 +1106,6 @@ ui::DataPack* ChromeMainDelegate::LoadServiceManifestDataPack() {
       command_line.GetSwitchValueASCII(switches::kProcessType);
   DCHECK(process_type.empty());
 
-#if defined(OS_MACOSX)
-  SetUpBundleOverrides();
-#endif
-
   base::FilePath resources_pack_path;
   base::PathService::Get(chrome::FILE_RESOURCES_PACK, &resources_pack_path);
 

chrome/browser/net/chrome_network_service_browsertest.cc

@@ -77,11 +77,6 @@ class ChromeNetworkServiceBrowserTest : public InProcessBrowserTest {
 };
 
 IN_PROC_BROWSER_TEST_F(ChromeNetworkServiceBrowserTest, PRE_EncryptedCookies) {
-#if defined(OS_MACOSX)
-  // |NetworkServiceTestHelper| doesn't work on browser_tests on macOS:
-  // crbug.com/843324.
-  return;
-#endif
   // First set a cookie with cookie encryption enabled.
   network::mojom::NetworkContextPtr context =
       CreateNetworkContext(/*enable_encrypted_cookies=*/true);
@@ -106,11 +101,6 @@ IN_PROC_BROWSER_TEST_F(ChromeNetworkServiceBrowserTest, PRE_EncryptedCookies) {
 
 IN_PROC_BROWSER_TEST_F(ChromeNetworkServiceBrowserTest,
                        MAYBE_EncryptedCookies) {
-#if defined(OS_MACOSX)
-  // |NetworkServiceTestHelper| doesn't work on browser_tests on macOS:
-  // crbug.com/843324.
-  return;
-#endif
   net::CookieCryptoDelegate* crypto_delegate =
       cookie_config::GetCookieCryptoDelegate();
   std::string ciphertext;

chrome/browser/net/network_context_configuration_browsertest.cc

@@ -894,12 +894,6 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest, DiskCache) {
   }
 }
 
-// Test certs cannot currently be installed on OSX with the network service
-// enabled.
-// TODO(mmenke): Once that is fixed, enable this test on OSX.
-// See https://crbug.com/757088
-#if !defined(OS_MACOSX)
-
 // Visits a URL with an HSTS header, and makes sure it is respected.
 IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest, PRE_Hsts) {
   net::test_server::EmbeddedTestServer ssl_server(
@@ -1017,8 +1011,6 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest, Hsts) {
   }
 }
 
-#endif  // !defined(OS_MACOSX)
-
 // Check that the SSLConfig is hooked up. PRE_SSLConfig checks that changing
 // local_state() after start modifies the SSLConfig, SSLConfig makes sure the
 // (now modified) initial value of local_state() is respected.
@@ -1043,19 +1035,8 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest, PRE_SSLConfig) {
   simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
       loader_factory(), simple_loader_helper.GetCallback());
   simple_loader_helper.WaitForCallback();
-#if defined(OS_MACOSX)
-  // TODO(https://crbug.com/757088):  Test certs don't work on OSX, with the
-  // network service.
-  if (GetParam().network_service_state != NetworkServiceState::kDisabled) {
-    EXPECT_FALSE(simple_loader_helper.response_body());
-  } else {
-    ASSERT_TRUE(simple_loader_helper.response_body());
-    EXPECT_EQ(*simple_loader_helper.response_body(), "Echo");
-  }
-#else
   ASSERT_TRUE(simple_loader_helper.response_body());
   EXPECT_EQ(*simple_loader_helper.response_body(), "Echo");
-#endif
 
   // Disallow TLS 1.0 via prefs.
   g_browser_process->local_state()->SetString(prefs::kSSLVersionMin,
@@ -1523,15 +1504,6 @@ class NetworkContextConfigurationFilePacBrowserTest
 IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationFilePacBrowserTest, FilePac) {
   bool network_service_disabled =
       !base::FeatureList::IsEnabled(network::features::kNetworkService);
-#if defined(OS_MACOSX)
-  // https://crbug.com/843324: the NetworkServiceTestHelper does not work on Mac
-  // so the TestHostResolver is not used to resolve the host name when the
-  // network service is enabled. The system host resolver is used instead
-  // and goed to the network, which we don't want in tests. (and it does not
-  // return the expected net::ERR_NOT_IMPLEMENTED).
-  if (!network_service_disabled)
-    return;
-#endif
   // PAC file URLs are not supported with the network service
   TestProxyConfigured(/*expect_success=*/network_service_disabled);
 }
@@ -1712,56 +1684,6 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
   EXPECT_EQ(net::ERR_PROXY_CONNECTION_FAILED, simple_loader->NetError());
 }
 
-// |NetworkServiceTestHelper| doesn't work on browser_tests on OSX.
-// See https://crbug.com/843324
-#if defined(OS_MACOSX)
-
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-#define INSTANTIATE_EXTENSION_TESTS(TestFixture)                          \
-  INSTANTIATE_TEST_CASE_P(                                                \
-      OnDiskApp, TestFixture,                                             \
-      ::testing::Values(TestCase({NetworkServiceState::kDisabled,         \
-                                  NetworkContextType::kOnDiskApp})));     \
-                                                                          \
-  INSTANTIATE_TEST_CASE_P(                                                \
-      InMemoryApp, TestFixture,                                           \
-      ::testing::Values(TestCase({NetworkServiceState::kDisabled,         \
-                                  NetworkContextType::kInMemoryApp})));   \
-                                                                          \
-  INSTANTIATE_TEST_CASE_P(                                                \
-      OnDiskAppWithIncognitoProfile, TestFixture,                         \
-      ::testing::Values(                                                  \
-          TestCase({NetworkServiceState::kDisabled,                       \
-                    NetworkContextType::kOnDiskAppWithIncognitoProfile})));
-#else  // !BUILDFLAG(ENABLE_EXTENSIONS)
-#define INSTANTIATE_EXTENSION_TESTS(TestFixture)
-#endif  // !BUILDFLAG(ENABLE_EXTENSIONS)
-
-// Instantiates tests with a prefix indicating which NetworkContext is being
-// tested, and a suffix of "/0" if the network service is disabled and "/1" if
-// it's enabled.
-#define INSTANTIATE_TEST_CASES_FOR_TEST_FIXTURE(TestFixture)               \
-  INSTANTIATE_EXTENSION_TESTS(TestFixture)                                 \
-  INSTANTIATE_TEST_CASE_P(                                                 \
-      SystemNetworkContext, TestFixture,                                   \
-      ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
-                                  NetworkContextType::kSystem})));         \
-                                                                           \
-  INSTANTIATE_TEST_CASE_P(                                                 \
-      SafeBrowsingNetworkContext, TestFixture,                             \
-      ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
-                                  NetworkContextType::kSystem})));         \
-                                                                           \
-  INSTANTIATE_TEST_CASE_P(                                                 \
-      ProfileMainNetworkContext, TestFixture,                              \
-      ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
-                                  NetworkContextType::kProfile})));        \
-                                                                           \
-  INSTANTIATE_TEST_CASE_P(                                                 \
-      IncognitoProfileMainNetworkContext, TestFixture,                     \
-      ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
-                                  NetworkContextType::kIncognitoProfile})))
-#else  // !defined(OS_MACOSX)
 // 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.
@@ -1837,8 +1759,6 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
                         TestCase({NetworkServiceState::kRestarted,         \
                                   NetworkContextType::kIncognitoProfile})));
 
-#endif  // !defined(OS_MACOSX)
-
 INSTANTIATE_TEST_CASES_FOR_TEST_FIXTURE(NetworkContextConfigurationBrowserTest);
 INSTANTIATE_TEST_CASES_FOR_TEST_FIXTURE(
     NetworkContextConfigurationFixedPortBrowserTest);

chrome/browser/service_process/service_process_control_browsertest.cc

@@ -16,6 +16,7 @@
 #include "base/process/process_iterator.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/scoped_path_override.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -69,6 +70,22 @@ class ServiceProcessControlBrowserTest
   }
 
   void SetUp() override {
+#if defined(OS_MACOSX)
+    // browser_tests and the child processes run as standalone executables,
+    // rather than bundled apps. For this test, set up the CHILD_PROCESS_EXE to
+    // point to a bundle so that the service process has an Info.plist.
+    base::FilePath exe_path;
+    ASSERT_TRUE(base::PathService::Get(base::DIR_EXE, &exe_path));
+    exe_path = exe_path.DirName()
+                   .DirName()
+                   .Append("Contents")
+                   .Append("Versions")
+                   .Append(chrome::kChromeVersion)
+                   .Append(chrome::kHelperProcessExecutablePath);
+    child_process_exe_override_ = std::make_unique<base::ScopedPathOverride>(
+        content::CHILD_PROCESS_EXE, exe_path);
+#endif
+
     InProcessBrowserTest::SetUp();
 
     // This should not be needed because TearDown() ends with a closed
@@ -80,10 +97,14 @@ class ServiceProcessControlBrowserTest
   void TearDown() override {
     if (ServiceProcessControl::GetInstance()->IsConnected())
       EXPECT_TRUE(ServiceProcessControl::GetInstance()->Shutdown());
+
 #if defined(OS_MACOSX)
     // ForceServiceProcessShutdown removes the process from launched on Mac.
     ForceServiceProcessShutdown("", 0);
+
+    child_process_exe_override_.reset();
 #endif  // OS_MACOSX
+
     if (service_process_.IsValid()) {
       int exit_code;
       EXPECT_TRUE(service_process_.WaitForExitWithTimeout(
@@ -117,6 +138,9 @@ class ServiceProcessControlBrowserTest
   }
 
  private:
+#if defined(OS_MACOSX)
+  std::unique_ptr<base::ScopedPathOverride> child_process_exe_override_;
+#endif
   base::Process service_process_;
 };
 

third_party/crashpad/README.chromium

@@ -37,3 +37,4 @@ $ git am --3way --message-id -p4 /tmp/patchdir
 Local Modifications:
  - codereview.settings has been excluded.
  - thread_log_messages.cc (using ThreadLocalStorage::Slot instead of StaticSlot)
+ - Add an additional rpath for crashpad_handler on Mac in component builds.

third_party/crashpad/crashpad/handler/BUILD.gn

@@ -147,6 +147,11 @@ crashpad_executable("crashpad_handler") {
         # so set the rpath for that too.
         "-rpath",
         "@loader_path/../../../../../../..",
+
+        # The handler can also be executed in an unbundled framework at
+        # Chromium Framework.framework/Versions/V/Helpers/
+        "-rpath",
+        "@loader_path/../../../..",
       ]
     }
   }