Fix resource leak code path around OpenManifestEntry.

Currently, there are some code paths which causes resource leak
in OpenManifestEntry (and its related methods).
There are mainly two cases:
1) If pnacl_options.translate() is true in original code,
open_cont is just leaked.
2) If stream_cc is created but not called, e.g. Plugin::StreamAsFile()
is failed somehow, or it is pNaCl related url, then the open_cont is
leaked.

It seems the root cause is PP_ComletionCallback/pp::CompletionCallback
usage. PP_CompletionCallback is designed for C, rather than C++,
so it does not manage the resources. pp::CompletionCallback is its
very simple wrapper. So, even when it is destructed without the invocation,
it just leakes the data.
Note that, WeakRef does not help such a case, because its resource management
fires only when the callback is invoked.
Probably, more generic wider solution would be minimize the usage
of these callbacks, such as the just interface to the PPAPI, and instead
base::Callback would be used internally in the code of chrome tree.

This CL just focuses on the OpenManifestEntry related code.

BUG=n/a
TEST=Ran trybots.

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

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

3 files changed, 63 insertions, 59 deletions

diff --git a/ppapi/native_client/src/trusted/plugin/plugin.cc b/ppapi/native_client/src/trusted/plugin/plugin.cc
index 0606934..6a94de5 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.cc
+++ b/ppapi/native_client/src/trusted/plugin/plugin.cc
@@ -1168,9 +1168,10 @@ bool Plugin::SetManifestObject(const nacl::string& manifest_json,
   return true;
 }
 
