Currently, there is a bug in the way we show the csd phishing interstitial.

Currently, there is a bug in the way we show the csd phishing interstitial.

Because we have to go from the UI to the IO back to the UI thread to display
the csd interstitial it's possible that a navigation starts while we're on the
IO thread which causes the interstitial constructor to fail on a DCHECK.

This change moves the SafeBrowsing service whitelist logic from the IO thread to
the UI thread so that the csd interstitial doesn't have to go through the IO
thread.

BUG=None
TEST=None


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

[mattm, 2011-05-31 22:00:41]

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:372:
resource.threat_type= SafeBrowsingService::CLIENT_SIDE_PHISHING_URL;
nit: space before =

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:383: }
will the CsdClient be leaked when this if block doesn't match?

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (left):

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:360: 
does this mean the whitelist isn't used for non-csd blocking pages?

[noelutz, 2011-06-01 01:18:01]

Thanks for your comments.  Please take another look,
noé.

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:372:
resource.threat_type= SafeBrowsingService::CLIENT_SIDE_PHISHING_URL;
On 2011/05/31 22:00:41, mattm wrote:
> nit: space before =

Done.

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:383: }
On 2011/05/31 22:00:41, mattm wrote:
> will the CsdClient be leaked when this if block doesn't match?

Nice catch!

We could either always call DoDisplayBlockingPage which always calls
OnBlockingPageComplete eventually which deletes the client object.  Or we can do
what I did namely only create the client object if we call
DoDisplayBlockingPage.  I think the latter option is a bit more straightforward.

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (left):

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:360: 
On 2011/05/31 22:00:41, mattm wrote:
> does this mean the whitelist isn't used for non-csd blocking pages?

It is.  But instead of checking the whitelist here we check it in
DoDisplayBlockingPage.

[mattm, 2011-06-01 02:14:52]

lgtm

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (left):

http://codereview.chromium.org/7080034/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:360: 
On 2011/06/01 01:18:01, noelutz wrote:
> On 2011/05/31 22:00:41, mattm wrote:
> > does this mean the whitelist isn't used for non-csd blocking pages?
> 
> It is.  But instead of checking the whitelist here we check it in
> DoDisplayBlockingPage.

oh, right.  I had confused which one calls the other.

[Brian Ryner, 2011-06-01 02:58:43]

lgtm

http://codereview.chromium.org/7080034/diff/6004/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7080034/diff/6004/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:370:
resource.redirect_urls = std::vector<GURL>();
This seems unnecessary, since you just created the UnsafeResource.

http://codereview.chromium.org/7080034/diff/6004/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:380:
tab_contents()->controller().DiscardNonCommittedEntries();
Does this actually cancel the underlying navigation or just remove it from
session history?

[noelutz, 2011-06-01 05:33:53]

Thanks.  Please take another look,
noé.

http://codereview.chromium.org/7080034/diff/6004/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7080034/diff/6004/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:370:
resource.redirect_urls = std::vector<GURL>();
On 2011/06/01 02:58:43, Brian Ryner wrote:
> This seems unnecessary, since you just created the UnsafeResource.

Done.

http://codereview.chromium.org/7080034/diff/6004/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:380:
tab_contents()->controller().DiscardNonCommittedEntries();
On 2011/06/01 02:58:43, Brian Ryner wrote:
> Does this actually cancel the underlying navigation or just remove it from
> session history?

not 100% sure but it's what the interstitial code is doing when you don't
proceed and it's also what the interstitial constructor checks for.

[Brian Ryner, 2011-06-01 05:35:55]

lgtm

http://codereview.chromium.org/7080034/diff/6004/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7080034/diff/6004/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:380:
tab_contents()->controller().DiscardNonCommittedEntries();
On 2011/06/01 05:33:53, noelutz wrote:
> On 2011/06/01 02:58:43, Brian Ryner wrote:
> > Does this actually cancel the underlying navigation or just remove it from
> > session history?
> 
> not 100% sure but it's what the interstitial code is doing when you don't
> proceed and it's also what the interstitial constructor checks for.

Sounds good, the behavior for "don't proceed" should be the same as what we want
here.

[commit-bot: I haz the power, 2011-06-01 06:22:01]

Change committed as 87441