Pepper: Simplify OpenResource() for Non-SFI.

components/nacl/renderer/ppb_nacl_private_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/nacl/renderer/ppb_nacl_private_impl.h"
 
 #include <numeric>
 #include <string>
 #include <vector>
 
@@ skipping 95 matching lines @@
     LAZY_INSTANCE_INITIALIZER;
 
 nacl::NexeLoadManager* GetNexeLoadManager(PP_Instance instance) {
   NexeLoadManagerMap& map = g_load_manager_map.Get();
   NexeLoadManagerMap::iterator iter = map.find(instance);
   if (iter != map.end())
     return iter->second;
   return NULL;
 }
 
+PP_NaClFileInfo MakeInvalidNaClFileInfo() {

[hidehiko, 2014/06/18 04:43:11]

Just FYI:
How about using a constant here?

const PP_NaClFileInfo kInvalidNaClFileInfo = {
  PP_kInvalidFileHandle,
  0,  // token_lo
  0,  // token_hi
};

[teravest, 2014/06/18 20:13:43]

Done.

+  PP_NaClFileInfo info;
+  info.handle = PP_kInvalidFileHandle;
+  info.token_lo = 0;
+  info.token_hi = 0;
+  return info;
+}
+
 int GetRoutingID(PP_Instance instance) {
   // Check that we are on the main renderer thread.
   DCHECK(content::RenderThread::Get());
   content::RendererPpapiHost *host =
       content::RendererPpapiHost::GetForPPInstance(instance);
   if (!host)
     return 0;
   return host->GetRoutingIDForWidget(instance);
 }
 
@@ skipping 12 matching lines @@
   return true;
 }
 
 void PostPPCompletionCallback(PP_CompletionCallback callback,
                               int32_t status) {
   ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
       FROM_HERE,
       base::Bind(callback.func, callback.user_data, status));
 }
 
+bool ManifestResolveKey(PP_Instance instance,

[hidehiko, 2014/06/18 04:43:11]

I'm rarely see this kind of forward declaration. Can we avoid?

[teravest, 2014/06/18 20:13:43]

I don't think so; I could try to reorganize the file so implementations come
before they're used, but I'd like for the function declarations to roughly match
the interface order.

I hope to get remove ManifestResolveKey from PPB_NaCl_Private soon, which should
make things better.

+                        bool is_helper_process,
+                        const std::string& key,
+                        std::string* full_url,
+                        PP_PNaClOptions* pnacl_options);
+
+void DownloadFile(PP_Instance instance,
+                  const char* url,
+                  base::Callback<void(int32_t, PP_NaClFileInfo)> callback);

[hidehiko, 2014/06/18 04:43:11]

const PP_NaClFileInfo& ? Ditto for below.

[teravest, 2014/06/18 20:13:44]

Done.

+
 // 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),
-        user_data_(user_data) {
+      : pp_instance_(pp_instance),
+        manifest_service_(*manifest_service),
+        user_data_(user_data),
+        weak_factory_(this) {
   }
 
   virtual ~ManifestServiceProxy() {
     Quit();
   }
 
   virtual void StartupInitializationComplete() OVERRIDE {
     if (!user_data_)
       return;
 
     if (!PP_ToBool(
             manifest_service_.StartupInitializationComplete(user_data_))) {
       user_data_ = NULL;
     }
   }
 
   virtual void OpenResource(
       const std::string& key,
       const ManifestServiceChannel::OpenResourceCallback& callback) OVERRIDE {
     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;
+    PP_PNaClOptions pnacl_options = {PP_FALSE, PP_FALSE, 2};

[dmichael (off chromium), 2014/06/17 21:52:28]

nit: This would be more readable if you zero-initialized and then set the fields
by name...
PP_PNaClOptions pnacl_options = {};
pnacl_options.foo = PP_FALSE;
etc

[teravest, 2014/06/18 20:13:44]

Done.

+    if (!ManifestResolveKey(pp_instance_, false, key, &url, &pnacl_options)) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(callback, PP_kInvalidFileHandle));
+      return;
     }
