Fix subtle bug with the in-process utility thread usage in unit tests.

Recently this started flaking. The problem was that the exit manager (or the shadowing one if it was there) would run before the in-process utility thread shutdown. Since the thread was holding on to g_one_utility_thread_lock, the exit manager would try to destroy that LazyInstance but it would fail because the lock was still owned by a different thread.

The fix is to ensure that the utility thread is gone. This is done by waiting for all UtilityProcessHost objects to get destructed. This wasn't happening before because the tests would just wait for their app-specific IPC from the utility thread, which is dispatched before the OnChannelError in ChildProcessHost ends up killing the UtilityProcessHost.

BUG=316919
R=scottmg@chromium.org

Review URL: https://codereview.chromium.org/128993004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243793 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc

@@ -27,7 +27,6 @@
 #include "components/policy/core/common/policy_types.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
-#include "content/public/browser/render_process_host.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/external_provider_interface.h"
@@ -113,6 +112,8 @@ class DeviceLocalAccountExternalPolicyLoaderTest : public testing::Test {
   scoped_refptr<DeviceLocalAccountExternalPolicyLoader> loader_;
   MockExternalPolicyProviderVisitor visitor_;
   scoped_ptr<extensions::ExternalProviderImpl> provider_;
+
+  content::InProcessUtilityThreadHelper in_process_utility_thread_helper_;
 };
 
 DeviceLocalAccountExternalPolicyLoaderTest::
@@ -143,13 +144,10 @@ void DeviceLocalAccountExternalPolicyLoaderTest::SetUp() {
       extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD,
       extensions::Extension::NO_FLAGS));
 
-  content::RenderProcessHost::SetRunRendererInProcess(true);
-
   VerifyAndResetVisitorCallExpectations();
 }
 
 void DeviceLocalAccountExternalPolicyLoaderTest::TearDown() {
-  content::RenderProcessHost::SetRunRendererInProcess(false);
   TestingBrowserProcess::GetGlobal()->SetSystemRequestContext(NULL);
 }
 

chrome/browser/extensions/extension_service_unittest.cc

