Fix lifetime of leveldb::MojoEnv instances.

components/leveldb/leveldb_app.h

Index: components/leveldb/leveldb_app.h
diff --git a/components/leveldb/leveldb_app.h b/components/leveldb/leveldb_app.h
index 1898f525984b5be631d6fe6b0f621e3483c682e9..03d65b8854aa5820de656a60b693fcbd4129f90b 100644
--- a/components/leveldb/leveldb_app.h
+++ b/components/leveldb/leveldb_app.h
@@ -17,6 +17,8 @@
 
 namespace leveldb {
 
+class MojoEnv;
+
 class LevelDBApp
     : public service_manager::Service,
       public service_manager::InterfaceFactory<mojom::LevelDBService> {
@@ -41,6 +43,9 @@ class LevelDBApp
   mojo::BindingSet<mojom::LevelDBService> bindings_;
 
   base::Thread file_thread_;
+  // env_ is owned by LevelDBApp (since it depends on the file_thread_ above),

[cmumford, 2017/04/12 21:51:34]

The only reason ChromiumEnv has a NOTREACHED in the destructor is to prevent the
default ChromiumEnv from being destroyed since it's owned by a LazyInstance.
What do you think about modifying the destructor to be:

ChromiumEnv::~ChromiumEnv() {
  DCHECK(!(default_env == this)) << "Cannot delete the default ChromiumEnv";
}

and then you can destroy MojoEnv and not introduce a leak?

[Marijn Kruisselbrink, 2017/04/12 22:00:34]

That it not the only reason. Destroying ChromiumEnv is in fact not supported at
all currently, as it will involve destroying the owned base::ConditionVariable,
which has a DCHECK to ensure you're not trying to destroy it while anybody is
waiting for it, and the BGThread is likely to be waiting for it. So at the very
least ~ChromiumEnv would need to somehow be changed to signal the BGThread to
quit itself, and then wait for the thread to actually die.

I have no idea if there are other places in ChromiumEnv that are also assuming
it never gets destroyed. This is just one case I actually ran into.

[cmumford, 2017/04/12 23:06:39]

That's right.

Also, this was prior to your change, but I don't see any reason for MojoEnv to
derive from ChromiumEnv. It overrides all but four functions, and doesn't get
any of it's UMA logging functionality in ChromiumEnv. I think that using
leveldb::Env would be simpler. You would need to implement StartThread, etc.,
and as you mention join the background thread before destruction.

[Marijn Kruisselbrink, 2017/04/12 23:46:22]

Yeah, maybe that's the way to go... If we do that, would there still be any
reason for this CL? Or would having a separate MojoEnv for each connection be
just fine (other than the slight inefficiency of having a different background
thread for each connection)?

+  // but doesn't support destruction, so it is leaked.
+  MojoEnv* env_;
 
   DISALLOW_COPY_AND_ASSIGN(LevelDBApp);
 };