win: Explain the CreateFile() client-side pipe-opening loop

util/win/registration_protocol_win.cc

 // Copyright 2015 The Crashpad Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
 #include "util/win/registration_protocol_win.h"
 
 #include <windows.h>
 
 #include "base/logging.h"
 #include "util/win/scoped_handle.h"
 
 namespace crashpad {
 
 bool SendToCrashHandlerServer(const base::string16& pipe_name,
                               const crashpad::ClientToServerMessage& message,
                               crashpad::ServerToClientMessage* response) {
-  int tries = 0;
-  for (;;) {
+  // Retry CreateFile() in a loop. If the handler isn’t actively waiting in
+  // ConnectNamedPipe() on a pipe instance because it’s busy doing something
+  // else, CreateFile() will fail with ERROR_PIPE_BUSY. WaitNamedPipe() waits
+  // until a pipe instance is ready, but there’s no way to wait for this
+  // condition and atomically open the client side of the pipe in a single
+  // operation. CallNamedPipe() implements similar retry logic to this, also in
+  // user-mode code.

[scottmg, 2015/11/10 20:02:20]

You could maybe also note that this means we don't retry in order to
pseudo-synchronize startup -- if the server-side pipe hasn't been created yet,
we'll just fail and return false.

[Mark Mentovai, 2015/11/10 20:12:19]

scottmg wrote:
Good call! Updated.

+  for (int tries = 0;;) {
     ScopedFileHANDLE pipe(
         CreateFile(pipe_name.c_str(),
                    GENERIC_READ | GENERIC_WRITE,
                    0,
                    nullptr,
                    OPEN_EXISTING,
                    SECURITY_SQOS_PRESENT | SECURITY_IDENTIFICATION,
                    nullptr));
     if (!pipe.is_valid()) {
       if (++tries == 5 || GetLastError() != ERROR_PIPE_BUSY) {
@@ skipping 19 matching lines @@
     BOOL result = TransactNamedPipe(
         pipe.get(),
         // This is [in], but is incorrectly declared non-const.
         const_cast<crashpad::ClientToServerMessage*>(&message),
         sizeof(message),
         response,
         sizeof(*response),
         &bytes_read,
         nullptr);
     if (!result) {
+      PLOG(ERROR) << "TransactNamedPipe";
+      return false;
+    }
+    if (bytes_read != sizeof(*response)) {
       LOG(ERROR) << "TransactNamedPipe: expected " << sizeof(*response)

[scottmg, 2015/11/10 20:02:19]

Duh, oops.

                  << ", observed " << bytes_read;
       return false;
     }
-    if (bytes_read != sizeof(*response)) {
-      LOG(ERROR) << "TransactNamedPipe read incorrect number of bytes";
-      return false;
-    }
     return true;
   }
 }
 
 }  // namespace crashpad