@@ -611,11 +611,9 @@ void ExtensionServiceTestBase::SetUpTestCase() {
 
 void ExtensionServiceTestBase::SetUp() {
   ExtensionErrorReporter::GetInstance()->ClearErrors();
-  content::RenderProcessHost::SetRunRendererInProcess(true);
 }
 
 void ExtensionServiceTestBase::TearDown() {
-  content::RenderProcessHost::SetRunRendererInProcess(false);
 }
 
 ExtensionServiceTestBase::ExtensionServiceInitParams

chrome/browser/extensions/extension_service_unittest.h

@@ -13,6 +13,7 @@
 #include "base/message_loop/message_loop.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/common/feature_switch.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -94,6 +95,7 @@ class ExtensionServiceTestBase : public testing::Test {
   extensions::ManagementPolicy* management_policy_;
   scoped_ptr<ExtensionSyncService> extension_sync_service_;
   size_t expected_extensions_count_;
+  content::InProcessUtilityThreadHelper in_process_utility_thread_helper_;
 
 #if defined OS_CHROMEOS
   chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;

chrome/browser/extensions/sandboxed_unpacker_unittest.cc

@@ -12,7 +12,6 @@
 #include "base/values.h"
 #include "chrome/browser/extensions/sandboxed_unpacker.h"
 #include "chrome/common/chrome_paths.h"
-#include "content/public/browser/render_process_host.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/common/constants.h"
@@ -61,9 +60,10 @@ class SandboxedUnpackerTest : public testing::Test {
    ASSERT_TRUE(extensions_dir_.CreateUniqueTempDir());
     browser_threads_.reset(new content::TestBrowserThreadBundle(
         content::TestBrowserThreadBundle::IO_MAINLOOP));
+    in_process_utility_thread_helper_.reset(
+        new content::InProcessUtilityThreadHelper);
     // It will delete itself.
     client_ = new MockSandboxedUnpackerClient;
-    content::RenderProcessHost::SetRunRendererInProcess(true);
   }
 
   virtual void TearDown() {
@@ -71,7 +71,6 @@ class SandboxedUnpackerTest : public testing::Test {
     // it posts a task to it.
     sandboxed_unpacker_ = NULL;
     base::RunLoop().RunUntilIdle();
-    content::RenderProcessHost::SetRunRendererInProcess(false);
   }
 
   void SetupUnpacker(const std::string& crx_name) {
@@ -105,6 +104,8 @@ class SandboxedUnpackerTest : public testing::Test {
   MockSandboxedUnpackerClient* client_;
   scoped_refptr<SandboxedUnpacker> sandboxed_unpacker_;
   scoped_ptr<content::TestBrowserThreadBundle> browser_threads_;
+  scoped_ptr<content::InProcessUtilityThreadHelper>
+      in_process_utility_thread_helper_;
 };
 
 TEST_F(SandboxedUnpackerTest, NoCatalogsSuccess) {

chrome/browser/extensions/updater/extension_updater_unittest.cc

@@ -46,7 +46,6 @@
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
-#include "content/public/browser/render_process_host.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/common/extension.h"
@@ -494,7 +493,6 @@ class ExtensionUpdaterTest : public testing::Test {
 
   virtual void SetUp() OVERRIDE {
     prefs_.reset(new TestExtensionPrefs(base::MessageLoopProxy::current()));
-    content::RenderProcessHost::SetRunRendererInProcess(true);
   }
 
   virtual void TearDown() OVERRIDE {
@@ -503,7 +501,6 @@ class ExtensionUpdaterTest : public testing::Test {
     // those objects are released.
     RunUntilIdle();
     prefs_.reset();
-    content::RenderProcessHost::SetRunRendererInProcess(false);
   }
 
   void RunUntilIdle() {
@@ -1393,6 +1390,7 @@ class ExtensionUpdaterTest : public testing::Test {
 
  private:
   content::TestBrowserThreadBundle thread_bundle_;
+  content::InProcessUtilityThreadHelper in_process_utility_thread_helper_;
 
 #if defined OS_CHROMEOS
   chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;

chrome/browser/safe_browsing/download_protection_service_unittest.cc

@@ -25,9 +25,9 @@
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/signature_util.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
-#include "content/public/browser/render_process_host.h"
 #include "content/public/test/mock_download_item.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
 #include "net/cert/x509_certificate.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/test_url_fetcher_factory.h"
@@ -157,7 +157,6 @@ class DownloadProtectionServiceTest : public testing::Test {
             content::TestBrowserThreadBundle::IO_MAINLOOP) {
   }
   virtual void SetUp() {
-    content::RenderProcessHost::SetRunRendererInProcess(true);
     // Start real threads for the IO and File threads so that the DCHECKs
     // to test that we're on the correct thread work.
     sb_service_ = new StrictMock<FakeSafeBrowsingService>();
@@ -185,7 +184,6 @@ class DownloadProtectionServiceTest : public testing::Test {
     // tasks currently running.
     FlushThreadMessageLoops();
     sb_service_ = NULL;
-    content::RenderProcessHost::SetRunRendererInProcess(false);
   }
 
   bool RequestContainsResource(const ClientDownloadRequest& request,
@@ -318,6 +316,7 @@ class DownloadProtectionServiceTest : public testing::Test {
   DownloadProtectionService::DownloadCheckResult result_;
   bool has_result_;
   content::TestBrowserThreadBundle test_browser_thread_bundle_;
+  content::InProcessUtilityThreadHelper in_process_utility_thread_helper_;
   base::FilePath testdata_path_;
 };
 

content/public/test/test_utils.cc

@@ -9,8 +9,11 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "content/public/browser/browser_child_process_host_iterator.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/common/process_type.h"
 #include "content/public/test/test_launcher.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -242,4 +245,35 @@ void WindowedNotificationObserver::Observe(
   running_ = false;
 }
 
+InProcessUtilityThreadHelper::InProcessUtilityThreadHelper()
+    : child_thread_count_(0) {
+  RenderProcessHost::SetRunRendererInProcess(true);
+  BrowserChildProcessObserver::Add(this);
+}
+
+InProcessUtilityThreadHelper::~InProcessUtilityThreadHelper() {
+  if (child_thread_count_) {
+    DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::UI));
+    DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::IO));
+    runner_ = new MessageLoopRunner;
+    runner_->Run();
+  }
+  BrowserChildProcessObserver::Remove(this);
+  RenderProcessHost::SetRunRendererInProcess(false);
+}
+
+void InProcessUtilityThreadHelper::BrowserChildProcessHostConnected(
+    const ChildProcessData& data) {
+  child_thread_count_++;
+}
+
+void InProcessUtilityThreadHelper::BrowserChildProcessHostDisconnected(
+    const ChildProcessData& data) {
+  if (--child_thread_count_)
+    return;
+
+  if (runner_.get())
+    runner_->Quit();
+}
+
 }  // namespace content

content/public/test/test_utils.h

@@ -9,6 +9,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
+#include "content/public/browser/browser_child_process_observer.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_observer.h"
@@ -184,6 +185,34 @@ class WindowedNotificationObserver : public NotificationObserver {
   DISALLOW_COPY_AND_ASSIGN(WindowedNotificationObserver);
 };
 
+// Unit tests can use code which runs in the utility process by having it run on
+// an in-process utility thread. This eliminates having two code paths in
+// production code to deal with unit tests, and also helps with the binary
+// separation on Windows since chrome.dll doesn't need to call into Blink code
+// for some utility code to handle the single process case.
+// Include this class as a member variable in your test harness if you take
+// advantage of this functionality to ensure that the in-process utility thread
+// is torn down correctly. See http://crbug.com/316919 for more information.
+// Note: this class should be declared after the TestBrowserThreadBundle and
+// ShadowingAtExitManager (if it exists) as it will need to be run before they
+// are torn down.
+class InProcessUtilityThreadHelper : public BrowserChildProcessObserver {
+ public:
+  InProcessUtilityThreadHelper();
+  virtual ~InProcessUtilityThreadHelper();
+
+ private:
+  virtual void BrowserChildProcessHostConnected(
+      const ChildProcessData& data) OVERRIDE;
+  virtual void BrowserChildProcessHostDisconnected(
+      const ChildProcessData& data) OVERRIDE;
+
+  int child_thread_count_;
+  scoped_refptr<MessageLoopRunner> runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(InProcessUtilityThreadHelper);
+};
+
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_TEST_TEST_UTILS_H_