Check NaCl debug mask (not just flag) before choosing PNaCl debug URL.

That way, the debug URL is chosen only if the app will be debugged.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=3765
R=bradnelson@google.com, tsepez@chromium.org
TBR=bradnelson@chromium.org, teravest@chromium.org

Review URL: https://codereview.chromium.org/212103002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@260067 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 137 insertions, 40 deletions

diff --git a/chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html b/chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html
index 7702afc..a24eb3e 100644
--- a/chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html
+++ b/chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html
@@ -20,10 +20,14 @@ function create(nmf_url) {
   embed.type = "application/x-pnacl";
   embed.addEventListener("load", function(evt) {
     load_util.shutdown("1 test passed.", true);
+    // Remove the embed so that the debugger will shut down.
+    embed.parentNode.removeChild(embed);
   }, true);
   embed.addEventListener("error", function(evt) {
     load_util.log("Load error: " + embed.lastError);
     load_util.shutdown("1 test failed.", false);
+    // Remove the embed so that the debugger will shut down.
+    embed.parentNode.removeChild(embed);
   }, true);
   document.body.appendChild(embed);
 }
diff --git a/chrome/test/nacl/nacl_browsertest.cc b/chrome/test/nacl/nacl_browsertest.cc
index 3227e99..6f4c03f 100644
--- a/chrome/test/nacl/nacl_browsertest.cc
+++ b/chrome/test/nacl/nacl_browsertest.cc
@@ -14,9 +14,13 @@
 #include "base/command_line.h"
 #include "base/environment.h"
 #include "base/path_service.h"
+#include "base/process/kill.h"
+#include "base/process/launch.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/nacl/nacl_browsertest_util.h"
+#include "components/nacl/browser/nacl_browser.h"
 #include "components/nacl/common/nacl_switches.h"
 #include "content/public/common/content_switches.h"
 
@@ -201,7 +205,8 @@ IN_PROC_BROWSER_TEST_F(NaClBrowserTestStatic, RelativeManifest) {
   RunLoadTest(FILE_PATH_LITERAL("manifest/relative_manifest.html"));
 }
 
-class NaClBrowserTestPnaclDebugURL : public NaClBrowserTestPnacl {
+// Test with the NaCl debug flag turned on.
+class NaClBrowserTestPnaclDebug : public NaClBrowserTestPnacl {
  public:
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
     NaClBrowserTestPnacl::SetUpCommandLine(command_line);
@@ -212,40 +217,89 @@ class NaClBrowserTestPnaclDebugURL : public NaClBrowserTestPnacl {
 #if defined(OS_WIN)
     command_line->AppendSwitch(switches::kNoSandbox);
 #endif
-    // Don't actually debug the app though.
+  }
+
+  // On some platforms this test does not work.
+  bool TestIsBroken() {
+    // TODO(jvoung): Make this test work on Windows 32-bit. When --no-sandbox
+    // is used, the required 1GB sandbox address space is not reserved.
+    // (see note in chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc)
+#if defined(OS_WIN)
+    if (base::win::OSInfo::GetInstance()->wow64_status() ==
+        base::win::OSInfo::WOW64_DISABLED &&
+        base::win::OSInfo::GetInstance()->architecture() ==
+        base::win::OSInfo::X86_ARCHITECTURE) {
+      return true;
+    }
+#endif
+    return false;
+  }
+
+  void StartTestScript(base::ProcessHandle* test_process,
+                       int debug_stub_port) {
+    // We call a python script that speaks to the debug stub, and
+    // lets the app continue, so that the load progress event completes.
+    CommandLine cmd(base::FilePath(FILE_PATH_LITERAL("python")));
+    base::FilePath script;
+    PathService::Get(base::DIR_SOURCE_ROOT, &script);
+    script = script.AppendASCII(
+        "chrome/browser/nacl_host/test/debug_stub_browser_tests.py");
+    cmd.AppendArgPath(script);
+    cmd.AppendArg(base::IntToString(debug_stub_port));
+    cmd.AppendArg("continue");
+    LOG(INFO) << cmd.GetCommandLineString();
+    base::LaunchProcess(cmd, base::LaunchOptions(), test_process);
+  }
+
+  void RunWithTestDebugger(const base::FilePath::StringType& test_url) {
+    base::ProcessHandle test_script;
+    scoped_ptr<base::Environment> env(base::Environment::Create());
+    nacl::NaClBrowser::GetInstance()->SetGdbDebugStubPortListener(
+        base::Bind(&NaClBrowserTestPnaclDebug::StartTestScript,
+                   base::Unretained(this), &test_script));
+    // Turn on debug stub logging.
+    env->SetVar("NACLVERBOSITY", "1");
+    RunLoadTest(test_url);
+    env->UnSetVar("NACLVERBOSITY");
+    nacl::NaClBrowser::GetInstance()->ClearGdbDebugStubPortListener();
+    int exit_code;
+    LOG(INFO) << "Waiting for script to exit (which waits for embed to die).";
+    base::WaitForExitCode(test_script, &exit_code);
+    EXPECT_EQ(0, exit_code);
+  }
+};
+
+// Test with the NaCl debug flag turned on, but mask off every URL
+// so that nothing is actually debugged.
+class NaClBrowserTestPnaclDebugMasked : public NaClBrowserTestPnaclDebug {
+ public:
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    NaClBrowserTestPnaclDebug::SetUpCommandLine(command_line);
     command_line->AppendSwitchASCII(switches::kNaClDebugMask,
                                     "!<all_urls>");
   }
 };
 
-IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclDebugURL,
-                       MAYBE_PNACL(PnaclDebugURLFlagAndURL)) {
-  // TODO(jvoung): Make this test work on Windows 32-bit. When --no-sandbox
-  // is used, the required 1GB sandbox address space is not reserved.
-  // (see note in chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc)
+// The tests which actually start a debug session must use the debug stub
+// to continue the app startup. However, NaCl on windows can't open the
+// debug stub socket in the browser process as needed by the test.
+// See http://crbug.com/157312.
 #if defined(OS_WIN)
-  if (base::win::OSInfo::GetInstance()->wow64_status() ==
-      base::win::OSInfo::WOW64_DISABLED &&
-      base::win::OSInfo::GetInstance()->architecture() ==
-      base::win::OSInfo::X86_ARCHITECTURE) {
-    return;
-  }
+#define MAYBE_PnaclDebugURLFlagAndURL DISABLED_PnaclDebugURLFlagAndURL
+#define MAYBE_PnaclDebugURLFlagNoURL DISABLED_PnaclDebugURLFlagNoURL
+#else
+#define MAYBE_PnaclDebugURLFlagAndURL PnaclDebugURLFlagAndURL
+#define MAYBE_PnaclDebugURLFlagNoURL PnaclDebugURLFlagNoURL
 #endif
-  RunLoadTest(FILE_PATH_LITERAL(
+IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclDebug,
+                       MAYBE_PnaclDebugURLFlagAndURL) {
+  RunWithTestDebugger(FILE_PATH_LITERAL(
       "pnacl_debug_url.html?nmf_file=pnacl_has_debug.nmf"));
 }
 
-IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclDebugURL,
-                       MAYBE_PNACL(PnaclDebugURLFlagNoURL)) {
-#if defined(OS_WIN)
-  if (base::win::OSInfo::GetInstance()->wow64_status() ==
-      base::win::OSInfo::WOW64_DISABLED &&
-      base::win::OSInfo::GetInstance()->architecture() ==
-      base::win::OSInfo::X86_ARCHITECTURE) {
-    return;
-  }
-#endif
-  RunLoadTest(FILE_PATH_LITERAL(
+IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclDebug,
+                       MAYBE_PnaclDebugURLFlagNoURL) {
+  RunWithTestDebugger(FILE_PATH_LITERAL(
       "pnacl_debug_url.html?nmf_file=pnacl_no_debug.nmf"));
 }
 
@@ -255,6 +309,16 @@ IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnacl,
       "pnacl_debug_url.html?nmf_file=pnacl_has_debug_flag_off.nmf"));
 }
 
+IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclDebugMasked,
+                       MAYBE_PNACL(PnaclDebugURLFlagMaskedOff)) {
+  if (TestIsBroken()) {
+    return;
+  }
+  // If the mask excludes debugging, it's as if the flag was off.
+  RunLoadTest(FILE_PATH_LITERAL(
+      "pnacl_debug_url.html?nmf_file=pnacl_has_debug_flag_off.nmf"));
+}
+
 IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnacl,
                        MAYBE_PNACL(PnaclErrorHandling)) {
   RunNaClIntegrationTest(FILE_PATH_LITERAL("pnacl_error_handling.html"));
diff --git a/components/nacl/browser/nacl_host_message_filter.cc b/components/nacl/browser/nacl_host_message_filter.cc
index 9e85266..9389b70 100644
--- a/components/nacl/browser/nacl_host_message_filter.cc
+++ b/components/nacl/browser/nacl_host_message_filter.cc
@@ -57,6 +57,8 @@ bool NaClHostMessageFilter::OnMessageReceived(const IPC::Message& message,
                                     OnOpenNaClExecutable)
     IPC_MESSAGE_HANDLER(NaClHostMsg_NaClGetNumProcessors,
                         OnNaClGetNumProcessors)
+    IPC_MESSAGE_HANDLER(NaClHostMsg_NaClDebugEnabledForURL,
+                        OnNaClDebugEnabledForURL)
 #endif
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
@@ -181,4 +183,10 @@ void NaClHostMessageFilter::OnOpenNaClExecutable(int render_view_id,
                                      reply_msg);
 }
 
