Delete PNaCl translation cache backend object when no longer needed

chrome/browser/nacl_host/pnacl_host.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 "chrome/browser/nacl_host/pnacl_host.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/nacl_host/nacl_browser.h"
 #include "chrome/browser/nacl_host/pnacl_translation_cache.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 
 using content::BrowserThread;
 
 namespace {
 static const base::FilePath::CharType kTranslationCacheDirectoryName[] =
     FILE_PATH_LITERAL("PnaclTranslationCache");
 // Delay to wait for initialization of the cache backend
 static const int kTranslationCacheInitializationDelayMs = 20;
 }
 
 PnaclHost::PnaclHost()
-    : cache_state_(CacheUninitialized), weak_factory_(this) {}
+    : pending_backend_operations_(0),
+      cache_state_(CacheUninitialized),
+      weak_factory_(this) {}
 
-PnaclHost::~PnaclHost() {}
+PnaclHost::~PnaclHost() {
+  // When PnaclHost is destroyed, it's too late to post anything to the cache
+  // thread (it will hang shutdown). So just leak the cache backend.
+  pnacl::PnaclTranslationCache* cache = disk_cache_.release();

[jvoung (off chromium), 2013/10/24 21:14:58]

Can this just be a one-liner (void)...release()?

Do we need to worry about entries being corrupt if we're not flushing?

[Derek Schuff, 2013/10/25 19:38:06]

Nope, gcc still warns about unused result if you do that. 
No, the cache backend is supposed to handle those in any case (e.g. in case of a
crash, or IIUC there are actually some cases where they do just leak instead of
blocking as well)

+  (void)cache;
+}
 
 PnaclHost* PnaclHost::GetInstance() { return Singleton<PnaclHost>::get(); }
 
 PnaclHost::PendingTranslation::PendingTranslation()
     : process_handle(base::kNullProcessHandle),
       render_view_id(0),
       nexe_fd(base::kInvalidPlatformFileValue),
       got_nexe_fd(false),
       got_cache_reply(false),
       got_cache_hit(false),
