From 6ed0388cbf008ea2c07ef81ef967f0639f982887 Mon Sep 17 00:00:00 2001
From: "polina@google.com"
 <polina@google.com@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Fri, 26 Aug 2011 22:38:37 +0000
Subject: NaCl PPAPI Proxy: wrap up with crash detection. Clean-up handling
 code to skip remote shutdown calls when the nexe is known to be dead. Add a
 test for crashing on other than the main thread, which depending on thread
 timing might happen when the main thread is servicing a PPP call or waiting
 for the next one. Add another test that will fail on a CHECK for an
 unsupported Pepper call off the main thread.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=1780, http://code.google.com/p/nativeclient/issues/detail?id=2105, http://code.google.com/p/nativeclient/issues/detail?id=1682
TEST=scons run_ppapi_crash_browser_test
Review URL: http://codereview.chromium.org/7741036

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@98507 0039d316-1c4b-4281-b951-d872f2087c98
---
 .../src/shared/ppapi_proxy/browser_globals.cc      |  3 +-
 .../src/shared/ppapi_proxy/browser_ppp.cc          | 10 +++--
 .../src/shared/ppapi_proxy/browser_ppp.h           |  2 +
 .../ppapi_proxy/plugin_ppp_messaging_rpc_server.cc |  2 +-
 ppapi/native_client/src/trusted/plugin/plugin.cc   | 15 ++++---
 .../tests/ppapi_browser/crash/nacl.scons           |  6 ++-
 .../tests/ppapi_browser/crash/ppapi_crash.html     | 41 +++++++++++++-----
 .../crash/ppapi_crash_off_main_thread.cc           | 48 ++++++++++++++++++++++
 .../crash/ppapi_crash_off_main_thread.nmf          |  7 ++++
 .../crash/ppapi_crash_ppapi_off_main_thread.cc     | 40 ++++++++++++++++++
 .../crash/ppapi_crash_ppapi_off_main_thread.nmf    |  7 ++++
 11 files changed, 158 insertions(+), 23 deletions(-)
 create mode 100644 ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_off_main_thread.cc
 create mode 100644 ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_off_main_thread.nmf
 create mode 100644 ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_ppapi_off_main_thread.cc
 create mode 100644 ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_ppapi_off_main_thread.nmf

(limited to 'ppapi')