+
+    DownloadFile(pp_instance_, url.c_str(),
+                 base::Bind(&ManifestServiceProxy::DidDownloadFile,
+                            weak_factory_.GetWeakPtr(),
+                            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);
+  void DidDownloadFile(ManifestServiceChannel::OpenResourceCallback callback,

[hidehiko, 2014/06/18 04:43:11]

Can be static? Or is it intentional change that the callback won't be called
after this instance is destructed?

Would be great to document the cancellation policy.

Also, if this instance is deleted during the file downloading, the opened file
handle will be leaked.

[teravest, 2014/06/18 20:13:44]

Changed this to be static. It's better without the weak ref issue.

+                       int32_t pp_error,
+                       PP_NaClFileInfo file_info) {
+    if (pp_error != PP_OK) {
+      callback.Run(base::kInvalidPlatformFileValue);
+      return;
+    }
+    callback.Run(file_info.handle);
   }
 
   void Quit() {
     if (!user_data_)
       return;
 
     bool result = PP_ToBool(manifest_service_.Quit(user_data_));
     DCHECK(!result);
     user_data_ = NULL;
   }
 
+  PP_Instance pp_instance_;
   PPP_ManifestService manifest_service_;
   void* user_data_;
+  base::WeakPtrFactory<ManifestServiceProxy> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ManifestServiceProxy);
 };
 
 blink::WebURLLoader* CreateWebURLLoader(const blink::WebDocument& document,
                                         const GURL& gurl) {
   blink::WebURLLoaderOptions options;
   options.untrustedHTTP = true;
 
   // Options settings here follow the original behavior in the trusted
   // plugin and PepperURLLoaderHost.
@@ skipping 45 matching lines @@
                   void* imc_handle,
                   PP_CompletionCallback callback) {
   CHECK(ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->
             BelongsToCurrentThread());
 
   // Create the manifest service proxy here, so on error case, it will be
   // destructed (without passing it to ManifestServiceChannel), and QUIT
   // 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;
   IPC::Sender* sender = content::RenderThread::Get();
   DCHECK(sender);
   int routing_id = 0;
   // If the nexe uses ppapi APIs, we need a routing ID.
   // To get the routing ID, we must be on the main thread.
   // Some nexes do not use ppapi and launch from the background thread,
   // so those nexes can skip finding a routing_id.
@@ skipping 737 matching lines @@
   if (load_manager)
     load_manager->ReportLoadError(error_info.error, error_info.string);
   return PP_FALSE;
 }
 
 bool ManifestResolveKey(PP_Instance instance,
                         bool is_helper_process,
                         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

[hidehiko, 2014/06/18 04:43:11]

Update this comment, too?

[teravest, 2014/06/18 20:13:43]

Not exactly sure what you want here, but I did update the comment some.

   // no existing .nmf file to parse.
   if (is_helper_process) {
     pnacl_options->translate = PP_FALSE;
     // We can only resolve keys in the files/ namespace.
     const std::string kFilesPrefix = "files/";
     if (key.find(kFilesPrefix) == std::string::npos) {
       nacl::NexeLoadManager* load_manager = GetNexeLoadManager(instance);
       if (load_manager)
         load_manager->ReportLoadError(PP_NACL_ERROR_MANIFEST_RESOLVE_URL,
                                       "key did not start with files/");
@@ skipping 318 matching lines @@
   }
 
   if (pp_error == PP_OK && target_file.IsValid())
     out_file_info->handle = target_file.TakePlatformFile();
   else
     out_file_info->handle = PP_kInvalidFileHandle;
 
   request.callback.func(request.callback.user_data, pp_error);
 }
 
-void DownloadFileCompletion(PP_NaClFileInfo* file_info,
-                            PP_CompletionCallback callback,
-                            FileDownloader::Status status,
-                            base::File file,
-                            int http_status) {
+void DownloadFileCompletion(
+    base::Callback<void(int32_t, PP_NaClFileInfo)> callback,
+    FileDownloader::Status status,
+    base::File file,
+    int http_status) {
   int32_t pp_error = FileDownloaderToPepperError(status);
-  if (pp_error == PP_OK) {
-    file_info->handle = file.TakePlatformFile();
-    file_info->token_lo = 0;
-    file_info->token_hi = 0;
-  }
-  callback.func(callback.user_data, pp_error);
+  PP_NaClFileInfo file_info = MakeInvalidNaClFileInfo();
+  if (pp_error == PP_OK)
+    file_info.handle = file.TakePlatformFile();

[hidehiko, 2014/06/18 04:43:11]

Setting the platform file handle to invalid file info looks a bit strange.
Can we create:
- a valid file info with the platform file, or
- an invalid file info iff an operation fails.
?

[teravest, 2014/06/18 20:13:44]

Done.

+  callback.Run(pp_error, file_info);
 }
 
 void DownloadFile(PP_Instance instance,
                   const char* url,
-                  struct PP_NaClFileInfo* file_info,
-                  struct PP_CompletionCallback callback) {
+                  base::Callback<void(int32_t, PP_NaClFileInfo)> callback) {
   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(

[dmichael (off chromium), 2014/06/17 21:52:28]

Is this only a cosmetic change, or are you trying to make this callable from a
background thread?

I'm pretty sure we haven't made accessing and using NexeLoadManager thread-safe.

If it's just cosmetic, it might be good to DCHECK that you're on the main thread
at the beginning of the method.

[teravest, 2014/06/18 20:13:44]

Done.

         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),
+                   MakeInvalidNaClFileInfo()));
     return;
   }
 
   // Handle special PNaCl support files which are installed on the user's
   // machine.
   std::string url_string(url);
   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),