@@ skipping 46 matching lines @@
 void PnaclHost::Init() {
   // Extra check that we're on the real IO thread since this version of
   // Init isn't used in unit tests.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(thread_checker_.CalledOnValidThread());
   base::FilePath cache_path(GetCachePath());
   if (cache_path.empty() || cache_state_ != CacheUninitialized)
     return;
   disk_cache_.reset(new pnacl::PnaclTranslationCache());
   cache_state_ = CacheInitializing;
-  disk_cache_->InitOnDisk(
+  int rv = disk_cache_->InitOnDisk(
       cache_path,
       base::Bind(&PnaclHost::OnCacheInitialized, weak_factory_.GetWeakPtr()));
+  if (rv != net::ERR_IO_PENDING)
+    OnCacheInitialized(rv);
 }
 
 // Initialize using the in-memory backend, and manually set the temporary file
 // directory instead of using the system directory.
 void PnaclHost::InitForTest(base::FilePath temp_dir) {
   DCHECK(thread_checker_.CalledOnValidThread());
   disk_cache_.reset(new pnacl::PnaclTranslationCache());
   cache_state_ = CacheInitializing;
   temp_dir_ = temp_dir;
-  disk_cache_->InitInMemory(
+  int rv = disk_cache_->InitInMemory(
       base::Bind(&PnaclHost::OnCacheInitialized, weak_factory_.GetWeakPtr()));
+  if (rv != net::ERR_IO_PENDING)
+    OnCacheInitialized(rv);
 }
 
 ///////////////////////////////////////// Temp files
 
 // Create a temporary file on the blocking pool
 // static
 void PnaclHost::DoCreateTemporaryFile(base::FilePath temp_dir,
                                       TempFileCallback cb) {
   DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
 
@@ skipping 86 matching lines @@
   pt.is_incognito = is_incognito;
   pending_translations_[id] = pt;
   SendCacheQueryAndTempFileRequest(cache_key, id);
 }
 
 // Dispatch the cache read request and the temp file creation request
 // simultaneously; currently we need a temp file regardless of whether the
 // request hits.
 void PnaclHost::SendCacheQueryAndTempFileRequest(const std::string& cache_key,
                                                  const TranslationID& id) {
+  pending_backend_operations_++;
   disk_cache_->GetNexe(
       cache_key,
       base::Bind(
           &PnaclHost::OnCacheQueryReturn, weak_factory_.GetWeakPtr(), id));
 
   CreateTemporaryFile(
       base::Bind(&PnaclHost::OnTempFileReturn, weak_factory_.GetWeakPtr(), id));
 }
 
 // Callback from the translation cache query. |id| is bound from
 // SendCacheQueryAndTempFileRequest, |net_error| is a net::Error code (which for
 // our purposes means a hit if it's net::OK (i.e. 0). |buffer| is allocated
 // by PnaclTranslationCache and now belongs to PnaclHost.
 // (Bound callbacks must re-lookup the TranslationID because the translation
 // could be cancelled before they get called).
 void PnaclHost::OnCacheQueryReturn(
     const TranslationID& id,
     int net_error,
     scoped_refptr<net::DrainableIOBuffer> buffer) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  pending_backend_operations_--;
   PendingTranslationMap::iterator entry(pending_translations_.find(id));
   if (entry == pending_translations_.end()) {
     LOG(ERROR) << "OnCacheQueryReturn: id not found";
+    DeInitIfSafe();
     return;
   }
   PendingTranslation* pt = &entry->second;
   pt->got_cache_reply = true;
   pt->got_cache_hit = (net_error == net::OK);
   if (pt->got_cache_hit)
     pt->nexe_read_buffer = buffer;
   CheckCacheQueryReady(entry);
 }
 
@@ skipping 174 matching lines @@
   if (it == pending_translations_.end()) {
     LOG(ERROR) << "StoreTranslatedNexe: TranslationID " << id.first << ","
                << id.second << " not found.";
     return;
   }
 
   if (buffer.get() == NULL) {
     LOG(ERROR) << "Error reading translated nexe";
     return;
   }
-
+  pending_backend_operations_++;
   disk_cache_->StoreNexe(it->second.cache_key,
                          buffer,
                          base::Bind(&PnaclHost::OnTranslatedNexeStored,
                                     weak_factory_.GetWeakPtr(),
                                     it->first));
 }
 
 // After we know the nexe has been stored, we can clean up, and unblock any
 // outstanding requests for the same file.
 // (Bound callbacks must re-lookup the TranslationID because the translation
 // could be cancelled before they get called).
 void PnaclHost::OnTranslatedNexeStored(const TranslationID& id, int net_error) {
   PendingTranslationMap::iterator entry(pending_translations_.find(id));
+  pending_backend_operations_--;
   if (entry == pending_translations_.end()) {
+    // If the renderer closed while we were storing the nexe, we land here.
+    // Make sure we try to de-init.
+    DeInitIfSafe();
     return;
   }
   std::string key(entry->second.cache_key);
   pending_translations_.erase(entry);
   RequeryMatchingTranslations(key);
 }
 
 // Check if any pending translations match |key|. If so, re-issue the cache
 // query. In the overlapped miss case, we expect a hit this time, but a miss
 // is also possible in case of an error.
 void PnaclHost::RequeryMatchingTranslations(const std::string& key) {
   // Check for outstanding misses to this same file
   for (PendingTranslationMap::iterator it = pending_translations_.begin();
        it != pending_translations_.end();
        ++it) {
     if (it->second.cache_key == key) {
       // Re-send the cache read request. This time we expect a hit, but if
       // something goes wrong, it will just handle it like a miss.
       it->second.got_cache_reply = false;
+      pending_backend_operations_++;
       disk_cache_->GetNexe(key,
                            base::Bind(&PnaclHost::OnCacheQueryReturn,
                                       weak_factory_.GetWeakPtr(),
                                       it->first));
     }
   }
 }
 
 //////////////////// GetNexeFd hit path
 