diff --git a/ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc b/ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc
index 667f344..b1fa08e 100644
--- a/ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc
+++ b/ppapi/native_client/src/shared/ppapi_proxy/browser_globals.cc
@@ -171,8 +171,7 @@ void CleanUpAfterDeadNexe(PP_Instance instance) {
   BrowserPpp* proxy = LookupBrowserPppForInstance(instance);
   if (proxy == NULL)
     return;
-  proxy->ShutdownModule();
-  proxy->plugin()->ReportDeadNexe();  // Deletes the proxy.
+  proxy->plugin()->ReportDeadNexe();  // Shuts down and deletes the proxy.
 }
 
 void SetPPBGetInterface(PPB_GetInterface get_interface_function,
diff --git a/ppapi/native_client/src/shared/ppapi_proxy/browser_ppp.cc b/ppapi/native_client/src/shared/ppapi_proxy/browser_ppp.cc
index 1227f3e..18928b8 100644
--- a/ppapi/native_client/src/shared/ppapi_proxy/browser_ppp.cc
+++ b/ppapi/native_client/src/shared/ppapi_proxy/browser_ppp.cc
@@ -125,16 +125,18 @@ void BrowserPpp::ShutdownModule() {
     CHECK(!is_nexe_alive_);
     return;  // The proxy has already been shut down.
   }
-  NaClSrpcError srpc_result =
-    PppRpcClient::PPP_ShutdownModule(main_channel_);
-  DebugPrintf("PPP_ShutdownModule: %s\n", NaClSrpcErrorString(srpc_result));
+  if (is_nexe_alive_) {
+    NaClSrpcError srpc_result =
+      PppRpcClient::PPP_ShutdownModule(main_channel_);
+    DebugPrintf("PPP_ShutdownModule: %s\n", NaClSrpcErrorString(srpc_result));
+  }
   NaClThreadJoin(&upcall_thread_);
   UnsetBrowserPppForInstance(plugin_->pp_instance());
   UnsetModuleIdForSrpcChannel(main_channel_);
   UnsetInstanceIdForSrpcChannel(main_channel_);
   main_channel_ = NULL;
   is_nexe_alive_ = false;
-  DebugPrintf("PPP_Shutdown: done\n");
+  DebugPrintf("PPP_Shutdown: main_channel=NULL\n");
 }
 
 const void* BrowserPpp::GetPluginInterface(const char* interface_name) {
diff --git a/ppapi/native_client/src/shared/ppapi_proxy/browser_ppp.h b/ppapi/native_client/src/shared/ppapi_proxy/browser_ppp.h
index 3ae6561..440b062 100644
--- a/ppapi/native_client/src/shared/ppapi_proxy/browser_ppp.h
+++ b/ppapi/native_client/src/shared/ppapi_proxy/browser_ppp.h
@@ -75,6 +75,8 @@ class BrowserPpp {
   int plugin_pid() const { return plugin_pid_; }
   plugin::Plugin* plugin() { return plugin_; }
 
+  void ReportDeadNexe() { is_nexe_alive_ = false; }
+
  private:
   // The "main" SRPC channel used to communicate with the plugin.
   // NULL if proxy has been shut down.
diff --git a/ppapi/native_client/src/shared/ppapi_proxy/plugin_ppp_messaging_rpc_server.cc b/ppapi/native_client/src/shared/ppapi_proxy/plugin_ppp_messaging_rpc_server.cc
index 40c8cb5..eb3d27d 100644
--- a/ppapi/native_client/src/shared/ppapi_proxy/plugin_ppp_messaging_rpc_server.cc
+++ b/ppapi/native_client/src/shared/ppapi_proxy/plugin_ppp_messaging_rpc_server.cc
@@ -26,6 +26,6 @@ void PppMessagingRpcServer::PPP_Messaging_HandleMessage(
   if (!DeserializeTo(rpc->channel, message_bytes, message_size, 1, &message))
     return;
   PPPMessagingInterface()->HandleMessage(instance, message);
-  DebugPrintf("PPP_Instance::HandleMessage\n");
+  DebugPrintf("PPP_Messaging::HandleMessage\n");
   rpc->result = NACL_SRPC_RESULT_OK;
 }
diff --git a/ppapi/native_client/src/trusted/plugin/plugin.cc b/ppapi/native_client/src/trusted/plugin/plugin.cc
index 6a3d5c8..1fdf449 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.cc
+++ b/ppapi/native_client/src/trusted/plugin/plugin.cc
@@ -472,7 +472,7 @@ void Plugin::LoadMethods() {
 }
 
 bool Plugin::HasMethod(uintptr_t method_id, CallType call_type) {
-  PLUGIN_PRINTF(("Plugin::HasMethod (method_id=%x) = ",
+  PLUGIN_PRINTF(("Plugin::HasMethod (method_id=%x)\n",
                  static_cast<int>(method_id)));
   if (GetMethodInfo(method_id, call_type)) {
     PLUGIN_PRINTF(("true\n"));
@@ -1305,6 +1305,8 @@ bool Plugin::StartProxiedExecution(NaClSrpcChannel* srpc_channel,
 
 void Plugin::ReportDeadNexe() {
   PLUGIN_PRINTF(("Plugin::ReportDeadNexe\n"));
+  if (ppapi_proxy_ != NULL)
+    ppapi_proxy_->ReportDeadNexe();
 
   if (nacl_ready_state() == DONE) {  // After loadEnd.
     int64_t crash_time = NaClGetTimeOfDayMicroseconds();
@@ -1321,7 +1323,9 @@ void Plugin::ReportDeadNexe() {
     CHECK(ppapi_proxy_ != NULL && !ppapi_proxy_->is_valid());
     ShutdownProxy();
   }
-  // else LoadNaClModule and NexeFileDidOpen will provide error handling.
+  // else ReportLoadError() and ReportAbortError() will be used by loading code
+  // to provide error handling and proxy shutdown.
+  //
   // NOTE: not all crashes during load will make it here.
   // Those in BrowserPpp::InitializeModule and creation of PPP interfaces
   // will just get reported back as PP_ERROR_FAILED.
@@ -1333,10 +1337,11 @@ void Plugin::ShutdownProxy() {
   // We do not call remote PPP_Instance::DidDestroy because the untrusted
   // side can no longer take full advantage of mostly asynchronous Pepper
   // per-Instance interfaces at this point.
-  if (BrowserPpp::is_valid(ppapi_proxy_))
+  if (ppapi_proxy_ != NULL) {
     ppapi_proxy_->ShutdownModule();
-  delete ppapi_proxy_;
-  ppapi_proxy_ = NULL;
+    delete ppapi_proxy_;
+    ppapi_proxy_ = NULL;
+  }
 }
 
 void Plugin::NaClManifestBufferReady(int32_t pp_error) {
diff --git a/ppapi/native_client/tests/ppapi_browser/crash/nacl.scons b/ppapi/native_client/tests/ppapi_browser/crash/nacl.scons
index 7da4e0f..2f942e1 100644
--- a/ppapi/native_client/tests/ppapi_browser/crash/nacl.scons
+++ b/ppapi/native_client/tests/ppapi_browser/crash/nacl.scons
@@ -12,7 +12,11 @@ Import('env')
 
 env.Prepend(CPPDEFINES=['XP_UNIX'])
 
-crash_types = ['via_check_failure', 'via_exit_call', 'in_callback']
+crash_types = ['via_check_failure',
+               'via_exit_call',
+               'in_callback',
+               'off_main_thread',
+               'ppapi_off_main_thread']
 published_files = []
 
 for crash_type in crash_types:
diff --git a/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash.html b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash.html
index 77b8dfe..c7df160 100644
--- a/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash.html
+++ b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash.html
@@ -23,21 +23,37 @@
            name="nacl_module"
            src="ppapi_crash_in_callback.nmf"
            width="0" height="0" />
+    <embed type="application/x-nacl" id="crash_ppapi_off_main_thread"
+           name="nacl_module"
+           src="ppapi_crash_ppapi_off_main_thread.nmf"
+           width="0" height="0" />
+    <embed type="application/x-nacl" id="crash_off_main_thread"
+           name="nacl_module"
+           src="ppapi_crash_off_main_thread.nmf"
+           width="0" height="0" />
 
     <script type="text/javascript">
       //<![CDATA[
       var tester = new Tester();
-      function AddTest(plugin, testName, expectedHandler, unexpectedHandler) {
+      function AddTest(plugin, testName, expectedEvent, unexpectedEvent,
+                       pingToDetectCrash) {
         tester.addAsyncTest(testName, function(test) {
-          plugin.addEventListener(
-            expectedHandler,
-            test.wrap(function(e) { test.pass(); }),
-            false);
-          plugin.addEventListener(
-            unexpectedHandler,
-            test.wrap(function(e) { test.fail(); }),
-            false);
+          test.expectEvent(plugin, expectedEvent,
+                           function(e) { test.pass(); });
+          test.expectEvent(plugin, unexpectedEvent,
+                           function(e) { test.fail(); });
           plugin.postMessage(testName);
+          // In case the nexe does not crash right away, we will ping it
+          // until we detect that it's death. DidChangeView and other events
+          // can do this too, but are less reliable.
+          if (pingToDetectCrash) {
+            function PingBack(message) {
+              test.log(message.data);
+              plugin.postMessage('Ping');
+            }
+            plugin.addEventListener('message', PingBack, false);
+            plugin.postMessage("Ping");
+          }
         });
         tester.waitFor(plugin);
       }
@@ -46,7 +62,12 @@
               'CrashViaCheckFailure', 'crash', 'error');
       AddTest($('crash_via_exit_call'),
               'CrashViaExitCall', 'crash', 'error');
-      AddTest($('crash_in_callback'), 'CrashInCallback', 'crash', 'error');
+      AddTest($('crash_in_callback'),
+              'CrashInCallback', 'crash', 'error');
+      AddTest($('crash_ppapi_off_main_thread'),
+              'CrashPPAPIOffMainThread', 'crash', 'error');
+      AddTest($('crash_off_main_thread'),
+              'CrashOffMainThread', 'crash', 'error', true);
 
       tester.run();
       //]]>
diff --git a/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_off_main_thread.cc b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_off_main_thread.cc
new file mode 100644
index 0000000..c5bbc47
--- /dev/null
+++ b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_off_main_thread.cc
@@ -0,0 +1,48 @@
+// Copyright (c) 2011 The Native Client Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Test that we handle crashes on threads other than main.
+
+#include <pthread.h>
+#include <stdio.h>
+
+#include "native_client/src/shared/platform/nacl_check.h"
+#include "native_client/tests/ppapi_test_lib/test_interface.h"
+
+namespace {
+
+void* CrashOffMainThreadFunction(void* thread_arg) {
+  printf("--- CrashOffMainThreadFunction\n");
+  usleep(1000);  // Try to wait until PPP_Messaging::HandleMessage returns.
+  CRASH;
+  return NULL;
+}
+
+// Depending on how the detached thread is scheduled, this will either crash
+// during PPP_Messaging::HandleMessage or after it and before the next PPP call.
+// If a crash occurs within a PPP call, it returns an RPC error.
+// If a crash occurs between PPP calls, the next call will return an RPC error.
+void CrashOffMainThread() {
+  printf("--- CrashOffMainThread\n");
+  pthread_t tid;
+  pthread_create(&tid, NULL /*attr*/, CrashOffMainThreadFunction, NULL);
+  pthread_detach(tid);
+}
+
+// This will allow us to ping the nexe to detect a crash that occured
+// while the main thread was waiting for the next PPP call.
+void Ping() {
+  LOG_TO_BROWSER("ping received");
+}
+
+}  // namespace
+
+void SetupTests() {
+  RegisterTest("CrashOffMainThread", CrashOffMainThread);
+  RegisterTest("Ping", Ping);
+}
+
+void SetupPluginInterfaces() {
+  // none
+}
diff --git a/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_off_main_thread.nmf b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_off_main_thread.nmf
new file mode 100644
index 0000000..322389f
--- /dev/null
+++ b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_off_main_thread.nmf
@@ -0,0 +1,7 @@
+{
+  "program": {
+    "x86-32": {"url": "ppapi_crash_off_main_thread_x86-32.nexe"},
+    "x86-64": {"url": "ppapi_crash_off_main_thread_x86-64.nexe"},
+    "arm": {"url": "ppapi_crash_off_main_thread_arm.nexe"}
+  }
+}
diff --git a/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_ppapi_off_main_thread.cc b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_ppapi_off_main_thread.cc
new file mode 100644
index 0000000..f32b10f
--- /dev/null
+++ b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_ppapi_off_main_thread.cc
@@ -0,0 +1,40 @@
+// Copyright (c) 2011 The Native Client Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Test that we kill the nexe on a CHECK and handle it gracefully on the
+// trusted side when untrusted code makes unsupported PPAPI calls
+// on other than the main thread.
+
+#include "native_client/src/shared/platform/nacl_check.h"
+#include "native_client/tests/ppapi_test_lib/get_browser_interface.h"
+#include "native_client/tests/ppapi_test_lib/test_interface.h"
+#include "ppapi/c/ppb_url_request_info.h"
+
+namespace {
+
+void* CrashOffMainThreadFunction(void* thread_arg) {
+  printf("--- CrashPPAPIOffMainThreadFunction\n");
+  PPBURLRequestInfo()->Create(pp_instance());  // Fatal CHECK failure.
+  return NULL;
+}
+
+
+// This will crash PPP_Messaging::HandleMessage.
+void CrashPPAPIOffMainThread() {
+  printf("--- CrashPPAPIOffMainThread\n");
+  pthread_t tid;
+  void* thread_result;
+  pthread_create(&tid, NULL /*attr*/, CrashOffMainThreadFunction, NULL);
+  pthread_join(tid, &thread_result);  // Wait for the thread to crash.
+}
+
+}  // namespace
+
+void SetupTests() {
+  RegisterTest("CrashPPAPIOffMainThread", CrashPPAPIOffMainThread);
+}
+
+void SetupPluginInterfaces() {
+  // none
+}
diff --git a/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_ppapi_off_main_thread.nmf b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_ppapi_off_main_thread.nmf
new file mode 100644
index 0000000..56aa503
--- /dev/null
+++ b/ppapi/native_client/tests/ppapi_browser/crash/ppapi_crash_ppapi_off_main_thread.nmf
@@ -0,0 +1,7 @@
+{
+  "program": {
+    "x86-32": {"url": "ppapi_crash_ppapi_off_main_thread_x86-32.nexe"},
+    "x86-64": {"url": "ppapi_crash_ppapi_off_main_thread_x86-64.nexe"},
+    "arm": {"url": "ppapi_crash_ppapi_off_main_thread_arm.nexe"}
+  }
+}
-- 
cgit v1.1