Show more different NaCl loading errors.

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 210749)
+++ chrome/browser/nacl_host/nacl_process_host.cc	(working copy)
@@ -289,8 +289,8 @@
   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;
   }
@@ -307,7 +307,7 @@
   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;
   }
@@ -393,7 +393,7 @@
 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;
   }
 
@@ -428,6 +428,7 @@
   // 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;
     }
   }
@@ -450,8 +451,7 @@
   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 {
@@ -496,8 +496,7 @@
 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;
@@ -518,7 +517,7 @@
                        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>(
@@ -540,7 +539,7 @@
   // 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;
     }
   }
@@ -548,8 +547,12 @@
 
   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;
@@ -557,6 +560,17 @@
   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;
 
@@ -755,8 +769,7 @@
                    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;
   }
 }