Small: Use a weak pointer for callbacks to V4LocalDatabaseManager

Use a weak pointer for callbacks to V4LocalDatabaseManager

BUG=543161, 647237
Committed: https://crrev.com/5de30503b1458fc131a0268196638a76617a9c6a
Cr-Commit-Position: refs/heads/master@{#426050}

[vakh (use Gerrit instead), 2016-10-17 23:56:53]

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

[vakh (use Gerrit instead), 2016-10-17 23:57:15]

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

[commit-bot: I haz the power, 2016-10-17 23:57:50]

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, 2016-10-18 01:24:50]

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

[commit-bot: I haz the power, 2016-10-18 01:24:51]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2016-10-18 03:13:00]

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:362: }
This seems like StopOnIOThread() maybe has posted things to run on the task
runner which can go past the end of the test function.  Should this
WaitForTasksOnTaskRunner() to keep things hermetic?

I want to suggest also somehow verifying that the callback didn't happen, but
... how?  Can't think of any way to isolate that without being grossly intrusive
implementation changes which would probably cause more errors than this would
ever catch.

[Nathan Parker, 2016-10-18 05:06:37]

lgtm

modulo shess's comment

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.cc:273:
v4_update_protocol_manager_.reset();
Does this mean that any calls to the protocol_manager don't need to use a weak
ptr in callbacks?  In general weak ptrs are safe to use when unnecessary, so
it's not a big deal, but maybe redundant.

[Nathan Parker, 2016-10-18 05:08:54]

nit: no need to attach two bugs to a CL unless they are otherwise unrelated.  It
just creates more email.  Generally a master bug with several dependent bugs
doesn't also have copies of all CLs on the master bug (it doesn't scale).

[vakh (use Gerrit instead), 2016-10-18 17:37:29]

On 2016/10/18 05:08:54, Nathan Parker wrote:
> nit: no need to attach two bugs to a CL unless they are otherwise unrelated. 
It
> just creates more email.  Generally a master bug with several dependent bugs
> doesn't also have copies of all CLs on the master bug (it doesn't scale).

I agree with you. Of the two issues attached here, one is a crasher bug and
other other is the overall feature so I think that in this case, linking to both
is useful because the CL goes beyond fixing the bug -- it fixes a behavior that
we considered worth fixing even before the crasher bug was filed.
(I've also made the crasher as a blocker for the feature.)

[vakh (use Gerrit instead), 2016-10-18 17:37:44]

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.cc:273:
v4_update_protocol_manager_.reset();
On 2016/10/18 05:06:37, Nathan Parker wrote:
> Does this mean that any calls to the protocol_manager don't need to use a weak
> ptr in callbacks?  In general weak ptrs are safe to use when unnecessary, so
> it's not a big deal, but maybe redundant.

Not sure exactly what you mean but all callbacks in this class now use a weak
pointer. I'm going to look at other V4* classes (specifically V4Database) also
(not immediately) and consider using weak-ptrs there also, if needed.

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:362: }
On 2016/10/18 03:13:00, Scott Hess wrote:
> This seems like StopOnIOThread() maybe has posted things to run on the task
> runner which can go past the end of the test function.  Should this
> WaitForTasksOnTaskRunner() to keep things hermetic?
> 

Good idea. Done.
It is already done in TearDown, but it is good for the test to be explicit about
it.

> I want to suggest also somehow verifying that the callback didn't happen, but
> ... how?  Can't think of any way to isolate that without being grossly
intrusive
> implementation changes which would probably cause more errors than this would
> ever catch.

Yes, I agree. I think it's going to be more trouble than the value it'd add.
Skipping for now.

[vakh (use Gerrit instead), 2016-10-18 20:04:31]

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

[vakh (use Gerrit instead), 2016-10-18 20:04:48]

WaitForTasksOnTaskRunner at the end of the test to ensure that the scheduled
tasks get run.

[vakh (use Gerrit instead), 2016-10-18 20:04:50]

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

[commit-bot: I haz the power, 2016-10-18 20:05:03]

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), 2016-10-18 20:05:38]

Patch uploaded now.

[Scott Hess - ex-Googler, 2016-10-18 20:09:56]

lgtm

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2427863002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:362: }
On 2016/10/18 17:37:44, vakh (Varun Khaneja) wrote:
> On 2016/10/18 03:13:00, Scott Hess wrote:
> > This seems like StopOnIOThread() maybe has posted things to run on the task
> > runner which can go past the end of the test function.  Should this
> > WaitForTasksOnTaskRunner() to keep things hermetic?
> > 
> 
> Good idea. Done.
> It is already done in TearDown, but it is good for the test to be explicit
about
> it.

Aha, I see what you mean, I hadn't connected the helper method with TearDown(),
but it makes sense.

Still, I think I prefer the inline version, unless the test file is pretty
uniformly depending on the setup of the test class.  Which I think would be
tough in this case, since some tests need to drain the tasks before performing
additional checks.

[vakh (use Gerrit instead), 2016-10-18 20:17:22]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-10-18 20:17:22]

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/2427863002/#ps20001
(title: "WaitForTasksOnTaskRunner at the end of the test to ensure that the
scheduled tasks get run.")

[commit-bot: I haz the power, 2016-10-18 20:17:50]

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, 2016-10-18 21:26:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:03:16]

Description was changed from

==========
Use a weak pointer for callbacks to V4LocalDatabaseManager

BUG=543161, 647237
==========

to

==========
Use a weak pointer for callbacks to V4LocalDatabaseManager

BUG=543161, 647237

Committed: https://crrev.com/5de30503b1458fc131a0268196638a76617a9c6a
Cr-Commit-Position: refs/heads/master@{#426050}
==========

[commit-bot: I haz the power, 2016-10-21 13:03:17]

Patchset 2 (id:??) landed as
https://crrev.com/5de30503b1458fc131a0268196638a76617a9c6a
Cr-Commit-Position: refs/heads/master@{#426050}