Fixing an AttachmentBroker problem which broke the windows host

remoting/host/ipc_util_win.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/ipc_util.h"
 
 #include <utility>
 
 #include "base/files/file.h"
 #include "base/logging.h"
@@ skipping 41 matching lines @@
 
   // Create the server end of the channel.
   ScopedHandle pipe;
   if (!CreateIpcChannel(channel_name, security_descriptor, &pipe)) {
     return false;
   }
 
   // Wrap the pipe into an IPC channel.
   std::unique_ptr<IPC::ChannelProxy> server(
       new IPC::ChannelProxy(listener, io_task_runner));
-  IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
-      server.get(), io_task_runner);
+  IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
+  if (!broker) {
+    LOG(WARNING) << "No AttachmentBroker registered.";

[erikchen, 2016/04/28 23:12:50]

Every process should have a broker. When does this LOG statement get emitted?

[joedow, 2016/04/28 23:20:49]

It occurred after you made your last change and I had to spend more time than I
would have liked tracking it down.  With my change I suppose now the message
will fail (which should be logged) instead of crashing.  I like having the
warning because in our case, we were spinning up a process on a TS session and
there was no debug output as to why the connection was failing.  This is a big
clue which should save them time.

I'd like to leave it as this would have saved me time tracking this issue down.

[erikchen, 2016/04/28 23:24:54]

Would making this a DCHECK(broker) or even a CHECK(broker) satisfy your
constraint for making future problems easy to debug? The presence of a
conditional/log statement implies that this is a valid state for the system
[which it isn't].

[joedow, 2016/04/28 23:39:26]

DCHECK sounds legit.

+  } else if (broker->IsPrivilegedBroker()) {
+    broker->RegisterCommunicationChannel(server.get(), io_task_runner);
+  }
   server->Init(IPC::ChannelHandle(pipe.Get()), IPC::Channel::MODE_SERVER,
-                true);
+               /*create_pipe_now=*/true);
 
   // Convert the channel name to the pipe name.
   std::string pipe_name(kChromePipeNamePrefix);
   pipe_name.append(channel_name);
 
   SECURITY_ATTRIBUTES security_attributes = {0};
   security_attributes.nLength = sizeof(security_attributes);
   security_attributes.lpSecurityDescriptor = nullptr;
   security_attributes.bInheritHandle = TRUE;
 
@@ skipping 54 matching lines @@
     PLOG(ERROR)
         << "Failed to create the server end of the Chromoting IPC channel";
     return false;
   }
 
   *pipe_out = std::move(pipe);
   return true;
 }
 
 } // namespace remoting