Delete PNaCl translation cache backend object when no longer needed

Delete PNaCl translation cache backend object when no longer needed

Deleting a disk cache backend causes it to post tasks to its cache thread to sync the files, and block the IO thread waiting for completion. Because PnaclHost is a singleton, it is not destroyed until the browser threads have gone away, which results in hangs on shutdown.
Previously the cache backend was deleted on renderer closing if it was unneeded, which worked when there was actual use of PNaCl, but did not work when it was a cache clear request that caused the backend to be initialized.
This CL makes PnaclHost also try to free the backend when entries are doomed, and when a nexe finishes storing.

R=jvoung@chromium.org
BUG=270948
TEST=unit_tests PnaclHostTest.*, PnaclTranslationCacheTest.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231128

[jvoung (off chromium), 2013-10-24 21:14:57]

What's the cost of ReInit'ing the cache?

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/pnacl_host.cc (right):

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
chrome/browser/nacl_host/pnacl_host.cc:37: pnacl::PnaclTranslationCache* cache =
disk_cache_.release();
Can this just be a one-liner (void)...release()?

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

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
chrome/browser/nacl_host/pnacl_host.cc:625: if (pending_translations_.empty() &&
pending_backend_operations_ <= 0) {
Should just be pending_backend_operations_ == 0? Seems bad if it becomes < 0?

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

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/pnacl_host.cc (right):

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
chrome/browser/nacl_host/pnacl_host.cc:37: pnacl::PnaclTranslationCache* cache =
disk_cache_.release();
On 2013/10/24 21:14:58, jvoung (cr) wrote:
> Can this just be a one-liner (void)...release()?
Nope, gcc still warns about unused result if you do that. 

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

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)

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
chrome/browser/nacl_host/pnacl_host.cc:625: if (pending_translations_.empty() &&
pending_backend_operations_ <= 0) {
On 2013/10/24 21:14:58, jvoung (cr) wrote:
> Should just be pending_backend_operations_ == 0? Seems bad if it becomes < 0?

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]

lgtm

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/pnacl_host.cc (right):

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
chrome/browser/nacl_host/pnacl_host.cc:625: if (pending_translations_.empty() &&
pending_backend_operations_ <= 0) {
On 2013/10/25 19:38:06, Derek Schuff wrote:
> On 2013/10/24 21:14:58, jvoung (cr) wrote:
> > Should just be pending_backend_operations_ == 0? Seems bad if it becomes <
0?
> 
> 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.

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

[Derek Schuff, 2013-10-25 19:57:08]

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/pnacl_host.cc (right):

https://codereview.chromium.org/28933003/diff/400001/chrome/browser/nacl_host...
chrome/browser/nacl_host/pnacl_host.cc:625: if (pending_translations_.empty() &&
pending_backend_operations_ <= 0) {
On 2013/10/25 19:51:43, jvoung (cr) wrote:
> On 2013/10/25 19:38:06, Derek Schuff wrote:
> > On 2013/10/24 21:14:58, jvoung (cr) wrote:
> > > Should just be pending_backend_operations_ == 0? Seems bad if it becomes <
> 0?
> > 
> > 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.
> 
> Maybe you could DCHECK() that it's never < 0 for testing, but reset otherwise.

good idea, done.

[commit-bot: I haz the power, 2013-10-25 20:00:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/28933003/520001

[commit-bot: I haz the power, 2013-10-25 22:07:36]

Change committed as 231128