NaCl: Remove SRPC-based manifest tests.

chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc

Index: chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc
diff --git a/chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc b/chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc
index 08a5c8b2497fc3140813b1d7ef82007cd152bfaa..c29dc83d838a342a1508accb69dbf12e0e012ab7 100644
--- a/chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc
+++ b/chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc
@@ -18,6 +18,7 @@
 #include "native_client/src/untrusted/irt/irt.h"
 #include "native_client/src/untrusted/nacl/nacl_irt.h"
 
+#include "ppapi/cpp/completion_callback.h"
 #include "ppapi/cpp/instance.h"
 #include "ppapi/cpp/module.h"
 #include "ppapi/cpp/var.h"
@@ -27,6 +28,8 @@
 
 std::vector<std::string> result;
 
+pp::Instance* g_instance = NULL;
+
 std::string LoadManifestSuccess(TYPE_nacl_irt_query *query_func) {
   struct nacl_irt_resource_open nacl_irt_resource_open;
   if (sizeof(nacl_irt_resource_open) !=
@@ -112,9 +115,32 @@ std::string LoadManifestNonExistentFile(
   return "Pass";
 }
 
+void RunTests() {
+  result.push_back(LoadManifestSuccess(&__nacl_irt_query));
+  result.push_back(LoadManifestNonExistentEntry(&__nacl_irt_query));
+  result.push_back(LoadManifestNonExistentFile(&__nacl_irt_query));
+}
+
+void PostReply(void* user_data, int32_t status) {
+  pp::VarArray reply = pp::VarArray();
+  for (size_t i = 0; i < result.size(); ++i)
+    reply.Set(i, pp::Var(result[i]));
+  g_instance->PostMessage(reply);
+}
+
+void* RunTestsOnBackgroundThread(void *thread_id) {
+  RunTests();
+  pp::Module::Get()->core()->CallOnMainThread(

[Mark Seaborn, 2014/09/04 17:41:36]

BTW, why does this require a CallOnMainThread()?  I thought
g_instance->PostMessage() would work on any thread.

[teravest, 2014/09/04 20:21:28]

pp::Instance::PostMessage() has to be called from the main thread. I could call
this from a background thread if I used PPB_Instance, it sounds like.

[Mark Seaborn, 2014/09/04 22:00:41]

Hmm, you mean the C++ wrappers for PPAPI add a thread restriction?

[teravest, 2014/09/04 22:12:39]

I believe that only happens for pp::Instance.
There's an explanatory comment here:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/instance...

And the pp::Instance object is deleted here:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/module.c...

Thanks to dmichael@ for pointing me at the details of this one.

[dmichael (off chromium), 2014/09/05 19:44:08]

In theory it doesn't matter for NaCl, since we just kill the process without
calling DidDestroy. But it's safer to not make assumptions about pp::Instance's
lifetime from other threads. pp::Instance's API doesn't really make it possible
for it to be thread-safe, unfortunately...  but it was designed before we had
thread-safe Pepper calls.

Most of the C++ wrappers work fine because the object is really just a handle
that holds a PP_Resource (and addrefs and removerefs the resource at appropriate
times). If the same PP_Resource is used from multiple threads, the best thing
would be for each thread to have its own pp::Foo (since it's just a handle
anyway).

+      0, pp::CompletionCallback(&PostReply, NULL));
+  return NULL;
+}
+
 class TestInstance : public pp::Instance {
  public:
-  explicit TestInstance(PP_Instance instance) : pp::Instance(instance) {}
+  explicit TestInstance(PP_Instance instance) : pp::Instance(instance) {
+    g_instance = this;
+  }
+
   virtual ~TestInstance() {}
   virtual void HandleMessage(const pp::Var& var_message) {
     if (!var_message.is_string()) {
@@ -123,12 +149,16 @@ class TestInstance : public pp::Instance {
     if (var_message.AsString() != "hello") {
       return;
     }
-    pp::VarArray reply = pp::VarArray();
-    for (size_t i = 0; i < result.size(); ++i) {
-      reply.Set(i, pp::Var(result[i]));
-    }
-    PostMessage(reply);
+
+    // We test the manifest routines again after PPAPI has initialized to
+    // ensure that they still work.
+    //
+    // irt_open_resource() isn't allowed to be called on the main thread once
+    // pepper starts, so these tests must happen on a background thread.
+    pthread_create(&thread_, NULL, &RunTestsOnBackgroundThread, NULL);

[Mark Seaborn, 2014/09/04 17:41:36]

Nit: can you check the return value?

Nit: thread_ could be a local var instead -- doesn't need to be a member.

[teravest, 2014/09/04 20:21:28]

Sure, do you want me to write a change to fix up these nits?

[Mark Seaborn, 2014/09/04 22:00:41]

It's your call -- I'll leave it up to you.

   }
+ private:
+  pthread_t thread_;
 };
 
 class TestModule : public pp::Module {
@@ -148,8 +178,6 @@ Module* CreateModule() {
 }
 
 int main() {
-  result.push_back(LoadManifestSuccess(&__nacl_irt_query));
-  result.push_back(LoadManifestNonExistentEntry(&__nacl_irt_query));
-  result.push_back(LoadManifestNonExistentFile(&__nacl_irt_query));
+  RunTests();
   return PpapiPluginMain();
 }