-void Plugin::UrlDidOpenForStreamAsFile(int32_t pp_error,
-                                       FileDownloader*& url_downloader,
-                                       PP_CompletionCallback callback) {
+void Plugin::UrlDidOpenForStreamAsFile(
+    int32_t pp_error,
+    FileDownloader* url_downloader,
+    pp::CompletionCallback callback) {
   PLUGIN_PRINTF(("Plugin::UrlDidOpen (pp_error=%" NACL_PRId32
                  ", url_downloader=%p)\n", pp_error,
                  static_cast<void*>(url_downloader)));
@@ -1180,7 +1181,7 @@ void Plugin::UrlDidOpenForStreamAsFile(int32_t pp_error,
   NaClFileInfoAutoCloser *info = new NaClFileInfoAutoCloser(&tmp_info);
 
   if (pp_error != PP_OK) {
-    PP_RunCompletionCallback(&callback, pp_error);
+    callback.Run(pp_error);
     delete info;
   } else if (info->get_desc() > NACL_NO_FILE_DESC) {
     std::map<nacl::string, NaClFileInfoAutoCloser*>::iterator it =
@@ -1189,9 +1190,9 @@ void Plugin::UrlDidOpenForStreamAsFile(int32_t pp_error,
       delete it->second;
     }
     url_file_info_map_[url_downloader->url()] = info;
-    PP_RunCompletionCallback(&callback, PP_OK);
+    callback.Run(PP_OK);
   } else {
-    PP_RunCompletionCallback(&callback, PP_ERROR_FAILED);
+    callback.Run(PP_ERROR_FAILED);
     delete info;
   }
 }
@@ -1211,7 +1212,7 @@ struct NaClFileInfo Plugin::GetFileInfo(const nacl::string& url) {
 }
 
 bool Plugin::StreamAsFile(const nacl::string& url,
-                          PP_CompletionCallback callback) {
+                          const pp::CompletionCallback& callback) {
   PLUGIN_PRINTF(("Plugin::StreamAsFile (url='%s')\n", url.c_str()));
   FileDownloader* downloader = new FileDownloader();
   downloader->Initialize(this);
diff --git a/ppapi/native_client/src/trusted/plugin/plugin.h b/ppapi/native_client/src/trusted/plugin/plugin.h
index 0fe9762..61bc961 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.h
+++ b/ppapi/native_client/src/trusted/plugin/plugin.h
@@ -40,6 +40,7 @@ class DescWrapperFactory;
 }  // namespace nacl
 
 namespace pp {
+class CompletionCallback;
 class URLLoader;
 class URLUtil_Dev;
 }
@@ -198,7 +199,7 @@ class Plugin : public pp::Instance {
   // a PP_Error indicating status. On success an open file descriptor
   // corresponding to the url body is recorded for further lookup.
   bool StreamAsFile(const nacl::string& url,
-                    PP_CompletionCallback pp_callback);
+                    const pp::CompletionCallback& callback);
 
   // Returns rich information for a file retrieved by StreamAsFile(). This info
   // contains a file descriptor. The caller must take ownership of this
@@ -354,8 +355,8 @@ class Plugin : public pp::Instance {
 
   // Callback used when loading a URL for SRPC-based StreamAsFile().
   void UrlDidOpenForStreamAsFile(int32_t pp_error,
-                                 FileDownloader*& url_downloader,
-                                 PP_CompletionCallback pp_callback);
+                                 FileDownloader* url_downloader,
+                                 pp::CompletionCallback pp_callback);
 
   // Copy the main service runtime's most recent NaClLog output to the
   // JavaScript console.  Valid to use only after a crash, e.g., via a
diff --git a/ppapi/native_client/src/trusted/plugin/service_runtime.cc b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
index 9bb7ff4..acf14c7 100644
--- a/ppapi/native_client/src/trusted/plugin/service_runtime.cc
+++ b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
@@ -210,7 +210,6 @@ bool PluginReverseInterface::OpenManifestEntry(nacl::string url_key,
 void PluginReverseInterface::OpenManifestEntry_MainThreadContinuation(
     OpenManifestEntryResource* p,
     int32_t err) {
-  OpenManifestEntryResource *open_cont;
   UNREFERENCED_PARAMETER(err);
   // CallOnMainThread continuations always called with err == PP_OK.
 
@@ -239,54 +238,7 @@ void PluginReverseInterface::OpenManifestEntry_MainThreadContinuation(
           "ResolveKey: %s -> %s (pnacl_translate(%d))\n",
           p->url.c_str(), mapped_url.c_str(), pnacl_options.translate());
 
-  open_cont = new OpenManifestEntryResource(*p);  // copy ctor!
-  CHECK(open_cont != NULL);
-  open_cont->url = mapped_url;
-  if (!pnacl_options.translate()) {
-    pp::CompletionCallback stream_cc = WeakRefNewCallback(
-        anchor_,
-        this,
-        &PluginReverseInterface::StreamAsFile_MainThreadContinuation,
-        open_cont);
-    // Normal files.
-    if (!PnaclUrls::IsPnaclComponent(mapped_url)) {
-      if (!plugin_->StreamAsFile(mapped_url,
-                                 stream_cc.pp_completion_callback())) {
-        NaClLog(4,
-                "OpenManifestEntry_MainThreadContinuation: "
-                "StreamAsFile failed\n");
-        nacl::MutexLocker take(&mu_);
-        *p->op_complete_ptr = true;  // done...
-        p->file_info->desc = -1;       // but failed.
-        NaClXCondVarBroadcast(&cv_);
-        return;
-      }
-      NaClLog(4,
-              "OpenManifestEntry_MainThreadContinuation: StreamAsFile okay\n");
-    } else {
-      // Special PNaCl support files, that are installed on the
-      // user machine.
-      int32_t fd = PnaclResources::GetPnaclFD(
-          plugin_,
-          PnaclUrls::PnaclComponentURLToFilename(mapped_url).c_str());
-      if (fd < 0) {
-        // We checked earlier if the pnacl component wasn't installed
-        // yet, so this shouldn't happen. At this point, we can't do much
-        // anymore, so just continue with an invalid fd.
-        NaClLog(4,
-                "OpenManifestEntry_MainThreadContinuation: "
-                "GetReadonlyPnaclFd failed\n");
-      }
-      nacl::MutexLocker take(&mu_);
-      *p->op_complete_ptr = true;  // done!
-      // TODO(ncbray): enable the fast loading and validation paths for this
-      // type of file.
-      p->file_info->desc = fd;
-      NaClXCondVarBroadcast(&cv_);
-      NaClLog(4,
-              "OpenManifestEntry_MainThreadContinuation: GetPnaclFd okay\n");
-    }
-  } else {
+  if (pnacl_options.translate()) {
     // Requires PNaCl translation, but that's not supported.
     NaClLog(4,
             "OpenManifestEntry_MainThreadContinuation: "
@@ -297,6 +249,56 @@ void PluginReverseInterface::OpenManifestEntry_MainThreadContinuation(
     NaClXCondVarBroadcast(&cv_);
     return;
   }
+
+  if (PnaclUrls::IsPnaclComponent(mapped_url)) {
+    // Special PNaCl support files, that are installed on the
+    // user machine.
+    int32_t fd = PnaclResources::GetPnaclFD(
+        plugin_,
+        PnaclUrls::PnaclComponentURLToFilename(mapped_url).c_str());
+    if (fd < 0) {
+      // We checked earlier if the pnacl component wasn't installed
+      // yet, so this shouldn't happen. At this point, we can't do much
+      // anymore, so just continue with an invalid fd.
+      NaClLog(4,
+              "OpenManifestEntry_MainThreadContinuation: "
+              "GetReadonlyPnaclFd failed\n");
+    }
+    nacl::MutexLocker take(&mu_);
+    *p->op_complete_ptr = true;  // done!
+    // TODO(ncbray): enable the fast loading and validation paths for this
+    // type of file.
+    p->file_info->desc = fd;
+    NaClXCondVarBroadcast(&cv_);
+    NaClLog(4,
+            "OpenManifestEntry_MainThreadContinuation: GetPnaclFd okay\n");
+    return;
+  }
+
+  // Hereafter, normal files.
+
+  // Because p is owned by the callback of this invocation, so it is necessary
+  // to create another instance.
+  OpenManifestEntryResource* open_cont = new OpenManifestEntryResource(*p);
+  open_cont->url = mapped_url;
+  pp::CompletionCallback stream_cc = WeakRefNewCallback(
+      anchor_,
+      this,
+      &PluginReverseInterface::StreamAsFile_MainThreadContinuation,
+      open_cont);
+
+  if (!plugin_->StreamAsFile(mapped_url, stream_cc)) {
+    NaClLog(4,
+            "OpenManifestEntry_MainThreadContinuation: "
+            "StreamAsFile failed\n");
+    // Here, StreamAsFile is failed and stream_cc is not called.
+    // However, open_cont will be released only by the invocation.
+    // So, we manually call it here with error.
+    stream_cc.Run(PP_ERROR_FAILED);
+    return;
+  }
+
+  NaClLog(4, "OpenManifestEntry_MainThreadContinuation: StreamAsFile okay\n");
   // p is deleted automatically
 }