Introduce NaClFileInfoAutoCloser as an RAII wrapper.

This eliminates two descriptor leaks -- the downloaded nmf file, and
the nexe itself.  One socket (on Linux) still leak.  Because Unixes
always use the lowest available number and the leaked descriptor is
the highest numbered one while the F@H nexe is running, this implies
that this might be a PPAPI proxy related leak, since proxy hookup is
the last thing that happens before the application itself runs.

See /home/bsy/fd-leak-log for time-lapsed view of what happens when a
F@H NaCl module finishes and another starts up.

TEST= manually running F@H and checking /proc/<renderer>/fd/*
BUG= 335186
R=bbudge@chromium.org, dmichael@chromium.org

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

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

4 files changed, 117 insertions, 35 deletions

diff --git a/ppapi/native_client/src/trusted/plugin/file_downloader.cc b/ppapi/native_client/src/trusted/plugin/file_downloader.cc
index f8d3a4c..2f50967 100644
--- a/ppapi/native_client/src/trusted/plugin/file_downloader.cc
+++ b/ppapi/native_client/src/trusted/plugin/file_downloader.cc
@@ -57,6 +57,38 @@ int32_t ConvertFileDescriptor(PP_FileHandle handle) {
 
 namespace plugin {
 
+NaClFileInfoAutoCloser::NaClFileInfoAutoCloser()
+    : info_(NoFileInfo()) {}
+
+NaClFileInfoAutoCloser::NaClFileInfoAutoCloser(NaClFileInfo* pass_ownership)
+    : info_(*pass_ownership) {
+  *pass_ownership = NoFileInfo();
+}
+
+void NaClFileInfoAutoCloser::FreeResources() {
+  if (-1 != get_desc()) {
+    PLUGIN_PRINTF(("NaClFileInfoAutoCloser::FreeResources close(%d)\n",
+                   get_desc()));
+    close(get_desc());
+  }
+  info_.desc = -1;
+}
+
+void NaClFileInfoAutoCloser::TakeOwnership(NaClFileInfo* pass_ownership) {
+  PLUGIN_PRINTF(("NaClFileInfoAutoCloser::set: taking ownership of %d\n",
+                 pass_ownership->desc));
+  CHECK(pass_ownership->desc == -1 || pass_ownership->desc != get_desc());
+  FreeResources();
+  info_ = *pass_ownership;
+  *pass_ownership = NoFileInfo();
+}
+
+NaClFileInfo NaClFileInfoAutoCloser::Release() {
+  NaClFileInfo info_to_return = info_;
+  info_ = NoFileInfo();
+  return info_to_return;
+}
+
 void FileDownloader::Initialize(Plugin* instance) {
   PLUGIN_PRINTF(("FileDownloader::FileDownloader (this=%p)\n",
                  static_cast<void*>(this)));
@@ -69,7 +101,7 @@ void FileDownloader::Initialize(Plugin* instance) {
   url_loader_trusted_interface_ = static_cast<const PPB_URLLoaderTrusted*>(
       pp::Module::Get()->GetBrowserInterface(PPB_URLLOADERTRUSTED_INTERFACE));
   temp_buffer_.resize(kTempBufferSize);
-  cached_file_info_ = NoFileInfo();
+  file_info_.FreeResources();
 }
 
 bool FileDownloader::OpenStream(
@@ -101,7 +133,7 @@ bool FileDownloader::Open(
   file_open_notify_callback_ = callback;
   mode_ = mode;
   buffer_.clear();
-  cached_file_info_ = NoFileInfo();
+  file_info_.FreeResources();
   pp::URLRequestInfo url_request(instance_);
 
   // Allow CORS.
@@ -180,29 +212,32 @@ void FileDownloader::OpenFast(const nacl::string& url,
                               uint64_t file_token_lo, uint64_t file_token_hi) {
   PLUGIN_PRINTF(("FileDownloader::OpenFast (url=%s)\n", url.c_str()));
 
-  cached_file_info_ = NoFileInfo();
+  file_info_.FreeResources();
   CHECK(instance_ != NULL);
   open_time_ = NaClGetTimeOfDayMicroseconds();
   status_code_ = NACL_HTTP_STATUS_OK;
   url_to_open_ = url;
   url_ = url;
   mode_ = DOWNLOAD_NONE;
-  file_handle_ = file_handle;
-  file_token_.lo = file_token_lo;
-  file_token_.hi = file_token_hi;
+  if (not_streaming() && file_handle != PP_kInvalidFileHandle) {
+    NaClFileInfo tmp_info = NoFileInfo();
+    tmp_info.desc = ConvertFileDescriptor(file_handle);
+    tmp_info.file_token.lo = file_token_lo;
+    tmp_info.file_token.hi = file_token_hi;
+    file_info_.TakeOwnership(&tmp_info);
+  }
 }
 
-struct NaClFileInfo FileDownloader::GetFileInfo() {
-  PLUGIN_PRINTF(("FileDownloader::GetFileInfo\n"));
-  if (cached_file_info_.desc != -1) {
-    return cached_file_info_;
-  } else if (not_streaming() && file_handle_ != PP_kInvalidFileHandle) {
-    cached_file_info_.desc = ConvertFileDescriptor(file_handle_);
-    if (cached_file_info_.desc != -1)
-      cached_file_info_.file_token = file_token_;
-    return cached_file_info_;
+NaClFileInfo FileDownloader::GetFileInfo() {
+  NaClFileInfo info_to_return = NoFileInfo();
+
+  PLUGIN_PRINTF(("FileDownloader::GetFileInfo, this %p\n", this));
+  if (file_info_.get_desc() != -1) {
+    info_to_return = file_info_.Release();
   }
-  return NoFileInfo();
+  PLUGIN_PRINTF(("FileDownloader::GetFileInfo -- returning %d\n",
+                 info_to_return.desc));
+  return info_to_return;
 }
 
 int64_t FileDownloader::TimeSinceOpenMilliseconds() const {
@@ -452,8 +487,11 @@ void FileDownloader::GotFileHandleNotify(int32_t pp_error,
   PLUGIN_PRINTF((
       "FileDownloader::GotFileHandleNotify (pp_error=%" NACL_PRId32 ")\n",
       pp_error));
-  if (pp_error == PP_OK)
-    cached_file_info_.desc = ConvertFileDescriptor(handle);
+  if (pp_error == PP_OK) {
+    NaClFileInfo tmp_info = NoFileInfo();
+    tmp_info.desc = ConvertFileDescriptor(handle);
+    file_info_.TakeOwnership(&tmp_info);
+  }
 
   stream_finish_callback_.RunAndClear(pp_error);
 }
diff --git a/ppapi/native_client/src/trusted/plugin/file_downloader.h b/ppapi/native_client/src/trusted/plugin/file_downloader.h
index 4fd7997..2dee587 100644
--- a/ppapi/native_client/src/trusted/plugin/file_downloader.h
+++ b/ppapi/native_client/src/trusted/plugin/file_downloader.h
@@ -41,6 +41,37 @@ typedef std::vector<char>* FileStreamData;
 typedef CallbackSource<FileStreamData> StreamCallbackSource;
 typedef pp::CompletionCallbackWithOutput<FileStreamData> StreamCallback;
 
+// RAII-style wrapper class
+class NaClFileInfoAutoCloser {
+ public:
+  NaClFileInfoAutoCloser();
+
+  explicit NaClFileInfoAutoCloser(NaClFileInfo* pass_ownership);
+
+  ~NaClFileInfoAutoCloser() {
+    FreeResources();
+  }
+
+  // Frees owned resources
+  void FreeResources();
+
+  void TakeOwnership(NaClFileInfo* pass_ownership);
+
+  // Return NaClFileInfo for temporary use, retaining ownership.
+  const NaClFileInfo& get() { return info_; }
+
+  // Returns POSIX descriptor for temporary use, retaining ownership.
+  int get_desc() { return info_.desc; }
+
+  // Returns ownership to caller
+  NaClFileInfo Release();
+
+ private:
+  NACL_DISALLOW_COPY_AND_ASSIGN(NaClFileInfoAutoCloser);
+
+  NaClFileInfo info_;
+};
+
 // A class that wraps PPAPI URLLoader and FileIO functionality for downloading
 // the url into a file and providing an open file descriptor.
 class FileDownloader {
@@ -51,7 +82,6 @@ class FileDownloader {
       : instance_(NULL),
         file_open_notify_callback_(pp::BlockUntilComplete()),
         stream_finish_callback_(pp::BlockUntilComplete()),
-        file_handle_(PP_kInvalidFileHandle),
         file_io_private_interface_(NULL),
         url_loader_trusted_interface_(NULL),
         open_time_(-1),
@@ -185,8 +215,6 @@ class FileDownloader {
   pp::CompletionCallback file_open_notify_callback_;
   pp::CompletionCallback stream_finish_callback_;
   pp::FileIO file_reader_;
-  PP_FileHandle file_handle_;
-  struct NaClFileToken file_token_;
   const PPB_FileIO_Private* file_io_private_interface_;
   const PPB_URLLoaderTrusted* url_loader_trusted_interface_;
   pp::URLLoader url_loader_;
@@ -200,7 +228,7 @@ class FileDownloader {
   std::deque<char> buffer_;
   UrlSchemeType url_scheme_;
   StreamCallbackSource* data_stream_callback_source_;
-  NaClFileInfo cached_file_info_;
+  NaClFileInfoAutoCloser file_info_;
 };
 }  // namespace plugin;
 #endif  // NATIVE_CLIENT_SRC_TRUSTED_PLUGIN_FILE_DOWNLOADER_H_
diff --git a/ppapi/native_client/src/trusted/plugin/plugin.cc b/ppapi/native_client/src/trusted/plugin/plugin.cc
index 3309d65..2923825 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.cc
+++ b/ppapi/native_client/src/trusted/plugin/plugin.cc
@@ -662,6 +662,12 @@ Plugin::~Plugin() {
         (shutdown_start - ready_time_) / NACL_MICROS_PER_MILLI);
   }
 
+  for (std::map<nacl::string, NaClFileInfoAutoCloser*>::iterator it =
+           url_file_info_map_.begin();
+       it != url_file_info_map_.end();
+       ++it) {
+    delete it->second;
+  }
   url_downloaders_.erase(url_downloaders_.begin(), url_downloaders_.end());
 
   ScriptablePlugin* scriptable_plugin_ = scriptable_plugin();
@@ -745,16 +751,17 @@ void Plugin::HistogramStartupTimeMedium(const std::string& name, float dt) {
 void Plugin::NexeFileDidOpen(int32_t pp_error) {
   PLUGIN_PRINTF(("Plugin::NexeFileDidOpen (pp_error=%" NACL_PRId32 ")\n",
                  pp_error));
-  struct NaClFileInfo info = nexe_downloader_.GetFileInfo();
+  NaClFileInfo tmp_info(nexe_downloader_.GetFileInfo());
+  NaClFileInfoAutoCloser info(&tmp_info);
   PLUGIN_PRINTF(("Plugin::NexeFileDidOpen (file_desc=%" NACL_PRId32 ")\n",
-                 info.desc));
+                 info.get_desc()));
   HistogramHTTPStatusCode(
       is_installed_ ?
           "NaCl.HttpStatusCodeClass.Nexe.InstalledApp" :
           "NaCl.HttpStatusCodeClass.Nexe.NotInstalledApp",
       nexe_downloader_.status_code());
   ErrorInfo error_info;
-  if (pp_error != PP_OK || info.desc == NACL_NO_FILE_DESC) {
+  if (pp_error != PP_OK || info.get_desc() == NACL_NO_FILE_DESC) {
     if (pp_error == PP_ERROR_ABORTED) {
       ReportLoadAbort();
     } else if (pp_error == PP_ERROR_NOACCESS) {
@@ -767,7 +774,7 @@ void Plugin::NexeFileDidOpen(int32_t pp_error) {
     }
     return;
   }
-  int32_t file_desc_ok_to_close = DUP(info.desc);
+  int32_t file_desc_ok_to_close = DUP(info.get_desc());
   if (file_desc_ok_to_close == NACL_NO_FILE_DESC) {
     error_info.SetReport(ERROR_NEXE_FH_DUP,
                          "could not duplicate loaded file handle.");
@@ -1029,10 +1036,11 @@ void Plugin::NaClManifestFileDidOpen(int32_t pp_error) {
   // The manifest file was successfully opened.  Set the src property on the
   // plugin now, so that the full url is available to error handlers.
   set_manifest_url(nexe_downloader_.url());
-  struct NaClFileInfo info = nexe_downloader_.GetFileInfo();
+  NaClFileInfo tmp_info(nexe_downloader_.GetFileInfo());
+  NaClFileInfoAutoCloser info(&tmp_info);
   PLUGIN_PRINTF(("Plugin::NaClManifestFileDidOpen (file_desc=%"
-                 NACL_PRId32 ")\n", info.desc));
-  if (pp_error != PP_OK || info.desc == NACL_NO_FILE_DESC) {
+                 NACL_PRId32 ")\n", info.get_desc()));
+  if (pp_error != PP_OK || info.get_desc() == NACL_NO_FILE_DESC) {
     if (pp_error == PP_ERROR_ABORTED) {
       ReportLoadAbort();
     } else if (pp_error == PP_ERROR_NOACCESS) {
@@ -1048,7 +1056,7 @@ void Plugin::NaClManifestFileDidOpen(int32_t pp_error) {
   }
   // SlurpFile closes the file descriptor after reading (or on error).
   // Duplicate our file descriptor since it will be handled by the browser.
-  int dup_file_desc = DUP(info.desc);
+  int dup_file_desc = DUP(info.get_desc());
   nacl::string json_buffer;
   file_utils::StatusCode status = file_utils::SlurpFile(
       dup_file_desc, json_buffer, kNaClManifestMaxFileBytes);
@@ -1212,25 +1220,33 @@ void Plugin::UrlDidOpenForStreamAsFile(int32_t pp_error,
                  static_cast<void*>(url_downloader)));
   url_downloaders_.erase(url_downloader);
   nacl::scoped_ptr<FileDownloader> scoped_url_downloader(url_downloader);
-  struct NaClFileInfo info = scoped_url_downloader->GetFileInfo();
+  NaClFileInfo tmp_info(scoped_url_downloader->GetFileInfo());
+  NaClFileInfoAutoCloser *info = new NaClFileInfoAutoCloser(&tmp_info);
 
   if (pp_error != PP_OK) {
     PP_RunCompletionCallback(&callback, pp_error);
-  } else if (info.desc > NACL_NO_FILE_DESC) {
+    delete info;
+  } else if (info->get_desc() > NACL_NO_FILE_DESC) {
+    std::map<nacl::string, NaClFileInfoAutoCloser*>::iterator it =
+        url_file_info_map_.find(url_downloader->url_to_open());
+    if (it != url_file_info_map_.end()) {
+      delete it->second;
+    }
     url_file_info_map_[url_downloader->url_to_open()] = info;
     PP_RunCompletionCallback(&callback, PP_OK);
   } else {
     PP_RunCompletionCallback(&callback, PP_ERROR_FAILED);
+    delete info;
   }
 }
 
 struct NaClFileInfo Plugin::GetFileInfo(const nacl::string& url) {
   struct NaClFileInfo info;
   memset(&info, 0, sizeof(info));
-  std::map<nacl::string, struct NaClFileInfo>::iterator it =
+  std::map<nacl::string, NaClFileInfoAutoCloser*>::iterator it =
       url_file_info_map_.find(url);
   if (it != url_file_info_map_.end()) {
-    info = it->second;
+    info = it->second->get();
     info.desc = DUP(info.desc);
   } else {
     info.desc = -1;
diff --git a/ppapi/native_client/src/trusted/plugin/plugin.h b/ppapi/native_client/src/trusted/plugin/plugin.h
index 240d314..4e0ad3c 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.h
+++ b/ppapi/native_client/src/trusted/plugin/plugin.h
@@ -455,7 +455,7 @@ class Plugin : public pp::InstancePrivate {
   std::set<FileDownloader*> url_downloaders_;
   // Keep track of file descriptors opened by StreamAsFile().
   // These are owned by the browser.
-  std::map<nacl::string, struct NaClFileInfo> url_file_info_map_;
+  std::map<nacl::string, NaClFileInfoAutoCloser*> url_file_info_map_;
 
   // Pending progress events.
   std::queue<ProgressEvent*> progress_events_;