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

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

This CL rewrites usages of leveldb::DB::Open() to leveldb_env::OpenDB(),
exposing memory usage of leveldb databases to Chrome's tracing.

The only API difference of leveldb_env::OpenDB() is related to how it returns
its result:

1. It returns (via output parameter) std::unique_ptr<leveldb::DB>, while
   leveldb::DB::Open() returns a raw leveldb::DB pointer.

2. It doesn't modify output parameter on failure, while leveldb::DB::Open()
   returns (writes) nullptr.

Both of these changes are consistent with Chrome's guidelines.

Under the hood leveldb_env::OpenDB() uses leveldb_env::DBTracker to open and
expose databases to Chrome's tracing. See crrev.com/2855953002 for details.

Finally, this CL does two more things:

1. Adds a presubmit check to enforce usage of leveldb_env::OpenDB().

2. Fixes existing memory dump providers that already dump some leveldb
   databases to properly attribute their memory usage. Further work on
   those MDPs is tracked by crbug.com/735269.

BUG=711518
Review-Url: https://codereview.chromium.org/2953473002
Cr-Commit-Position: refs/heads/master@{#488099}
Committed: https://chromium.googlesource.com/chromium/src/+/1474c2bfd6194e04828472b5b0bda9b5a50912c8

[DmitrySkiba, 2017-07-06 00:11:23]

Description was changed from

==========
Register leveldb databases in DBRegistry.

TODO description
==========

to

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

This CL rewrites usages of leveldb::DB::Open() to leveldb_env::OpenDB(),
exposing memory usage of leveldb databases to Chrome's tracing.

The only API difference of leveldb_env::OpenDB() is related to how it returns
its result:

1. It returns (via output parameter) std::unique_ptr<leveldb::DB>, while
   leveldb::DB::Open() returns a raw leveldb::DB pointer.

2. It doesn't modify output parameter on failure, while leveldb::DB::Open()
   returns (writes) nullptr.

Both of these changes are consistent with Chrome's guidelines.

Under the hood leveldb_env::OpenDB() uses leveldb_env::DBTracker to open and
expose databases to Chrome's tracing. See crrev.com/2855953002 for details.

Finally, this CL does two more things:

1. Adds a presubmit check to enforce usage of leveldb_env::OpenDB().

2. Fixes existing memory dump providers that already dump some leveldb
   databases to properly attribute their memory usage. Further work on
   those MDPs is tracked by crbug.com/735269.

BUG=711518
==========

[DmitrySkiba, 2017-07-06 01:01:53]

PTAL

I decided that it's not worth adding any sort of checks to
DBTracker::GetMemoryDumpName() because:

1. There shouldn't be any untracked databases. Presubmit check makes sure no new
ones are added.

2. memory-infra will show an edge in leveldatabase MDP even if it wasn't added
directly. I.e. if a database 0xf00 is not tracked, but added as an edge (via
ProcessMemoryDump::AddSuballocation(..., DBTracker::GetMemoryDumpName(0xf00)),
it will end up showing in leveldatabase MDP in the UI.

[DmitrySkiba, 2017-07-06 01:02:40]

dskiba@chromium.org changed reviewers:
+ pwnall@chromium.org

[DmitrySkiba, 2017-07-06 01:02:41]

PTAL

I decided that it's not worth adding any sort of checks to
DBTracker::GetMemoryDumpName() because:

1. There shouldn't be any untracked databases. Presubmit check makes sure no new
ones are added.

2. memory-infra will show an edge in leveldatabase MDP even if it wasn't added
directly. I.e. if a database 0xf00 is not tracked, but added as an edge (via
ProcessMemoryDump::AddSuballocation(..., DBTracker::GetMemoryDumpName(0xf00)),
it will end up showing in leveldatabase MDP in the UI.

[DmitrySkiba, 2017-07-06 01:30:55]

The CQ bit was checked by dskiba@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 01:31:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 02:02:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 02:02:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pwnall, 2017-07-06 14:32:07]

On 2017/07/06 01:02:41, DmitrySkiba wrote:
> PTAL
> 
> I decided that it's not worth adding any sort of checks to
> DBTracker::GetMemoryDumpName() because:
> 
> 1. There shouldn't be any untracked databases. Presubmit check makes sure no
new
> ones are added.
> 
> 2. memory-infra will show an edge in leveldatabase MDP even if it wasn't added
> directly. I.e. if a database 0xf00 is not tracked, but added as an edge (via
> ProcessMemoryDump::AddSuballocation(..., DBTracker::GetMemoryDumpName(0xf00)),
> it will end up showing in leveldatabase MDP in the UI.

How about the case where a leveldb::DBImpl* (LevelDB's leveldb::DB subclass)
somehow makes its way into Chrome's code and GetMemoryDumpName() gets called on
it?

I still think that GetMemoryDumpName()'s doc comment should state that it should
be used with leveldb::DB* values coming from OpenDB.

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

https://codereview.chromium.org/2953473002/diff/80001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/2953473002/diff/80001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:227:
leveldb_env::OpenDB(options, path.AsUTF8Unsafe(), &db);
I expect you'll need to #include "third_party/leveldatabase/env_chromium.h" for
this.

https://codereview.chromium.org/2953473002/diff/80001/components/leveldb_prot...
File components/leveldb_proto/leveldb_database.cc (left):

https://codereview.chromium.org/2953473002/diff/80001/components/leveldb_prot...
components/leveldb_proto/leveldb_database.cc:74: CHECK(db);
Can you please leave the CHECK in? I think CHECK(db_) should do.

https://codereview.chromium.org/2953473002/diff/80001/components/sync/model_i...
File components/sync/model_impl/model_type_store_backend.cc (right):

https://codereview.chromium.org/2953473002/diff/80001/components/sync/model_i...
components/sync/model_impl/model_type_store_backend.cc:189: DCHECK(status.ok());
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]

Also added note to GetMemoryDumpName() that db should be tracked, and renamed
the argument to |tracked_db|.

https://codereview.chromium.org/2953473002/diff/80001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/2953473002/diff/80001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:227:
leveldb_env::OpenDB(options, path.AsUTF8Unsafe(), &db);
On 2017/07/06 14:32:25, pwnall wrote:
> I expect you'll need to #include "third_party/leveldatabase/env_chromium.h"
for
> this.

Actually, it's already there.

https://codereview.chromium.org/2953473002/diff/80001/components/leveldb_prot...
File components/leveldb_proto/leveldb_database.cc (left):

https://codereview.chromium.org/2953473002/diff/80001/components/leveldb_prot...
components/leveldb_proto/leveldb_database.cc:74: CHECK(db);
On 2017/07/06 14:32:25, pwnall wrote:
> Can you please leave the CHECK in? I think CHECK(db_) should do.

I think this is better done on OpenDB() side - I've moved CHECK() there and
documented that it never returns NULL on success.

https://codereview.chromium.org/2953473002/diff/80001/components/sync/model_i...
File components/sync/model_impl/model_type_store_backend.cc (right):

https://codereview.chromium.org/2953473002/diff/80001/components/sync/model_i...
components/sync/model_impl/model_type_store_backend.cc:189: DCHECK(status.ok());
On 2017/07/06 14:32:25, pwnall wrote:
> 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_) ?

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.

[DmitrySkiba, 2017-07-06 23:15:25]

The CQ bit was checked by dskiba@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 23:15:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 23:18:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 23:18:29]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[DmitrySkiba, 2017-07-06 23:55:44]

The CQ bit was checked by dskiba@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 23:55:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 00:26:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 00:26:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[DmitrySkiba, 2017-07-07 19:36:01]

The CQ bit was checked by dskiba@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 19:36:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 21:20:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 21:20:33]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-07-07 22:30:30]

