Issue 1092005

src/trusted/plugin/srpc/plugin.cc

Index: src/trusted/plugin/srpc/plugin.cc
===================================================================
--- src/trusted/plugin/srpc/plugin.cc	(revision 1695)
+++ src/trusted/plugin/srpc/plugin.cc	(working copy)
@@ -16,6 +16,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 
+#include <list>
 #include <string>
 
 #include "native_client/src/include/portability_string.h"
@@ -58,10 +59,10 @@
       "__moduleReady", PROPERTY_GET, "", "i");
   AddMethodToMap(SetModuleReadyProperty,
       "__moduleReady", PROPERTY_SET, "i", "");
-  AddMethodToMap(GetSrcProperty,
-      "src", PROPERTY_GET, "", "s");
-  AddMethodToMap(SetSrcProperty,
-      "src", PROPERTY_SET, "s", "");
+  AddMethodToMap(GetNexeProperty,
+      "nexe", PROPERTY_GET, "", "s");
+  AddMethodToMap(SetNexeProperty,
+      "nexe", PROPERTY_SET, "s", "");
   AddMethodToMap(GetVideoUpdateModeProperty,
       "videoUpdateMode", PROPERTY_GET, "", "i");
   AddMethodToMap(SetVideoUpdateModeProperty,
@@ -234,14 +235,58 @@
   return false;
 }
 
-bool Plugin::GetSrcProperty(void *obj, SrpcParams *params) {
+bool Plugin::GetNexeProperty(void *obj, SrpcParams *params) {
   Plugin *plugin = reinterpret_cast<Plugin*>(obj);
   params->outs[0]->u.sval =
     PortablePluginInterface::MemAllocStrdup(plugin->local_url_);
   return true;
 }
 
-bool Plugin::SetSrcProperty(void *obj, SrpcParams *params) {
+// TODO(adonovan): Expose this as a user configuration option.
+static const enum { BEFORE_ARCH, AFTER_ARCH, NEVER } kTryPNaCl = BEFORE_ARCH;

[bsy, 2010/03/23 21:20:37]

might want ALWAYS/ONLY, for those people who prefer to ensure that the web stay
platform neutral. :-)

+// TODO(adonovan): Compatibility hack.  Allows x86 bigots to avoid
+// renaming their executables.  Not for release.
+static const bool kTryWithoutArchitecture = true;
+
+// Given a base nexe URL (architecture unspecified), computes the list
+// of URLs to try, e.g.:
+//
+//   <base>-armv7.nexe
+//   <base>.pnacl
+//   <base>.nexe
+//
+// The order and elements of the returned list depend on the options
+// above.
+static void GetNexeURLs(const std::string& base_url,
+                        std::list<std::string> *urls) {
+  // Issue a helpful warning, since we're making an incompatible change.
+  unsigned int idx = base_url.find(".nexe");
+  if (idx != std::string::npos && idx + 5 == base_url.size()) {
+    dprintf(("embed.nexe = %s; \".nexe\" suffix should be omitted",
+             base_url.c_str()));
+  }
+
+  if (kTryPNaCl == BEFORE_ARCH) urls->push_back(base_url + ".pnacl");
+
+  // TODO(adonovan): is there a better macro for us to use?
+  urls->push_back(base_url +
+#if NACL_TARGET_ARCH == x86 && NACL_TARGET_SUBARCH == 64
+      "-x86-64.nexe"

[bsy, 2010/03/23 21:20:37]

not sure i like '-' as separator, esp since tools that parse filenames will have
to understand that in this case, it's the 2nd '-' that is the actual separator.

i actually prefer the subdirectory approach, since in the limit, data files may
also be architecture dependent, and this turns into a general PATH-like search
mechanism.  ditto for dynamic library loading -- the app may want to load a
dynamic library, but should the user be able to specify whether the pnacl
version is tried first?  last?  etc.

+#elif NACL_TARGET_ARCH == x86 && NACL_TARGET_SUBARCH == 32
+      "-x86-32.nexe"
+#elif NACL_TARGET_ARCH == arm
+      "-arm.nexe"
+  // FIXME(adonovan): Shouldn't we be more specific than just "ARM",
+  // We should probably be as specific as uname -m, e.g. "armv7l"?
+#else
+#error "Unknown NACL_TARGET_ARCH/SUBARCH."
+#endif
+                  );
+  if (kTryPNaCl == AFTER_ARCH) urls->push_back(base_url + ".pnacl");
+  if (kTryWithoutArchitecture) urls->push_back(base_url);
+}
+
+bool Plugin::SetNexeProperty(void *obj, SrpcParams *params) {
   Plugin *plugin = reinterpret_cast<Plugin*>(obj);
   if (NULL != plugin->service_runtime_interface_) {
     dprintf(("Plugin::SetProperty: unloading previous\n"));
@@ -255,12 +300,17 @@
   }
   // Load the new module if the origin of the page is valid.
   const char* url = params->ins[0]->u.sval;
-  dprintf(("Plugin::SetProperty src = '%s'\n", url));
-  LoadNaClAppNotify* callback = new(std::nothrow) LoadNaClAppNotify(plugin,
-                                                                    url);
+  dprintf(("Plugin::SetProperty nexe = '%s'\n", url));
+  std::list<std::string> urls;
+  GetNexeURLs(url, &urls);
+  LoadNaClAppNotify* callback =
+      new(std::nothrow) LoadNaClAppNotify(plugin, urls);
   if ((NULL == callback) || (!callback->StartDownload())) {
     dprintf(("Failed to load URL to local file.\n"));
     // callback is always deleted in URLNotify
+    // FIXME(adonovan): it is?  even when StartDownload failed?
+    // FIXME(adonovan): shouldn't this result in the onfail handler
+    // getting called?

[bsy, 2010/03/23 21:20:37]

i think StartDownload always fails a download by calling URLNotify, since
originally this was single threaded and all that's done is to put something on a
work queue -- possibly having initiated the DNS lookup for the URL via the dns
helper process.  not sure what the actual reasons why StartDownload would fail
here are.

though perhaps the oom case probably should result in the on fail handlers being
called.

[adonovan, 2010/03/25 00:44:05]

You're right, the notification always occurs, even on failure.
Perhaps, but it doesn't seem particularly bad.  

     return false;
   }
   return true;