Tiny: Post PerformFullHashCheck on IO thread, instead of calling it synchronously.

Post PerformFullHashCheck on IO thread, instead of calling it synchronously.
This makes it consistent with the CheckBrowseUrl expectation document in database_manager.h as:

  // Called on the IO thread to check if the given url is safe or not.  If we
  // can synchronously determine that the url is safe, CheckUrl returns true.
  // Otherwise it returns false, and "client" is called asynchronously with the
  // result when it is ready.

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

[vakh (use Gerrit instead), 2016-10-13 22:58:06]

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

[commit-bot: I haz the power, 2016-10-13 22:58:28]

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-13 23:04:42]

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

[commit-bot: I haz the power, 2016-10-13 23:48:43]

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

[commit-bot: I haz the power, 2016-10-13 23:48:44]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2016-10-14 00:40:59]

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

https://codereview.chromium.org/2421703002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.cc:154:
full_hash_to_store_and_hash_prefixes));
I was thinking this might introduce an extra spin of the loop versus v3, but ...
v3 transitions through the UI thread on fetch?  To get the extended-reporting
flag.  OMG.

Anyhow, where I was going with that is that I think right now this would post a
task, and that task might startup a fetcher which would post another task, so
worst-case you'll have two additional tasks to process.  Best case you'll have
one.  I think right now you have enough info to reduce that to one in either
case (either post the task leading directly to RespondToClient, or start the
fetcher leading to RespondToClient later).

[I am open to the argument "That's too complicated" or "It can be a TODO" or
"The fetcher can callback synchronously, and it's hard to detect."]

