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

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 28 matching lines @@
 #include "base/threading/thread.h"
 #include "base/process_util.h"
 #include "chrome/browser/nacl_host/nacl_broker_service_win.h"
 #include "native_client/src/trusted/service_runtime/win/debug_exception_handler.h"
 #endif
 
 using content::BrowserThread;
 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> {
  public:
   DebugContext()
       : can_send_start_msg_(false),
         child_process_host_(NULL) {
   }
 
   ~DebugContext() {
@@ skipping 169 matching lines @@
 };
 
 #if defined(OS_WIN)
 static bool RunningOnWOW64() {
   return (base::win::OSInfo::GetInstance()->wow64_status() ==
           base::win::OSInfo::WOW64_ENABLED);
 }
 #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(
       content::PROCESS_TYPE_NACL_LOADER, this));
   process_->SetName(WideToUTF16Hack(url));
 #if defined(OS_WIN)
   if (!RunningOnWOW64()) {
     debug_context_ = new DebugContext();
   }
 #endif
@@ skipping 16 matching lines @@
       LOG(ERROR) << "nacl::Close() failed";
     }
   }
   for (size_t i = 0; i < internal_->sockets_for_sel_ldr.size(); i++) {
     if (nacl::Close(internal_->sockets_for_sel_ldr[i]) != 0) {
       LOG(ERROR) << "nacl::Close() failed";
     }
   }
 
   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
 }
 
 // Attempt to ensure the IRT will be available when we need it, but don't wait.
 bool NaClBrowser::EnsureIrtAvailable() {
   if (IrtAvailable())
     return true;
 
@@ skipping 13 matching lines @@
 // This is called at browser startup.
 // static
 void NaClProcessHost::EarlyStartup() {
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   // Open the IRT file early to make sure that it isn't replaced out from
   // under us by autoupdate.
   NaClBrowser::GetInstance()->EnsureIrtAvailable();
 #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
   // one side through the browser to sel_ldr, socket pairs are created
   // in the browser and then passed to the renderer and sel_ldr.
   //
   // This is mainly for the benefit of Windows, where sockets cannot
   // be passed in messages, but are copied via DuplicateHandle().
   // This means the sandboxed renderer cannot send handles to the
   // browser process.
 
   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]);
     SetCloseOnExec(pair[1]);
   }
 
   // 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(
     const FilePath& nacl_gdb, CommandLine* line) {
   CommandLine* cmd_line = new CommandLine(nacl_gdb);
   // We can't use PrependWrapper because our parameters contain spaces.
   cmd_line->AppendArg("--eval-command");
   const FilePath::StringType& irt_path =
       NaClBrowser::GetInstance()->GetIrtFilePath().value();
   cmd_line->AppendArgNative(FILE_PATH_LITERAL("nacl-irt ") + irt_path);
@@ skipping 87 matching lines @@
 #elif defined(OS_POSIX)
   process_->Launch(nacl_loader_prefix.empty(),  // use_zygote
                    base::EnvironmentVector(),
                    cmd_line.release());
 #endif
 
   return true;
 }
 
 void NaClProcessHost::OnProcessLaunchedByBroker(base::ProcessHandle handle) {
+#if defined(OS_WIN)
+  process_launched_by_broker_ = true;
+#endif
   process_->SetHandle(handle);
   OnProcessLaunched();
 }
 
 void NaClProcessHost::OnProcessCrashed(int exit_code) {
   std::string message = base::StringPrintf(
       "NaCl process exited with status %i (0x%x)", exit_code, exit_code);
   LOG(ERROR) << message;
 }
 
@@ skipping 314 matching lines @@
   process_->Send(new NaClProcessMsg_Start(handles_for_sel_ldr));
 #endif
 
   internal_->sockets_for_sel_ldr.clear();
 }
 
 bool NaClProcessHost::OnMessageReceived(const IPC::Message& msg) {
   NOTREACHED() << "Invalid message with type = " << msg.type();
   return false;
 }