Show more different NaCl loading errors.

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 <algorithm>
 #include <string>
 #include <vector>
 
@@ skipping 271 matching lines @@
     const base::FilePath& manifest_path) {
   nacl_host_message_filter_ = nacl_host_message_filter;
   reply_msg_ = reply_msg;
   manifest_path_ = manifest_path;
 
   // Start getting the IRT open asynchronously while we launch the NaCl process.
   // We'll make sure this actually finished in StartWithLaunchedProcess, below.
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   nacl_browser->EnsureAllResourcesAvailable();
   if (!nacl_browser->IsOk()) {
-    LOG(ERROR) << "NaCl process launch failed: could not find all the "
-        "resources needed to launch the process";
+    SendErrorToRenderer("could not find all the resources needed"
+                        " to launch the process");
     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.
 
   NaClHandle pair[2];
   // Create a connected socket
   if (NaClSocketPair(pair) == -1) {
-    LOG(ERROR) << "NaCl process launch failed: could not create a socket pair";
+    SendErrorToRenderer("could not create a socket pair");
     delete this;
     return;
   }
   internal_->socket_for_renderer = pair[0];
   internal_->socket_for_sel_ldr = pair[1];
   SetCloseOnExec(pair[0]);
   SetCloseOnExec(pair[1]);
 
   // Launch the process
   if (!LaunchSelLdr()) {
@@ skipping 65 matching lines @@
   if (!script.empty()) {
     cmd_line.AppendArg("--command");
     cmd_line.AppendArgNative(script.value());
   }
   return base::LaunchProcess(cmd_line, base::LaunchOptions(), NULL);
 }
 
 bool NaClProcessHost::LaunchSelLdr() {
   std::string channel_id = process_->GetHost()->CreateChannel();
   if (channel_id.empty()) {
-    LOG(ERROR) << "NaCl process launch failed: could not create channel";
+    SendErrorToRenderer("could not create channel");
     return false;
   }
 
   CommandLine::StringType nacl_loader_prefix;
 #if defined(OS_POSIX)
   nacl_loader_prefix = CommandLine::ForCurrentProcess()->GetSwitchValueNative(
       switches::kNaClLoaderCmdPrefix);
 #endif  // defined(OS_POSIX)
 
   // Build command line for nacl.
@@ skipping 14 matching lines @@
 #endif
 
   base::FilePath exe_path = ChildProcessHost::GetChildPath(flags);
   if (exe_path.empty())
     return false;
 
 #if defined(OS_WIN)
   // On Windows 64-bit NaCl loader is called nacl64.exe instead of chrome.exe
   if (RunningOnWOW64()) {
     if (!NaClBrowser::GetInstance()->GetNaCl64ExePath(&exe_path)) {
+      SendErrorToRenderer("could not resolve module");
       return false;
     }
   }
 #endif
 
   scoped_ptr<CommandLine> cmd_line(new CommandLine(exe_path));
   nacl::CopyNaClCommandLineArguments(cmd_line.get());
 
   cmd_line->AppendSwitchASCII(switches::kProcessType,
                               switches::kNaClLoaderProcess);
   cmd_line->AppendSwitchASCII(switches::kProcessChannelID, channel_id);
   if (NaClBrowser::GetDelegate()->DialogsAreSuppressed())
     cmd_line->AppendSwitch(switches::kNoErrorDialogs);
 
   if (!nacl_loader_prefix.empty())
     cmd_line->PrependWrapper(nacl_loader_prefix);
 
   // On Windows we might need to start the broker process to launch a new loader
 #if defined(OS_WIN)
   if (RunningOnWOW64()) {
     if (!NaClBrokerService::GetInstance()->LaunchLoader(
             weak_factory_.GetWeakPtr(), channel_id)) {
-      LOG(ERROR) << "NaCl process launch failed: broker service did not launch "
-          "process";
+      SendErrorToRenderer("broker service did not launch process");
       return false;
     }
   } else {
     process_->Launch(new NaClSandboxedProcessLauncherDelegate,
                      cmd_line.release());
   }
 #elif defined(OS_POSIX)
   process_->Launch(nacl_loader_prefix.empty(),  // use_zygote
                    base::EnvironmentVector(),
                    cmd_line.release());
@@ skipping 24 matching lines @@
 
 void NaClProcessHost::OnProcessLaunched() {
   if (!StartWithLaunchedProcess())
     delete this;
 }
 
 // Called when the NaClBrowser singleton has been fully initialized.
 void NaClProcessHost::OnResourcesReady() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   if (!nacl_browser->IsReady()) {
-    LOG(ERROR) << "NaCl process launch failed: could not acquire shared "
-        "resources needed by NaCl";
+    SendErrorToRenderer("could not acquire shared resources needed by NaCl");
     delete this;
   } else if (!SendStart()) {
     delete this;
   }
 }
 
 bool NaClProcessHost::ReplyToRenderer(
     const IPC::ChannelHandle& channel_handle) {
   nacl::FileDescriptor handle_for_renderer;
 #if defined(OS_WIN)
   // Copy the handle into the renderer process.
   HANDLE handle_in_renderer;
   if (!DuplicateHandle(base::GetCurrentProcessHandle(),
                        reinterpret_cast<HANDLE>(
                            internal_->socket_for_renderer),
                        nacl_host_message_filter_->peer_handle(),
                        &handle_in_renderer,
                        0,  // Unused given DUPLICATE_SAME_ACCESS.
                        FALSE,
                        DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
-    LOG(ERROR) << "DuplicateHandle() failed";
+    SendErrorToRenderer("DuplicateHandle() failed");
     return false;
   }
   handle_for_renderer = reinterpret_cast<nacl::FileDescriptor>(
       handle_in_renderer);
 #else
   // No need to dup the imc_handle - we don't pass it anywhere else so
   // it cannot be closed.
   nacl::FileDescriptor imc_handle;
   imc_handle.fd = internal_->socket_for_renderer;
   imc_handle.auto_close = true;
   handle_for_renderer = imc_handle;
 #endif
 
 #if defined(OS_WIN)
   // If we are on 64-bit Windows, the NaCl process's sandbox is
   // managed by a different process from the renderer's sandbox.  We
   // need to inform the renderer's sandbox about the NaCl process so
   // that the renderer can send handles to the NaCl process using
   // BrokerDuplicateHandle().
   if (RunningOnWOW64()) {
     if (!content::BrokerAddTargetPeer(process_->GetData().handle)) {
-      LOG(ERROR) << "Failed to add NaCl process PID";
+      SendErrorToRenderer("Failed to add NaCl process PID");

[dmichael (off chromium), 2013/07/10 17:07:20]

nit: let's make the capitalization consistent. Here, you're using upper-case,
elsewhere you are using lowercase (not counting DuplicateHandle, of course)

[halyavin, 2013/07/11 09:03:36]

Done.

       return false;
     }
   }
 #endif
 
   const ChildProcessData& data = process_->GetData();
   NaClHostMsg_LaunchNaCl::WriteReplyParams(

[dmichael (off chromium), 2013/07/10 17:07:20]

For the sake of making the code more self-documenting, what about changing
SendErrorToRenderer to SendReplyToRenderer, and using it here? And that method
can have something like:
// This method must only be called once.
DCHECK(nacl_host_message_filter_);
DCHECK(reply_msg_);

[halyavin, 2013/07/11 09:03:36]

We are passing actual data here instead of empty nacl::NaClLaunchResult in
SendErrorToRenderer.

[dmichael (off chromium), 2013/07/11 16:12:59]

I know, I'm suggesting you add an argument to SendReplyToRenderer. In the Error
cases, you can pass a default NaClLaunchResult (or maybe still have
SendErrorToRenderer that wraps SendReplyToRenderer if you prefer). There's
enough behavior involved that we should put that code in 1 place (you have to
send the message and clear 2 members).

-      reply_msg_, handle_for_renderer,
-      channel_handle, base::GetProcId(data.handle), data.id);
+      reply_msg_,
+      nacl::NaClLaunchResult(handle_for_renderer,
+                             channel_handle,
+                             base::GetProcId(data.handle),
+                             data.id),
+      std::string());

[dmichael (off chromium), 2013/07/10 17:07:20]

an inline comment might be nice "std::string() /* error_message */);"

[halyavin, 2013/07/11 09:03:36]

Done.

   nacl_host_message_filter_->Send(reply_msg_);
   nacl_host_message_filter_ = NULL;
   reply_msg_ = NULL;
   internal_->socket_for_renderer = NACL_INVALID_HANDLE;

[dmichael (off chromium), 2013/07/10 17:07:20]

^^^ You're currently not doing this for the error case; is that on purpose?

[halyavin, 2013/07/11 09:03:36]

Yes, this is done on purpose. The handle is set to NACL_INVALID_HANDLE since it
would be closed while sending to another process and so we should not close it
in destructor. Since we don't send this handle on error, we should left it as is
and let destructor to close it.

I realized that there are resource leaks here while investigating this but I
would like to fix them in a separate CL.

[halyavin, 2013/07/11 12:49:44]

The handle leak fix is here: https://codereview.chromium.org/18474005

[dmichael (off chromium), 2013/07/11 16:12:59]

If we're sending a message to the other process on error anyway, maybe it would
be simpler to send the handle and let that process always close it?

   return true;
 }
 
+void NaClProcessHost::SendErrorToRenderer(const std::string& error_message) {
+  LOG(ERROR) << "NaCl process launch failed: " << error_message;
+  if (nacl_host_message_filter_ != NULL && reply_msg_ != NULL) {
+    NaClHostMsg_LaunchNaCl::WriteReplyParams(
+        reply_msg_, nacl::NaClLaunchResult(), error_message);
+    nacl_host_message_filter_->Send(reply_msg_);
+    nacl_host_message_filter_ = NULL;
+    reply_msg_ = NULL;
+  }
+}
+
 // 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() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   SocketDescriptor s;
   // We allocate currently unused TCP port for debug stub tests. The port
   // number is passed to the test via debug stub port listener.
   if (nacl_browser->HasGdbDebugStubPortListener()) {
@@ skipping 178 matching lines @@
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
   if (nacl_browser->IsReady()) {
     return SendStart();
   } else if (nacl_browser->IsOk()) {
     nacl_browser->WaitForResources(
         base::Bind(&NaClProcessHost::OnResourcesReady,
                    weak_factory_.GetWeakPtr()));
     return true;
   } else {
-    LOG(ERROR) << "NaCl process failed to launch: previously failed to acquire "
-        "shared resources";
+    SendErrorToRenderer("previously failed to acquire shared resources");
     return false;
   }
 }
 
 void NaClProcessHost::OnQueryKnownToValidate(const std::string& signature,
                                              bool* result) {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   *result = nacl_browser->QueryKnownToValidate(signature, off_the_record_);
 }
 
@@ skipping 142 matching lines @@
   } else {
     NaClStartDebugExceptionHandlerThread(
         process_handle.Take(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif