Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.

Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.
A rough sketch of how this works is here: https://goto.google.com/pver4-LoadUpdateDatabase-doc

This relands https://codereview.chromium.org/1954393002 which was reverted by https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset v4_update_protocol_manager_, which we were later tried to dereference in DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
Committed: https://crrev.com/26771182e06d28496aca4c73e095d3c9a43713ce
Cr-Commit-Position: refs/heads/master@{#393989}

[vakh (use Gerrit instead), 2016-05-16 06:26:40]

Add check for v4_update_protocol_manager_ on DatabaseReady callback

[vakh (use Gerrit instead), 2016-05-16 06:28:09]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org, shess@chromium.org

[Scott Hess - ex-Googler, 2016-05-16 07:55:25]

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:216: // destroy
v4_update_protocol_manager_
Does this mean that StopOnIOThread() was called?  If so, then  should the test
be be against enabled_, and the database also not be set?

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:230:
v4_database_.reset();
Actually, thinking of the above made me wonder - if the database is created on
task_runner_, should this reset happen on the io thread?  Or should it be
something like task_runner_->DeleteSoon()?

[vakh (use Gerrit instead), 2016-05-16 18:29:15]

Check enabled_ before resetting v4_database_ and scheduling next update

[vakh (use Gerrit instead), 2016-05-16 18:36:15]

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:216: // destroy
v4_update_protocol_manager_
On 2016/05/16 07:55:25, Scott Hess wrote:
> Does this mean that StopOnIOThread() was called?  If so, then  should the test
> be be against enabled_, and the database also not be set?

Done. It's cleaner.

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:230:
v4_database_.reset();
On 2016/05/16 07:55:25, Scott Hess wrote:
> Actually, thinking of the above made me wonder - if the database is created on
> task_runner_, should this reset happen on the io thread?  Or should it be
> something like task_runner_->DeleteSoon()?

[I may have misunderstood you here.]
The database is created on the task runner, but v4_database_ is set on the IO
thread only (in DatabaseReady) so it's probably OK.

[Nathan Parker, 2016-05-16 18:40:01]

lgtm

[Scott Hess - ex-Googler, 2016-05-16 19:49:53]

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:230:
v4_database_.reset();
On 2016/05/16 18:36:15, vakh wrote:
> On 2016/05/16 07:55:25, Scott Hess wrote:
> > Actually, thinking of the above made me wonder - if the database is created
on
> > task_runner_, should this reset happen on the io thread?  Or should it be
> > something like task_runner_->DeleteSoon()?
> 
> [I may have misunderstood you here.]
> The database is created on the task runner, but v4_database_ is set on the IO
> thread only (in DatabaseReady) so it's probably OK.

The variable manipulation is fine, but the reset() will call ~V4Database(),
which may not be appropriate to call on the I/O thread, for the same basic
reasons V4Database() isn't being called on the I/O thread.

Since V4Database is handling how things are vectored to the task_runner_, having
this call DeleteSoon() directly seems like a layering violation.  Maybe it makes
more sense to have the destructor version of Create():
// static
void V4Database::Destroy(std::unique_ptr<V4Database> v4_database) {
  v4_database->task_runner_->DeleteSoon(v4_database);
}
That way the caller has to pass an owned reference in, and you don't have to
worry about stale refs.

I can think of two general reasons.  One is simply that you should destruct on
the same thread you constructed on, because it's likely that the reason driving
one decision will be the same for the other (like if a database or store has a
cached file descriptor, that needs to be closed on the task_runner_).  The other
is that you probably need a way to order the destructor after all previous
messages have drained.  Any outstanding queries posted to the task_runner_ could
race with destruction of the database instance, unless destruction itself is
posted to the task_runner_.

If the above doesn't ring true, let me know and I'll walk over to discuss in
person.  If I'm wrong, it would suck to have it implemented :-).

https://codereview.chromium.org/1983723002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1983723002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:214: // other words,
call StopOnIOThread).
Who is "the user" in this?  It may be cleaner to just directly mention that the
check is in case StopOnIOThread() was called during database startup, and not
worry about who called it or why.

[vakh (use Gerrit instead), 2016-05-16 20:02:16]

git fetch && git pull

[Scott Hess - ex-Googler, 2016-05-16 20:20:09]

Also, in your new CL description, the fact that this is relanding a reverted CL
is a subsidiary piece of info.  You should include all the original CL's info,
because a year from now when someone is trying to track some problem down via
git-annotate, that's the information they're going to want to see.

Also, include links for the original CL and revert.  The fact that this was
reverted is probably of most interest to a sheriff, and having to copy/paste
into an up-to-date git client is annoying when the tree is broken.  I generally
handle all of this by including the entire original description, then above the
BUG= section something like:

[This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002 .]

Another option is to use crrev links, like:

