Pepper: Simplify OpenResource() for Non-SFI.

The Non-SFI implementation of OpenResource is pretty complicated. This is
because the logic to support that operation was entirely in the trusted
plugin. Now, we can perform the necessary logic entirely in Chromium, so
it can be made much simpler.

CQ_EXTRA_TRYBOTS=tryserver.chromium:linux_rel_precise32
BUG=239656

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

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

components/nacl/renderer/ppb_nacl_private_impl.cc

@@ -113,6 +113,12 @@ nacl::NexeLoadManager* GetNexeLoadManager(PP_Instance instance) {
   return NULL;
 }
 
+static const PP_NaClFileInfo kInvalidNaClFileInfo = {
+    PP_kInvalidFileHandle,
+    0,  // token_lo
+    0,  // token_hi
+};
+
 int GetRoutingID(PP_Instance instance) {
   // Check that we are on the main renderer thread.
   DCHECK(content::RenderThread::Get());
@@ -145,15 +151,30 @@ void PostPPCompletionCallback(PP_CompletionCallback callback,
       base::Bind(callback.func, callback.user_data, status));
 }
 
+bool ManifestResolveKey(PP_Instance instance,
+                        bool is_helper_process,
+                        const std::string& key,
+                        std::string* full_url,
+                        PP_PNaClOptions* pnacl_options);
+
+typedef base::Callback<void(int32_t, const PP_NaClFileInfo&)>
+DownloadFileCallback;
+
+void DownloadFile(PP_Instance instance,
+                  const char* url,
+                  const DownloadFileCallback& callback);
+
 // Thin adapter from PPP_ManifestService to ManifestServiceChannel::Delegate.
 // Note that user_data is managed by the caller of LaunchSelLdr. Please see
 // also PP_ManifestService's comment for more details about resource
 // management.
 class ManifestServiceProxy : public ManifestServiceChannel::Delegate {
  public:
-  ManifestServiceProxy(const PPP_ManifestService* manifest_service,
+  ManifestServiceProxy(PP_Instance pp_instance,
+                       const PPP_ManifestService* manifest_service,
                        void* user_data)
-      : manifest_service_(*manifest_service),
+      : pp_instance_(pp_instance),
+        manifest_service_(*manifest_service),
         user_data_(user_data) {
   }
 
@@ -174,25 +195,47 @@ class ManifestServiceProxy : public ManifestServiceChannel::Delegate {
   virtual void OpenResource(
       const std::string& key,
       const ManifestServiceChannel::OpenResourceCallback& callback) OVERRIDE {
+    DCHECK(ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->
+               BelongsToCurrentThread());
+
     if (!user_data_)
       return;
 
-    // The allocated callback will be freed in DidOpenResource, which is always
-    // called regardless whether OpenResource() succeeds or fails.
-    if (!PP_ToBool(manifest_service_.OpenResource(
-            user_data_,
-            key.c_str(),
-            DidOpenResource,
-            new ManifestServiceChannel::OpenResourceCallback(callback)))) {
-      user_data_ = NULL;
+    std::string url;
+    // TODO(teravest): Clean up pnacl_options logic in JsonManifest so we don't
+    // have to initialize it like this here.
+    PP_PNaClOptions pnacl_options;
+    pnacl_options.translate = PP_FALSE;
+    pnacl_options.is_debug = PP_FALSE;
+    pnacl_options.opt_level = 2;
+    if (!ManifestResolveKey(pp_instance_, false, key, &url, &pnacl_options)) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(callback, PP_kInvalidFileHandle));
+      return;
     }
+
+    // We have to call DidDownloadFile, even if this object is destroyed, so
+    // that the handle inside PP_NaClFileInfo isn't leaked. This means that the
+    // callback passed to this function shouldn't have a weak pointer to an
+    // object either.
+    //
+    // TODO(teravest): Make a type like PP_NaClFileInfo to use for DownloadFile
+    // that would close the file handle on destruction.
+    DownloadFile(pp_instance_, url.c_str(),
+                 base::Bind(&ManifestServiceProxy::DidDownloadFile, callback));
   }
 
  private:
-  static void DidOpenResource(void* user_data, PP_FileHandle file_handle) {
-    scoped_ptr<ManifestServiceChannel::OpenResourceCallback> callback(
-        static_cast<ManifestServiceChannel::OpenResourceCallback*>(user_data));
-    callback->Run(file_handle);
+  static void DidDownloadFile(
+      ManifestServiceChannel::OpenResourceCallback callback,
+      int32_t pp_error,
+      const PP_NaClFileInfo& file_info) {
+    if (pp_error != PP_OK) {
+      callback.Run(base::kInvalidPlatformFileValue);
+      return;
+    }
+    callback.Run(file_info.handle);
   }
 
   void Quit() {
@@ -204,6 +247,7 @@ class ManifestServiceProxy : public ManifestServiceChannel::Delegate {
     user_data_ = NULL;
   }
 
+  PP_Instance pp_instance_;
   PPP_ManifestService manifest_service_;
   void* user_data_;
   DISALLOW_COPY_AND_ASSIGN(ManifestServiceProxy);
@@ -271,7 +315,8 @@ void LaunchSelLdr(PP_Instance instance,
   // will be called in its destructor so that the caller of this function
   // can free manifest_service_user_data properly.
   scoped_ptr<ManifestServiceChannel::Delegate> manifest_service_proxy(
-      new ManifestServiceProxy(manifest_service_interface,
+      new ManifestServiceProxy(instance,
+                               manifest_service_interface,
                                manifest_service_user_data));
 
   FileDescriptor result_socket;
@@ -1029,8 +1074,8 @@ bool ManifestResolveKey(PP_Instance instance,
                         const std::string& key,
                         std::string* full_url,
                         PP_PNaClOptions* pnacl_options) {
-  // For "helper" processes (llc and ld), we resolve keys manually as there is
-  // no existing .nmf file to parse.
+  // For "helper" processes (llc and ld, for PNaCl translation), we resolve
+  // keys manually as there is no existing .nmf file to parse.
   if (is_helper_process) {
     pnacl_options->translate = PP_FALSE;
     // We can only resolve keys in the files/ namespace.
@@ -1368,34 +1413,39 @@ void DownloadNexeCompletion(const DownloadNexeRequest& request,
   request.callback.func(request.callback.user_data, pp_error);
 }
 
-void DownloadFileCompletion(PP_NaClFileInfo* file_info,
-                            PP_CompletionCallback callback,
+void DownloadFileCompletion(
+    const DownloadFileCallback& callback,
     FileDownloader::Status status,
     base::File file,
     int http_status) {
   int32_t pp_error = FileDownloaderToPepperError(status);
+  PP_NaClFileInfo file_info;
   if (pp_error == PP_OK) {
-    file_info->handle = file.TakePlatformFile();
-    file_info->token_lo = 0;
-    file_info->token_hi = 0;
+    file_info.handle = file.TakePlatformFile();
+    file_info.token_lo = 0;
+    file_info.token_hi = 0;
+  } else {
+    file_info = kInvalidNaClFileInfo;
   }
-  callback.func(callback.user_data, pp_error);
+
+  callback.Run(pp_error, file_info);
 }
 
 void DownloadFile(PP_Instance instance,
                   const char* url,
-                  struct PP_NaClFileInfo* file_info,
-                  struct PP_CompletionCallback callback) {
+                  const DownloadFileCallback& callback) {
+  DCHECK(ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->
+             BelongsToCurrentThread());
   CHECK(url);
-  CHECK(file_info);
 
   NexeLoadManager* load_manager = GetNexeLoadManager(instance);
   DCHECK(load_manager);
   if (!load_manager) {
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_ERROR_FAILED)));
+        base::Bind(callback,
+                   static_cast<int32_t>(PP_ERROR_FAILED),
+                   kInvalidNaClFileInfo));
     return;
   }
 
@@ -1405,21 +1455,22 @@ void DownloadFile(PP_Instance instance,
   if (url_string.find(kPNaClTranslatorBaseUrl, 0) == 0) {
     PP_FileHandle handle = GetReadonlyPnaclFd(url);
     if (handle == PP_kInvalidFileHandle) {
-      ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+      base::MessageLoop::current()->PostTask(
           FROM_HERE,
-          base::Bind(callback.func, callback.user_data,
-                     static_cast<int32_t>(PP_ERROR_FAILED)));
+          base::Bind(callback,
+                     static_cast<int32_t>(PP_ERROR_FAILED),
+                     kInvalidNaClFileInfo));
       return;
     }
     // TODO(ncbray): enable the fast loading and validation paths for this type
     // of file.
-    file_info->handle = handle;
-    file_info->token_lo = 0;
-    file_info->token_hi = 0;
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    PP_NaClFileInfo file_info;
+    file_info.handle = handle;
+    file_info.token_lo = 0;
+    file_info.token_hi = 0;
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_OK)));
+        base::Bind(callback, static_cast<int32_t>(PP_OK), file_info));
     return;
   }
 
@@ -1427,10 +1478,11 @@ void DownloadFile(PP_Instance instance,
   // before downloading it.
   const GURL& test_gurl = load_manager->plugin_base_url().Resolve(url);
   if (!test_gurl.is_valid()) {
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_ERROR_FAILED)));
+        base::Bind(callback,
+                   static_cast<int32_t>(PP_ERROR_FAILED),
+                   kInvalidNaClFileInfo));
     return;
   }
 