@@ skipping 48 matching lines @@
           base::Bind(base::IgnoreResult(base::ClosePlatformFile),
                      to_erase->second.nexe_fd));
       std::string key(to_erase->second.cache_key);
       bool may_be_cached = TranslationMayBeCached(to_erase);
       pending_translations_.erase(to_erase);
       // No translations will be waiting for entries that will not be stored.
       if (may_be_cached)
         RequeryMatchingTranslations(key);
     }
   }
-  if (pending_translations_.empty()) {
-    cache_state_ = CacheUninitialized;
-    // Freeing the backend causes it to flush to disk, so do it when the
-    // last renderer closes rather than on destruction.
-    disk_cache_.reset();
-  }
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PnaclHost::DeInitIfSafe, weak_factory_.GetWeakPtr()));
 }
 
 ////////////////// Cache data removal
 void PnaclHost::ClearTranslationCacheEntriesBetween(
     base::Time initial_time,
     base::Time end_time,
     const base::Closure& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (cache_state_ == CacheUninitialized) {
     Init();
   }
   if (cache_state_ == CacheInitializing) {
     // If the backend hasn't yet initialized, try the request again later.
     BrowserThread::PostDelayedTask(
         BrowserThread::IO,
         FROM_HERE,
         base::Bind(&PnaclHost::ClearTranslationCacheEntriesBetween,
                    weak_factory_.GetWeakPtr(),
                    initial_time,
                    end_time,
                    callback),
         base::TimeDelta::FromMilliseconds(
             kTranslationCacheInitializationDelayMs));
     return;
   }
+  pending_backend_operations_++;
   int rv = disk_cache_->DoomEntriesBetween(
       initial_time,
       end_time,
-      base::Bind(&PnaclHost::OnEntriesDoomed, callback));
+      base::Bind(
+          &PnaclHost::OnEntriesDoomed, weak_factory_.GetWeakPtr(), callback));
   if (rv != net::ERR_IO_PENDING)
     OnEntriesDoomed(callback, rv);
 }
 
-// static
 void PnaclHost::OnEntriesDoomed(const base::Closure& callback, int net_error) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, callback);
+  pending_backend_operations_--;
+  // When clearing the cache, the UI is blocked on all the cache-clearing
+  // operations, and freeing the backend actually blocks the IO thread. So
+  // instead of calling DeInitIfSafe directly, post it for later.
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PnaclHost::DeInitIfSafe, weak_factory_.GetWeakPtr()));
 }
+
+// Destroying the cache backend causes it to post tasks to the cache thread to
+// flush to disk. Because PnaclHost is a singleton, it does not get destroyed
+// until all the browser threads have gone away and it's too late to post
+// anything (attempting to do so hangs shutdown).  So we make sure to destroy it
+// when we no longer have any outstanding operations that need it. These include
+// pending translations, cache clear requests, and requests to read or write
+// translated nexes.  We check when renderers close, when cache clear requests
+// finish, and when backend operations complete.
+
+// It is not safe to delete the backend while it is initializing, nor if it has
+// outstanding entry open requests; it is in theory safe to delete it with
+// outstanding read/write requests, but because that distinction is hidden
+// inside PnaclTranslationCache, we do not delete the backend if there are any
+// backend requests in flight.  As a last resort in the destructor, we just leak
+// the backend to avoid hanging shutdown.
+void PnaclHost::DeInitIfSafe() {
+  if (pending_translations_.empty() && pending_backend_operations_ <= 0) {

[jvoung (off chromium), 2013/10/24 21:14:58]

Should just be pending_backend_operations_ == 0? Seems bad if it becomes < 0?

[Derek Schuff, 2013/10/25 19:38:06]

Yeah, it's bad and definitely not expected, the question is just what to do. It
seemed better to just try to reset (and then later retry initialization if
necessary), than to continue in that state.

[jvoung (off chromium), 2013/10/25 19:51:43]

Maybe you could DCHECK() that it's never < 0 for testing, but reset otherwise.

[Derek Schuff, 2013/10/25 19:57:09]

good idea, done.

+    cache_state_ = CacheUninitialized;
+    disk_cache_.reset();
+  }
+}