+void NaClHostMessageFilter::OnNaClDebugEnabledForURL(const GURL& nmf_url,
+                                                     bool* should_debug) {
+  *should_debug =
+      nacl::NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(nmf_url);
+}
+
 }  // namespace nacl
diff --git a/components/nacl/browser/nacl_host_message_filter.h b/components/nacl/browser/nacl_host_message_filter.h
index c7bf900..3dc4b45 100644
--- a/components/nacl/browser/nacl_host_message_filter.h
+++ b/components/nacl/browser/nacl_host_message_filter.h
@@ -67,6 +67,8 @@ class NaClHostMessageFilter : public content::BrowserMessageFilter {
   void AsyncReturnTemporaryFile(int pp_instance,
                                 base::PlatformFile fd,
                                 bool is_hit);
+  void OnNaClDebugEnabledForURL(const GURL& nmf_url, bool* should_debug);
+
   int render_process_id_;
 
   // off_the_record_ is copied from the profile partly so that it can be
diff --git a/components/nacl/common/nacl_host_messages.h b/components/nacl/common/nacl_host_messages.h
index 3a1e447..da7478b 100644
--- a/components/nacl/common/nacl_host_messages.h
+++ b/components/nacl/common/nacl_host_messages.h
@@ -104,5 +104,14 @@ IPC_SYNC_MESSAGE_CONTROL2_3(NaClHostMsg_OpenNaClExecutable,
                             uint64 /* file_token_lo */,
                             uint64 /* file_token_hi */)
 
+// A renderer sends this to the browser process to determine how many
+// processors are online.
 IPC_SYNC_MESSAGE_CONTROL0_1(NaClHostMsg_NaClGetNumProcessors,
                             int /* Number of processors */)
+
+// A renderer sends this to the browser process to determine if the
+// NaCl application started from the given NMF URL will be debugged.
+// If not (filtered out by commandline flags), it sets should_debug to false.
+IPC_SYNC_MESSAGE_CONTROL1_1(NaClHostMsg_NaClDebugEnabledForURL,
+                            GURL /* alleged URL of NMF file */,
+                            bool /* should debug */)
diff --git a/components/nacl/renderer/ppb_nacl_private_impl.cc b/components/nacl/renderer/ppb_nacl_private_impl.cc
index 3b89d18..1162133 100644
--- a/components/nacl/renderer/ppb_nacl_private_impl.cc
+++ b/components/nacl/renderer/ppb_nacl_private_impl.cc
@@ -467,9 +467,18 @@ void InstanceDestroyed(PP_Instance instance) {
   map.erase(instance);
 }
 
-PP_Bool NaClDebugStubEnabled() {
-  return PP_FromBool(CommandLine::ForCurrentProcess()->HasSwitch(
-                         switches::kEnableNaClDebug));
+PP_Bool NaClDebugEnabledForURL(const char* alleged_nmf_url) {
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableNaClDebug))
+    return PP_FALSE;
+  bool should_debug;
+  IPC::Sender* sender = content::RenderThread::Get();
+  DCHECK(sender);
+  if(!sender->Send(new NaClHostMsg_NaClDebugEnabledForURL(
+         GURL(alleged_nmf_url),
+         &should_debug))) {
+    return PP_FALSE;
+  }
+  return PP_FromBool(should_debug);
 }
 
 const char* GetSandboxArch() {
@@ -560,7 +569,7 @@ const PPB_NaCl_Private nacl_interface = {
   &ReportLoadError,
   &InstanceCreated,
   &InstanceDestroyed,
-  &NaClDebugStubEnabled,
+  &NaClDebugEnabledForURL,
   &GetSandboxArch,
   &GetUrlScheme,
   &LogToConsole,
diff --git a/ppapi/api/private/ppb_nacl_private.idl b/ppapi/api/private/ppb_nacl_private.idl
index 193916c..9e321f6 100644
--- a/ppapi/api/private/ppb_nacl_private.idl
+++ b/ppapi/api/private/ppb_nacl_private.idl
@@ -295,10 +295,10 @@ interface PPB_NaCl_Private {
   /* Performs internal cleanup when an instance is destroyed. */
   void InstanceDestroyed([in] PP_Instance instance);
 
-  /* Return true if the NaCl debug stub is enabled and the loaded app
-   * will be attached to a debugger.
+  /* Return true if the NaCl debug stub is enabled and the app loaded from
+   * alleged_nmf_url will be attached to a debugger.
    */
-  PP_Bool NaClDebugStubEnabled();
+  PP_Bool NaClDebugEnabledForURL([in] str_t alleged_nmf_url);
 
   /* Returns the kind of SFI sandbox implemented by NaCl on this
   /* platform.
diff --git a/ppapi/c/private/ppb_nacl_private.h b/ppapi/c/private/ppb_nacl_private.h
index dd0a04f..c389c84 100644
--- a/ppapi/c/private/ppb_nacl_private.h
+++ b/ppapi/c/private/ppb_nacl_private.h
@@ -3,7 +3,7 @@
  * found in the LICENSE file.
  */
 
-/* From private/ppb_nacl_private.idl modified Thu Mar 27 10:43:40 2014. */
+/* From private/ppb_nacl_private.idl modified Thu Mar 27 14:44:04 2014. */
 
 #ifndef PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
 #define PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
@@ -299,10 +299,10 @@ struct PPB_NaCl_Private_1_0 {
   void (*InstanceCreated)(PP_Instance instance);
   /* Performs internal cleanup when an instance is destroyed. */
   void (*InstanceDestroyed)(PP_Instance instance);
-  /* Return true if the NaCl debug stub is enabled and the loaded app
-   * will be attached to a debugger.
+  /* Return true if the NaCl debug stub is enabled and the app loaded from
+   * alleged_nmf_url will be attached to a debugger.
    */
-  PP_Bool (*NaClDebugStubEnabled)(void);
+  PP_Bool (*NaClDebugEnabledForURL)(const char* alleged_nmf_url);
   /* Returns the kind of SFI sandbox implemented by NaCl on this
    * platform.
    */
diff --git a/ppapi/native_client/src/trusted/plugin/plugin.cc b/ppapi/native_client/src/trusted/plugin/plugin.cc
index e550805..e98c4c8 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.cc
+++ b/ppapi/native_client/src/trusted/plugin/plugin.cc
@@ -1160,7 +1160,8 @@ bool Plugin::SetManifestObject(const nacl::string& manifest_json,
   bool is_pnacl = (mime_type() == kPnaclMIMEType);
   bool nonsfi_mode_enabled =
       PP_ToBool(nacl_interface_->IsNonSFIModeEnabled());
-  bool pnacl_debug = GetNaClInterface()->NaClDebugStubEnabled();
+  bool pnacl_debug = GetNaClInterface()->NaClDebugEnabledForURL(
+      manifest_base_url().c_str());
   const char* sandbox_isa = nacl_interface_->GetSandboxArch();
   nacl::scoped_ptr<JsonManifest> json_manifest(
       new JsonManifest(url_util_,
diff --git a/ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c b/ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c
index 1c1fd05..c50d0ef 100644
--- a/ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c
+++ b/ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c
@@ -3176,9 +3176,9 @@ static void Pnacl_M25_PPB_NaCl_Private_InstanceDestroyed(PP_Instance instance) {
   iface->InstanceDestroyed(instance);
 }
 
-static PP_Bool Pnacl_M25_PPB_NaCl_Private_NaClDebugStubEnabled(void) {
+static PP_Bool Pnacl_M25_PPB_NaCl_Private_NaClDebugEnabledForURL(const char* alleged_nmf_url) {
   const struct PPB_NaCl_Private_1_0 *iface = Pnacl_WrapperInfo_PPB_NaCl_Private_1_0.real_iface;
-  return iface->NaClDebugStubEnabled();
+  return iface->NaClDebugEnabledForURL(alleged_nmf_url);
 }
 
 static const char* Pnacl_M25_PPB_NaCl_Private_GetSandboxArch(void) {
@@ -5121,7 +5121,7 @@ static const struct PPB_NaCl_Private_1_0 Pnacl_Wrappers_PPB_NaCl_Private_1_0 = {
     .ReportLoadError = (void (*)(PP_Instance instance, PP_NaClError error, const char* error_message, const char* console_message))&Pnacl_M25_PPB_NaCl_Private_ReportLoadError,
     .InstanceCreated = (void (*)(PP_Instance instance))&Pnacl_M25_PPB_NaCl_Private_InstanceCreated,
     .InstanceDestroyed = (void (*)(PP_Instance instance))&Pnacl_M25_PPB_NaCl_Private_InstanceDestroyed,
-    .NaClDebugStubEnabled = (PP_Bool (*)(void))&Pnacl_M25_PPB_NaCl_Private_NaClDebugStubEnabled,
+    .NaClDebugEnabledForURL = (PP_Bool (*)(const char* alleged_nmf_url))&Pnacl_M25_PPB_NaCl_Private_NaClDebugEnabledForURL,
     .GetSandboxArch = (const char* (*)(void))&Pnacl_M25_PPB_NaCl_Private_GetSandboxArch,
     .GetUrlScheme = (PP_UrlSchemeType (*)(struct PP_Var url))&Pnacl_M25_PPB_NaCl_Private_GetUrlScheme,
     .LogToConsole = (void (*)(PP_Instance instance, const char* message))&Pnacl_M25_PPB_NaCl_Private_LogToConsole,