Test open_resource IRT in NewlibPackagedAppTest.SuccessfulLoad

ppapi/tests/extensions/packaged_app/test_packaged_app.cc

Index: ppapi/tests/extensions/packaged_app/test_packaged_app.cc
diff --git a/ppapi/tests/extensions/packaged_app/test_packaged_app.cc b/ppapi/tests/extensions/packaged_app/test_packaged_app.cc
index 754cc61809b0b9b750a24e1d58c55dfcb2cd8c89..39c1b1490efbb64ef0d16e2870a9c0366d8f9a99 100644
--- a/ppapi/tests/extensions/packaged_app/test_packaged_app.cc
+++ b/ppapi/tests/extensions/packaged_app/test_packaged_app.cc
@@ -2,19 +2,113 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <pthread.h>
+#include <unistd.h>
+
+#include <sstream>
+#include <string>
+
+#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"
 
 namespace {
 
+std::string g_last_error;
+pp::Instance* g_instance = NULL;
+
+// This should be the same as FileDescriptorSet::kMaxDescriptorsPerMessage in
+// ipc/file_descriptor_set_posix.h.

[Mark Seaborn, 2014/12/02 00:18:06]

Why not #include that and use that constant here?  Also, that constant is
currrently set to 7.

[Yusuke Sato, 2014/12/02 01:20:28]

This is because Chrome's base/ now strongly depends on C++11 and doesn't seem to
compile with some of NaCl toolchains at all.
Ok, changed to 7. I'll increase the number when I submit the other CL for
batch-opening resource files.

[Mark Seaborn, 2014/12/08 23:27:14]

Oh, I see.  That isn't obvious at all, so can you add a comment and a TODO to
fix that and use FileDescriptorSet::kMaxDescriptorsPerMessage?

How much of base/ fails to compile for NaCl tests?  We knew this would be likely
to happen, and that we'd have to decide which tests we continue to build against
the old nacl-gcc toolchains.

+dschuff (who is working on the nacl-clang toolchain).  Derek, could you
investigate the test situation and maybe file an issue about this?

[Yusuke Sato, 2014/12/09 03:45:54]

Added a TODO. Done.
Since I only tried to include "ipc/file_descriptor_set_posix.h", I'm not sure
how much of base/ is incompatible with NaCl toolchains unfortunately. As for
ipc/file_descriptor_set_posix.h, it's incompatible because some of the
toolchains don't support 'override' and 'nullptr'.

+const size_t kMaxDescriptorsPerMessage = 128;
+
+// Returns true if the resource file whose name is |key| exists and its content
+// matches |content|.
+bool LoadManifestInternal(TYPE_nacl_irt_query* query_func,

[Mark Seaborn, 2014/12/02 00:18:06]

You can just call nacl_interface_query() here rather than passing a function
pointer around.

[Yusuke Sato, 2014/12/02 01:20:27]

Done.

+                          const std::string& key,
+                          const std::string& content) {
+  struct nacl_irt_resource_open nacl_irt_resource_open;
+  if (sizeof(nacl_irt_resource_open) !=
+      (*query_func)(NACL_IRT_RESOURCE_OPEN_v0_1, &nacl_irt_resource_open,
+                    sizeof(nacl_irt_resource_open))) {
+    g_last_error = "irt manifest api not found";
+    return false;
+  }
+  int desc;
+  int error;
+  error = nacl_irt_resource_open.open_resource(key.c_str(), &desc);
+  if (0 != error) {
+    g_last_error = "Can't open file " + key;
+    return false;
+  }
+
+  std::string str;
+
+  char buffer[4096];
+  int len;
+  while ((len = read(desc, buffer, sizeof(buffer) - 1)) > 0) {
+    // Null terminate.
+    buffer[len] = '\0';
+    str += buffer;
+  }
+  // Do not call close(desc) so that LoadManifest calls the second

[Mark Seaborn, 2014/12/02 00:18:06]

Not sure I understand this comment.  Are you saying that calling close() could
cause the FD (and not the FD table index) to be reused?

[Yusuke Sato, 2014/12/02 01:20:27]

Removed the comment and added close(desc) instead. Since closed FDs will never
be reused, this test code was just confusing.

+  // open_resource without closing the first one.
+
+  if (str != content) {
+    g_last_error = "Wrong file content: file=" + key + ", expected=" + content +
+                   ", actual=" + str;
+    return false;
+  }
+
+  return true;
+}
+
+// Tests if open_resource works in a packaged app. This test is similar to
+// NaClBrowserTest*.IrtManifestFile, but unlike the NaCl test, this one tests
+// the "fast path" in DownloadNexe() in ppb_nacl_private_impl.cc which opens
+// resource files without using URLLoader.
+void LoadManifest(TYPE_nacl_irt_query* query_func) {
+  for (size_t i = 0; i <= kMaxDescriptorsPerMessage; ++i) {
+    std::stringstream key;
+    key << "test_file" << i;
+    std::string content = "Test File Content" + std::string(i % 2 ? "2" : "");
+    if (!LoadManifestInternal(query_func, key.str(), content))
+      break;
+    // Open the same resource file again to make sure each file descriptor
+    // returned from open_resource has its own file offset.
+    if (!LoadManifestInternal(query_func, key.str(), content))
+      break;
+  }
+}
+
+void PostReply(void* user_data, int32_t status) {
+  if (!g_last_error.empty())
+    g_instance->PostMessage(g_last_error.c_str());
+  else
+    g_instance->PostMessage("hello");
+}
+
+void* RunTestsOnBackgroundThread(void* thread_id) {
+  LoadManifest(&__nacl_irt_query);

[Mark Seaborn, 2014/12/02 00:18:06]

This should use nacl_interface_query(), defined in <irt.h>, rather than
__nacl_irt_query, which is internal.

[Yusuke Sato, 2014/12/02 01:20:28]

Done.

+  pp::Module::Get()->core()->CallOnMainThread(
+      0, pp::CompletionCallback(&PostReply, NULL));
+  return NULL;
+}
+
 class MyInstance : public pp::Instance {
  public:
-  explicit MyInstance(PP_Instance instance) : pp::Instance(instance) { }
+  explicit MyInstance(PP_Instance instance) : pp::Instance(instance) {
+    g_instance = this;
+  }
   virtual ~MyInstance() { }
 
   virtual bool Init(uint32_t argc, const char* argn[], const char* argv[]) {
-    PostMessage("hello");
+    pthread_t thread;
+    pthread_create(&thread, NULL, &RunTestsOnBackgroundThread, NULL);

[Mark Seaborn, 2014/12/02 00:18:06]

Nit: check return value, if only with CHECK (or you could use thread-creation
functions from base/).

Can you add a comment to say why this is run on a background thread?

[Yusuke Sato, 2014/12/02 01:20:27]

Done.

+    pthread_detach(thread);
     return true;
   }
 };