+                     MakeInvalidNaClFileInfo()));
       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;
   }
 
   // We have to ensure that this url resolves relative to the plugin base url
   // 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),
+                   MakeInvalidNaClFileInfo()));
     return;
   }
 
   // Try the fast path for retrieving the file first.
   uint64_t file_token_lo = 0;
   uint64_t file_token_hi = 0;
   PP_FileHandle file_handle = OpenNaClExecutable(instance,
                                                  url,
                                                  &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;
   }
 
   // The fast path didn't work, we'll fetch the file using URLLoader and write
   // it to local storage.
   base::File target_file(CreateTemporaryFile(instance));
   GURL gurl(url);
 
   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),
+                   MakeInvalidNaClFileInfo()));
   }
   const blink::WebDocument& document =
       plugin_instance->GetContainer()->element().document();
   scoped_ptr<blink::WebURLLoader> url_loader(
       CreateWebURLLoader(document, gurl));
   blink::WebURLRequest url_request = CreateWebURLRequest(document, gurl);
 
   ProgressEventRateLimiter* tracker = new ProgressEventRateLimiter(instance);
 
   // FileDownloader deletes itself after invoking DownloadNexeCompletion.
   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,
+                                    PP_NaClFileInfo file_info) {
+  DCHECK(callback.func);

[dmichael (off chromium), 2014/06/17 21:52:28]

nit: This DCHECK seems either unnecessary or like it should be inside
ExternalDownloadFile (to make it easier to catch a bad call at the right place)

[teravest, 2014/06/18 20:13:43]

Done.

+  if (pp_error == PP_OK) {
+    out_file_info->handle = file_info.handle;

[hidehiko, 2014/06/18 04:43:11]

Maybe:
*out_file_info = file_info; 

[teravest, 2014/06/18 20:13:43]

Done.

+    out_file_info->token_lo = file_info.token_lo;
+    out_file_info->token_hi = file_info.token_hi;
+  }
+  callback.func(callback.user_data, pp_error);
+}
+
+void ExternalDownloadFile(PP_Instance instance,
+                          const char* url,
+                          struct PP_NaClFileInfo* file_info,
+                          struct PP_CompletionCallback callback) {
+  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) {
   HistogramEnumerate("NaCl.LoadStatus.SelLdr", load_status, max_status);
   NexeLoadManager* load_manager = GetNexeLoadManager(instance);
   DCHECK(load_manager);
   if (!load_manager)
     return;
 
   // Gather data to see if being installed changes load outcomes.
@@ skipping 43 matching lines @@
   &RequestNaClManifest,
   &GetManifestBaseURL,
   &ProcessNaClManifest,
   &DevInterfacesEnabled,
   &ManifestGetProgramURL,
   &ExternalManifestResolveKey,
   &GetPNaClResourceInfo,
   &GetCpuFeatureAttrs,
   &PostMessageToJavaScript,
   &DownloadNexe,
-  &DownloadFile,
+  &ExternalDownloadFile,
   &ReportSelLdrStatus,
   &LogTranslateTime
 };
 
 }  // namespace
 
 const PPB_NaCl_Private* GetNaClPrivateInterface() {
   return &nacl_interface;
 }
 
 }  // namespace nacl