Fixes for Safe Browsing with unrelated pending navigations.

Fixes for Safe Browsing with unrelated pending navigations.

BUG=546226
Committed: https://crrev.com/bfc4060d7805f12c2417fc71a1a544cfb63e88f2
Cr-Commit-Position: refs/heads/master@{#366195}

[mattm, 2015-12-08 05:22:07]

Patchset #1 (id:1) has been deleted

[mattm, 2015-12-08 05:22:13]

Patchset #1 (id:20001) has been deleted

[mattm, 2015-12-08 05:36:44]

mattm@chromium.org changed reviewers:
+ creis@chromium.org, nparker@chromium.org

[mattm, 2015-12-08 05:36:45]

nparker, creis: please review
meacer: fyi

One note: I'm a little uneasy about removing the conditions for whether we
actually get the NavigationEntry. On the other hand, doing so found a lot of bad
tests, and it *seems* like even if there is some race condition where we got a
NULL back it's a different bug anyway since it means safebrowsing is trying to
do stuff after a navigation already occurred.
(I was also pondering if doing the NavigationEntry lookup when the
UnsafeResource is created and saving the relevant values in the UnsafeResource
might be a bit safer, though it does make things more tightly coupled.)

[Charlie Reis, 2015-12-11 05:39:25]

On 2015/12/08 05:36:45, mattm wrote:
> nparker, creis: please review
> meacer: fyi

Thanks for taking this on, and for the extra test coverage.  I'll probably rely
on nparker for the in-depth review (since I'm less familiar with this code), but
I'm taking a look at how the NavigationEntries are used.

> 
> One note: I'm a little uneasy about removing the conditions for whether we
> actually get the NavigationEntry. On the other hand, doing so found a lot of
bad
> tests, and it *seems* like even if there is some race condition where we got a
> NULL back it's a different bug anyway since it means safebrowsing is trying to
> do stuff after a navigation already occurred.
> (I was also pondering if doing the NavigationEntry lookup when the
> UnsafeResource is created and saving the relevant values in the UnsafeResource
> might be a bit safer, though it does make things more tightly coupled.)

Ah, removing those null checks isn't safe.  As I mention below, the
NavigationEntry can be null for the initial blank page (e.g., after
window.open), which itself can have real iframes injected into it by its opener.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.cc:408: if
(!web_contents())
Unfortunately, you might still need a null check on the relevant NavigationEntry
here.

A newly created WebContents may not have a last committed NavigationEntry (e.g.,
after window.open()), and yet it can still create subframes with malicious
pages.  That means that a subresource hit could occur with no last committed
entry.

(As a side note, I've spent a lot of time trying to eliminate this case by
creating a NavigationEntry for the initial blank page and replacing it when
necessary.  There's still a lot of failing tests, though, so I need to get back
to it in https://crbug.com/524208.)

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.cc:677:
web_contents()->GetController().GetLastCommittedEntry();
Is this meant to be called while the interstitial is shown, or after the user
has proceeded through it?  If it's while the interstitial is visible, then this
would need the pending entry for main frames and the last committed for
subframes, as elsewhere.

(Maybe adding a comment here would help if this choice is intentional.)

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:752:
OnPhishingDetectionDoneVerdictNotPhishingButSBMatchOnSubresourceWhileNavPending)
{
That's a mouthful!  Would a shorter name be worthwhile, or does this follow a
convention?

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:146:
main_frame_url_(main_frame_url),
Any reason main_frame_url_ is listed before unsafe_resources_ in the members but
in the other order in the constructor parameters?

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details_unittest.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details_unittest.cc:550: const char*
kUnrelatedReferrerURL = "http://www.unrelatedreferrer.com/some/path";
nit: 80 chars

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:96: return NULL;
nit: nullptr

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:99: return
contents->GetController().GetLastCommittedEntry();
This should have a comment explaining why we return the pending entry for the
main frame and the last committed for the subframe.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:55: content::NavigationEntry*
GetNavigationEntryForResource() const;
Worth having a comment here about why using this is important?

Also, this can return null (as noted before), so the other call sites should
probably check for that.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:218: // Normally
safebrowsing interstitial types which block the main page load
nit: Blank line before.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:221: // pending
navigation when the SafeBrowsingBlockingPage is created. This test
"This test" -> I'm confused.  This isn't test code.

[Nathan Parker, 2015-12-11 19:48:07]

I actually would defer to Charlie for naventry lifetime questions.  As for
functionality, I'm glad to see this get fixed!

I've partially reviewed, but have a general question before I get to far (below)

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.cc:408: if
(!web_contents())
On 2015/12/11 05:39:24, Charlie Reis wrote:
> Unfortunately, you might still need a null check on the relevant
NavigationEntry
> here.
> 
> A newly created WebContents may not have a last committed NavigationEntry
(e.g.,
> after window.open()), and yet it can still create subframes with malicious
> pages.  That means that a subresource hit could occur with no last committed
> entry.
> 
> (As a side note, I've spent a lot of time trying to eliminate this case by
> creating a NavigationEntry for the initial blank page and replacing it when
> necessary.  There's still a lot of failing tests, though, so I need to get
back
> to it in https://crbug.com/524208.)

How is web_contents_ different from what
resource.GetNavigationEntryForResource() uses?  If web_contents_ null, then
would GetNavigationEntryForResource() also be?

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details.cc:185: NavigationEntry* nav_entry =
resource_.GetNavigationEntryForResource();
Is there any case where web_contents_ would be different from that returned by
GetWebContentsByID(render_process_host_id, render_view_id)?

[Charlie Reis, 2015-12-11 21:10:04]

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.cc:408: if
(!web_contents())
On 2015/12/11 19:48:06, nparker wrote:
> On 2015/12/11 05:39:24, Charlie Reis wrote:
> > Unfortunately, you might still need a null check on the relevant
> NavigationEntry
> > here.
> > 
> > A newly created WebContents may not have a last committed NavigationEntry
> (e.g.,
> > after window.open()), and yet it can still create subframes with malicious
> > pages.  That means that a subresource hit could occur with no last committed
> > entry.
> > 
> > (As a side note, I've spent a lot of time trying to eliminate this case by
> > creating a NavigationEntry for the initial blank page and replacing it when
> > necessary.  There's still a lot of failing tests, though, so I need to get
> back
> > to it in https://crbug.com/524208.)
> 
> How is web_contents_ different from what
> resource.GetNavigationEntryForResource() uses?

resource.GetNavigationEntryForResource() handles the fact that we should use the
pending entry for main frame interstitials and the last committed entry for
subframe interstitials.  (Using GetActiveEntry() on the WebContents is wrong.)


>  If web_contents_ null, then
> would GetNavigationEntryForResource() also be?

If the render_process_host_id and render_view_id of UnsafeResource have the same
lifetime as web_contents_, then yes.  I'm not sure how those are implemented,
though-- sounds like a question for mattm.

[mattm, 2015-12-15 01:42:25]

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.cc:408: if
(!web_contents())
On 2015/12/11 05:39:24, Charlie Reis wrote:
> Unfortunately, you might still need a null check on the relevant
NavigationEntry
> here.
> 
> A newly created WebContents may not have a last committed NavigationEntry
(e.g.,
> after window.open()), and yet it can still create subframes with malicious
> pages.  That means that a subresource hit could occur with no last committed
> entry.
> 
> (As a side note, I've spent a lot of time trying to eliminate this case by
> creating a NavigationEntry for the initial blank page and replacing it when
> necessary.  There's still a lot of failing tests, though, so I need to get
back
> to it in https://crbug.com/524208.)

Thanks, done.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.cc:408: if
(!web_contents())
On 2015/12/11 21:10:04, Charlie Reis wrote:
> On 2015/12/11 19:48:06, nparker wrote:
> > On 2015/12/11 05:39:24, Charlie Reis wrote:
> > > Unfortunately, you might still need a null check on the relevant
> > NavigationEntry
> > > here.
> > > 
> > > A newly created WebContents may not have a last committed NavigationEntry
> > (e.g.,
> > > after window.open()), and yet it can still create subframes with malicious
> > > pages.  That means that a subresource hit could occur with no last
committed
> > > entry.
> > > 
> > > (As a side note, I've spent a lot of time trying to eliminate this case by
> > > creating a NavigationEntry for the initial blank page and replacing it
when
> > > necessary.  There's still a lot of failing tests, though, so I need to get
> > back
> > > to it in https://crbug.com/524208.)
> > 
> > How is web_contents_ different from what
> > resource.GetNavigationEntryForResource() uses?
> 
> resource.GetNavigationEntryForResource() handles the fact that we should use
the
> pending entry for main frame interstitials and the last committed entry for
> subframe interstitials.  (Using GetActiveEntry() on the WebContents is wrong.)
> 
> 
> >  If web_contents_ null, then
> > would GetNavigationEntryForResource() also be?
> 
> If the render_process_host_id and render_view_id of UnsafeResource have the
same
> lifetime as web_contents_, then yes.  I'm not sure how those are implemented,
> though-- sounds like a question for mattm.

The UnsafeResource doesn't really have any enforced lifetime, someone could hold
one and call GetNavigationEntryForResource after the page has navigated or
closed, but that would be an error. I added a comment about that.

A CSDH object is for a specific WebContents, but the OnSafeBrowsingHit goes to
all CSDH objects, so they need to check if it is specifically for them. So it
could be possible that web_contents() is null if this were an unrelated CSDH
that is shutting down but hasn't been destroyed yet.

If we are talking only about the CSDH that actually refers to the same
WebContents, then I'm not sure it's actually possible for web_contents() to be
null and GetNavigationEntryForResource not to be. It would depend if
tab_util::GetWebContentsByID can still return the WebContents but the
WebContentsObservers already had their web_contents_ reset.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.cc:677:
web_contents()->GetController().GetLastCommittedEntry();
On 2015/12/11 05:39:24, Charlie Reis wrote:
> Is this meant to be called while the interstitial is shown, or after the user
> has proceeded through it?  If it's while the interstitial is visible, then
this
> would need the pending entry for main frames and the last committed for
> subframes, as elsewhere.
> 
> (Maybe adding a comment here would help if this choice is intentional.)

If the interstitial was a blocking main page load, then this could only be
called after it was proceeded through. If it was a subresource hit, it could be
called while the interstitial is still showing.

This is called after CSD checks finish and CSD only runs on a committed page, so
this should always use the last committed entry. I added a comment.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:752:
OnPhishingDetectionDoneVerdictNotPhishingButSBMatchOnSubresourceWhileNavPending)
{
On 2015/12/11 05:39:24, Charlie Reis wrote:
> That's a mouthful!  Would a shorter name be worthwhile, or does this follow a
> convention?

It's following the same convention as other tests in this file, though I agree
it's unwieldy.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:146:
main_frame_url_(main_frame_url),
On 2015/12/11 05:39:24, Charlie Reis wrote:
> Any reason main_frame_url_ is listed before unsafe_resources_ in the members
but
> in the other order in the constructor parameters?

Not particularly. I wanted to keep main_frame_url_ next to
is_main_frame_load_blocked_ in the members, but just stuck it on the end of the
parameter list. Changed.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details.cc:185: NavigationEntry* nav_entry =
resource_.GetNavigationEntryForResource();
On 2015/12/11 19:48:06, nparker wrote:
> Is there any case where web_contents_ would be different from that returned by
> GetWebContentsByID(render_process_host_id, render_view_id)?

I don't think so. The web_contents_ is passed in from SafeBrowsingBlockingPage,
and SBBP gets it's web_contents_ from the same
GetWebContentsByID(unsafe_resource.render_process_host_id,unsafe_resource.render_view_id):

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details_unittest.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details_unittest.cc:550: const char*
kUnrelatedReferrerURL = "http://www.unrelatedreferrer.com/some/path";
On 2015/12/11 05:39:24, Charlie Reis wrote:
> nit: 80 chars

Done.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:96: return NULL;
On 2015/12/11 05:39:24, Charlie Reis wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:99: return
contents->GetController().GetLastCommittedEntry();
On 2015/12/11 05:39:24, Charlie Reis wrote:
> This should have a comment explaining why we return the pending entry for the
> main frame and the last committed for the subframe.

Done.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:55: content::NavigationEntry*
GetNavigationEntryForResource() const;
On 2015/12/11 05:39:24, Charlie Reis wrote:
> Worth having a comment here about why using this is important?

done.

> Also, this can return null (as noted before), so the other call sites should
> probably check for that.

done.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:218: // Normally
safebrowsing interstitial types which block the main page load
On 2015/12/11 05:39:25, Charlie Reis wrote:
> nit: Blank line before.

Done.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:221: // pending
navigation when the SafeBrowsingBlockingPage is created. This test
On 2015/12/11 05:39:25, Charlie Reis wrote:
> "This test" -> I'm confused.  This isn't test code.

I couldn't quite think of proper wording. This isn't a test, but it's not a real
safebrowsing hit either. You can go to chrome://interstitials to view an example
of various interstitial pages. I replaced it with "demo".

[Nathan Parker, 2015-12-16 23:30:24]

lgtm

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1070:
"document.body.appendChild(img);"));
odd formatting

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1092:
namespace {
I'm not clear on the point of going in and out of anonymous namespaces here. 
Why isn't this whole file in 
namespace safe_browsing { namespace { ... } }
?

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:180:
safe_browsing::SafeBrowsingBlockingPage* CreateSafeBrowsingBlockingPage(
Can you add a comment at the top saying this is all for creating demo pages on
chrome://interstitials?  That's not obviousl

[mattm, 2015-12-17 01:43:56]

mattm@chromium.org changed reviewers:
+ bauerb@chromium.org

[mattm, 2015-12-17 01:43:57]

+bauerb for chrome/browser/ui/webui/interstitials/ OWNERS

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1070:
"document.body.appendChild(img);"));
On 2015/12/16 23:30:24, nparker wrote:
> odd formatting

It is odd, but that's what clang-format wanted..
I fixed.

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1092:
namespace {
On 2015/12/16 23:30:24, nparker wrote:
> I'm not clear on the point of going in and out of anonymous namespaces here. 
> Why isn't this whole file in 
> namespace safe_browsing { namespace { ... } }
> ?

There were some threads on chromium-dev about namespaces in test code
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/pKY2G4GHcu0
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/MwPIo2BnVhM
Summary: helpers should be in anonymous namespace, the tests themselves should
not (though usually it works if you do).

The going in and out is because some helper classes are mixed in instead of all
at the beginning of the file.

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/1509073002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:180:
safe_browsing::SafeBrowsingBlockingPage* CreateSafeBrowsingBlockingPage(
On 2015/12/16 23:30:24, nparker wrote:
> Can you add a comment at the top saying this is all for creating demo pages on
> chrome://interstitials?  That's not obviousl

Indeed, I was rather confused when I first came across this code. Done.

[Bernhard Bauer, 2015-12-17 16:02:02]

LGTM with some small nits:

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:753:
OnPhishingDetectionDoneVerdictNotPhishingButSBMatchOnSubresourceWhileNavPending)
{
Umm... I think you might win an award for longest test name ever. 😉

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:109: return
scoped_ptr<net::test_server::HttpResponse>();
Turns out you can actually return nullptr here ☺

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:135:
protected:
Why not private? Is this subclassed?

[Bernhard Bauer, 2015-12-17 16:54:25]

Also, is there a reason why this CL is protected?

[Charlie Reis, 2015-12-17 19:24:19]

One more null check needed in threat_details.cc.  Otherwise the uses of
NavigationEntry LGTM, and mainly nits below.

Also, +1 to making this CL public.  I don't see a need for it to be protected.

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/1509073002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:221: // pending
navigation when the SafeBrowsingBlockingPage is created. This test
On 2015/12/15 01:42:25, mattm wrote:
> On 2015/12/11 05:39:25, Charlie Reis wrote:
> > "This test" -> I'm confused.  This isn't test code.
> 
> I couldn't quite think of proper wording. This isn't a test, but it's not a
real
> safebrowsing hit either. You can go to chrome://interstitials to view an
example
> of various interstitial pages. I replaced it with "demo".

Ah, cool!  I didn't realize that page existed.

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/client_side_detection_host.cc:683: // be used since
it may no longer by valid (eg, if the UnsafeResource was for
nit: s/by/be/

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details.cc (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details.cc:187: GURL page_url =
nav_entry->GetURL();
nav_entry might be null.

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:62: // This method must not be used if
any navigations have occured since the
nit: occurred

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:63: // hit (eg, it should only be used
while the interstitial is showing, or
nit: Are there any other cases it could be used?  If not, s/eg/ie/
(Hard to tell if it's trying to be an exhaustive list right now.)

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:6: // not used in
displaying any real interstitials.
nit: Please put this comment above the class declaration on line 41.

[mattm, 2015-12-18 21:40:56]

Description was changed from

==========
Fixes for Safe Browsing with unrelated pending navigations.

BUG=546226
==========

to

==========
Fixes for Safe Browsing with unrelated pending navigations.

BUG=546226
==========

[mattm, 2015-12-18 21:41:04]

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/client_side_detection_host.cc:683: // be used since
it may no longer by valid (eg, if the UnsafeResource was for
On 2015/12/17 19:24:18, Charlie Reis wrote:
> nit: s/by/be/

Done.

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:109: return
scoped_ptr<net::test_server::HttpResponse>();
On 2015/12/17 16:02:02, Bernhard Bauer wrote:
> Turns out you can actually return nullptr here ☺

Done.

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:135:
protected:
On 2015/12/17 16:02:02, Bernhard Bauer wrote:
> Why not private? Is this subclassed?

Done.

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/threat_details.cc (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/threat_details.cc:187: GURL page_url =
nav_entry->GetURL();
On 2015/12/17 19:24:18, Charlie Reis (OOO till Jan 4) wrote:
> nav_entry might be null.

Done, thanks for catching! I must have gotten interrupted in the middle of
fixing these :/

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:62: // This method must not be used if
any navigations have occured since the
On 2015/12/17 19:24:18, Charlie Reis wrote:
> nit: occurred

Done.

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:63: // hit (eg, it should only be used
while the interstitial is showing, or
On 2015/12/17 19:24:18, Charlie Reis (OOO till Jan 4) wrote:
> nit: Are there any other cases it could be used?  If not, s/eg/ie/
> (Hard to tell if it's trying to be an exhaustive list right now.)

Clarified.

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/1509073002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:6: // not used in
displaying any real interstitials.
On 2015/12/17 19:24:18, Charlie Reis wrote:
> nit: Please put this comment above the class declaration on line 41.

Done.

[mattm, 2015-12-18 21:43:23]

The CQ bit was checked by mattm@chromium.org

[mattm, 2015-12-18 21:43:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, bauerb@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1509073002/#ps120001
(title: "review changes for comment #13-15")

[commit-bot: I haz the power, 2015-12-18 21:43:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1509073002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1509073002/120001

[commit-bot: I haz the power, 2015-12-18 23:11:48]

Description was changed from

==========
Fixes for Safe Browsing with unrelated pending navigations.

BUG=546226
==========

to

==========
Fixes for Safe Browsing with unrelated pending navigations.

BUG=546226
==========

[commit-bot: I haz the power, 2015-12-18 23:11:49]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-12-18 23:12:58]

Description was changed from

==========
Fixes for Safe Browsing with unrelated pending navigations.

BUG=546226
==========

to

==========
Fixes for Safe Browsing with unrelated pending navigations.

BUG=546226

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

[commit-bot: I haz the power, 2015-12-18 23:12:58]

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