@@ -1442,13 +1494,13 @@ void DownloadFile(PP_Instance instance,
                                                  &file_token_lo,
                                                  &file_token_hi);
   if (file_handle != PP_kInvalidFileHandle) {
-    file_info->handle = file_handle;
-    file_info->token_lo = file_token_lo;
-    file_info->token_hi = file_token_hi;
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    PP_NaClFileInfo file_info;
+    file_info.handle = file_handle;
+    file_info.token_lo = file_token_lo;
+    file_info.token_hi = file_token_hi;
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_OK)));
+        base::Bind(callback, static_cast<int32_t>(PP_OK), file_info));
     return;
   }
 
@@ -1460,10 +1512,11 @@ void DownloadFile(PP_Instance instance,
   content::PepperPluginInstance* plugin_instance =
       content::PepperPluginInstance::Get(instance);
   if (!plugin_instance) {
-    ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
+    base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback.func, callback.user_data,
-                   static_cast<int32_t>(PP_ERROR_FAILED)));
+        base::Bind(callback,
+                   static_cast<int32_t>(PP_ERROR_FAILED),
+                   kInvalidNaClFileInfo));
   }
   const blink::WebDocument& document =
       plugin_instance->GetContainer()->element().document();
@@ -1477,12 +1530,34 @@ void DownloadFile(PP_Instance instance,
   FileDownloader* file_downloader = new FileDownloader(
       url_loader.Pass(),
       target_file.Pass(),
-      base::Bind(&DownloadFileCompletion, file_info, callback),
+      base::Bind(&DownloadFileCompletion, callback),
       base::Bind(&ProgressEventRateLimiter::ReportProgress,
                  base::Owned(tracker), url));
   file_downloader->Load(url_request);
 }
 
