Test that debug stub works with browser.

chrome/browser/nacl_host/nacl_process_host.cc

 // Copyright (c) 2012 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 "chrome/browser/nacl_host/nacl_process_host.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 638 matching lines @@
   reply_msg_ = NULL;
   internal_->sockets_for_renderer.clear();
   return true;
 }
 
 // TCP port we chose for NaCl debug stub. It can be any other number.
 static const int kDebugStubPort = 4014;
 
 #if defined(OS_POSIX)
 SocketDescriptor NaClProcessHost::GetDebugStubSocketHandle() {
-  SocketDescriptor s = net::TCPListenSocket::CreateAndBind("127.0.0.1",
-                                                           kDebugStubPort);
+  NaClBrowser* nacl_browser = NaClBrowser::GetInstance();

[Mark Seaborn, 2012/10/23 01:14:50]

Instead of adding HasDebugStubPortListener() etc., could you just add global
function pointer and do something like this here:

  if (g_replacement_socket_binder_for_testing)
    return g_replacement_socket_binder_for_testing()

The new code in nacl_browser.{h,cc} seems like overkill.

[halyavin, 2012/10/23 11:59:29]

Test lives in an object. A function can't call an object unless I introduce one
more global variable. Also global variables introduce inter-test dependencies.
Another test can fail, if previous test forgot to clear some global variable. So
I don't think this is a good idea.

[Mark Seaborn, 2012/12/04 21:46:42]

NaClBrowser::GetInstance() returns a global singleton argument, so the problem
still applies.  But maybe scoping the globals inside an object, as you've done,
is cleaner.

+  SocketDescriptor s;
+  LOG(INFO) << "Create debug stub socket";
+  if (nacl_browser->HasDebugStubPortListener()) {
+    LOG(INFO) << "allocate ephemeral port";

[Mark Seaborn, 2012/10/23 01:14:50]

These are too many LOG(INFO)s to commit.

[halyavin, 2012/10/23 11:59:29]

I inserted them for debugging and forgot to remove after upload. Done.

+    s = net::TCPListenSocket::CreateAndBind("127.0.0.1", 0);
+    int port = net::TCPListenSocket::GetBindedPort(s);
+    LOG(INFO) << "port number=" << port;
+    if (port < 0) {
+      LOG(ERROR) << "failed to open ephemeral server port for debug stub";
+      close(s);
+      return net::TCPListenSocket::kInvalidSocket;
+    }
+    nacl_browser->DebugStubNewPortAllocated(port);
+  } else {
+    s = net::TCPListenSocket::CreateAndBind("127.0.0.1", kDebugStubPort);
+  }
   if (listen(s, 1)) {
     LOG(ERROR) << "listen() failed on debug stub socket";
+    close(s);

[Mark Seaborn, 2012/10/23 01:14:50]

This is fixing an existing leak, right?  A separate change would be better, but
please at least note the fix in the commit message.

[halyavin, 2012/10/23 11:59:29]

Done.

     return net::TCPListenSocket::kInvalidSocket;
   }
   return s;
 }
 #endif
 
 bool NaClProcessHost::StartNaClExecution() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
   nacl::NaClStartParams params;
@@ skipping 214 matching lines @@
   } else {
     NaClStartDebugExceptionHandlerThread(
         process_handle.Take(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif