PPAPI: Make blocking completion callbacks work.

This also makes scoped_refptr<TrackedCallback> the "new" way to pass completion callbacks in an API. This allows the Enter object to handle checking for blocking callbacks on the main thread to report error, and blocking if on the background thread. This way, interfaces don't have to write any special cases for blocking callbacks.

When built with enable_pepper_threading=1 locally, URLLoader tests all pass for blocking completion callbacks. I haven't updated all tests yet.

BUG=92909
TEST=


Review URL: https://chromiumcodereview.appspot.com/10081020

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

5 files changed, 188 insertions, 75 deletions

diff --git a/ppapi/tests/test_broker.cc b/ppapi/tests/test_broker.cc
index 478168b..ab27b0b 100644
--- a/ppapi/tests/test_broker.cc
+++ b/ppapi/tests/test_broker.cc
@@ -212,8 +212,8 @@ void TestBroker::RunTests(const std::string& filter) {
   RUN_TEST(Create, filter);
   RUN_TEST(Create, filter);
   RUN_TEST(GetHandleFailure, filter);
-  RUN_TEST(ConnectFailure, filter);
-  RUN_TEST(ConnectAndPipe, filter);
+  RUN_TEST_FORCEASYNC_AND_NOT(ConnectFailure, filter);
+  RUN_TEST_FORCEASYNC_AND_NOT(ConnectAndPipe, filter);
 }
 
 std::string TestBroker::TestCreate() {
@@ -230,20 +230,11 @@ std::string TestBroker::TestCreate() {
 
 // Test connection on invalid resource.
 std::string TestBroker::TestConnectFailure() {
-  // Callback NOT force async. Connect should fail.  The callback will not be
-  // posted so there's no need to wait for the callback to complete.
-  TestCompletionCallback cb_1(instance_->pp_instance(), false);
-  ASSERT_EQ(PP_ERROR_BADRESOURCE,
-            broker_interface_->Connect(
-                0, pp::CompletionCallback(cb_1).pp_completion_callback()));
-
-  // Callback force async. Connect will return PP_OK_COMPLETIONPENDING and the
-  // callback will be posted.  However, the callback should fail.
-  TestCompletionCallback cb_2(instance_->pp_instance(), true);
-  ASSERT_EQ(PP_OK_COMPLETIONPENDING,
-            broker_interface_->Connect(
-                0, pp::CompletionCallback(cb_2).pp_completion_callback()));
-  ASSERT_EQ(PP_ERROR_BADRESOURCE, cb_2.WaitForResult());
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  callback.WaitForResult(broker_interface_->Connect(0,
+      callback.GetCallback().pp_completion_callback()));
+  CHECK_CALLBACK_BEHAVIOR(callback);
+  ASSERT_EQ(PP_ERROR_BADRESOURCE, callback.result());
 
   PASS();
 }
@@ -268,11 +259,11 @@ std::string TestBroker::TestConnectAndPipe() {
       instance_->pp_instance());
   ASSERT_TRUE(broker);
 
-  TestCompletionCallback cb_3(instance_->pp_instance());
-  ASSERT_EQ(PP_OK_COMPLETIONPENDING,
-            broker_interface_->Connect(
-                broker, pp::CompletionCallback(cb_3).pp_completion_callback()));
-  ASSERT_EQ(PP_OK, cb_3.WaitForResult());
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  callback.WaitForResult(broker_interface_->Connect(broker,
+      callback.GetCallback().pp_completion_callback()));
+  CHECK_CALLBACK_BEHAVIOR(callback);
+  ASSERT_EQ(PP_OK, callback.result());
 
   int32_t handle = kInvalidHandle;
   ASSERT_EQ(PP_OK, broker_interface_->GetHandle(broker, &handle));
diff --git a/ppapi/tests/test_case.cc b/ppapi/tests/test_case.cc
index 26f6d40..f6fb485 100644
--- a/ppapi/tests/test_case.cc
+++ b/ppapi/tests/test_case.cc
@@ -6,6 +6,7 @@
 
 #include <sstream>
 
+#include "ppapi/tests/pp_thread.h"
 #include "ppapi/tests/test_utils.h"
 #include "ppapi/tests/testing_instance.h"
 
@@ -107,8 +108,10 @@ bool TestCase::MatchesFilter(const std::string& test_name,
   return filter.empty() || (test_name == filter);
 }
 
-std::string TestCase::CheckResourcesAndVars() {
-  std::string errors;
+std::string TestCase::CheckResourcesAndVars(std::string errors) {
+  if (!errors.empty())
+    return errors;
+
   if (testing_interface_) {
     // TODO(dmichael): Fix tests that leak resources and enable the following:
     /*
@@ -143,4 +146,27 @@ std::string TestCase::CheckResourcesAndVars() {
   return errors;
 }
 
+// static
+void TestCase::QuitMainMessageLoop(PP_Instance instance) {
+  PP_Instance* heap_instance = new PP_Instance(instance);
+  pp::CompletionCallback callback(&DoQuitMainMessageLoop, heap_instance);
+  pp::Module::Get()->core()->CallOnMainThread(0, callback);
+}
 
+// static
+void TestCase::DoQuitMainMessageLoop(void* pp_instance, int32_t result) {
+  PP_Instance* instance = static_cast<PP_Instance*>(pp_instance);
+  GetTestingInterface()->QuitMessageLoop(*instance);
+  delete instance;
+}
+
+void TestCase::RunOnThreadInternal(void (*thread_func)(void*),
+                                   void* thread_param,
+                                   const PPB_Testing_Dev* testing_interface) {
+    PP_ThreadType thread;
+    PP_CreateThread(&thread, thread_func, thread_param);
+    // Run a message loop so pepper calls can be dispatched. The background
+    // thread will set result_ and make us Quit when it's done.
+    testing_interface->RunMessageLoop(instance_->pp_instance());
+    PP_JoinThread(thread);
+}
diff --git a/ppapi/tests/test_case.h b/ppapi/tests/test_case.h
index 2973084..2cf2ddf 100644
--- a/ppapi/tests/test_case.h
+++ b/ppapi/tests/test_case.h
@@ -12,9 +12,11 @@
 
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/c/dev/ppb_testing_dev.h"
+#include "ppapi/cpp/dev/message_loop_dev.h"
 #include "ppapi/cpp/dev/scrollbar_dev.h"
 #include "ppapi/cpp/view.h"
 #include "ppapi/tests/test_utils.h"
+#include "ppapi/tests/testing_instance.h"
 
 #if (defined __native_client__)
 #include "ppapi/cpp/var.h"
@@ -78,6 +80,8 @@ class TestCase {
 
   const PPB_Testing_Dev* testing_interface() { return testing_interface_; }
 
+  static void QuitMainMessageLoop(PP_Instance instance);
+
  protected:
 #if !(defined __native_client__)
   // Overridden by each test to supply a ScriptableObject corresponding to the
@@ -104,12 +108,37 @@ class TestCase {
   // Check for leaked resources and vars at the end of the test. If any exist,
   // return a string with some information about the error. Otherwise, return
   // an empty string.
-  std::string CheckResourcesAndVars();
+  //
+  // You should pass the error string from the test so far; if it is non-empty,
+  // CheckResourcesAndVars will do nothing and return the same string.
+  std::string CheckResourcesAndVars(std::string errors);
+
+  // Run the given test method on a background thread and return the result.
+  template <class T>
+  std::string RunOnThread(std::string(T::*test_to_run)()) {
+#ifdef ENABLE_PEPPER_THREADING
+    if (!testing_interface_) {
+      return "Testing blocking callbacks requires the testing interface. In "
+             "Chrome, use the --enable-pepper-testing flag.";
+    }
+    // These tests are only valid if running out-of-process (threading is not
+    // supported in-process). Just consider it a pass.
+    if (!testing_interface_->IsOutOfProcess())
+      return std::string();
+    ThreadedTestRunner<T> runner(instance_->pp_instance(),
+        static_cast<T*>(this), test_to_run);
+    RunOnThreadInternal(&ThreadedTestRunner<T>::ThreadFunction, &runner,
+                        testing_interface_);
+    return runner.result();
+#else
+    // If threading's not enabled, just treat it as success.
+    return std::string();
+#endif
+  }
 
   // Pointer to the instance that owns us.
   TestingInstance* instance_;
 
- protected:
   // NULL unless InitTestingInterface is called.
   const PPB_Testing_Dev* testing_interface_;
 
@@ -127,6 +156,49 @@ class TestCase {
   }
 
  private:
+  template <class T>
+  class ThreadedTestRunner {
+   public:
+    typedef std::string(T::*TestMethodType)();
+    ThreadedTestRunner(PP_Instance instance,
+                       T* test_case,
+                       TestMethodType test_to_run)
+        : instance_(instance),
+          test_case_(test_case),
+          test_to_run_(test_to_run) {
+    }
+    const std::string& result() { return result_; }
+    static void ThreadFunction(void* runner) {
+      static_cast<ThreadedTestRunner<T>*>(runner)->Run();
+    }
+
+   private:
+    void Run() {
+      // TODO(dmichael): Create and attach a pp::MessageLoop for this thread so
+      //                 nested loops work.
+      result_ = (test_case_->*test_to_run_)();
+      // Tell the main thread to quit its nested message loop, now that the test
+      // is complete.
+      TestCase::QuitMainMessageLoop(instance_);
+    }
+
+    std::string result_;
+    PP_Instance instance_;
+    T* test_case_;
+    TestMethodType test_to_run_;
+  };
+
+  // The internals for RunOnThread. This allows us to avoid including
+  // pp_thread.h in this header file, since it includes system headers like
+  // windows.h.
+  // RunOnThreadInternal launches a new thread to run |thread_func|, waits
+  // for it to complete using RunMessageLoop(), then joins.
+  void RunOnThreadInternal(void (*thread_func)(void*),
+                           void* thread_param,
+                           const PPB_Testing_Dev* testing_interface);
+
+  static void DoQuitMainMessageLoop(void* pp_instance, int32_t result);
+
   // Passed when creating completion callbacks in some tests. This determines
   // what kind of callback we use for the test.
   CallbackType callback_type_;
@@ -184,12 +256,39 @@ class TestCaseFactory {
 #define RUN_TEST(name, test_filter) \
   if (MatchesFilter(#name, test_filter)) { \
     set_callback_type(PP_OPTIONAL); \
-    std::string error_message = Test##name(); \
-    if (error_message.empty()) \
-      error_message = CheckResourcesAndVars(); \
-    instance_->LogTest(#name, error_message); \
+    instance_->LogTest(#name, CheckResourcesAndVars(Test##name())); \
+  }
+
+// Like RUN_TEST above but forces functions taking callbacks to complete
+// asynchronously on success or error.
+#define RUN_TEST_FORCEASYNC(name, test_filter) \
+  if (MatchesFilter(#name, test_filter)) { \
+    set_callback_type(PP_REQUIRED); \
+    instance_->LogTest(#name"ForceAsync", \
+                       CheckResourcesAndVars(Test##name())); \
   }
 
+#define RUN_TEST_BLOCKING(test_case, name, test_filter) \
+  if (MatchesFilter(#name, test_filter)) { \
+    set_callback_type(PP_BLOCKING); \
+    instance_->LogTest(#name"Blocking", \
+        CheckResourcesAndVars(RunOnThread(&test_case::Test##name))); \
+  }
+
+#define RUN_TEST_FORCEASYNC_AND_NOT(name, test_filter) \
+  do { \
+    RUN_TEST_FORCEASYNC(name, test_filter); \
+    RUN_TEST(name, test_filter); \
+  } while (false)
+
+// Run a test with all possible callback types.
+#define RUN_CALLBACK_TEST(test_case, name, test_filter) \
+  do { \
+    RUN_TEST_FORCEASYNC(name, test_filter); \
+    RUN_TEST(name, test_filter); \
+    RUN_TEST_BLOCKING(test_case, name, test_filter); \
+  } while (false)
+
 #define RUN_TEST_WITH_REFERENCE_CHECK(name, test_filter) \
   if (MatchesFilter(#name, test_filter)) { \
     set_callback_type(PP_OPTIONAL); \
@@ -204,21 +303,6 @@ class TestCaseFactory {
     instance_->LogTest(#name, error_message); \
   }
 
-// Like RUN_TEST above but forces functions taking callbacks to complete
-// asynchronously on success or error.
-#define RUN_TEST_FORCEASYNC(name, test_filter) \
-  if (MatchesFilter(#name"ForceAsync", test_filter)) { \
-    set_callback_type(PP_REQUIRED); \
-    instance_->LogTest(#name"ForceAsync", Test##name()); \
-  }
-
-#define RUN_TEST_FORCEASYNC_AND_NOT(name, test_filter) \
-  do { \
-    RUN_TEST_FORCEASYNC(name, test_filter); \
-    RUN_TEST(name, test_filter); \
-  } while (false)
-
-
 // Helper macros for checking values in tests, and returning a location
 // description of the test fails.
 #define ASSERT_TRUE(cmd) \
diff --git a/ppapi/tests/test_url_loader.cc b/ppapi/tests/test_url_loader.cc
index 15ea546..9024491 100644
--- a/ppapi/tests/test_url_loader.cc
+++ b/ppapi/tests/test_url_loader.cc
@@ -33,8 +33,9 @@ namespace {
 int32_t WriteEntireBuffer(PP_Instance instance,
                           pp::FileIO* file_io,
                           int32_t offset,
-                          const std::string& data) {
-  TestCompletionCallback callback(instance);
+                          const std::string& data,
+                          CallbackType callback_type) {
+  TestCompletionCallback callback(instance, callback_type);
   int32_t write_offset = offset;
   const char* buf = data.c_str();
   int32_t size = data.size();
@@ -95,29 +96,29 @@ bool TestURLLoader::Init() {
 }
 
 void TestURLLoader::RunTests(const std::string& filter) {
-  RUN_TEST_FORCEASYNC_AND_NOT(BasicGET, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(BasicPOST, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(BasicFilePOST, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(BasicFileRangePOST, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(CompoundBodyPOST, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(EmptyDataPOST, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(BinaryDataPOST, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(CustomRequestHeader, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(FailsBogusContentLength, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(StreamToFile, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(UntrustedSameOriginRestriction, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(TrustedSameOriginRestriction, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(UntrustedCrossOriginRequest, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(TrustedCrossOriginRequest, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(UntrustedJavascriptURLRestriction, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(TrustedJavascriptURLRestriction, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(UntrustedHttpRequests, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(TrustedHttpRequests, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(FollowURLRedirect, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(AuditURLRedirect, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(AbortCalls, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(UntendedLoad, filter);
-  RUN_TEST_FORCEASYNC_AND_NOT(PrefetchBufferThreshold, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, BasicGET, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, BasicPOST, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, BasicFilePOST, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, BasicFileRangePOST, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, CompoundBodyPOST, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, EmptyDataPOST, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, BinaryDataPOST, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, CustomRequestHeader, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, FailsBogusContentLength, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, StreamToFile, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, UntrustedSameOriginRestriction, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, TrustedSameOriginRestriction, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, UntrustedCrossOriginRequest, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, TrustedCrossOriginRequest, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, UntrustedJavascriptURLRestriction, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, TrustedJavascriptURLRestriction, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, UntrustedHttpRequests, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, TrustedHttpRequests, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, FollowURLRedirect, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, AuditURLRedirect, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, AbortCalls, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, UntendedLoad, filter);
+  RUN_CALLBACK_TEST(TestURLLoader, PrefetchBufferThreshold, filter);
 }
 
 std::string TestURLLoader::ReadEntireFile(pp::FileIO* file_io,
@@ -222,7 +223,8 @@ int32_t TestURLLoader::PrepareFileForPost(
     return callback.result();
   }
 
-  int32_t rv = WriteEntireBuffer(instance_->pp_instance(), &file_io, 0, data);
+  int32_t rv = WriteEntireBuffer(instance_->pp_instance(), &file_io, 0, data,
+                                 callback_type());
   if (rv != PP_OK) {
     message->assign("FileIO::Write failed.");
     return rv;
@@ -787,8 +789,13 @@ std::string TestURLLoader::TestUntendedLoad() {
           total_bytes_to_be_received);
     if (bytes_received == total_bytes_to_be_received)
       break;
-    pp::Module::Get()->core()->CallOnMainThread(10, callback);
-    callback.WaitForResult();
+    // TODO(dmichael): This should probably compare pp::MessageLoop::GetCurrent
+    //                 with GetForMainThread. We only need to yield on the main
+    //                 thread.
+    if (callback_type() != PP_BLOCKING) {
+      pp::Module::Get()->core()->CallOnMainThread(10, callback);
+      callback.WaitForResult();
+    }
   }
 
   // The loader should now have the data and have finished successfully.
diff --git a/ppapi/tests/test_utils.cc b/ppapi/tests/test_utils.cc
index 7405e93..cae96dc 100644
--- a/ppapi/tests/test_utils.cc
+++ b/ppapi/tests/test_utils.cc
@@ -97,6 +97,7 @@ TestCompletionCallback::TestCompletionCallback(PP_Instance instance)
       //                 what the tests currently expect.
       callback_type_(PP_OPTIONAL),
       post_quit_task_(false),
+      run_count_(0),  // TODO(dmichael): Remove when all tests are updated.
       instance_(instance) {
 }
 
@@ -172,10 +173,10 @@ void TestCompletionCallback::WaitForAbortResult(int32_t result) {
                       final_result));
       return;
     }
-  } else if (result != PP_OK) {
+  } else if (result < PP_OK) {
     errors_.assign(
-        ReportError("TestCompletionCallback: Expected PP_ERROR_ABORTED or"
-                    "PP_OK. Ran synchronously.",
+        ReportError("TestCompletionCallback: Expected PP_ERROR_ABORTED or "
+                    "non-error response. Ran synchronously.",
                     result));
     return;
   }
@@ -198,6 +199,7 @@ void TestCompletionCallback::Reset() {
   result_ = PP_OK_COMPLETIONPENDING;
   have_result_ = false;
   post_quit_task_ = false;
+  run_count_ = 0;  // TODO(dmichael): Remove when all tests are updated.
   errors_.clear();
 }
 
@@ -205,9 +207,12 @@ void TestCompletionCallback::Reset() {
 void TestCompletionCallback::Handler(void* user_data, int32_t result) {
   TestCompletionCallback* callback =
       static_cast<TestCompletionCallback*>(user_data);
+  // If this check fails, it means that the callback was invoked twice or that
+  // the PPAPI call completed synchronously, but also ran the callback.
   PP_DCHECK(!callback->have_result_);
   callback->result_ = result;
   callback->have_result_ = true;
+  callback->run_count_++;  // TODO(dmichael): Remove when all tests are updated.
   if (callback->post_quit_task_) {
     callback->post_quit_task_ = false;
     GetTestingInterface()->QuitMessageLoop(callback->instance_);