Instrument early NaCl load failiures on Windows.

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 84 matching lines @@
   int flags = DUPLICATE_SAME_ACCESS;
   if (close_source)
     flags |= DUPLICATE_CLOSE_SOURCE;
   if (!DuplicateHandle(GetCurrentProcess(),
                        reinterpret_cast<HANDLE>(sourceh),
                        processh,
                        &channel,
                        0,  // Unused given DUPLICATE_SAME_ACCESS.
                        FALSE,
                        flags)) {
-    DLOG(ERROR) << "DuplicateHandle() failed";
+    LOG(ERROR) << "DuplicateHandle() failed";
     return false;
   }
   handles_for_sel_ldr->push_back(
       reinterpret_cast<nacl::FileDescriptor>(channel));
 #else
   nacl::FileDescriptor channel;
   channel.fd = sourceh;
   channel.auto_close = close_source;
   handles_for_sel_ldr->push_back(channel);
 #endif
@@ skipping 147 matching lines @@
     scoped_refptr<ExtensionInfoMap> extension_info_map) {
   chrome_render_message_filter_ = chrome_render_message_filter;
   reply_msg_ = reply_msg;
   extension_info_map_ = extension_info_map;
 
   // 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()) {
-    DLOG(ERROR) << "Cannot launch NaCl process";
+    LOG(ERROR) << "NaCl process launch failed: could not find all the "
+        "resources needed to launch the process.";

[Mark Seaborn, 2013/01/16 00:27:54]

This is being picky, but most log messages in Chrome don't end with a "."

[Nick Bray (chromium), 2013/01/16 05:54:52]

Done.

     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.
 
   nacl::Handle pair[2];
   // Create a connected socket
   if (nacl::SocketPair(pair) == -1) {
+    LOG(ERROR) << "NaCl process launch failed: 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 14 matching lines @@
             base::kProcessAccessDuplicateHandle |
             base::kProcessAccessQueryInformation |
             base::kProcessAccessWaitForTermination,
             &process)) {
       process_->SetHandle(process);
       if (!StartWithLaunchedProcess()) {
         delete this;
         return;
       }
     } else {
-      DLOG(ERROR) << "Failed to get process handle";
+      LOG(ERROR) << "Failed to get process handle";
     }
   }
 }
 #else
 void NaClProcessHost::OnChannelConnected(int32 peer_pid) {
 }
 #endif
 
 #if defined(OS_WIN)
 void NaClProcessHost::OnProcessLaunchedByBroker(base::ProcessHandle handle) {
@@ skipping 148 matching lines @@
       manifest_url_.SchemeIs(extensions::kExtensionScheme)) {
     std::string path = manifest_url_.path();
     TrimString(path, "/", &path);  // Remove first slash
     return extension->path().AppendASCII(path);
   }
   return FilePath();
 }
 
 bool NaClProcessHost::LaunchSelLdr() {
   std::string channel_id = process_->GetHost()->CreateChannel();
-  if (channel_id.empty())
+  if (channel_id.empty()) {
+    LOG(ERROR) << "NaCl process launch failed: 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.
 
 #if defined(OS_MACOSX)
@@ skipping 12 matching lines @@
 #endif
 
   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()) {
     FilePath module_path;
-    if (!PathService::Get(base::FILE_MODULE, &module_path))
+    if (!PathService::Get(base::FILE_MODULE, &module_path)) {
+      LOG(ERROR) << "NaCl process launch failed: could not resolve module.";
       return false;
+    }
     exe_path = module_path.DirName().Append(chrome::kNaClAppName);
   }
 #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);
@@ skipping 20 matching lines @@
     // call OnProcessLaunched.
     return base::LaunchProcess(*gdb_cmd_line, base::LaunchOptions(), NULL);
 #elif defined(OS_LINUX)
     wait_for_nacl_gdb_ = true;
 #endif
   }
 
   // On Windows we might need to start the broker process to launch a new loader
 #if defined(OS_WIN)
   if (RunningOnWOW64()) {
-    return NaClBrokerService::GetInstance()->LaunchLoader(
-        weak_factory_.GetWeakPtr(), channel_id);
+    if (!NaClBrokerService::GetInstance()->LaunchLoader(
+        weak_factory_.GetWeakPtr(), channel_id)) {

[Mark Seaborn, 2013/01/16 00:27:54]

Indent function argument

[Nick Bray (chromium), 2013/01/16 05:54:52]

How?  This is technically indented 2x relative to the start of the first line. 
Breaking the line before and indented block is strange.

[Mark Seaborn, 2013/01/16 16:03:51]

You should write

if (!NaClBrokerService::GetInstance()->LaunchLoader(
         weak_factory_.GetWeakPtr(), channel_id)) {

rather than

if (!NaClBrokerService::GetInstance()->LaunchLoader(
    weak_factory_.GetWeakPtr(), channel_id)) {

The latter is rather like writing this outside of an if():

NaClBrokerService::GetInstance()->LaunchLoader(
weak_factory_.GetWeakPtr(), channel_id);

+      LOG(ERROR) << "NaCl process launch failed: broker service did not launch "
+          "process.";
+      return false;
+    }
   } else {
     process_->Launch(FilePath(), cmd_line.release());
   }
 #elif defined(OS_POSIX)
   process_->Launch(nacl_loader_prefix.empty(),  // use_zygote
                    base::EnvironmentVector(),
                    cmd_line.release());
 #endif
 
   return true;
@@ skipping 18 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() || !SendStart()) {
-    DLOG(ERROR) << "Cannot launch NaCl process";
+  if (!nacl_browser->IsReady()) {
+    LOG(ERROR) << "NaCl process launch failed: could not aquire shared "

[Mark Seaborn, 2013/01/16 00:27:54]

"acquire"

[Nick Bray (chromium), 2013/01/16 05:54:52]

Done.

+        "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),
                        chrome_render_message_filter_->peer_handle(),
                        &handle_in_renderer,
                        0,  // Unused given DUPLICATE_SAME_ACCESS.
                        FALSE,
                        DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
-    DLOG(ERROR) << "DuplicateHandle() failed";
+    LOG(ERROR) << "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)) {
-      DLOG(ERROR) << "Failed to add NaCl process PID";
+      LOG(ERROR) << "Failed to add NaCl process PID";
       return false;
     }
   }
 #endif
 
   const ChildProcessData& data = process_->GetData();
   ChromeViewHostMsg_LaunchNaCl::WriteReplyParams(
       reply_msg_, handle_for_renderer,
       channel_handle, base::GetProcId(data.handle), data.id);
   chrome_render_message_filter_->Send(reply_msg_);
@@ skipping 183 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 aquire "

[Mark Seaborn, 2013/01/16 00:27:54]

"acquire"

[Nick Bray (chromium), 2013/01/16 05:54:52]

Done.

+        "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 62 matching lines @@
   } else {
     NaClStartDebugExceptionHandlerThread(
         process_handle.Take(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif