[tracing] Add separate dump provider for sql connection

[tracing] Add separate dump provider for sql connection

The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.

BUG=466141
Committed: https://crrev.com/3be5b1ecdf66f6eaaa0ba98f9b36cc2d93bf54d9
Cr-Commit-Position: refs/heads/master@{#369161}

[ssid, 2015-11-11 17:21:51]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[ssid, 2015-11-11 17:21:51]

PTAL, Thanks.

[ssid, 2016-01-07 18:29:51]

Patchset #3 (id:40001) has been deleted

[ssid, 2016-01-07 18:49:10]

Patchset #3 (id:60001) has been deleted

[ssid, 2016-01-07 19:15:30]

Patchset #3 (id:80001) has been deleted

[ssid, 2016-01-07 19:18:28]

Rebased. PTAL, thanks.

[ssid, 2016-01-08 15:09:59]

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

[commit-bot: I haz the power, 2016-01-08 15:10:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1434993002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1434993002/100001

[commit-bot: I haz the power, 2016-01-08 16:20:31]

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

[commit-bot: I haz the power, 2016-01-08 16:20:32]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-01-11 19:27:43]

LGTM with some minor comments.
Thanks

https://codereview.chromium.org/1434993002/diff/100001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1434993002/diff/100001/sql/connection.cc#newc...
sql/connection.cc:1820: memory_dump_provider_.reset(
Add a DCHECK(!memory_dump_provider_)
The mdp_ must be empty at this point, otherwise the MDM will have a registration
that has been deleted (Read will cause the MDM to use-after-free )

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
File sql/connection_memory_dump_provider.cc (right):

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.cc:17: DCHECK(db_);
this dcheck doesn't seem to be really needed as you have a full if below

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.cc:28: base::AutoLock l(lock_);
add a comment explaining why you need this lock here.

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
File sql/connection_memory_dump_provider.h (right):

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:5: #ifndef
SQL_SQL_CONNECTION_MEMORY_DUMP_PROVIDER_H
nit s/SQL_SQL_/SQL_/

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:24: void reset_database() {
non-trivial methods should be CamelCased and be defined in the .cc
considering that you are taking a lock here, you definitely don't want this to
be considered as "trivial" :)

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:25: base::AutoLock l(lock_);
nit: s/l/lock/ typically I see this in the codebase in this way (16 vs 280
results)

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:35: // Not owned by
ConnectionMemoryDumpProvider. Weak reference to the database
remove the "weak reference to ..." it's confusing as it sounds like weak
pointer, which is not.

I think that just sqlite3* db_;  // not owned is sufficient

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:39: std::string name_;
maybe s/name_/connection_name_/ ?

[ssid, 2016-01-12 11:47:10]

Patchset #4 (id:120001) has been deleted

[ssid, 2016-01-12 12:23:08]

Made suggested changes. Thanks

https://codereview.chromium.org/1434993002/diff/100001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1434993002/diff/100001/sql/connection.cc#newc...
sql/connection.cc:1820: memory_dump_provider_.reset(
On 2016/01/11 19:27:42, Primiano Tucci wrote:
> Add a DCHECK(!memory_dump_provider_)
> The mdp_ must be empty at this point, otherwise the MDM will have a
registration
> that has been deleted (Read will cause the MDM to use-after-free )

Done.

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
File sql/connection_memory_dump_provider.cc (right):

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.cc:17: DCHECK(db_);
On 2016/01/11 19:27:42, Primiano Tucci wrote:
> this dcheck doesn't seem to be really needed as you have a full if below

Done.

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.cc:28: base::AutoLock l(lock_);
On 2016/01/11 19:27:42, Primiano Tucci wrote:
> add a comment explaining why you need this lock here.

Done.

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
File sql/connection_memory_dump_provider.h (right):

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:5: #ifndef
SQL_SQL_CONNECTION_MEMORY_DUMP_PROVIDER_H
On 2016/01/11 19:27:42, Primiano Tucci wrote:
> nit s/SQL_SQL_/SQL_/

Done.

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:24: void reset_database() {
On 2016/01/11 19:27:42, Primiano Tucci wrote:
> non-trivial methods should be CamelCased and be defined in the .cc
> considering that you are taking a lock here, you definitely don't want this to
> be considered as "trivial" :)

Done.

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:25: base::AutoLock l(lock_);
On 2016/01/11 19:27:42, Primiano Tucci wrote:
> nit: s/l/lock/ typically I see this in the codebase in this way (16 vs 280
> results)

Done.

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:35: // Not owned by
ConnectionMemoryDumpProvider. Weak reference to the database
On 2016/01/11 19:27:42, Primiano Tucci wrote:
> remove the "weak reference to ..." it's confusing as it sounds like weak
> pointer, which is not.
> 
> I think that just sqlite3* db_;  // not owned is sufficient

Done.

https://codereview.chromium.org/1434993002/diff/100001/sql/connection_memory_...
sql/connection_memory_dump_provider.h:39: std::string name_;
On 2016/01/11 19:27:42, Primiano Tucci wrote:
> maybe s/name_/connection_name_/ ?

Done.

[ssid, 2016-01-12 12:23:49]

ssid@chromium.org changed reviewers:
+ shess@chromium.org

[ssid, 2016-01-12 12:23:50]

+shess This is to fix the race that can happen when connection is cleared when
taking a dump. PTAL, Thanks

[Primiano Tucci (use gerrit), 2016-01-12 13:41:11]

Description was changed from

==========
[tracing] Add separate dump provider for sql connection

The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.

BUG=
==========

to

==========
[tracing] Add separate dump provider for sql connection

The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.

BUG=466141
==========

[Scott Hess - ex-Googler, 2016-01-12 19:33:13]

shess@chromium.org changed reviewers:
+ michaeln@chromium.org

[Scott Hess - ex-Googler, 2016-01-12 19:33:14]

LGTM.

Michael, in case he wants to add comments.  As noted below, I think this is
right.  It's overall a bit sketchy, but I don't think it's sketchier than the
problem being solved, so I'm not inclined to churn on that front.

https://codereview.chromium.org/1434993002/diff/140001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1434993002/diff/140001/sql/connection.cc#newc...
sql/connection.cc:481: }
OK, reasoning this out, a dump before the reset is fine, a dump after the reset
if fine (db_ will be null), and the reset is locked to push a dump during the
reset to after.  Then ownership is removed from here, and passed over to the
manager, so it that unregister/delete will be interleaved with dumps on that
thread, so the necessary lock will exist until it is no longer needed.

I think that's it.

[Primiano Tucci (use gerrit), 2016-01-12 19:38:02]

https://codereview.chromium.org/1434993002/diff/140001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1434993002/diff/140001/sql/connection.cc#newc...
sql/connection.cc:481: }
On 2016/01/12 19:33:14, Scott Hess wrote:
> OK, reasoning this out, a dump before the reset is fine, a dump after the
reset
> if fine (db_ will be null), and the reset is locked to push a dump during the
> reset to after.  Then ownership is removed from here, and passed over to the
> manager, so it that unregister/delete will be interleaved with dumps on that
> thread, so the necessary lock will exist until it is no longer needed.
> 
> I think that's it.

Yup, this reasoning matches perfectly ours :)

