Fix ClassifyNavigation.

Fix ClassifyNavigation.

If there is a slow pending load for a page, and in the meantime the page uses replaceState, the existence of the pending entry could cause a misclassification of the replaceState as SAME_PAGE.

Ensure that SAME_PAGE classification happens only if the pages bear some relationship.

BUG=369661
TEST=Existing tests, NavigationTypeClassification_InPageWhilePending.

Committed: https://crrev.com/f9c055737d01f84199e5e921f20378491db34ca8
Cr-Commit-Position: refs/heads/master@{#325321}

[Charlie Reis, 2015-04-13 23:45:40]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2015-04-13 23:45:41]

Hmm, looks like we're getting some test failures.

Also, the "TEST=Existing tests" line in the CL description confuses me, since it
sounds like this is fixing a bug.  Can you add a test showing why the old
behavior was wrong?

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1033:
existing_redirect_chain[0] == pending_entry_->GetURL()) ||
It would help to have a test showing why this is the right thing to check.  I'm
having trouble reasoning about why it's needed.

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1034:
existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {
Why are we comparing GetURL() and GetVirtualURL() here?

[Avi (use Gerrit), 2015-04-14 02:00:43]

I would need to dig a bit; I needed these to pass specific unit tests and I'm
not sure offhand why they're failing.

The idea is that this would pass all the existing tests.

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1033:
existing_redirect_chain[0] == pending_entry_->GetURL()) ||
On 2015/04/13 23:45:41, Charlie Reis (slow until 4-15) wrote:
> It would help to have a test showing why this is the right thing to check. 
I'm
> having trouble reasoning about why it's needed.

There's a test where if you navigate to url1 that redirects to url2, and then
you navigate to url1 again and again it redirects to url2, that's a SAME_PAGE.

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1034:
existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {
On 2015/04/13 23:45:40, Charlie Reis (slow until 4-15) wrote:
> Why are we comparing GetURL() and GetVirtualURL() here?

To cover SAME_PAGE for view-source:

[Avi (use Gerrit), 2015-04-14 14:39:45]

Added the patches to the unit tests that correct them.

[Avi (use Gerrit), 2015-04-14 20:15:42]

Charlie, I added a specific new test that fails without this fix, and that
passes with it. Please take a look.

[Avi (use Gerrit), 2015-04-15 14:51:46]

On 2015/04/14 20:15:42, Avi wrote:
> Charlie, I added a specific new test that fails without this fix, and that
> passes with it. Please take a look.

Now updated to be clearer about what the fundamental issue is.

[Charlie Reis, 2015-04-15 17:43:08]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2015-04-15 17:43:09]

I spent some time trying to follow this and I still don't quite get the virtual
URL case.

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1033:
existing_redirect_chain[0] == pending_entry_->GetURL()) ||
On 2015/04/14 02:00:43, Avi wrote:
> On 2015/04/13 23:45:41, Charlie Reis (slow until 4-15) wrote:
> > It would help to have a test showing why this is the right thing to check. 
> I'm
> > having trouble reasoning about why it's needed.
> 
> There's a test where if you navigate to url1 that redirects to url2, and then
> you navigate to url1 again and again it redirects to url2, that's a SAME_PAGE.