LGTM w/ nits.

https://codereview.chromium.org/2953473002/diff/80001/components/sync/model_i...
File components/sync/model_impl/model_type_store_backend.cc (right):

https://codereview.chromium.org/2953473002/diff/80001/components/sync/model_i...
components/sync/model_impl/model_type_store_backend.cc:189: DCHECK(status.ok());
On 2017/07/06 23:15:13, DmitrySkiba wrote:
> On 2017/07/06 14:32:25, pwnall wrote:
> > 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_) ?
> 
> 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.

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.

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.cc:1254: return
base::StringPrintf("leveldatabase/0x%" PRIXPTR,
nit: I'd prefer that GetMemoryDumpName() DCHECKs that tracked_db is in the
linked list. I can do it if you don't get around to.

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
File third_party/leveldatabase/env_chromium.h (right):

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.h:261: // memory-infra and is enumerated
by VisitDatabases() method.
nit 1:  Can you please add a statement that this is an implementation detail,
and other modules should call leveldb_env::OpenDB()?

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.h:262: // The function guarantees that:
nit 2: You could say that the function has the same guarantees as
leveldb_env::OpenDB(). I think a redirect is acceptable here, as this is an
implementation detail, and most readers should end up reading
leveldb_env::OpenDB() instead. Also, the redirect is a reminder that OpenDB()
relies on OpenDatabase() for these guarantees.

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

https://codereview.chromium.org/2953473002/diff/80001/components/sync/model_i...
File components/sync/model_impl/model_type_store_backend.cc (right):

https://codereview.chromium.org/2953473002/diff/80001/components/sync/model_i...
components/sync/model_impl/model_type_store_backend.cc:189: DCHECK(status.ok());
On 2017/07/07 22:30:29, pwnall wrote:
> On 2017/07/06 23:15:13, DmitrySkiba wrote:
> > On 2017/07/06 14:32:25, pwnall wrote:
> > > 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_) ?
> > 
> > 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.
> 
> 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.

