Fix Mojo broker crash on Windows.

This occurred because a ChildBrokerHost noticed that its connection to the child process died and so it nulled out its channel. On POSIX, this leads to the destruction of the ChildBrokerHost immediately and so the singleton BrokerState is notified synchronously. On Windows, there's another channel for sandboxing related sync IPCs which uses IO completition ports. To ease lifetime issues because of that, the ChildBrokerHost is only deleted in response to errors from that second channel. So if the first channel dies first, it is possible that BrokerState tries to connect to a message pipe in that ChildBrokerState with a null channel.

The fix is to null check the channel and to inform the other side of the message pipe that its peer has died so that it can fire peer closed notifications.

BUG=573244

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

Cr-Commit-Position: refs/heads/master@{#367194}

6 files changed, 80 insertions, 17 deletions

diff --git a/mojo/edk/system/broker_messages.h b/mojo/edk/system/broker_messages.h
index 0ce0960..13fe980 100644
--- a/mojo/edk/system/broker_messages.h
+++ b/mojo/edk/system/broker_messages.h
@@ -53,12 +53,31 @@ const uint64_t kBrokerRouteId = 1;
 // They are sent over RawChannel.
 enum MultiplexMessages {
   // Messages from child to parent.
+
+  // Tells the parent that the given pipe id has been bound to a
+  // MessagePipeDispatcher in the child process. The parent will then respond
+  // with either PEER_PIPE_CONNECTED or PEER_DIED when the other side is also
+  // bound.
   CONNECT_MESSAGE_PIPE = 0,
+  // Tells the parent to remove its bookkeeping for the given peer id since
+  // another MessagePipeDispatcher has connected to the pipe in the same
+  // process.
   CANCEL_CONNECT_MESSAGE_PIPE,
 
+
   // Messages from parent to child.
+
+  // Tells the child to open a channel to a given process. This will be followed
+  // by a PEER_PIPE_CONNECTED connecting a message pipe from the child process
+  // to the given process over the new channel.
   CONNECT_TO_PROCESS,
+
+  // Connect a given message pipe to another process.
   PEER_PIPE_CONNECTED,
+
+  // Informs the child that the other end of the message pipe is in a process
+  // that died.
+  PEER_DIED,
 };
 
 struct ConnectMessagePipeMessage {
@@ -79,6 +98,11 @@ struct PeerPipeConnectedMessage {
   base::ProcessId process_id;
 };
 
+struct PeerDiedMessage {
+  MultiplexMessages type;  // PEER_DIED
+  uint64_t pipe_id;
+};
+
 }  // namespace edk
 }  // namespace mojo
 