[[I'm not fresh enough to reason about interactions with StopOnIOThread().]]

[vakh (use Gerrit instead), 2016-10-14 18:25:52]

On 2016/10/14 00:40:59, Scott Hess wrote:
>
https://codereview.chromium.org/2421703002/diff/1/components/safe_browsing_db...
> File components/safe_browsing_db/v4_local_database_manager.cc (right):
> 
>
https://codereview.chromium.org/2421703002/diff/1/components/safe_browsing_db...
> components/safe_browsing_db/v4_local_database_manager.cc:154:
> full_hash_to_store_and_hash_prefixes));
> I was thinking this might introduce an extra spin of the loop versus v3, but
...
> v3 transitions through the UI thread on fetch?  To get the extended-reporting
> flag.  OMG.
> 
> Anyhow, where I was going with that is that I think right now this would post
a
> task, and that task might startup a fetcher which would post another task, so
> worst-case you'll have two additional tasks to process.  Best case you'll have
> one.  I think right now you have enough info to reduce that to one in either
> case (either post the task leading directly to RespondToClient, or start the
> fetcher leading to RespondToClient later).
> 
> [I am open to the argument "That's too complicated" or "It can be a TODO" or
> "The fetcher can callback synchronously, and it's hard to detect."]
> 
> [[I'm not fresh enough to reason about interactions with StopOnIOThread().]]

Almost: "The fetcher can callback synchronously, and it's hard to detect."
Except, it's not hard to detect. However, it is an implementation detail of
V4GetHashProtocolManager.
As far as V4LocalDatabaseManager is concerned, it only knows that its
OnFullHashResponse
will be called back when the results are ready, and it may or may not be async.

So, in order to fulfill its promise of doing the full hash check async without
getting into the implementation details of V4GetHashProtocolManager, I think
it is best for V4LocalDatabaseManager to just post the task.

[Scott Hess - ex-Googler, 2016-10-14 19:11:43]

On 2016/10/14 18:25:52, vakh (Varun Khaneja) wrote:
> On 2016/10/14 00:40:59, Scott Hess wrote:
> >
>
https://codereview.chromium.org/2421703002/diff/1/components/safe_browsing_db...
> > File components/safe_browsing_db/v4_local_database_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2421703002/diff/1/components/safe_browsing_db...
> > components/safe_browsing_db/v4_local_database_manager.cc:154:
> > full_hash_to_store_and_hash_prefixes));
> > I was thinking this might introduce an extra spin of the loop versus v3, but
> ...
> > v3 transitions through the UI thread on fetch?  To get the
extended-reporting
> > flag.  OMG.
> > 
> > Anyhow, where I was going with that is that I think right now this would
post
> a
> > task, and that task might startup a fetcher which would post another task,
so
> > worst-case you'll have two additional tasks to process.  Best case you'll
have
> > one.  I think right now you have enough info to reduce that to one in either
> > case (either post the task leading directly to RespondToClient, or start the
> > fetcher leading to RespondToClient later).
> > 
> > [I am open to the argument "That's too complicated" or "It can be a TODO" or
> > "The fetcher can callback synchronously, and it's hard to detect."]
> > 
> > [[I'm not fresh enough to reason about interactions with StopOnIOThread().]]
> 
> Almost: "The fetcher can callback synchronously, and it's hard to detect."
> Except, it's not hard to detect. However, it is an implementation detail of
> V4GetHashProtocolManager.
> As far as V4LocalDatabaseManager is concerned, it only knows that its
> OnFullHashResponse
> will be called back when the results are ready, and it may or may not be
async.
> 
> So, in order to fulfill its promise of doing the full hash check async without
> getting into the implementation details of V4GetHashProtocolManager, I think
> it is best for V4LocalDatabaseManager to just post the task.

OK, makes sense.

Reading things again, I think the CL comment should talk about matching the
CheckBrowseUrl() API, _not_ about matching PVer3 (because that's irrelevant
long-term).  Also, you should put a comment in the code about posting the task
to make sure it's async, because otherwise it's not obvious why one wouldn't
just call directly.  I guess an alternative to a comment would be to write a
test which requires async response in a case where the previous code would fail.

[vakh (use Gerrit instead), 2016-10-14 21:05:00]

Description was changed from

==========
Post PerformFullHashCheck on IO thread, instead of calling it synchronously.
This makes it consistent with the PVer3 implementation.

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

to

==========
Post PerformFullHashCheck on IO thread, instead of calling it synchronously.
This makes it consistent with the CheckBrowseUrl expectation document in
database_manager.h as:

  // Called on the IO thread to check if the given url is safe or not.  If we
  // can synchronously determine that the url is safe, CheckUrl returns true.
  // Otherwise it returns false, and "client" is called asynchronously with the
  // result when it is ready.

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

[vakh (use Gerrit instead), 2016-10-14 21:05:08]

On 2016/10/14 19:11:43, Scott Hess wrote:
> On 2016/10/14 18:25:52, vakh (Varun Khaneja) wrote:
> > On 2016/10/14 00:40:59, Scott Hess wrote:
> > >
> >
>
https://codereview.chromium.org/2421703002/diff/1/components/safe_browsing_db...
> > > File components/safe_browsing_db/v4_local_database_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2421703002/diff/1/components/safe_browsing_db...
> > > components/safe_browsing_db/v4_local_database_manager.cc:154:
> > > full_hash_to_store_and_hash_prefixes));
> > > I was thinking this might introduce an extra spin of the loop versus v3,
but
> > ...
> > > v3 transitions through the UI thread on fetch?  To get the
> extended-reporting
> > > flag.  OMG.
> > > 
> > > Anyhow, where I was going with that is that I think right now this would
> post
> > a
> > > task, and that task might startup a fetcher which would post another task,
> so
> > > worst-case you'll have two additional tasks to process.  Best case you'll
> have
> > > one.  I think right now you have enough info to reduce that to one in
either
> > > case (either post the task leading directly to RespondToClient, or start
the
> > > fetcher leading to RespondToClient later).
> > > 
> > > [I am open to the argument "That's too complicated" or "It can be a TODO"
or
> > > "The fetcher can callback synchronously, and it's hard to detect."]
> > > 
> > > [[I'm not fresh enough to reason about interactions with
StopOnIOThread().]]
> > 
> > Almost: "The fetcher can callback synchronously, and it's hard to detect."
> > Except, it's not hard to detect. However, it is an implementation detail of
> > V4GetHashProtocolManager.
> > As far as V4LocalDatabaseManager is concerned, it only knows that its
> > OnFullHashResponse
> > will be called back when the results are ready, and it may or may not be
> async.
> > 
> > So, in order to fulfill its promise of doing the full hash check async
without
> > getting into the implementation details of V4GetHashProtocolManager, I think
> > it is best for V4LocalDatabaseManager to just post the task.
> 
> OK, makes sense.
> 
> Reading things again, I think the CL comment should talk about matching the
> CheckBrowseUrl() API, _not_ about matching PVer3 (because that's irrelevant
> long-term).  Also, you should put a comment in the code about posting the task
> to make sure it's async, because otherwise it's not obvious why one wouldn't
> just call directly.  I guess an alternative to a comment would be to write a
> test which requires async response in a case where the previous code would
fail.

Done

[Scott Hess - ex-Googler, 2016-10-14 21:06:38]

On 2016/10/14 21:05:08, vakh (Varun Khaneja) wrote:
> Done

Upload solution?

[Scott Hess - ex-Googler, 2016-10-14 21:10:58]

On 2016/10/14 21:06:38, Scott Hess wrote:
> On 2016/10/14 21:05:08, vakh (Varun Khaneja) wrote:
> > Done
> 
> Upload solution?

Oops, maybe you intended the CL description change to be the solution.  I'm
concerned that it's "obvious" that PerformFullHashCheck() can be called inline,
it's somewhat implied that it will hit up the server, which is obviously async
already.  So I was hoping for a comment on the new PostTask along the lines of
"Posting to make sure results are async even if the request can be satisfied
from cache".  Or a test to make sure the async happens even if the request is
satisfied from cache.

[vakh (use Gerrit instead), 2016-10-14 21:11:50]