+void ExternalDownloadFileCompletion(struct PP_NaClFileInfo* out_file_info,
+                                    struct PP_CompletionCallback callback,
+                                    int32_t pp_error,
+                                    const PP_NaClFileInfo& file_info) {
+  if (pp_error == PP_OK)
+    *out_file_info = file_info;
+  callback.func(callback.user_data, pp_error);
+}
+
+void ExternalDownloadFile(PP_Instance instance,
+                          const char* url,
+                          struct PP_NaClFileInfo* file_info,
+                          struct PP_CompletionCallback callback) {
+  DCHECK(callback.func);
+  CHECK(ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->
+            BelongsToCurrentThread());
+  DownloadFile(instance, url,
+               base::Bind(&ExternalDownloadFileCompletion,
+                          file_info,
+                          callback));
+}
+
 void ReportSelLdrStatus(PP_Instance instance,
                         int32_t load_status,
                         int32_t max_status) {
@@ -1546,7 +1621,7 @@ const PPB_NaCl_Private nacl_interface = {
   &GetCpuFeatureAttrs,
   &PostMessageToJavaScript,
   &DownloadNexe,
-  &DownloadFile,
+  &ExternalDownloadFile,
   &ReportSelLdrStatus,
   &LogTranslateTime
 };

ppapi/api/private/ppb_nacl_private.idl

@@ -156,14 +156,6 @@ interface PPP_ManifestService {
 
   /* Called when PPAPI initialization in the NaCl plugin is finished. */
   PP_Bool StartupInitializationComplete([inout] mem_t user_data);
-
-  /* Called when irt_open_resource() is invoked in the NaCl plugin.
-   * Upon completion, callback will be invoked with given callback_user_data
-   * and the result file handle (or PP_kInvalidFileHandle on error). */
-  PP_Bool OpenResource([inout] mem_t user_data,
-                       [in] str_t entry_key,
-                       [in] PP_OpenResourceCompletionCallback callback,
-                       [inout] mem_t callback_user_data);
 };
 
 /* Corresponds to NaClFileInfo in

ppapi/c/private/ppb_nacl_private.h

@@ -3,7 +3,7 @@
  * found in the LICENSE file.
  */
 
-/* From private/ppb_nacl_private.idl modified Fri Jun 13 15:14:51 2014. */
+/* From private/ppb_nacl_private.idl modified Wed Jun 18 19:15:01 2014. */
 
 #ifndef PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
 #define PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
@@ -194,13 +194,6 @@ struct PPP_ManifestService_1_0 {
   PP_Bool (*Quit)(void* user_data);
   /* Called when PPAPI initialization in the NaCl plugin is finished. */
   PP_Bool (*StartupInitializationComplete)(void* user_data);
-  /* Called when irt_open_resource() is invoked in the NaCl plugin.
-   * Upon completion, callback will be invoked with given callback_user_data
-   * and the result file handle (or PP_kInvalidFileHandle on error). */
-  PP_Bool (*OpenResource)(void* user_data,
-                          const char* entry_key,
-                          PP_OpenResourceCompletionCallback callback,
-                          void* callback_user_data);
 };
 
 typedef struct PPP_ManifestService_1_0 PPP_ManifestService;

ppapi/native_client/src/trusted/plugin/service_runtime.cc

@@ -51,41 +51,6 @@
 
 namespace plugin {
 
-class OpenManifestEntryAsyncCallback {
- public:
-  OpenManifestEntryAsyncCallback(PP_OpenResourceCompletionCallback callback,
-                                 void* callback_user_data)
-      : callback_(callback), callback_user_data_(callback_user_data) {
-  }
-
-  ~OpenManifestEntryAsyncCallback() {
-    if (callback_)
-      callback_(callback_user_data_, PP_kInvalidFileHandle);
-  }
-
-  void Run(int32_t pp_error) {
-#if defined(OS_WIN)
-    // Currently, this is used only for non-SFI mode, and now the mode is not
-    // supported on windows.
-    // TODO(hidehiko): Support it on Windows when we switch to use
-    // ManifestService also in SFI-mode.
-    NACL_NOTREACHED();
-#elif defined(OS_POSIX)
-    // On posix, PlatformFile is the file descriptor.
-    callback_(callback_user_data_, (pp_error == PP_OK) ? info_.desc : -1);
-    callback_ = NULL;
-#endif
-  }
-
-  NaClFileInfo* mutable_info() { return &info_; }
-
- private:
-  NaClFileInfo info_;
-  PP_OpenResourceCompletionCallback callback_;
-  void* callback_user_data_;
-  DISALLOW_COPY_AND_ASSIGN(OpenManifestEntryAsyncCallback);
-};
-
 namespace {
 
 class ManifestService {
@@ -116,25 +81,6 @@ class ManifestService {
     return true;
   }
 
-  bool OpenResource(const char* entry_key,
-                    PP_OpenResourceCompletionCallback callback,
-                    void* callback_user_data) {
-    // Release this instance if the ServiceRuntime is already destructed.
-    if (anchor_->is_abandoned()) {
-      callback(callback_user_data, PP_kInvalidFileHandle);
-      delete this;
-      return false;
-    }
-
-    OpenManifestEntryAsyncCallback* open_manifest_callback =
-        new OpenManifestEntryAsyncCallback(callback, callback_user_data);
-    plugin_reverse_->OpenManifestEntryAsync(
-        entry_key,
-        open_manifest_callback->mutable_info(),
-        open_manifest_callback);
-    return true;
-  }
-
   static PP_Bool QuitTrampoline(void* user_data) {
     return PP_FromBool(static_cast<ManifestService*>(user_data)->Quit());
   }
@@ -144,15 +90,6 @@ class ManifestService {
                        StartupInitializationComplete());
   }
 
-  static PP_Bool OpenResourceTrampoline(
-      void* user_data,
-      const char* entry_key,
-      PP_OpenResourceCompletionCallback callback,
-      void* callback_user_data) {
-    return PP_FromBool(static_cast<ManifestService*>(user_data)->OpenResource(
-        entry_key, callback, callback_user_data));
-  }
-
  private:
   // Weak reference to check if plugin_reverse is legally accessible or not.
   nacl::WeakRefAnchor* anchor_;
@@ -165,22 +102,11 @@ class ManifestService {
 const PPP_ManifestService kManifestServiceVTable = {
   &ManifestService::QuitTrampoline,
   &ManifestService::StartupInitializationCompleteTrampoline,
-  &ManifestService::OpenResourceTrampoline,
 };
 
 }  // namespace
 
 OpenManifestEntryResource::~OpenManifestEntryResource() {
-  MaybeRunCallback(PP_ERROR_ABORTED);
-}
-
-void OpenManifestEntryResource::MaybeRunCallback(int32_t pp_error) {
-  if (!callback)
-    return;
-
-  callback->Run(pp_error);
-  delete callback;
-  callback = NULL;
 }
 
 PluginReverseInterface::PluginReverseInterface(
@@ -243,7 +169,7 @@ bool PluginReverseInterface::OpenManifestEntry(nacl::string url_key,
   // the main thread before this function can return. The pointers it contains
   // to stack variables will not leak.
   OpenManifestEntryResource* to_open =
-      new OpenManifestEntryResource(url_key, info, &op_complete, NULL);
+      new OpenManifestEntryResource(url_key, info, &op_complete);
   CHECK(to_open != NULL);
   NaClLog(4, "PluginReverseInterface::OpenManifestEntry: %s\n",
           url_key.c_str());
@@ -293,16 +219,6 @@ bool PluginReverseInterface::OpenManifestEntry(nacl::string url_key,
   return true;
 }
 
-void PluginReverseInterface::OpenManifestEntryAsync(
-    const nacl::string& entry_key,
-    struct NaClFileInfo* info,
-    OpenManifestEntryAsyncCallback* callback) {
-  bool op_complete = false;
-  OpenManifestEntryResource to_open(
-      entry_key, info, &op_complete, callback);
-  OpenManifestEntry_MainThreadContinuation(&to_open, PP_OK);
-}
-
 // Transfer point from OpenManifestEntry() which runs on the main thread
 // (Some PPAPI actions -- like StreamAsFile -- can only run on the main thread).
 // OpenManifestEntry() is waiting on a condvar for this continuation to
@@ -334,7 +250,6 @@ void PluginReverseInterface::OpenManifestEntry_MainThreadContinuation(
       p->file_info->desc = -1;  // but failed.
       NaClXCondVarBroadcast(&cv_);
     }
-    p->MaybeRunCallback(PP_OK);
     return;
   }
   nacl::string mapped_url = pp::Var(pp_mapped_url).AsString();
@@ -354,7 +269,6 @@ void PluginReverseInterface::OpenManifestEntry_MainThreadContinuation(
       p->file_info->desc = -1;  // but failed.
       NaClXCondVarBroadcast(&cv_);
     }
-    p->MaybeRunCallback(PP_OK);
     return;
   }
 
@@ -362,9 +276,6 @@ void PluginReverseInterface::OpenManifestEntry_MainThreadContinuation(
   // to create another instance.
   OpenManifestEntryResource* open_cont = new OpenManifestEntryResource(*p);
   open_cont->url = mapped_url;
-  // Callback is now delegated from p to open_cont. So, here we manually clear
-  // complete callback.
-  p->callback = NULL;
 
   pp::CompletionCallback stream_cc = WeakRefNewCallback(
       anchor_,
@@ -403,7 +314,6 @@ void PluginReverseInterface::StreamAsFile_MainThreadContinuation(
     *p->op_complete_ptr = true;
     NaClXCondVarBroadcast(&cv_);
   }
-  p->MaybeRunCallback(PP_OK);
 }
 
 bool PluginReverseInterface::CloseManifestEntry(int32_t desc) {

ppapi/native_client/src/trusted/plugin/service_runtime.h

@@ -34,7 +34,6 @@ class DescWrapper;
 
 namespace plugin {
 
-class OpenManifestEntryAsyncCallback;
 class Plugin;
 class SrpcClient;
 class ServiceRuntime;
@@ -69,20 +68,16 @@ struct OpenManifestEntryResource {
  public:
   OpenManifestEntryResource(const std::string& target_url,
                             struct NaClFileInfo* finfo,
-                            bool* op_complete,
-                            OpenManifestEntryAsyncCallback* callback)
+                            bool* op_complete)
       : url(target_url),
         file_info(finfo),
-        op_complete_ptr(op_complete),
-        callback(callback) {}
+        op_complete_ptr(op_complete) {}
   ~OpenManifestEntryResource();
-  void MaybeRunCallback(int32_t pp_error);
 
   std::string url;
   struct NaClFileInfo* file_info;
   PP_NaClFileInfo pp_file_info;
   bool* op_complete_ptr;
-  OpenManifestEntryAsyncCallback* callback;
 };
 
 // Do not invoke from the main thread, since the main methods will
@@ -122,15 +117,6 @@ class PluginReverseInterface: public nacl::ReverseInterface {
                                        int64_t offset,
                                        int64_t bytes_to_write);
 
-  // This is a sibling of OpenManifestEntry. While OpenManifestEntry is
-  // a sync function and must be called on a non-main thread,
-  // OpenManifestEntryAsync must be called on the main thread. Upon completion
-  // (even on error), callback will be invoked. The caller has responsibility
-  // to keep the memory passed to info until callback is invoked.
-  void OpenManifestEntryAsync(const nacl::string& key,
-                              struct NaClFileInfo* info,
-                              OpenManifestEntryAsyncCallback* callback);
-
  protected:
   virtual void OpenManifestEntry_MainThreadContinuation(
       OpenManifestEntryResource* p,