Run pre-classification checks in the browser before starting client-side phishing detection.

Run pre-classification checks in the browser before starting client-side phishing detection.

This CL just adds a framework for running these checks, and adjusts the
renderer-side logic to delay phishing classification until the browser has
sent an IPC signaling that it should run.  Future CL's will add the actual
checks, which will include a whitelist lookup, intranet hostname checks, and
per-day ping limit.

BUG=none
TEST=updated existing tests

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

[Brian Ryner, 2011-01-26 00:49:15]

This may not be quite ready to go yet, but I'm hoping to get some feedback on
how I can test this on the browser side.  As the code is organized now, I would
need a TabContents object in order to get the appropriate notifications sent.  I
could do this by writing an InProcessBrowserTest, but if I go that route, how
would I verify that the IPC was sent to the renderer?  Any other suggestions for
testing?

Thanks,
Brian

[noelutz, 2011-01-26 01:31:20]

Very nice.  Just a few minor comments.

Thanks,
noe.

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

http://codereview.chromium.org/6398001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:61: virtual
~ShouldClassifyUrlRequest() {}
make the destructor private to make sure we delete that object?

http://codereview.chromium.org/6398001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:77:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Maybe comment why you're not simply calling Finish() since you are already on
the right thread?

http://codereview.chromium.org/6398001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:87: tab_contents_
= NULL;
delete this or call Finish()?

[lzheng, 2011-01-27 18:31:27]

I didn't dig into details of this CL but looked at the higher level since you
mentioned it is not fully ready yet.

As for the test, do you think it is possible to pass in a mock or a fake
delegate and verify OnStartPhishingDetection is called?

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

http://codereview.chromium.org/6398001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:197: {
{ and the later } should not be needed.

[Brian Ryner, 2011-02-01 21:45:45]

I got the browser-side test working, so this is ready for review now.

Thanks,
Brian

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

http://codereview.chromium.org/6398001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:61: virtual
~ShouldClassifyUrlRequest() {}
On 2011/01/26 01:31:21, noelutz wrote:
> make the destructor private to make sure we delete that object?

Done.

http://codereview.chromium.org/6398001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:77:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/01/26 01:31:21, noelutz wrote:
> Maybe comment why you're not simply calling Finish() since you are already on
> the right thread?

Done.

http://codereview.chromium.org/6398001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:87: tab_contents_
= NULL;
On 2011/01/26 01:31:21, noelutz wrote:
> delete this or call Finish()?

Originally I was thinking we might not be able to cancel the request
synchronously, for example if we had already started an operation on another
thread.  But, that may not actually be a problem, and I agree it's clearer for
now if we just do this synchronously.

http://codereview.chromium.org/6398001/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:197: {
On 2011/01/27 18:31:27, lzheng wrote:
> { and the later } should not be needed.

I need an enclosing scope since I'm defining variables inside of the "case". 
But, my formatting didn't match the style guide, so I've fixed that.

[Brian Ryner, 2011-02-02 01:04:17]

Adding John to take a look at the RenderView additions.

[jam, 2011-02-03 19:38:36]

On 2011/02/02 01:04:17, Brian Ryner wrote:
> Adding John to take a look at the RenderView additions.

lgtm

[lzheng, 2011-02-04 20:05:22]

Good work.

http://codereview.chromium.org/6398001/diff/9001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6398001/diff/9001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:216:
controller->tab_contents());
what's the life-cycle of controller->tab_contents()? Will it be around during
request's async calls?

http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.cc (right):

http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:147: 
Can you add a comment here to explain that this means browser has not confirmed
this url should be classified yet?

http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.h (right):

http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.h:53: //
classification.  May modify page_text.  Note that it is an error to
this comment needs slight update because it cals MaybeStartClassification now

[Brian Ryner, 2011-02-10 01:12:52]

Please have another look.

http://codereview.chromium.org/6398001/diff/9001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6398001/diff/9001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:216:
controller->tab_contents());
On 2011/02/04 20:05:22, lzheng wrote:
> what's the life-cycle of controller->tab_contents()? Will it be around during
> request's async calls?

The request registers for the TAB_CONTENTS_DESTROYED notification on the
TabContents that it's attached to.  Once this is received, the TabContents
pointer is nulled out, and the request will no longer call any methods on it.

http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.cc (right):

http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:147: 
On 2011/02/04 20:05:22, lzheng wrote:
> Can you add a comment here to explain that this means browser has not
confirmed
> this url should be classified yet?

Done.

http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
File chrome/renderer/safe_browsing/phishing_classifier_delegate.h (right):

http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
chrome/renderer/safe_browsing/phishing_classifier_delegate.h:53: //
classification.  May modify page_text.  Note that it is an error to
On 2011/02/04 20:05:22, lzheng wrote:
> this comment needs slight update because it cals MaybeStartClassification now

Done, also added back some comments that were removed during the refactoring.

[noelutz, 2011-02-10 01:36:07]

I like it.  Just a couple minor questions mostly to make sure I understand the
CL.

