[S] Use a weak_factory for V4Database callbacks + test

[S] Use a weak_factory for V4Database callbacks + test

For details about why this crash may be happening, and why this may be
the right fix, see:
http://crbug.com/683147#c5

BUG=683147
Review-Url: https://codereview.chromium.org/2649643002
Cr-Commit-Position: refs/heads/master@{#445342}
Committed: https://chromium.googlesource.com/chromium/src/+/77c324f4a61823e06b32cbd10b3c3434959ca367

[vakh (use Gerrit instead), 2017-01-20 22:21:16]

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

[vakh (use Gerrit instead), 2017-01-20 22:21:40]

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

[commit-bot: I haz the power, 2017-01-20 22:21:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-20 22:23:21]

Nit: remove FRIEND_TEST_ALL_PREFIXES

[vakh (use Gerrit instead), 2017-01-20 22:23:23]

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

[commit-bot: I haz the power, 2017-01-20 22:23:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-20 22:23:39]

Patchset #1 (id:1) has been deleted

[vakh (use Gerrit instead), 2017-01-20 22:42:54]

InvalidateWeakPtrs in V4Database::Destroy

[vakh (use Gerrit instead), 2017-01-20 22:42:55]

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

[commit-bot: I haz the power, 2017-01-20 22:43:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-01-20 22:48:15]

I don't think this works.  Weak pointers and their factories don't have locks to
protect against races, so you cannot make cross-thread calls to create or
invalidate or get().

The standard approach is for a thread to create the factory (like you did), then
to pass a new weak pointer as a parameter to a task on another thread.  That
thread can pass the parameter _back_, but it cannot call get() or otherwise
reason about internal state.  When the parameter comes back to the original
thread, _then_ it can call get() and find that the weak pointer had been
invalidated.

[Scott Hess - ex-Googler, 2017-01-20 23:09:22]

On 2017/01/20 22:48:15, Scott Hess wrote:
> I don't think this works.  Weak pointers and their factories don't have locks
to
> protect against races, so you cannot make cross-thread calls to create or
> invalidate or get().
> 
> The standard approach is for a thread to create the factory (like you did),
then
> to pass a new weak pointer as a parameter to a task on another thread.  That
> thread can pass the parameter _back_, but it cannot call get() or otherwise
> reason about internal state.  When the parameter comes back to the original
> thread, _then_ it can call get() and find that the weak pointer had been
> invalidated.

Per offline discussion, I'm walking this back.  The thread binding happens in
get, and is unbound in invalidate, and those are on the same thread, so I guess
that's not a problem.

Now to go review again!

[Scott Hess - ex-Googler, 2017-01-20 23:21:00]

OK, I think LGTM, then.

In the broader scope, if there's going to be a weak pointer, it might make sense
for V4LocalDatabaseManager to hold it, rather than holding the unique_ptr.  Then
you have two peers on different threads passing weak pointers to tasks for the
other thread, and I think reasoning about ownership maybe becomes easier.  It
might make sense to think of it in terms of "The database which does the stuff"
and "The thing which sends tasks to the db task runner" as separate objects,
perhaps allowing no cross-thread calls for each object.

[vakh (use Gerrit instead), 2017-01-20 23:49:35]

On 2017/01/20 23:09:22, Scott Hess wrote:
> On 2017/01/20 22:48:15, Scott Hess wrote:
> > I don't think this works.  Weak pointers and their factories don't have
locks
> to
> > protect against races, so you cannot make cross-thread calls to create or
> > invalidate or get().
> > 
> > The standard approach is for a thread to create the factory (like you did),
> then
> > to pass a new weak pointer as a parameter to a task on another thread.  That
> > thread can pass the parameter _back_, but it cannot call get() or otherwise
> > reason about internal state.  When the parameter comes back to the original
> > thread, _then_ it can call get() and find that the weak pointer had been
> > invalidated.
> 
> Per offline discussion, I'm walking this back.  The thread binding happens in
> get, and is unbound in invalidate, and those are on the same thread, so I
guess
> that's not a problem.
> 
> Now to go review again!

Good point! Per this comment in weak_ptr.h:
<quote>
// To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory
// is dereferenced, the factory and its WeakPtrs become bound to the calling
// thread or current SequencedWorkerPool token, and cannot be dereferenced or
// invalidated on any other task runner. Bound WeakPtrs can still be handed
// off to other task runners, e.g. to use to post tasks back to object on the
// bound sequence.
</quote>

The pointer is dereferenced and invalidated on the IO thread only so we should
be OK.
I'll try to run unit tests with ASAN build on my machine (though it may or may
not work).

[commit-bot: I haz the power, 2017-01-21 00:08:36]

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

[commit-bot: I haz the power, 2017-01-21 00:08:36]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2017-01-21 01:01:09]