Acknowledged.

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.cc:1254: return
base::StringPrintf("leveldatabase/0x%" PRIXPTR,
On 2017/07/07 22:30:29, pwnall wrote:
> nit: I'd prefer that GetMemoryDumpName() DCHECKs that tracked_db is in the
> linked list. I can do it if you don't get around to.

I can see value in that DCHECK, but I'm not sure what is the right way to add
it. The thing is that GetMemoryDumpName() is called from VisitDatabases visitor
right now, so we either have to split it in two (internal version that doesn't
lock, external that locks), or we introduce two GetMemoryDumpName() versions:
one that takes DB and has DHCECK (and calls another), and the one that takes
TrackedDB, and doesn't lock. Neither option is ideal to me. I suggest we move
further with this implementation, and and continue searching for a proper way in
a follow-up CL (which I'll create).

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
File third_party/leveldatabase/env_chromium.h (right):

https://codereview.chromium.org/2953473002/diff/160001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.h:262: // The function guarantees that:
On 2017/07/07 22:30:29, pwnall wrote:
> nit 2: You could say that the function has the same guarantees as
> leveldb_env::OpenDB(). I think a redirect is acceptable here, as this is an
> implementation detail, and most readers should end up reading
> leveldb_env::OpenDB() instead. Also, the redirect is a reminder that OpenDB()
> relies on OpenDatabase() for these guarantees.

Done.

[DmitrySkiba, 2017-07-17 22:56:05]

dskiba@chromium.org changed reviewers:
+ hashimoto@chromium.org, jochen@chromium.org, jsbell@chromium.org,
mathp@chromium.org, megjablon@chromium.org, mek@chromium.org,
michaeln@chromium.org, nyquist@chromium.org, pavely@chromium.org,
peter@chromium.org, rdevlin.cronin@chromium.org, tzik@chromium.org,
zea@chromium.org

[DmitrySkiba, 2017-07-17 22:56:05]

Folks, welcome to the party! Please review responsibly!

See CL description for info. Here are how two common levedb::DB::Open()
usage patterns translate to levedb_env::OpenDB():

1.  levedb::DB* raw_db = nullptr;
    status = levedb::DB::Open(options, name, &raw_db);
    if (status.ok()) {
      db_.reset(raw_db);
    }
    ...

    translates into

    status = levedb_env::OpenDB(options, name, &db_);
    ...


2.  levedb::DB* raw_db = nullptr;
    status = levedb::DB::Open(options, name, &raw_db);
    db_.reset(raw_db);
    ...

    translates into

    db_.reset();
    status = leveldb_env::OpenDB(options, name, &db_);
    ...

--------------------------------------------------------------------------------

jochen@
  PRESUBMIT.py

nyquist@
  chrome/browser/android/history_report/*

tzik@
  chrome/browser/sync_file_system/*
  storage/browser/fileapi/*

megjablon@
  components/data_reduction_proxy/core/browser/data_store_impl.cc

hashimoto@
  components/drive/resource_metadata_storage.cc

mek@
  components/leveldb/*
  content/browser/service_worker/service_worker_database.cc

mathp@
  components/leveldb_proto/leveldb_database.cc

pavely@
  components/sync/*

michaeln@
  content/browser/dom_storage/*

jsbell@
  content/browser/indexed_db/leveldb/leveldb_database.cc

peter@
  content/browser/notifications/notification_database.cc

rdevlin.cronin@
  extensions/browser/value_store/*

zea@
  google_apis/gcm/engine/gcm_store_impl.cc

[Devlin, 2017-07-17 23:06:19]

extensions/browser/value_store* lgtm.

Thanks for the detailed migration description!

[pavely, 2017-07-18 00:16:34]

components/sync/* lgtm

[michaeln, 2017-07-18 00:18:49]

dom_storage lgtm

https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.cc:1253: static DBTracker* instance = new
DBTracker();
is this getter (and ctor) thread safe? unless there's a reason not to, i think
i'd vote for using a base::LazyInstance<> here for consistency

[DmitrySkiba, 2017-07-18 00:22:40]

On 2017/07/18 00:18:49, michaeln wrote:
> dom_storage lgtm
> 
>
https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
> File third_party/leveldatabase/env_chromium.cc (right):
> 
>
https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
> third_party/leveldatabase/env_chromium.cc:1253: static DBTracker* instance =
new
> DBTracker();
> is this getter (and ctor) thread safe? unless there's a reason not to, i think
> i'd vote for using a base::LazyInstance<> here for consistency

Yup, this is now the recommended pattern (over LazyInstance), see
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

[michaeln, 2017-07-18 00:47:22]

https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.cc:1253: static DBTracker* instance = new
DBTracker();
On 2017/07/18 00:18:49, michaeln wrote:
> is this getter (and ctor) thread safe? unless there's a reason not to, i think
> i'd vote for using a base::LazyInstance<> here for consistency

Ack, thnx!

[jochen (gone - plz use gerrit), 2017-07-18 07:09:11]

lgtm

[hashimoto, 2017-07-18 08:11:52]

components/drive/resource_metadata_storage.cc lgtm

[Marijn Kruisselbrink, 2017-07-18 17:12:25]

lgtm

https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.cc:1267: CHECK((status.ok() && db) ||
(!status.ok() && !db));
nit: might be slightly nicer as something like CHECK_EQ(status.ok(), db !=
nullptr) ?

[megjablon, 2017-07-18 18:22:38]

lgtm

[nyquist, 2017-07-18 18:48:16]

chrome/browser/android/history_report lgtm

https://codereview.chromium.org/2953473002/diff/200001/components/leveldb_pro...
File components/leveldb_proto/leveldb_database.cc (left):

https://codereview.chromium.org/2953473002/diff/200001/components/leveldb_pro...
components/leveldb_proto/leveldb_database.cc:75: CHECK(db);
This CHECK seems to be kept in place some places, is there a particular reason
why it is not changed here to become CHECK(db_) instead of removed?

[DmitrySkiba, 2017-07-18 19:01:05]

https://codereview.chromium.org/2953473002/diff/200001/components/leveldb_pro...
File components/leveldb_proto/leveldb_database.cc (left):

https://codereview.chromium.org/2953473002/diff/200001/components/leveldb_pro...
components/leveldb_proto/leveldb_database.cc:75: CHECK(db);
On 2017/07/18 18:48:16, nyquist wrote:
> This CHECK seems to be kept in place some places, is there a particular reason
> why it is not changed here to become CHECK(db_) instead of removed?

OpenDB() guarantees that now (i.e. CHECK effectively moved there). Specifically
it guarantees that (1) db_ is not touched on failure, and (2) db_ is not NULL on
success.

https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/2953473002/diff/200001/third_party/leveldatab...
third_party/leveldatabase/env_chromium.cc:1267: CHECK((status.ok() && db) ||
(!status.ok() && !db));
On 2017/07/18 17:12:25, Marijn Kruisselbrink wrote:
> nit: might be slightly nicer as something like CHECK_EQ(status.ok(), db !=
> nullptr) ?

I like the way it's now because it explicitly lists two conditions that we
accept. I'll add a comment explaining that.

[tzik, 2017-07-19 04:49:43]

lgtm

[jsbell, 2017-07-19 16:59:30]

lgtm

[Peter Beverloo, 2017-07-19 17:04:28]

lgtm

[Mathieu, 2017-07-19 17:05:34]

lgtm

[Nicolas Zea, 2017-07-19 18:02:41]

gcm lgtm

[DmitrySkiba, 2017-07-19 20:45:57]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2017-07-19 20:45:58]

The patchset sent to the CQ was uploaded after l-g-t-m from pwnall@chromium.org,
michaeln@chromium.org, rdevlin.cronin@chromium.org, jochen@chromium.org,
jsbell@chromium.org, pavely@chromium.org, tzik@chromium.org, mathp@chromium.org,
megjablon@chromium.org, nyquist@chromium.org, peter@chromium.org,
hashimoto@chromium.org, mek@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2953473002/#ps220001
(title: "Rebase; add comments to CHECK()")

[commit-bot: I haz the power, 2017-07-19 20:46:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-19 21:47:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-19 21:47:21]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DmitrySkiba, 2017-07-19 23:17:52]

The CQ bit was checked by dskiba@chromium.org

[commit-bot: I haz the power, 2017-07-19 23:18:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-20 02:19:27]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1500506272572870,
"parent_rev": "57d461f80f1d36ca3c85a1da43ad8538c5e992cf", "commit_rev":
"1474c2bfd6194e04828472b5b0bda9b5a50912c8"}

[commit-bot: I haz the power, 2017-07-20 02:19:41]

Description was changed from

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

This CL rewrites usages of leveldb::DB::Open() to leveldb_env::OpenDB(),
exposing memory usage of leveldb databases to Chrome's tracing.

The only API difference of leveldb_env::OpenDB() is related to how it returns
its result:

1. It returns (via output parameter) std::unique_ptr<leveldb::DB>, while
   leveldb::DB::Open() returns a raw leveldb::DB pointer.

2. It doesn't modify output parameter on failure, while leveldb::DB::Open()
   returns (writes) nullptr.

Both of these changes are consistent with Chrome's guidelines.

Under the hood leveldb_env::OpenDB() uses leveldb_env::DBTracker to open and
expose databases to Chrome's tracing. See crrev.com/2855953002 for details.

Finally, this CL does two more things:

1. Adds a presubmit check to enforce usage of leveldb_env::OpenDB().

2. Fixes existing memory dump providers that already dump some leveldb
   databases to properly attribute their memory usage. Further work on
   those MDPs is tracked by crbug.com/735269.

BUG=711518
==========

to

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

This CL rewrites usages of leveldb::DB::Open() to leveldb_env::OpenDB(),
exposing memory usage of leveldb databases to Chrome's tracing.

The only API difference of leveldb_env::OpenDB() is related to how it returns
its result:

1. It returns (via output parameter) std::unique_ptr<leveldb::DB>, while
   leveldb::DB::Open() returns a raw leveldb::DB pointer.

2. It doesn't modify output parameter on failure, while leveldb::DB::Open()
   returns (writes) nullptr.

Both of these changes are consistent with Chrome's guidelines.

Under the hood leveldb_env::OpenDB() uses leveldb_env::DBTracker to open and
expose databases to Chrome's tracing. See crrev.com/2855953002 for details.

Finally, this CL does two more things:

1. Adds a presubmit check to enforce usage of leveldb_env::OpenDB().

2. Fixes existing memory dump providers that already dump some leveldb
   databases to properly attribute their memory usage. Further work on
   those MDPs is tracked by crbug.com/735269.

BUG=711518

Review-Url: https://codereview.chromium.org/2953473002
Cr-Commit-Position: refs/heads/master@{#488099}
Committed:
https://chromium.googlesource.com/chromium/src/+/1474c2bfd6194e04828472b5b0bd...
==========

[commit-bot: I haz the power, 2017-07-20 02:19:44]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/1474c2bfd6194e04828472b5b0bd...