[ssid, 2016-01-13 11:20:13]

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

[commit-bot: I haz the power, 2016-01-13 11:20:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1434993002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1434993002/140001

[commit-bot: I haz the power, 2016-01-13 12:01:19]

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

[commit-bot: I haz the power, 2016-01-13 12:01:19]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-01-13 12:56:31]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-01-13 12:56:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1434993002/#ps140001
(title: "Fixes.")

[commit-bot: I haz the power, 2016-01-13 12:56:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1434993002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1434993002/140001

[commit-bot: I haz the power, 2016-01-13 14:22:05]

Description was changed from

==========
[tracing] Add separate dump provider for sql connection

The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.

BUG=466141
==========

to

==========
[tracing] Add separate dump provider for sql connection

The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.

BUG=466141
==========

[commit-bot: I haz the power, 2016-01-13 14:22:06]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-01-13 14:23:21]

Description was changed from

==========
[tracing] Add separate dump provider for sql connection

The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.

BUG=466141
==========

to

==========
[tracing] Add separate dump provider for sql connection

The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.

BUG=466141

Committed: https://crrev.com/3be5b1ecdf66f6eaaa0ba98f9b36cc2d93bf54d9
Cr-Commit-Position: refs/heads/master@{#369161}
==========

[commit-bot: I haz the power, 2016-01-13 14:23:22]

Patchset 4 (id:??) landed as
https://crrev.com/3be5b1ecdf66f6eaaa0ba98f9b36cc2d93bf54d9
Cr-Commit-Position: refs/heads/master@{#369161}

[michaeln, 2016-01-14 01:37:40]

https://codereview.chromium.org/1434993002/diff/140001/sql/connection_memory_...
File sql/connection_memory_dump_provider.cc (right):

https://codereview.chromium.org/1434993002/diff/140001/sql/connection_memory_...
sql/connection_memory_dump_provider.cc:44: int status = sqlite3_db_status(db_,
SQLITE_DBSTATUS_CACHE_USED, &cache_size,
maybe a comment or check of some kind about SQLITE_THREADSAFE being turned on:  
DCHECK(sqlite3_threadsafe())

[Scott Hess - ex-Googler, 2016-01-21 20:30:07]

https://codereview.chromium.org/1434993002/diff/140001/sql/connection_memory_...
File sql/connection_memory_dump_provider.cc (right):

https://codereview.chromium.org/1434993002/diff/140001/sql/connection_memory_...
sql/connection_memory_dump_provider.cc:44: int status = sqlite3_db_status(db_,
SQLITE_DBSTATUS_CACHE_USED, &cache_size,
On 2016/01/14 01:37:40, michaeln wrote:
> maybe a comment or check of some kind about SQLITE_THREADSAFE being turned on:
 
> DCHECK(sqlite3_threadsafe())

SQLITE_THREADSAFE is out of reach, here, but sqlite3_threadsafe() returns
whatever SQLITE_THREADSAFE is in the sqlite compile.

If you were feeling like adding a DCHECK(), though, add it at registration
rather than here, so that it actually fires early enough to matter (this code
might never run in a local build for a variety of reasons, especially if
someone's working on sqlite stuff, not on memory-tracing stuff).

[Actually, we're so dependent on SQLite being thread-safe that I don't think it
matters whether anyone checks this, but another option would be to put it in
sqlite_features_unittest.cc.]