Fix memory leak when errors happens in NaClProcessHost::Launch. Allow Launch to

chrome/browser/nacl_host/nacl_process_host.cc

Index: chrome/browser/nacl_host/nacl_process_host.cc
===================================================================
--- chrome/browser/nacl_host/nacl_process_host.cc	(revision 127497)
+++ chrome/browser/nacl_host/nacl_process_host.cc	(working copy)
@@ -46,6 +46,8 @@
 using content::ChildProcessData;
 using content::ChildProcessHost;
 
+static const char dummy[1<<16]="1";

[Mark Seaborn, 2012/03/21 20:41:29]

Left in by mistake?

[halyavin, 2012/03/22 08:35:47]

Sorry, I was doing an experiment in this working copy and forgot to revert the
change.

+
 #if defined(OS_WIN)
 class NaClProcessHost::DebugContext
   : public base::RefCountedThreadSafe<NaClProcessHost::DebugContext> {
@@ -235,7 +237,11 @@
 #endif
 
 NaClProcessHost::NaClProcessHost(const std::wstring& url)
-    : reply_msg_(NULL),
+    :
+#if defined(OS_WIN)
+      process_launched_by_broker_(false),
+#endif
+      reply_msg_(NULL),
       internal_(new NaClInternal()),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
   process_.reset(content::BrowserChildProcessHost::Create(
@@ -272,21 +278,13 @@
   }
 
   if (reply_msg_) {
-    // The process failed to launch for some reason.
-    // Don't keep the renderer hanging.
-    reply_msg_->set_reply_error();
-    chrome_render_message_filter_->Send(reply_msg_);
+    SendNaClLaunchError();
   }
 #if defined(OS_WIN)
-  if (RunningOnWOW64()) {
-    // If the nacl-gdb switch is not empty, the NaCl loader has been launched
-    // without the broker and so we shouldn't inform the broker about
-    // the loader termination.
-    if (CommandLine::ForCurrentProcess()->GetSwitchValuePath(
-            switches::kNaClGdb).empty()) {
-      NaClBrokerService::GetInstance()->OnLoaderDied();
-    }
-  } else {
+  if (process_launched_by_broker_) {
+    NaClBrokerService::GetInstance()->OnLoaderDied();
+  }
+  if (!RunningOnWOW64()) {

[Mark Seaborn, 2012/03/21 20:41:29]

On merging with HEAD this becomes "if (debug_context_ != NULL)"

[halyavin, 2012/03/22 08:35:47]

Done.

     debug_context_->SetChildProcessHost(NULL);
   }
 #endif
@@ -320,22 +318,36 @@
 #endif
 }
 
-bool NaClProcessHost::Launch(
+void NaClProcessHost::SendNaClLaunchError() {

[Mark Seaborn, 2012/03/21 20:41:29]

This method only has one call site, doesn't it?  You could just inline it.

[halyavin, 2012/03/22 08:35:47]

Done.

+  // The process failed to launch for some reason.
+  // Don't keep the renderer hanging.
+  reply_msg_->set_reply_error();
+  chrome_render_message_filter_->Send(reply_msg_);
+  reply_msg_ = NULL;
+  chrome_render_message_filter_ = NULL;
+}
+
+void NaClProcessHost::Launch(
     ChromeRenderMessageFilter* chrome_render_message_filter,
     int socket_count,
     IPC::Message* reply_msg) {
+  chrome_render_message_filter_ = chrome_render_message_filter;
+  reply_msg_ = reply_msg;
+
   // Place an arbitrary limit on the number of sockets to limit
   // exposure in case the renderer is compromised.  We can increase
   // this if necessary.
   if (socket_count > 8) {
-    return false;
+    delete this;
+    return;
   }
 
   // Start getting the IRT open asynchronously while we launch the NaCl process.
   // We'll make sure this actually finished in OnProcessLaunched, below.
   if (!NaClBrowser::GetInstance()->EnsureIrtAvailable()) {
     LOG(ERROR) << "Cannot launch NaCl process after IRT file open failed";
-    return false;
+    delete this;
+    return;
   }
 
   // Rather than creating a socket pair in the renderer, and passing
@@ -350,8 +362,10 @@
   for (int i = 0; i < socket_count; i++) {
     nacl::Handle pair[2];
     // Create a connected socket
-    if (nacl::SocketPair(pair) == -1)
-      return false;
+    if (nacl::SocketPair(pair) == -1) {
+      delete this;
+      return;
+    }
     internal_->sockets_for_renderer.push_back(pair[0]);
     internal_->sockets_for_sel_ldr.push_back(pair[1]);
     SetCloseOnExec(pair[0]);
@@ -360,14 +374,8 @@
 
   // Launch the process
   if (!LaunchSelLdr()) {
-    return false;
+    delete this;
   }
-
-  chrome_render_message_filter_ = chrome_render_message_filter;
-
-  // On success, we take responsibility for sending the reply.
-  reply_msg_ = reply_msg;
-  return true;
 }
 
 scoped_ptr<CommandLine> NaClProcessHost::LaunchWithNaClGdb(
@@ -475,6 +483,9 @@
 }
 
 void NaClProcessHost::OnProcessLaunchedByBroker(base::ProcessHandle handle) {
+#if defined(OS_WIN)
+  process_launched_by_broker_ = true;
+#endif
   process_->SetHandle(handle);
   OnProcessLaunched();
 }