Mojo EDK: Clean shutdown for ScopedIPCSupport in tests

This properly supports clean shutdown in test processes which initialize the EDK, blocking ScopedIPCSupport destruction to wait on EDK shutdown. Fixes some rare races in child process exit which can cause premature message pipe breakage. BUG=666356 R=yzshen@chromium.org TBR=vabr@chromium.org (ICWYU) TBR=ben@chromium.org (toplevel ICWYU) Review-Url: https://codereview.chromium.org/2514093002 Cr-Commit-Position: refs/heads/master@{#433795}
Denger-Network · Nov 22, 2016 · cf1d7d0 · cf1d7d0
1 parent 64f23d2
commit cf1d7d0
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 58 deletions.
diff --git a/chrome/renderer/autofill/form_autocomplete_browsertest.cc b/chrome/renderer/autofill/form_autocomplete_browsertest.cc
@@ -4,6 +4,7 @@
 
 #include <tuple>
 
+#include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"

diff --git a/chrome/renderer/chrome_render_frame_observer_browsertest.cc b/chrome/renderer/chrome_render_frame_observer_browsertest.cc
@@ -6,6 +6,7 @@
 
 #include <tuple>
 
+#include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "chrome/test/base/chrome_render_view_test.h"
 #include "components/translate/content/common/translate.mojom.h"

diff --git a/components/password_manager/content/renderer/credential_manager_client_browsertest.cc b/components/password_manager/content/renderer/credential_manager_client_browsertest.cc
@@ -10,6 +10,7 @@
 #include <tuple>
 
 #include "base/location.h"
+#include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/public/renderer/render_frame.h"

diff --git a/ipc/ipc_mojo_bootstrap_unittest.cc b/ipc/ipc_mojo_bootstrap_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/base_paths.h"
 #include "base/files/file.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "ipc/ipc.mojom.h"

diff --git a/mojo/edk/test/run_all_unittests.cc b/mojo/edk/test/run_all_unittests.cc
@@ -43,11 +43,8 @@ int main(int argc, char** argv) {
 
   mojo::test::TestSupport::Init(new mojo::edk::test::TestSupportImpl());
   base::TestIOThread test_io_thread(base::TestIOThread::kAutoStart);
-  // Leak this because its destructor calls mojo::edk::ShutdownIPCSupport which
-  // really does nothing in the new EDK but does depend on the current message
-  // loop, which is destructed inside base::LaunchUnitTests.
-  new mojo::edk::test::ScopedIPCSupport(test_io_thread.task_runner());
 
+  mojo::edk::test::ScopedIPCSupport ipc_support(test_io_thread.task_runner());
   return base::LaunchUnitTests(
       argc, argv,
       base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));

diff --git a/mojo/edk/test/scoped_ipc_support.cc b/mojo/edk/test/scoped_ipc_support.cc
@@ -4,9 +4,10 @@
 
 #include "mojo/edk/test/scoped_ipc_support.h"
 
-#include <utility>
-
-#include "base/message_loop/message_loop.h"
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "mojo/edk/embedder/embedder.h"
 
 namespace mojo {
@@ -21,40 +22,39 @@ base::TaskRunner* GetIoTaskRunner() {
   return g_io_task_runner;
 }
 
-namespace internal {
-
-ScopedIPCSupportHelper::ScopedIPCSupportHelper() {
-}
-
-ScopedIPCSupportHelper::~ScopedIPCSupportHelper() {
-  ShutdownIPCSupport();
-  run_loop_.Run();
-}
-
-void ScopedIPCSupportHelper::Init(
-    ProcessDelegate* process_delegate,
-    scoped_refptr<base::TaskRunner> io_thread_task_runner) {
-  io_thread_task_runner_ = io_thread_task_runner;
-  InitIPCSupport(process_delegate, io_thread_task_runner_);
-}
-
-void ScopedIPCSupportHelper::OnShutdownCompleteImpl() {
-  run_loop_.Quit();
-}
-
-}  // namespace internal
-
 ScopedIPCSupport::ScopedIPCSupport(
-    scoped_refptr<base::TaskRunner> io_thread_task_runner) {
+    scoped_refptr<base::TaskRunner> io_thread_task_runner)
+    : shutdown_event_(base::WaitableEvent::ResetPolicy::MANUAL,
+                      base::WaitableEvent::InitialState::NOT_SIGNALED) {
   g_io_task_runner = io_thread_task_runner.get();
-  helper_.Init(this, std::move(io_thread_task_runner));
+  InitIPCSupport(this, io_thread_task_runner);
 }
 
 ScopedIPCSupport::~ScopedIPCSupport() {
+  // ShutdownIPCSupport always runs OnShutdownComplete on the current
+  // ThreadTaskRunnerHandle if set. Otherwise it's run on the IPC thread. We
+  // account for both possibilities here to avoid unnecessarily starting a new
+  // MessageLoop or blocking the existing one.
+  //
+  // TODO(rockot): Clean this up. ShutdownIPCSupport should probably always call
+  // call OnShutdownComplete from the IPC thread.
+  ShutdownIPCSupport();
+  if (base::ThreadTaskRunnerHandle::IsSet()) {
+    base::RunLoop run_loop;
+    shutdown_closure_ = base::Bind(IgnoreResult(&base::TaskRunner::PostTask),
+                                   base::ThreadTaskRunnerHandle::Get(),
+                                   FROM_HERE, run_loop.QuitClosure());
+    run_loop.Run();
+  } else {
+    shutdown_event_.Wait();
+  }
 }
 
 void ScopedIPCSupport::OnShutdownComplete() {
-  helper_.OnShutdownCompleteImpl();
+  if (!shutdown_closure_.is_null())
+    shutdown_closure_.Run();
+  else
+    shutdown_event_.Signal();
 }
 
 }  // namespace test

diff --git a/mojo/edk/test/scoped_ipc_support.h b/mojo/edk/test/scoped_ipc_support.h
@@ -7,39 +7,16 @@
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
-#include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/task_runner.h"
 #include "mojo/edk/embedder/process_delegate.h"
-#include "mojo/edk/embedder/scoped_platform_handle.h"
 
 namespace mojo {
 namespace edk {
 namespace test {
 
 base::TaskRunner* GetIoTaskRunner();
 
-namespace internal {
-
-class ScopedIPCSupportHelper {
- public:
-  ScopedIPCSupportHelper();
-  ~ScopedIPCSupportHelper();
-
-  void Init(ProcessDelegate* process_delegate,
-            scoped_refptr<base::TaskRunner> io_thread_task_runner);
-
-  void OnShutdownCompleteImpl();
-
- private:
-  scoped_refptr<base::TaskRunner> io_thread_task_runner_;
-
-  base::RunLoop run_loop_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedIPCSupportHelper);
-};
-
-}  // namespace internal
-
 // A simple class that calls |InitIPCSupport()| on construction and
 // |ShutdownIPCSupport()| on destruction.
 class ScopedIPCSupport : public ProcessDelegate {
@@ -53,7 +30,8 @@ class ScopedIPCSupport : public ProcessDelegate {
   // Note: Executed on the I/O thread.
   void OnShutdownComplete() override;
 
-  internal::ScopedIPCSupportHelper helper_;
+  base::Closure shutdown_closure_;
+  base::WaitableEvent shutdown_event_;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedIPCSupport);
 };