Fix lifetime of leveldb::MojoEnv instances.

components/leveldb/leveldb_app.h

 // Copyright 2016 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.
 
 #ifndef COMPONENTS_LEVELDB_LEVELDB_APP_H_
 #define COMPONENTS_LEVELDB_LEVELDB_APP_H_
 
 #include <memory>
 
 #include "base/threading/thread.h"
 #include "components/leveldb/public/interfaces/leveldb.mojom.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "services/service_manager/public/cpp/binder_registry.h"
 #include "services/service_manager/public/cpp/interface_factory.h"
 #include "services/service_manager/public/cpp/service.h"
 #include "services/tracing/public/cpp/provider.h"
 
 namespace leveldb {
 
+class MojoEnv;
+
 class LevelDBApp
     : public service_manager::Service,
       public service_manager::InterfaceFactory<mojom::LevelDBService> {
  public:
   LevelDBApp();
   ~LevelDBApp() override;
 
  private:
   // |Service| override:
   void OnStart() override;
   void OnBindInterface(const service_manager::ServiceInfo& source_info,
                        const std::string& interface_name,
                        mojo::ScopedMessagePipeHandle interface_pipe) override;
 
   // |InterfaceFactory<mojom::LevelDBService>| implementation:
   void Create(const service_manager::Identity& remote_identity,
               leveldb::mojom::LevelDBServiceRequest request) override;
 
   tracing::Provider tracing_;
   std::unique_ptr<mojom::LevelDBService> service_;
   service_manager::BinderRegistry registry_;
   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);
 };
 
 }  // namespace leveldb
 
 #endif  // COMPONENTS_LEVELDB_LEVELDB_APP_H_