Used condvar timed wait instead of indefinite wait for WaitForSelLdrStart.

There are various reasons that the condvar signal will never happen:
- We currently do a CallOnMainThread in StartSelLdrContinuation
  to invoke the callback that does the signaling. However, that
  CallOnMainThread might not get scheduled as we are tearing down
  the embed.
- The IPC reply from the browser -> renderer might get completely
  dropped as the embed is being destroyed, so we may never even
  get to StartSelLdrContinuation.

Why care about the condvar being blocked at all?

The ~Plugin is blocked on ~PnaclCoordinator, which is
blocked on trying to join the PnaclTranslateThread's thread. However
if the surfaway is "early enough" then PnaclTranslateThread's thread
may be waiting for SelLdrStart (the condvar).

Another reason that ~PnaclCoordinator on the main thread
could be blocked is that it attempts to acquire the
subprocess_mu_. However, the subprocess_mu_ is being held
by the translate thread while it is waiting on the condvar.
I tried shifting the lock acquisition until after the
process is started and to the point that the code is
actually trying to write to the shared field. However, that
doesn't help because there is still the thread join... and
we need the thread join because it protects against
the thread using fields of PnaclCoordinator while it's
being free'd.

Use timeout in background thread NaCl process startup condvar.
Don't wait too long though, because the renderer is hung
in the meantime.

There are other places we block indefinitely on condvars
(reverse service thread), which show up in crash reports
(see the BUG), but that's not addressed here.

BUG=350426
REPRO (but not TEST)= www/~jvoung/test_embed_kills/
not yet sure of how to make a test that isn't flaky, since you need to fuzz the surfaway time a bit to trigger (~150 ms) and the cond var timed-wait will make tests that do repro slow.

R=dschuff@chromium.org, teravest@chromium.org

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

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

3 files changed, 34 insertions, 8 deletions

diff --git a/ppapi/native_client/src/trusted/plugin/plugin.cc b/ppapi/native_client/src/trusted/plugin/plugin.cc
index 4fd6ba0..3f2d002 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.cc
+++ b/ppapi/native_client/src/trusted/plugin/plugin.cc
@@ -217,7 +217,7 @@ bool Plugin::LoadNaClModuleFromBackgroundThread(
                  static_cast<void*>(service_runtime)));
 
   // Now start the SelLdr instance.  This must be created on the main thread.
-  bool service_runtime_started;
+  bool service_runtime_started = false;
   pp::CompletionCallback sel_ldr_callback =
       callback_factory_.NewCallback(&Plugin::SignalStartSelLdrDone,
                                     &service_runtime_started,
@@ -227,7 +227,11 @@ bool Plugin::LoadNaClModuleFromBackgroundThread(
                                     service_runtime, params,
                                     sel_ldr_callback);
   pp::Module::Get()->core()->CallOnMainThread(0, callback, 0);
-  service_runtime->WaitForSelLdrStart();
+  if (!service_runtime->WaitForSelLdrStart()) {
+    PLUGIN_PRINTF(("Plugin::LoadNaClModuleFromBackgroundThread "
+                   "WaitForSelLdrStart timed out!\n"));
+    return false;
+  }
   PLUGIN_PRINTF(("Plugin::LoadNaClModuleFromBackgroundThread "
                  "(service_runtime_started=%d)\n",
                  service_runtime_started));
diff --git a/ppapi/native_client/src/trusted/plugin/service_runtime.cc b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
index acf14c7..2a3048e 100644
--- a/ppapi/native_client/src/trusted/plugin/service_runtime.cc
+++ b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
@@ -608,11 +608,30 @@ void ServiceRuntime::StartSelLdrContinuation(int32_t pp_error,
   pp::Module::Get()->core()->CallOnMainThread(0, callback, pp_error);
 }
 
-void ServiceRuntime::WaitForSelLdrStart() {
+bool ServiceRuntime::WaitForSelLdrStart() {
+  // Time to wait on condvar (for browser to create a new sel_ldr process on
+  // our behalf). Use 6 seconds to be *fairly* conservative.
+  //
+  // On surfaway, the CallOnMainThread above may never get scheduled
+  // to unblock this condvar, or the IPC reply from the browser to renderer
+  // might get canceled/dropped. However, it is currently important to
+  // avoid waiting indefinitely because ~PnaclCoordinator will attempt to
+  // join() the PnaclTranslateThread, and the PnaclTranslateThread is waiting
+  // for the signal before exiting.
+  static int64_t const kWaitTimeMicrosecs = 6 * NACL_MICROS_PER_UNIT;
+  int64_t left_to_wait = kWaitTimeMicrosecs;
+  int64_t deadline = NaClGetTimeOfDayMicroseconds() + left_to_wait;
   nacl::MutexLocker take(&mu_);
-  while(!start_sel_ldr_done_) {
-    NaClXCondVarWait(&cond_, &mu_);
+  while(!start_sel_ldr_done_ && left_to_wait > 0) {
+    struct nacl_abi_timespec left_timespec;
+    left_timespec.tv_sec = left_to_wait / NACL_MICROS_PER_UNIT;
+    left_timespec.tv_nsec =
+        (left_to_wait % NACL_MICROS_PER_UNIT) * NACL_NANOS_PER_MICRO;
+    NaClXCondVarTimedWaitRelative(&cond_, &mu_, &left_timespec);
+    int64_t now = NaClGetTimeOfDayMicroseconds();
+    left_to_wait = deadline - now;
   }
+  return start_sel_ldr_done_;
 }
 
 void ServiceRuntime::SignalStartSelLdrDone() {
diff --git a/ppapi/native_client/src/trusted/plugin/service_runtime.h b/ppapi/native_client/src/trusted/plugin/service_runtime.h
index 995555c..79ef7c2 100644
--- a/ppapi/native_client/src/trusted/plugin/service_runtime.h
+++ b/ppapi/native_client/src/trusted/plugin/service_runtime.h
@@ -222,10 +222,13 @@ class ServiceRuntime {
                    pp::CompletionCallback callback);
 
   // If starting sel_ldr from a background thread, wait for sel_ldr to
-  // actually start.
-  void WaitForSelLdrStart();
+  // actually start. Returns |false| if timed out waiting for the process
+  // to start. Otherwise, returns |true| if StartSelLdr is complete
+  // (either successfully or unsuccessfully).
+  bool WaitForSelLdrStart();
 
-  // Signal to waiting threads that StartSelLdr is complete.
+  // Signal to waiting threads that StartSelLdr is complete (either
+  // successfully or unsuccessfully).
   // Done externally, in case external users want to write to shared
   // memory that is yet to be fenced.
   void SignalStartSelLdrDone();