[This relands https://crrev.com/32b9468ab517 which was reverted by
https://crrev.com/517133dfc95a .]

Though I'm usually too lazy for that, as full git hashes are TOO DARN LONG, but
you also don't want your shortref too be too short.

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:230:
v4_database_.reset();
On 2016/05/16 19:49:52, Scott Hess wrote:
> If the above doesn't ring true, let me know and I'll walk over to discuss in
> person.  If I'm wrong, it would suck to have it implemented :-).

Seriously - my expectation is you'll think "Oh, yeah, I see what you mean. 
Easily fixed."  If it's "WAT?" or "OMG, that's an entire new nest of spiders",
maybe it would be better to put a pin in this issue and address it in a followup
CL.

[vakh (use Gerrit instead), 2016-05-16 20:32:00]

Description was changed from

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.

Original commit: 32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: 517133dfc95a9ca2b599bfe7452205e5b5d81fd4
This reverts commit 517133dfc95a9ca2b599bfe7452205e5b5d81fd4

BUG=543161
==========

to

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
==========

[vakh (use Gerrit instead), 2016-05-16 20:32:18]

Description was changed from

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
==========

to

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
==========

[vakh (use Gerrit instead), 2016-05-16 20:45:33]

Destory v4_database on its taskrunner instead of the IO thread

[vakh (use Gerrit instead), 2016-05-16 20:50:21]

I've edited to CL description to include:
1. Direct link to original CL.
2. Direct link to revert CL.
3. Reason for failure.

Please let me know if there's anything I missed.

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1983723002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:230:
v4_database_.reset();
On 2016/05/16 19:49:52, Scott Hess wrote:
> On 2016/05/16 18:36:15, vakh wrote:
> > On 2016/05/16 07:55:25, Scott Hess wrote:
> > > Actually, thinking of the above made me wonder - if the database is
created
> on
> > > task_runner_, should this reset happen on the io thread?  Or should it be
> > > something like task_runner_->DeleteSoon()?
> > 
> > [I may have misunderstood you here.]
> > The database is created on the task runner, but v4_database_ is set on the
IO
> > thread only (in DatabaseReady) so it's probably OK.
> 
> The variable manipulation is fine, but the reset() will call ~V4Database(),
> which may not be appropriate to call on the I/O thread, for the same basic
> reasons V4Database() isn't being called on the I/O thread.
> 
> Since V4Database is handling how things are vectored to the task_runner_,
having
> this call DeleteSoon() directly seems like a layering violation.  Maybe it
makes
> more sense to have the destructor version of Create():
> // static
> void V4Database::Destroy(std::unique_ptr<V4Database> v4_database) {
>   v4_database->task_runner_->DeleteSoon(v4_database);
> }
> That way the caller has to pass an owned reference in, and you don't have to
> worry about stale refs.
> 
> I can think of two general reasons.  One is simply that you should destruct on
> the same thread you constructed on, because it's likely that the reason
driving
> one decision will be the same for the other (like if a database or store has a
> cached file descriptor, that needs to be closed on the task_runner_).  The
other
> is that you probably need a way to order the destructor after all previous
> messages have drained.  Any outstanding queries posted to the task_runner_
could
> race with destruction of the database instance, unless destruction itself is
> posted to the task_runner_.
> 
> If the above doesn't ring true, let me know and I'll walk over to discuss in
> person.  If I'm wrong, it would suck to have it implemented :-).

Done. Good catch!
Or: Oh, yeah, I see what you mean. Easily fixed.

[vakh (use Gerrit instead), 2016-05-16 21:05:49]

Description was changed from

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
==========

to

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.
A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
==========

[vakh (use Gerrit instead), 2016-05-16 21:48:58]

schedule destroying the v4_database off IO thread if the local_db_manager isn't
enabled, and update a related comment

[Scott Hess - ex-Googler, 2016-05-16 22:17:19]

lgtm.

https://codereview.chromium.org/1983723002/diff/100001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1983723002/diff/100001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:221:
V4Database::Destroy(std::move(v4_database));
Good call!  This is what I love about threading, it never stops giving.

[vakh (use Gerrit instead), 2016-05-16 22:35:49]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-05-16 22:35:49]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1983723002/#ps100001
(title: "schedule destroying the v4_database off IO thread if the
local_db_manager isn't enabled, and update…")

[commit-bot: I haz the power, 2016-05-16 22:36:26]

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

[commit-bot: I haz the power, 2016-05-17 00:17:08]

Description was changed from

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.
A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
==========

to

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.
A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
==========

[commit-bot: I haz the power, 2016-05-17 00:17:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-17 00:18:59]

Description was changed from

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.
A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161
==========

to

==========
Reland: Initialize and reset V4LocalDBManager. Instantiate V4Stores.
A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

This relands https://codereview.chromium.org/1954393002 which was reverted by
https://codereview.chromium.org/1983603002

Reason for failure: StopOnIOThread ran first and reset
v4_update_protocol_manager_, which we were later tried to dereference in
DatabaseReady()

--
Original commit: http://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Which was reverted as: http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4

This CL reverts commit http://crrev.com/517133dfc95a9ca2b599bfe7452205e5b5d81fd4
--

BUG=543161

Committed: https://crrev.com/26771182e06d28496aca4c73e095d3c9a43713ce
Cr-Commit-Position: refs/heads/master@{#393989}
==========

[commit-bot: I haz the power, 2016-05-17 00:19:00]

Patchset 6 (id:??) landed as
https://crrev.com/26771182e06d28496aca4c73e095d3c9a43713ce
Cr-Commit-Position: refs/heads/master@{#393989}

[qyearsley, 2016-05-17 21:03:47]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1984283003/ by qyearsley@chromium.org.

The reason for reverting is: Speculative revert for http://crbug.com/612523;
will re-land if that builder still fails the same tests after the revert.