Use leveldb_env::OpenDB() to open leveldb databases.

components/sync/model_impl/model_type_store_backend.cc

Index: components/sync/model_impl/model_type_store_backend.cc
diff --git a/components/sync/model_impl/model_type_store_backend.cc b/components/sync/model_impl/model_type_store_backend.cc
index a17453122e1be09df40f1d75ed62e4e07d4ae794..f4129b983b599a03d2ffecb50490112cdbd297e9 100644
--- a/components/sync/model_impl/model_type_store_backend.cc
+++ b/components/sync/model_impl/model_type_store_backend.cc
@@ -178,7 +178,6 @@ ModelTypeStore::Result ModelTypeStoreBackend::Init(
 
 leveldb::Status ModelTypeStoreBackend::OpenDatabase(const std::string& path,
                                                     leveldb::Env* env) {
-  leveldb::DB* db_raw = nullptr;
   leveldb::Options options;
   options.create_if_missing = true;
   options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
@@ -186,10 +185,8 @@ leveldb::Status ModelTypeStoreBackend::OpenDatabase(const std::string& path,
   if (env)
     options.env = env;
 
-  leveldb::Status status = leveldb::DB::Open(options, path, &db_raw);
-  DCHECK(status.ok() != (db_raw == nullptr));
-  if (status.ok())
-    db_.reset(db_raw);
+  leveldb::Status status = leveldb_env::OpenDB(options, path, &db_);
+  DCHECK(status.ok());

[pwnall, 2017/07/06 14:32:25]

I think the intention here was to DCHECK that db_ is not nullptr when
status.ok(). Can you please switch this to DCHECK(!status.ok() || db_) ?

[DmitrySkiba, 2017/07/06 23:15:13]

Good catch! This also caused tests to fail.

However, as OpenDB() guarantees that: (1) db_ is not modified on failure, and
(2) db_ is not NULL on success (as of latest patchset), I've removed DCHECK
completely.

[pwnall, 2017/07/07 22:30:29]

AFAIK, DCHECK can be used to assert that I've understood another module's
preconditions / guarantees, and to make sure that the API contract doesn't
change in a way that the type system wouldn't reflect.  From that perspective,
DCHECKs that are redundant with documentation or with the called code's DCHECKs
seem acceptable to me, and better than the prospect of having subtle bugs get
introduced later on.

That being said, I don't own this code, so in the end it's up to the author /
owner. The guarantees you've added to leveldb_env::OpenDB() make perfect sense
to me, and I agree with having a DCHECK there.

[DmitrySkiba, 2017/07/17 22:09:25]

Acknowledged.

   return status;
 }