Add OnChannelError() handler to QuitListener rather than hang on close.

Without an OnChannelError() handler on the QuitListener, the test can
hang if the IPC channel disconnects before the Quit message is received.
Adding an explicit handler will make it easier to diagnose this failure
mode.

Also migrates DCHECK()s in some existing OnChannelError() handlers in
tests to use CHECK(), to avoid those tests potentially hanging in
Release builds.

Bug: 816606, 816620, 764015
Change-Id: Ibe6ffc7ecd02f6029c74a7b3c1190030b656056f
Reviewed-on: https://chromium-review.googlesource.com/936427Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539262}

ipc/ipc_channel_mojo_unittest.cc

@@ -84,7 +84,7 @@ class ListenerThatExpectsOK : public IPC::Listener {
     // The connection should be healthy while the listener is waiting
     // message.  An error can occur after that because the peer
     // process dies.
-    DCHECK(received_ok_);
+    CHECK(received_ok_);
   }
 
   static void SendOK(IPC::Sender* sender) { SendString(sender, "OK"); }
@@ -544,9 +544,7 @@ class ListenerWithSimpleAssociatedInterface
     return true;
   }
 
-  void OnChannelError() override {
-    DCHECK(received_quit_);
-  }
+  void OnChannelError() override { CHECK(received_quit_); }
 
   void RegisterInterfaceFactory(IPC::Channel* channel) {
     channel->GetAssociatedInterfaceSupport()->AddAssociatedInterface(
@@ -729,9 +727,7 @@ class ListenerWithSimpleProxyAssociatedInterface
     return true;
   }
 
-  void OnChannelError() override {
-    DCHECK(received_quit_);
-  }
+  void OnChannelError() override { CHECK(received_quit_); }
 
   void OnAssociatedInterfaceRequest(
       const std::string& interface_name,

ipc/ipc_channel_proxy_unittest.cc

@@ -49,9 +49,16 @@ namespace IPC {
 
 namespace {
 
+void CreateRunLoopAndRun(base::RunLoop** run_loop_ptr) {
+  base::RunLoop run_loop;
+  *run_loop_ptr = &run_loop;
+  run_loop.Run();
+  *run_loop_ptr = nullptr;
+}
+
 class QuitListener : public IPC::Listener {
  public:
-  QuitListener() : bad_message_received_(false) {}
+  QuitListener() = default;
 
   bool OnMessageReceived(const IPC::Message& message) override {
     IPC_BEGIN_MESSAGE_MAP(QuitListener, message)
@@ -65,19 +72,26 @@ class QuitListener : public IPC::Listener {
     bad_message_received_ = true;
   }
 
-  void OnQuit() { base::RunLoop::QuitCurrentWhenIdleDeprecated(); }
+  void OnChannelError() override { CHECK(quit_message_received_); }
+
+  void OnQuit() {
+    quit_message_received_ = true;
+    run_loop_->QuitWhenIdle();
+  }
 
   void OnBadMessage(const BadType& bad_type) {
     // Should never be called since IPC wouldn't be deserialized correctly.
     CHECK(false);
   }
 
-  bool bad_message_received_;
+  bool bad_message_received_ = false;
+  bool quit_message_received_ = false;
+  base::RunLoop* run_loop_ = nullptr;
 };
 
 class ChannelReflectorListener : public IPC::Listener {
  public:
-  ChannelReflectorListener() : channel_(NULL) {}
+  ChannelReflectorListener() = default;
 
   void Init(IPC::Channel* channel) {
     DCHECK(!channel_);
@@ -111,11 +125,13 @@ class ChannelReflectorListener : public IPC::Listener {
 
   void OnQuit() {
     channel_->Send(new WorkerMsg_Quit());
-    base::RunLoop::QuitCurrentWhenIdleDeprecated();
+    run_loop_->QuitWhenIdle();
   }
 
+  base::RunLoop* run_loop_ = nullptr;
+
  private:
-  IPC::Channel* channel_;
+  IPC::Channel* channel_ = nullptr;
 };
 
 class MessageCountFilter : public IPC::MessageFilter {
@@ -128,19 +144,11 @@ class MessageCountFilter : public IPC::MessageFilter {
     CHANNEL_CLOSING,
     FILTER_REMOVED
   };
-  MessageCountFilter()
-      : messages_received_(0),
-        supported_message_class_(0),
-        is_global_filter_(true),
-        last_filter_event_(NONE),
-        message_filtering_enabled_(false) {}
 
+  MessageCountFilter() = default;
   MessageCountFilter(uint32_t supported_message_class)
-      : messages_received_(0),
-        supported_message_class_(supported_message_class),
-        is_global_filter_(false),
-        last_filter_event_(NONE),
-        message_filtering_enabled_(false) {}
+      : supported_message_class_(supported_message_class),
+        is_global_filter_(false) {}
 
   void OnFilterAdded(IPC::Channel* channel) override {
     EXPECT_TRUE(channel);
@@ -221,12 +229,12 @@ class MessageCountFilter : public IPC::MessageFilter {
  private:
   ~MessageCountFilter() override = default;
 
-  size_t messages_received_;
-  uint32_t supported_message_class_;
-  bool is_global_filter_;
+  size_t messages_received_ = 0;
+  uint32_t supported_message_class_ = 0;
+  bool is_global_filter_ = true;
 
-  FilterEvent last_filter_event_;
-  bool message_filtering_enabled_;
+  FilterEvent last_filter_event_ = NONE;
+  bool message_filtering_enabled_ = false;
 };
 
 class IPCChannelProxyTest : public IPCChannelMojoTestBase {
@@ -259,7 +267,7 @@ class IPCChannelProxyTest : public IPCChannelMojoTestBase {
 
   void SendQuitMessageAndWaitForIdle() {
     sender()->Send(new WorkerMsg_Quit);
-    base::RunLoop().Run();
+    CreateRunLoopAndRun(&listener_->run_loop_);
     EXPECT_TRUE(WaitForClientShutdown());
   }
 
@@ -398,7 +406,7 @@ class IPCChannelBadMessageTest : public IPCChannelMojoTestBase {
 
   void SendQuitMessageAndWaitForIdle() {
     sender()->Send(new WorkerMsg_Quit);
-    base::RunLoop().Run();
+    CreateRunLoopAndRun(&listener_->run_loop_);
     EXPECT_TRUE(WaitForClientShutdown());
   }
 
@@ -424,7 +432,8 @@ DEFINE_IPC_CHANNEL_MOJO_TEST_CLIENT(ChannelProxyClient) {
   Connect(&listener);
   listener.Init(channel());
 
-  base::RunLoop().Run();
+  CreateRunLoopAndRun(&listener.run_loop_);
+
   Close();
 }