Add the result of the inclusion whitelist to the OffDomainInclusionDetector's analysis.

Add the result of the inclusion whitelist to the OffDomainInclusionDetector's analysis.

BUG=412468
Committed: https://crrev.com/fab341f7b6f9c7bbc739c34554c666921de87833
Cr-Commit-Position: refs/heads/master@{#310989}

[gab, 2015-01-06 14:28:45]

gab@chromium.org changed reviewers:
+ grt@chromium.org

[gab, 2015-01-06 14:28:45]

Welcome back and happy new year Greg!

PTAL as I'm trying to alleviate Matt's review load.

Thanks!
Gab

[gab, 2015-01-06 14:32:46]

On 2015/01/06 14:28:45, gab wrote:
> Welcome back and happy new year Greg!
> 
> PTAL as I'm trying to alleviate Matt's review load.
> 
> Thanks!
> Gab

I should note: this is currently based on
https://codereview.chromium.org/835523002/ but the dependency is only in the
assumption that SafeBrowsingDatabase::ContainsInclusionWhitelistedUrl() exists,
everything else is otherwise independent.

[grt (UTC plus 2), 2015-01-07 14:35:49]

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.h:178: // error occurs. This
method is expected to be called on the IO thread.
"expected to be called"? what if the unexpected happens? if it must be called on
the IO thread, then please state that explicitly (do so for the other comments
in this file as well if the same is true for them).

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
(right):

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:22:
: OffDomainInclusionDetector(database_manager,
cool!

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:39:
// Assumed to be called on the IO thread for now as it accesses the safe
Assumed to? If there's a DCHECK here, then it "Must" be called on the IO thread.
Please update comment.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:90:
// Record the type of request analyzed to be able to do ratio analysis w.r.t
nit: add a period after 't' at the end of the line ("w.r.t.").

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:101:
// This can happen in a few scenarios where the referer is dropped (e.g.,
referer -> referrer here and below (the former is a historical accident that
doesn't need to be propagated in the comment).

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
(right):

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:37:
OffDomainInclusionDetector(
explicit

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:42:
explicit OffDomainInclusionDetector(
no explicit

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
(right):

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:24:
using testing::StrictMock;
unused

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:89:
void SetUp() override {
move this into the protected: block below. actually, i don't think the ctor
needs to be public, either.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:95:
ON_CALL(*mock_safe_browsing_database_manager_,
is ON_CALL enough to prevent the mock from logging uninteresting calls? use
NiceMock if you don't care about methods being called on the mock
(https://code.google.com/p/googlemock/wiki/CookBook#The_Nice,_the_Strict,_and_...).
if you find that there's no logging of uninteresting calls at the moment, i
wonder if MatchInclusionWhitelistUrl is actually being called by the test.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:159:
scoped_refptr<MockSafeBrowsingDatabaseManager>
does this need to be a member of the test fixture? the detector will hold a ref
on the db manager.

[gab, 2015-01-07 17:52:01]

Thanks, done.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/database_manager.h:178: // error occurs. This
method is expected to be called on the IO thread.
On 2015/01/07 14:35:48, grt wrote:
> "expected to be called"? what if the unexpected happens? if it must be called
on
> the IO thread, then please state that explicitly (do so for the other comments
> in this file as well if the same is true for them).

Done.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc
(right):

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:22:
: OffDomainInclusionDetector(database_manager,
On 2015/01/07 14:35:48, grt wrote:
> cool!

Yay C++11 :-)!

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:39:
// Assumed to be called on the IO thread for now as it accesses the safe
On 2015/01/07 14:35:48, grt wrote:
> Assumed to? If there's a DCHECK here, then it "Must" be called on the IO
thread.
> Please update comment.

Done.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:90:
// Record the type of request analyzed to be able to do ratio analysis w.r.t
On 2015/01/07 14:35:48, grt wrote:
> nit: add a period after 't' at the end of the line ("w.r.t.").

Done.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:101:
// This can happen in a few scenarios where the referer is dropped (e.g.,
On 2015/01/07 14:35:48, grt wrote:
> referer -> referrer here and below (the former is a historical accident that
> doesn't need to be propagated in the comment).

Done.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h
(right):

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:37:
OffDomainInclusionDetector(
On 2015/01/07 14:35:48, grt wrote:
> explicit

Done.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:42:
explicit OffDomainInclusionDetector(
On 2015/01/07 14:35:48, grt wrote:
> no explicit

Done.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
(right):

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:24:
using testing::StrictMock;
On 2015/01/07 14:35:48, grt wrote:
> unused

Oops, indeed, last minute change, thanks :-)!

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:89:
void SetUp() override {
On 2015/01/07 14:35:48, grt wrote:
> move this into the protected: block below. actually, i don't think the ctor
> needs to be public, either.

Done.

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:95:
ON_CALL(*mock_safe_browsing_database_manager_,
On 2015/01/07 14:35:48, grt wrote:
> is ON_CALL enough to prevent the mock from logging uninteresting calls? use
> NiceMock if you don't care about methods being called on the mock
>
(https://code.google.com/p/googlemock/wiki/CookBook#The_Nice,_the_Strict,_and_...).
> if you find that there's no logging of uninteresting calls at the moment, i
> wonder if MatchInclusionWhitelistUrl is actually being called by the test.

Oh interesting, didn't know about NiceMock :-)!

I went away from StrictMock as it logged uninteresting calls failures whereas in
this case I only care about mocking MatchInclusionWhitelistUrl()'s return value
without wanting to care about the implementation detail of when it is called

I hadn't realize that the default mock was still logging warnings as logs are
gobbled up and only shown on failure when using multiple test-launcher-jobs.

Along with ON_CALL this seems to be exactly what I want for this use case
(https://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect).

https://codereview.chromium.org/836963003/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:159:
scoped_refptr<MockSafeBrowsingDatabaseManager>
On 2015/01/07 14:35:48, grt wrote:
> does this need to be a member of the test fixture? the detector will hold a
ref
> on the db manager.

Good point, I was originally thinking it would to set up mock expectations but
since they're now set up inline in SetUp() it doesn't need to be here :-)!

[grt (UTC plus 2), 2015-01-07 18:37:04]

lgtm

[gab, 2015-01-07 20:18:55]

gab@chromium.org changed reviewers:
+ jwd@chromium.org, mattm@chromium.org

[gab, 2015-01-07 20:18:56]

Thanks,

@Matt for safebrowsing rubberstamp (once you've had a chance to take a look at
https://codereview.chromium.org/835523002/ which this CL is based on)

@Jesse for histograms addition

Thanks!
Gab

[jwd, 2015-01-08 16:26:25]

lgtm

[mattm, 2015-01-08 23:14:17]

lgtm

[gab, 2015-01-10 10:19:27]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2015-01-10 10:19:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836963003/40001

[commit-bot: I haz the power, 2015-01-10 11:52:08]

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

[commit-bot: I haz the power, 2015-01-10 11:52:09]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2015-01-10 17:14:57]

Patchset #4 (id:60001) has been deleted

[gab, 2015-01-10 17:16:53]

Fixed a leak in the test, take a look at latest patch set, landing now (happy to
address any comments you have on the addition in a follow-up).

[gab, 2015-01-10 17:17:02]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2015-01-10 17:17:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836963003/80001

[commit-bot: I haz the power, 2015-01-10 19:08:45]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-01-10 19:09:37]

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

[mattm, 2015-01-13 00:22:50]

https://codereview.chromium.org/836963003/diff/80001/chrome/browser/safe_brow...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
(right):

https://codereview.chromium.org/836963003/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:114:
base::RunLoop().RunUntilIdle();
Is it actually asynchronous? SafeBrowsingService is DeleteOnUIThread, but this
should be on the UI thread already so I thought it would be synchronously
deleted once you reset the off_domain_inclusion_detector_. Or is there some
other thing holding a ref too?

[gab, 2015-01-13 18:45:08]

https://codereview.chromium.org/836963003/diff/80001/chrome/browser/safe_brow...
File
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
(right):

https://codereview.chromium.org/836963003/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:114:
base::RunLoop().RunUntilIdle();
On 2015/01/13 00:22:50, mattm wrote:
> Is it actually asynchronous? SafeBrowsingService is DeleteOnUIThread, but this
> should be on the UI thread already so I thought it would be synchronously
> deleted once you reset the off_domain_inclusion_detector_. Or is there some
> other thing holding a ref too?

Oh right, hadn't noticed that DeleteOnThread was smart enough not to post an
async task if it was already on the right thread, see [1] below, (which it
always is in unit_tests).

This thus makes it weird that ASAN would be tripped (I just tried locally and
confirmed that deleting OffDomainInclusionDetector synchronously ends up
deleting the SafeBrowsingService).

FWIW, I took this paradigm from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...

I'm now trying 2 different patch sets @
https://codereview.chromium.org/848063002 to see if a simpler version of this
still trips ASAN.

Perhaps the problem is that ASAN does its leak check between TearDown() and
~OffDomainInclusionDetectorTest(), but that would seem weird to me as then
everything owned by the test fixture would be reported as a leak...

Or perhaps CurrentlyOn(UI) @ [1] somehow returns false at some point passed
TearDown()... (though, from local testing, that appears to work...).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

I'll figure it out and come up with a fix, thanks for bringing it up.