Acknowledged.

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1034:
existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {
On 2015/04/14 02:00:43, Avi wrote:
> On 2015/04/13 23:45:40, Charlie Reis (slow until 4-15) wrote:
> > Why are we comparing GetURL() and GetVirtualURL() here?
> 
> To cover SAME_PAGE for view-source:

Sorry, I still don't understand this one.  I just tried hitting enter on an
already committed view-source URL, and both the existing and pending entry have
http://www.chromium.org for GetURL() and view-source:http://www.chromium.org for
GetVirtualURL().

Why would the pending entry have a virtual URL that matches the existing entry's
actual URL?  We could compare GetVirtualURL() instead if there's a case where
that matters, but I think comparing GetURL() might be equivalent.

We should either remove this line or have a test showing why it matters.

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1015: class
StallDelegate : public ResourceDispatcherHostDelegate {
FYI for Nasko, since he was writing a more general version of this, but it
hasn't landed yet.  We can simplify this case once it lands.

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1806:
params.redirects.push_back(GURL("http://foo1"));
This whole block should get deleted, right?  It's mostly a no-op, apart from
these push_back lines which make it wrong.

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1875:
params.page_id = 0;
Same.

[Charlie Reis, 2015-04-15 17:43:42]

(also added Nasko as FYI for the test code)

[Avi (use Gerrit), 2015-04-15 19:14:16]

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1034:
existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {
On 2015/04/15 17:43:08, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/14 02:00:43, Avi wrote:
> > On 2015/04/13 23:45:40, Charlie Reis (slow until 4-15) wrote:
> > > Why are we comparing GetURL() and GetVirtualURL() here?
> > 
> > To cover SAME_PAGE for view-source:
> 
> Sorry, I still don't understand this one.  I just tried hitting enter on an
> already committed view-source URL, and both the existing and pending entry
have
> http://www.chromium.org for GetURL() and
http://view-source:http://www.chromium.org for
> GetVirtualURL().
> 
> Why would the pending entry have a virtual URL that matches the existing
entry's
> actual URL?  We could compare GetVirtualURL() instead if there's a case where
> that matters, but I think comparing GetURL() might be equivalent.
> 
> We should either remove this line or have a test showing why it matters.

I remember putting this line in for a test, but I can't make things fail without
this, so I'll put it for a new patchset.

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1806:
params.redirects.push_back(GURL("http://foo1"));
On 2015/04/15 17:43:08, Charlie Reis (slow until 4-15) wrote:
> This whole block should get deleted, right?  It's mostly a no-op, apart from
> these push_back lines which make it wrong.

I'm not sure what you mean by the push_back lines making it wrong.

[Charlie Reis, 2015-04-15 19:57:36]

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1082083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1034:
existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {
On 2015/04/15 19:14:16, Avi wrote:
> On 2015/04/15 17:43:08, Charlie Reis (slow until 4-15) wrote:
> > On 2015/04/14 02:00:43, Avi wrote:
> > > On 2015/04/13 23:45:40, Charlie Reis (slow until 4-15) wrote:
> > > > Why are we comparing GetURL() and GetVirtualURL() here?
> > > 
> > > To cover SAME_PAGE for view-source:
> > 
> > Sorry, I still don't understand this one.  I just tried hitting enter on an
> > already committed view-source URL, and both the existing and pending entry
> have
> > http://www.chromium.org for GetURL() and
> http://view-source:http://www.chromium.org for
> > GetVirtualURL().
> > 
> > Why would the pending entry have a virtual URL that matches the existing
> entry's
> > actual URL?  We could compare GetVirtualURL() instead if there's a case
where
> > that matters, but I think comparing GetURL() might be equivalent.
> > 
> > We should either remove this line or have a test showing why it matters.
> 
> I remember putting this line in for a test, but I can't make things fail
without
> this, so I'll put it for a new patchset.

Ok, thanks.  Let me know if you find the case it matters for.

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1806:
params.redirects.push_back(GURL("http://foo1"));
On 2015/04/15 19:14:16, Avi wrote:
> On 2015/04/15 17:43:08, Charlie Reis (slow until 4-15) wrote:
> > This whole block should get deleted, right?  It's mostly a no-op, apart from
> > these push_back lines which make it wrong.
> 
> I'm not sure what you mean by the push_back lines making it wrong.

You've already done all of these assignments on lines 1778-1786.  |params|
hasn't changed since then (since RendererDidNavigate treats it as const), but
here you're setting page_id to 0 (again), url to url2 (again), and appending
foo1 and foo2 to redirects, which already contains them.  Now there's 4 entries
in that vector.

[Avi (use Gerrit), 2015-04-15 20:19:26]

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1082083002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1806:
params.redirects.push_back(GURL("http://foo1"));
On 2015/04/15 19:57:36, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/15 19:14:16, Avi wrote:
> > On 2015/04/15 17:43:08, Charlie Reis (slow until 4-15) wrote:
> > > This whole block should get deleted, right?  It's mostly a no-op, apart
from
> > > these push_back lines which make it wrong.
> > 
> > I'm not sure what you mean by the push_back lines making it wrong.
> 
> You've already done all of these assignments on lines 1778-1786.  |params|
> hasn't changed since then (since RendererDidNavigate treats it as const), but
> here you're setting page_id to 0 (again), url to url2 (again), and appending
> foo1 and foo2 to redirects, which already contains them.  Now there's 4
entries
> in that vector.

D'oh. Yeah.

[Charlie Reis, 2015-04-15 20:32:37]

Thanks!  LGTM.

[Avi (use Gerrit), 2015-04-15 20:37:13]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-04-15 20:37:30]

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

[commit-bot: I haz the power, 2015-04-15 22:06:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-15 22:08:17]

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