lgtm

I'm still not sure there isn't a small (smaller than before) race here, but it's
better than before.

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

https://codereview.chromium.org/2649643002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:227: weak_factory_.GetWeakPtr(),
callback_task_runner,
Hmm, but this one is going to get checked on the task runner, rather than the IO
thread, ya?

https://codereview.chromium.org/2649643002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2649643002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:209: base::WeakPtrFactory<V4Database>
weak_factory_;
A proposal: How about week_factory_on_io_?  That makes it clear.

[vakh (use Gerrit instead), 2017-01-21 01:16:11]

Nit: s/weak_factory_/weak_factory_on_io_

[vakh (use Gerrit instead), 2017-01-21 01:16:12]

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

[vakh (use Gerrit instead), 2017-01-21 01:16:17]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2017-01-21 01:16:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2649643002/#ps60001
(title: "Nit: s/weak_factory_/weak_factory_on_io_")

[vakh (use Gerrit instead), 2017-01-21 01:16:31]

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

https://codereview.chromium.org/2649643002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:227: weak_factory_.GetWeakPtr(),
callback_task_runner,
On 2017/01/21 01:01:08, Nathan Parker wrote:
> Hmm, but this one is going to get checked on the task runner, rather than the
IO
> thread, ya?

The GetWeakPtr method has been called on the IO thread, and that's what seems to
matter, based on what I gather from reading that comment.

Trying various ASAN and MSAN trybots to see what happens.

https://codereview.chromium.org/2649643002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2649643002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:209: base::WeakPtrFactory<V4Database>
weak_factory_;
On 2017/01/21 01:01:08, Nathan Parker wrote:
> A proposal: How about week_factory_on_io_?  That makes it clear.

Done.

[vakh (use Gerrit instead), 2017-01-21 01:16:41]

The CQ bit was unchecked by vakh@chromium.org

[commit-bot: I haz the power, 2017-01-21 01:16:44]

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

[vakh (use Gerrit instead), 2017-01-21 01:16:47]

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

[commit-bot: I haz the power, 2017-01-21 01:17:21]

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-01-21 02:23:12]

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

[commit-bot: I haz the power, 2017-01-21 02:23:13]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2017-01-21 04:18:20]

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

https://codereview.chromium.org/2649643002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:227: weak_factory_.GetWeakPtr(),
callback_task_runner,
On 2017/01/21 01:16:31, vakh (Varun Khaneja) wrote:
> On 2017/01/21 01:01:08, Nathan Parker wrote:
> > Hmm, but this one is going to get checked on the task runner, rather than
the
> IO
> > thread, ya?
> 
> The GetWeakPtr method has been called on the IO thread, and that's what seems
to
> matter, based on what I gather from reading that comment.
> 
> Trying various ASAN and MSAN trybots to see what happens.

Oh no!  I think this is what I was worrying about!

I think this usage will deref the weak pointer (can get()) on the wrong thread. 
This can be an opaque parameter, but not a target.

That said, it should narrow the race condition.  So I'm not sure about making a
value judgement about shipping it (it should get strictly better, though it's
still wrong).  I think fixing this should gate turning the experiment up for
more people, though.

I think this is back to the db_task_runner_ thread needs to create the weak
pointer and post it over for the foreground to reference.  Maybe for the short
term it is easiest to just have the create callback return both an owning
unique_ptr and a weak_ptr used for messaging.  That's ugly, but I think it
allows correct operation.  Basically, the unique_ptr is stored and _only_ passed
to Destroy().  On the other end, the destructor will destroy the weak-pointer
factory, and any in-flight postings will short-circuit.

[vakh (use Gerrit instead), 2017-01-23 07:13:39]

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2017-01-23 07:13:47]

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-01-23 08:41:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485155619431000,
"parent_rev": "faceec0c256adfb7e60792293d7f4dccf4506715", "commit_rev":
"77c324f4a61823e06b32cbd10b3c3434959ca367"}

[commit-bot: I haz the power, 2017-01-23 08:42:01]

Description was changed from

==========
[S] Use a weak_factory for V4Database callbacks + test

For details about why this crash may be happening, and why this may be
the right fix, see:
http://crbug.com/683147#c5

BUG=683147
==========

to

==========
[S] Use a weak_factory for V4Database callbacks + test

For details about why this crash may be happening, and why this may be
the right fix, see:
http://crbug.com/683147#c5

BUG=683147

Review-Url: https://codereview.chromium.org/2649643002
Cr-Commit-Position: refs/heads/master@{#445342}
Committed:
https://chromium.googlesource.com/chromium/src/+/77c324f4a61823e06b32cbd10b3c...
==========

[commit-bot: I haz the power, 2017-01-23 08:42:03]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/77c324f4a61823e06b32cbd10b3c...