On 2016/10/14 21:10:58, Scott Hess wrote:
> On 2016/10/14 21:06:38, Scott Hess wrote:
> > On 2016/10/14 21:05:08, vakh (Varun Khaneja) wrote:
> > > Done
> > 
> > Upload solution?
> 
> Oops, maybe you intended the CL description change to be the solution.  I'm
> concerned that it's "obvious" that PerformFullHashCheck() can be called
inline,
> it's somewhat implied that it will hit up the server, which is obviously async
> already.  So I was hoping for a comment on the new PostTask along the lines of
> "Posting to make sure results are async even if the request can be satisfied
> from cache".  Or a test to make sure the async happens even if the request is
> satisfied from cache.

Yes, I forgot to add the test. Working on it now. Should be up soon.
Thanks for catching it.

[vakh (use Gerrit instead), 2016-10-14 22:32:10]

Test to ensure that PerformFullHashCheck is called async

[vakh (use Gerrit instead), 2016-10-14 22:32:13]

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

[commit-bot: I haz the power, 2016-10-14 22:32:39]

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-14 22:34:03]

On 2016/10/14 21:10:58, Scott Hess wrote:
> On 2016/10/14 21:06:38, Scott Hess wrote:
> > On 2016/10/14 21:05:08, vakh (Varun Khaneja) wrote:
> > > Done
> > 
> > Upload solution?
> 
> Oops, maybe you intended the CL description change to be the solution.  I'm
> concerned that it's "obvious" that PerformFullHashCheck() can be called
inline,
> it's somewhat implied that it will hit up the server, which is obviously async
> already.  So I was hoping for a comment on the new PostTask along the lines of
> "Posting to make sure results are async even if the request can be satisfied
> from cache".  Or a test to make sure the async happens even if the request is
> satisfied from cache.

Done. Added a test to ensure that PerformFullHashCheck is called async
when the synchronous result is false.

[commit-bot: I haz the power, 2016-10-14 23:23:08]

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

[commit-bot: I haz the power, 2016-10-14 23:23:09]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2016-10-17 23:24:14]

lgtm

[Nathan Parker, 2016-10-18 04:56:54]

lgtm

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

https://codereview.chromium.org/2421703002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:150:
BrowserThread::PostTask(
Can you add a comment for why this is here? It will add an extra spin on this
thread for about 0.1% of URL checks.

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

Add comment about why PerformFullHashCheck is being posted

[vakh (use Gerrit instead), 2016-10-18 17:21:31]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-10-18 17:21:31]

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/2421703002/#ps40001
(title: "Add comment about why PerformFullHashCheck is being posted")

[commit-bot: I haz the power, 2016-10-18 17:22:04]

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 18:49:52]

Description was changed from

==========
Post PerformFullHashCheck on IO thread, instead of calling it synchronously.
This makes it consistent with the CheckBrowseUrl expectation document in
database_manager.h as:

  // Called on the IO thread to check if the given url is safe or not.  If we
  // can synchronously determine that the url is safe, CheckUrl returns true.
  // Otherwise it returns false, and "client" is called asynchronously with the
  // result when it is ready.

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

to

==========
Post PerformFullHashCheck on IO thread, instead of calling it synchronously.
This makes it consistent with the CheckBrowseUrl expectation document in
database_manager.h as:

  // Called on the IO thread to check if the given url is safe or not.  If we
  // can synchronously determine that the url is safe, CheckUrl returns true.
  // Otherwise it returns false, and "client" is called asynchronously with the
  // result when it is ready.

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

[commit-bot: I haz the power, 2016-10-18 18:49:54]

Committed patchset #3 (id:40001)

[huangs, 2016-10-18 21:38:06]

This CL may be causing current failures for ChromiumOS bots; it's "patient 0".

See:
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

#141. interactive_ui_tests on Ubuntu-12.04 interactive_ui_tests on Ubuntu-12.04

Even though the test passes, in the detailed log

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

WallpaperManagerBrowserTest.SmallChildWallpaper

fails, and then recovers.  This is the first recent incidence. Now it seems to
consistently fail.

[Guido Urdaneta, 2016-10-20 08:17:09]

We confirmed that this wasn't the cause of the bot failures.

[commit-bot: I haz the power, 2016-10-21 13:01:50]

Description was changed from

==========
Post PerformFullHashCheck on IO thread, instead of calling it synchronously.
This makes it consistent with the CheckBrowseUrl expectation document in
database_manager.h as:

  // Called on the IO thread to check if the given url is safe or not.  If we
  // can synchronously determine that the url is safe, CheckUrl returns true.
  // Otherwise it returns false, and "client" is called asynchronously with the
  // result when it is ready.

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

to

==========
Post PerformFullHashCheck on IO thread, instead of calling it synchronously.
This makes it consistent with the CheckBrowseUrl expectation document in
database_manager.h as:

  // Called on the IO thread to check if the given url is safe or not.  If we
  // can synchronously determine that the url is safe, CheckUrl returns true.
  // Otherwise it returns false, and "client" is called asynchronously with the
  // result when it is ready.

BUG=543161

Committed: https://crrev.com/ee48f463535c73de660d4bda1185373e0fde11b7
Cr-Commit-Position: refs/heads/master@{#426010}
==========

[commit-bot: I haz the power, 2016-10-21 13:01:51]

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