Thanks,
noe.

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.cc:85:
&ShouldClassifyUrlRequest::Finish));
I just want to make sure I understand this correctly.  Later if one of the
checks fail we would simply call Cancel()?

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.cc:208: if
(load_details->main_frame() &&
nit: Maybe add a comment that explains what you are testing for here.

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(right):

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:283:
ClientSideDetectionService::Create(model_path, NULL));
I created a separate empty constructor for this in my CL.  Feel free to do the
same and I will merge.

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:293:
EXPECT_EQ(rvh()->routing_id(), received_msg_.routing_id());
Maybe do a reload and check that you don't send an IPC in this case?

[lzheng, 2011-02-10 20:37:36]

Did you get rid of the changes in render_view.h and render_view.cc
intensionally?


On 2011/02/10 01:12:52, Brian Ryner wrote:
> Please have another look.
> 
>
http://codereview.chromium.org/6398001/diff/9001/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/client_side_detection_service.cc (right):
> 
>
http://codereview.chromium.org/6398001/diff/9001/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/client_side_detection_service.cc:216:
> controller->tab_contents());
> On 2011/02/04 20:05:22, lzheng wrote:
> > what's the life-cycle of controller->tab_contents()? Will it be around
during
> > request's async calls?
> 
> The request registers for the TAB_CONTENTS_DESTROYED notification on the
> TabContents that it's attached to.  Once this is received, the TabContents
> pointer is nulled out, and the request will no longer call any methods on it.
> 
>
http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
> File chrome/renderer/safe_browsing/phishing_classifier_delegate.cc (right):
> 
>
http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
> chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:147: 
> On 2011/02/04 20:05:22, lzheng wrote:
> > Can you add a comment here to explain that this means browser has not
> confirmed
> > this url should be classified yet?
> 
> Done.
> 
>
http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
> File chrome/renderer/safe_browsing/phishing_classifier_delegate.h (right):
> 
>
http://codereview.chromium.org/6398001/diff/9001/chrome/renderer/safe_browsin...
> chrome/renderer/safe_browsing/phishing_classifier_delegate.h:53: //
> classification.  May modify page_text.  Note that it is an error to
> On 2011/02/04 20:05:22, lzheng wrote:
> > this comment needs slight update because it cals MaybeStartClassification
now
> 
> Done, also added back some comments that were removed during the refactoring.

[Brian Ryner, 2011-02-10 21:15:20]

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.cc:85:
&ShouldClassifyUrlRequest::Finish));
On 2011/02/10 01:36:07, noelutz wrote:
> I just want to make sure I understand this correctly.  Later if one of the
> checks fail we would simply call Cancel()?

My original thinking was that once we actually add some checks here, we'd also
add a boolean parameter to Finish() that says whether it should start
classification or not.  That said, this wouldn't be any different currently than
calling Cancel().

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service.cc:208: if
(load_details->main_frame() &&
On 2011/02/10 01:36:07, noelutz wrote:
> nit: Maybe add a comment that explains what you are testing for here.

As we discussed offline, the back/forward check here is not really correct. 
What we should do for back/forward navigations is first check the cache (which
Garrett is adding in http://codereview.chromium.org/6374017/), and if there's no
cached entry, let classification proceed normally.  Otherwise, a user may see a
phishing warning, but it would disappear if they go back and forward again,
which would be confusing.

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(right):

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:283:
ClientSideDetectionService::Create(model_path, NULL));
On 2011/02/10 01:36:07, noelutz wrote:
> I created a separate empty constructor for this in my CL.  Feel free to do the
> same and I will merge.

I'm not so sure I'm a fan of the second constructor.  It duplicates all of the
initialization, which makes it more likely that we'll forget at some point to
add a new initializer to both constructers.  Do you actually need to have the
empty constructor for your test, or could you just have your
MockClientSideDetectionService initialize the base class with an empty FilePath
and a NULL URLRequestContextGetter?

http://codereview.chromium.org/6398001/diff/13001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:293:
EXPECT_EQ(rvh()->routing_id(), received_msg_.routing_id());
On 2011/02/10 01:36:07, noelutz wrote:
> Maybe do a reload and check that you don't send an IPC in this case?

Since I'm no longer doing anything different on back/forward navigation, there's
probably no need to test that case explicitly.

I wouldn't mind doing a test for the in-page navigation case, but it would be a
little difficult -- since I wouldn't expect an IPC, I wouldn't be able to quit
the message loop the same way as I'm doing above.  I could possibly call
RunAllPending() (more than once if needed)... thoughts?

[Brian Ryner, 2011-02-10 21:17:55]

On 2011/02/10 20:37:36, lzheng wrote:
> Did you get rid of the changes in render_view.h and render_view.cc
> intensionally?

Yep.  http://codereview.chromium.org/6250176/ converted
PhishingClassifierDelegate into a RenderViewObserver, so it automatically gets a
chance to handle all IPC's going to the RenderView.

[Brian Ryner, 2011-02-11 00:52:10]

This change removes the back/forward check, adds the comment Noe suggested, and
adds a test for in-page navigation as I mentioned above.  Please take another
look.

[lzheng, 2011-02-11 17:55:43]

LGTM.
On 2011/02/11 00:52:10, Brian Ryner wrote:
> This change removes the back/forward check, adds the comment Noe suggested,
and
> adds a test for in-page navigation as I mentioned above.  Please take another
> look.