diff --git a/mojo/edk/system/broker_state.cc b/mojo/edk/system/broker_state.cc
index 90ea00d..a81f1a1 100644
--- a/mojo/edk/system/broker_state.cc
+++ b/mojo/edk/system/broker_state.cc
@@ -93,8 +93,13 @@ void BrokerState::ConnectMessagePipe(uint64_t pipe_id,
   if (pending_child_connects_.find(pipe_id) != pending_child_connects_.end()) {
     // A child process has already tried to connect.
     ChildBrokerHost* child_host = pending_child_connects_[pipe_id];
-    AttachMessagePipe(message_pipe, pipe_id, child_host->channel());
-    child_host->ConnectMessagePipe(pipe_id, 0);
+    if (child_host && child_host->channel()) {
+      AttachMessagePipe(message_pipe, pipe_id, child_host->channel());
+      child_host->ConnectMessagePipe(pipe_id, 0);
+    } else {
+      message_pipe->OnError(RawChannel::Delegate::ERROR_READ_SHUTDOWN);
+    }
+
     pending_child_connects_.erase(pipe_id);
     return;
   }
@@ -124,14 +129,12 @@ void BrokerState::ChildBrokerHostDestructed(
   base::AutoLock auto_lock(lock_);
 
   for (auto it = pending_child_connects_.begin();
-       it != pending_child_connects_.end();) {
+       it != pending_child_connects_.end(); ++it) {
     if (it->second == child_broker_host) {
-      // Since we can't do it = pending_child_connects_.erase(it); until
-      // hash_map uses unordered_map on posix.
-      auto cur = it++;
-      pending_child_connects_.erase(cur);
-    } else {
-      it++;
+      // Signify that the process has died. When another process tries to
+      // connect to the message pipe, we will tell it that the peer has died so
+      // that it can fire a peer closed notification.
+      it->second = nullptr;
     }
   }
 
@@ -139,8 +142,8 @@ void BrokerState::ChildBrokerHostDestructed(
   for (auto it = connected_processes_.begin();
        it != connected_processes_.end();) {
     if ((*it).first == pid || (*it).second == pid) {
-      // Since we can't do it = pending_child_connects_.erase(it); until
-      // hash_map uses unordered_map on posix.
+      // Since we can't do it = connected_processes_.erase(it); until hash_map
+      // uses unordered_map on posix.
       auto cur = it++;
       connected_processes_.erase(cur);
     } else {
@@ -159,12 +162,16 @@ void BrokerState::HandleConnectMessagePipe(ChildBrokerHost* pipe_process,
   if (pending_child_connects_.find(pipe_id) != pending_child_connects_.end()) {
     // Another child process is waiting to connect to the given pipe.
     ChildBrokerHost* pending_pipe_process = pending_child_connects_[pipe_id];
-    EnsureProcessesConnected(pipe_process->GetProcessId(),
-                             pending_pipe_process->GetProcessId());
-    pending_pipe_process->ConnectMessagePipe(
-        pipe_id, pipe_process->GetProcessId());
-    pipe_process->ConnectMessagePipe(
-        pipe_id, pending_pipe_process->GetProcessId());
+    if (pending_pipe_process && pending_pipe_process->channel()) {
+      EnsureProcessesConnected(pipe_process->GetProcessId(),
+                               pending_pipe_process->GetProcessId());
+      pending_pipe_process->ConnectMessagePipe(
+          pipe_id, pipe_process->GetProcessId());
+      pipe_process->ConnectMessagePipe(
+          pipe_id, pending_pipe_process->GetProcessId());
+    } else {
+      pipe_process->PeerDied(pipe_id);
+    }
     pending_child_connects_.erase(pipe_id);
     return;
   }
diff --git a/mojo/edk/system/child_broker.cc b/mojo/edk/system/child_broker.cc
index 6903c82..85d9e52 100644
--- a/mojo/edk/system/child_broker.cc
+++ b/mojo/edk/system/child_broker.cc
@@ -220,6 +220,17 @@ void ChildBroker::OnReadMessage(
       CHECK(connected_pipes_.find(pipe) == connected_pipes_.end());
       AttachMessagePipe(pipe, pipe_id, channels_[peer_pid]);
     }
+  } else if (type == PEER_DIED) {
+    DCHECK(!platform_handles);
+    const PeerDiedMessage* message =
+        static_cast<const PeerDiedMessage*>(message_view.bytes());
+
+    uint64_t pipe_id = message->pipe_id;
+
+    CHECK(pending_connects_.find(pipe_id) != pending_connects_.end());
+    MessagePipeDispatcher* pipe = pending_connects_[pipe_id];
+    pending_connects_.erase(pipe_id);
+    pipe->OnError(ERROR_READ_SHUTDOWN);
   } else {
     NOTREACHED();
   }
diff --git a/mojo/edk/system/child_broker_host.cc b/mojo/edk/system/child_broker_host.cc
index 20a73d9..bf594c1 100644
--- a/mojo/edk/system/child_broker_host.cc
+++ b/mojo/edk/system/child_broker_host.cc
@@ -105,6 +105,19 @@ void ChildBrokerHost::ConnectMessagePipe(uint64_t pipe_id,
   child_channel_->channel()->WriteMessage(std::move(message));
 }
 
+void ChildBrokerHost::PeerDied(uint64_t pipe_id) {
+  if (!child_channel_)
+    return;  // Can happen at process shutdown on Windows.
+  PeerDiedMessage data;
+  memset(&data, 0, sizeof(data));
+  data.type = PEER_DIED;
+  data.pipe_id = pipe_id;
+  scoped_ptr<MessageInTransit> message(new MessageInTransit(
+      MessageInTransit::Type::MESSAGE, sizeof(data), &data));
+  message->set_route_id(kBrokerRouteId);
+  child_channel_->channel()->WriteMessage(std::move(message));
+}
+
 ChildBrokerHost::~ChildBrokerHost() {
   DCHECK(internal::g_io_thread_task_runner->RunsTasksOnCurrentThread());
   BrokerState::GetInstance()->ChildBrokerHostDestructed(this);
diff --git a/mojo/edk/system/child_broker_host.h b/mojo/edk/system/child_broker_host.h
index 943ff13..057b9e0 100644
--- a/mojo/edk/system/child_broker_host.h
+++ b/mojo/edk/system/child_broker_host.h
@@ -48,6 +48,10 @@ class MOJO_SYSTEM_IMPL_EXPORT ChildBrokerHost
   // be 0.
   void ConnectMessagePipe(uint64_t pipe_id, base::ProcessId process_id);
 
+  // Sends a message to the child process informing it that the peer process has
+  // died before it could connect.
+  void PeerDied(uint64_t pipe_id);
+
   RoutedRawChannel* channel() { return child_channel_; }
 
  private:
diff --git a/mojo/edk/system/message_pipe_dispatcher.cc b/mojo/edk/system/message_pipe_dispatcher.cc
index 119124a..11dc586 100644
--- a/mojo/edk/system/message_pipe_dispatcher.cc
+++ b/mojo/edk/system/message_pipe_dispatcher.cc
@@ -986,6 +986,10 @@ void MessagePipeDispatcher::OnError(Error error) {
         // Balance AddRef in CloseOnIO.
         call_release = true;
       }
+    } else if (!channel_ && !transferable_ &&
+               non_transferable_state_ == WAITING_FOR_CONNECT_TO_CLOSE) {
+      // Balance AddRef in CloseOnIO.
+      call_release = true;
     }
     awakable_list_.AwakeForStateChange(GetHandleSignalsStateImplNoLock());
     started_transport_.Release();