If we show a SafeBrowsing warning we always send the client-side detection

If we show a SafeBrowsing warning we always send the client-side detection
ping back to the server.  That will be used to detect client-side false
negatives as well as help us train a model for FakeAV pages.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93534

[Garrett Casto, 2011-07-19 21:18:48]

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.cc:347: // ID and store
it for later.  For phishing hits we also require that the
The "In this case ..." sentence seems to have two clauses that say the same
thing. Also, do you want to mention why we only send back information if the top
level url matches for phishing hits?

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.cc:455: if
(request->is_phishing()) {
So we probably don't want to show a warning if we previously showed a SB
interstitial, regardless of if the client model thinks that it's phishing. It
would just leading to a confusing experience. It would probably also be good if
we didn't show a warning if we had previously disregarded the server SB
interstitial, but we can worry about that later.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.h:89: // actually
clicked through the warning.  this method is called on the UI
this -> This

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:339: bool
isphishing = false;
is_phishing?

[mattm, 2011-07-19 21:25:55]

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:339: bool
isphishing = false;
nit: is_phishing

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.cc:1054: void
SafeBrowsingService::AddObserver(Observer* observer) {
Add thread DCHECKS?

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:454:
EXPECT_CALL(observer_, OnSafeBrowsingHit(_)).Times(0);
Is this unnecessary due to using StrictMock?

[Brian Ryner, 2011-07-19 21:27:09]

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.cc:481:
tab_contents()->controller().GetActiveEntry()->unique_id() ==
You could avoid some of the repetition in this method by using temporary
variables, but up to you.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.h:89: // actually
clicked through the warning.  this method is called on the UI
Capitalize "this".

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:478:
.WillOnce(DoAll(DeleteArg<0>(), QuitUIMessageLoop()));
Is there an easy way to check that this situation doesn't cause the interstitial
to reappear?

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:264: if
(!request->SerializeToString(&request_data) && cb.get()) {
This doesn't seem quite right -- don't we want to return early, even if cb is
null?

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:112: Observer() {}
Since this should always be subclassed, make the ctor and dtor protected?

http://codereview.chromium.org/7408001/diff/1/chrome/renderer/safe_browsing/p...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/renderer/safe_browsing/p...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:203:
DCHECK(last_url_sent_to_classifier_.spec() == verdict.url());
Maybe switch this to DCHECK_EQ while you're here?

[noelutz, 2011-07-19 22:28:08]

Thanks for your comments.  Please take another look.

Thanks,
noe.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.cc:347: // ID and store
it for later.  For phishing hits we also require that the
On 2011/07/19 21:18:48, Garrett Casto wrote:
> The "In this case ..." sentence seems to have two clauses that say the same
> thing. Also, do you want to mention why we only send back information if the
top
> level url matches for phishing hits?

Removed the top level phishing check.  We can check that on the server.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.cc:455: if
(request->is_phishing()) {
On 2011/07/19 21:18:48, Garrett Casto wrote:
> So we probably don't want to show a warning if we previously showed a SB
> interstitial, regardless of if the client model thinks that it's phishing. It
> would just leading to a confusing experience. It would probably also be good
if
> we didn't show a warning if we had previously disregarded the server SB
> interstitial, but we can worry about that later.

I already implemented that a while back in the SafeBrowsingService class (see
whitelist code).

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.cc:481:
tab_contents()->controller().GetActiveEntry()->unique_id() ==
On 2011/07/19 21:27:10, Brian Ryner wrote:
> You could avoid some of the repetition in this method by using temporary
> variables, but up to you.

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.h:89: // actually
clicked through the warning.  this method is called on the UI
On 2011/07/19 21:27:10, Brian Ryner wrote:
> Capitalize "this".

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.h:89: // actually
clicked through the warning.  this method is called on the UI
On 2011/07/19 21:18:48, Garrett Casto wrote:
> this -> This

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:478:
.WillOnce(DoAll(DeleteArg<0>(), QuitUIMessageLoop()));
On 2011/07/19 21:27:10, Brian Ryner wrote:
> Is there an easy way to check that this situation doesn't cause the
interstitial
> to reappear?

Well, I do check that NULL is passed for the callback.  Since no callback is
provided to SendClientReportPhishingRequest I don't see how an interstitial
could be shown.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:264: if
(!request->SerializeToString(&request_data) && cb.get()) {
On 2011/07/19 21:27:10, Brian Ryner wrote:
> This doesn't seem quite right -- don't we want to return early, even if cb is
> null?

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:339: bool
isphishing = false;
On 2011/07/19 21:18:48, Garrett Casto wrote:
> is_phishing?

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:339: bool
isphishing = false;
On 2011/07/19 21:25:55, mattm wrote:
> nit: is_phishing

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.cc:1054: void
SafeBrowsingService::AddObserver(Observer* observer) {
On 2011/07/19 21:25:55, mattm wrote:
> Add thread DCHECKS?

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:112: Observer() {}
On 2011/07/19 21:27:10, Brian Ryner wrote:
> Since this should always be subclassed, make the ctor and dtor protected?

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:454:
EXPECT_CALL(observer_, OnSafeBrowsingHit(_)).Times(0);
On 2011/07/19 21:25:55, mattm wrote:
> Is this unnecessary due to using StrictMock?

Done.

http://codereview.chromium.org/7408001/diff/1/chrome/renderer/safe_browsing/p...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.cc (right):

http://codereview.chromium.org/7408001/diff/1/chrome/renderer/safe_browsing/p...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:203:
DCHECK(last_url_sent_to_classifier_.spec() == verdict.url());
On 2011/07/19 21:27:10, Brian Ryner wrote:
> Maybe switch this to DCHECK_EQ while you're here?

Done.

[Brian Ryner, 2011-07-19 22:35:24]

LGTM

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/7408001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:478:
.WillOnce(DoAll(DeleteArg<0>(), QuitUIMessageLoop()));
On 2011/07/19 22:28:08, noelutz wrote:
> On 2011/07/19 21:27:10, Brian Ryner wrote:
> > Is there an easy way to check that this situation doesn't cause the
> interstitial
> > to reappear?
> 
> Well, I do check that NULL is passed for the callback.  Since no callback is
> provided to SendClientReportPhishingRequest I don't see how an interstitial
> could be shown.

Ah, I missed that.  Seems reasonable.

[mattm, 2011-07-20 23:55:40]

lgtm

[commit-bot: I haz the power, 2011-07-22 03:08:54]

Change committed as 93534