history.replaceState(..., location.href) clearing page-action icons.

history.replaceState(..., location.href) clearing page-action icons.

history.replaceState() can confuse the browser into thinking that a reload is happening when there is, in fact, an in-page navigation.  After checking that we're not crossing an origin boundary, we can let the otherwise untrusted renderer resolve the ambiguity.

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

[Tom Sepez, 2012-08-27 19:58:29]

Hi Ben, please review or suggest suitable reviewer.  In particular, I'm not sure
of all of the implications here.  Thanks.

[Tom Sepez, 2012-08-28 17:00:07]

Adding Charlie since it said "navigation".

[Charlie Reis, 2012-08-28 20:06:16]

This needs a test in navigation_controller_impl_unittest.cc.

I'm also finding it tricky to follow the logic (e.g., why patch set 2 didn't
work) and don't have a deep knowledge of replaceState.  Is there no way for us
to tell whether it's a pushState/replaceState event?  I'm wondering if Brett
knows this logic better than I do.

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:118: // scenarios can
be differentiated with the TransitionType.
Is this going to be a problem for the original bug?  For example, if a page
navigates to #foo and then does history.back(), would you treat it as a new
navigation and lose the page-action icon?

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.h (right):

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.h:166: return
IsURLInPageNavigation(url, false);
Isn't overloading usually frowned upon in Chrome?

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.h:171: // we need this
form which lets this the (untrustworthy) renderer resolve the
nit: which lets the

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.h:172: // ambiguity, but
only when the URLs are equal. This should be safe since the
maybe: "URLs are equal (apart from refs)"?

[Tom Sepez, 2012-08-28 20:17:41]

Ok.  Glad to add a test.

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:118: // scenarios can
be differentiated with the TransitionType.
Yep.  And my original patch tried to address this, but disrupted too many tests.

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.h (right):

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.h:166: return
IsURLInPageNavigation(url, false);
On 2012/08/28 20:06:16, creis wrote:
> Isn't overloading usually frowned upon in Chrome?
Yep, but it avoids having to write a comment about "if you don't know what to
pass here because you don't have a suggestion from the renderer, pass false".

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.h:171: // we need this
form which lets this the (untrustworthy) renderer resolve the
On 2012/08/28 20:06:16, creis wrote:
> nit: which lets the

Done.

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.h:172: // ambiguity, but
only when the URLs are equal. This should be safe since the
On 2012/08/28 20:06:16, creis wrote:
> maybe: "URLs are equal (apart from refs)"?
Current impl is strict equality.

[Ben Goodger (Google), 2012-08-28 21:21:55]

I'm not a good reviewer for this stuff.

[Tom Sepez, 2012-08-28 21:23:15]

Hey Brett,  do you have insight here?  thanks.

[Charlie Reis, 2012-08-28 21:48:21]

Thanks for the test!  It shows that I must be missing something.  :)

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:118: // scenarios can
be differentiated with the TransitionType.
On 2012/08/28 20:17:41, Tom Sepez wrote:
> Yep.  And my original patch tried to address this, but disrupted too many
tests.

What's the plan for fixing that case?  Do we need a more specific TODO to get
back to it in a followup CL?

http://codereview.chromium.org/10871090/diff/14002/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10871090/diff/14002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl_unittest.cc:1438: //
Distinguish reload vs. history.replaceState() of same url.
This comment is confusing, since you don't test reload here.  Maybe rephrase as
"Ensure history.replaceState() of the same URL is not treated as a reload"?

http://codereview.chromium.org/10871090/diff/14002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl_unittest.cc:1449: //
This should generate a new entry.
Again, this is tricky to follow because there's a difference between generating
a new entry and whether it's considered in-page.  Maybe "This should generate a
new in-page entry" would be clearer.

Either way, this seems backwards to me.  replaceState is supposed to replace the
current entry, so I would not expect this to generate a new entry.  In contrast,
fragment navigations (below) seem like they should generate a new entry (as we
used to verify), and I don't understand why that changed.

What am I missing?

[Tom Sepez, 2012-08-28 21:57:59]

thanks.  new comments.

http://codereview.chromium.org/10871090/diff/14002/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10871090/diff/14002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl_unittest.cc:1438: //
Distinguish reload vs. history.replaceState() of same url.
On 2012/08/28 21:48:21, creis wrote:
> This comment is confusing, since you don't test reload here.  Maybe rephrase
as
> "Ensure history.replaceState() of the same URL is not treated as a reload"?

Done.

http://codereview.chromium.org/10871090/diff/14002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl_unittest.cc:1449: //
This should generate a new entry.
On 2012/08/28 21:48:21, creis wrote:
> Again, this is tricky to follow because there's a difference between
generating
> a new entry and whether it's considered in-page.  Maybe "This should generate
a
> new in-page entry" would be clearer.
> 
> Either way, this seems backwards to me.  replaceState is supposed to replace
the
> current entry, so I would not expect this to generate a new entry.  In
contrast,
> fragment navigations (below) seem like they should generate a new entry (as we
> used to verify), and I don't understand why that changed.
> 
> What am I missing?

This appears to be an artifact of the interaction between
test_rvh()->SendNavigate() and controller.RendererDidNavigate().  The first such
controller.RendererDidNavigate() call is being treated as a new page, but the
subsequent ones have the context to return the right result.

[Charlie Reis, 2012-08-28 22:17:23]

Thanks.

Please also see my question about the remaining bug:
http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...

http://codereview.chromium.org/10871090/diff/14002/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10871090/diff/14002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl_unittest.cc:1449: //
This should generate a new entry.
On 2012/08/28 21:57:59, Tom Sepez wrote:
> On 2012/08/28 21:48:21, creis wrote:
> > Again, this is tricky to follow because there's a difference between
> generating
> > a new entry and whether it's considered in-page.  Maybe "This should
generate
> a
> > new in-page entry" would be clearer.
> > 
> > Either way, this seems backwards to me.  replaceState is supposed to replace
> the
> > current entry, so I would not expect this to generate a new entry.  In
> contrast,
> > fragment navigations (below) seem like they should generate a new entry (as
we
> > used to verify), and I don't understand why that changed.
> > 
> > What am I missing?
> 
> This appears to be an artifact of the interaction between
> test_rvh()->SendNavigate() and controller.RendererDidNavigate().  The first
such
> controller.RendererDidNavigate() call is being treated as a new page, but the
> subsequent ones have the context to return the right result.  

Can we just call RendererDidNavigate() on the initial "main page" navigation, to
avoid this strange interaction?  It seems to be clouding the meaning of
replaceState.

(I also still think fragment navigations are supposed to generate new entries,
since you can go back/forward to them.)

[Tom Sepez, 2012-08-28 22:46:57]

One other thing I just noticed is that the use of the page_id is inconsistent in
this test between sendNavigate(0, ...) and params.page_id = 1.  So I need to dig
deeper to find out why they'd want to do it that way.  If they intended that at
all.

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10871090/diff/11001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:118: // scenarios can
be differentiated with the TransitionType.
On 2012/08/28 21:48:21, creis wrote:
> On 2012/08/28 20:17:41, Tom Sepez wrote:
> > Yep.  And my original patch tried to address this, but disrupted too many
> tests.
> 
> What's the plan for fixing that case?  Do we need a more specific TODO to get
> back to it in a followup CL?
I'd assumed it was covered by the existing TODO().  This doesn't introduce new
TODO items.

[Tom Sepez, 2012-08-28 22:57:08]

New comments about sub-frames vs main page.  The subtle page id issue.

[Tom Sepez, 2012-08-28 23:26:39]

Sub-frame comment was innacurate.  It's just a matter of page ids.

[Charlie Reis, 2012-08-29 23:16:20]

I'd like to resolve the strangeness in this CL if possible.  However, since I'll
soon be OOO, I'm ok with fixing the main issue (or at least, the part of it not
described by jcampan's TODO comment) first and following up with a fix for the
did_replace_entry issue.  So, conditional LGTM.

http://codereview.chromium.org/10871090/diff/17003/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10871090/diff/17003/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl_unittest.cc:1455:
EXPECT_FALSE(details.did_replace_entry);
This is the part that seems broken to me.  If this is accurately simulating a
replaceState navigation, then I would think it should be EXPECT_TRUE.

Sounds like you're making progress on investigating it.  If you can manually
confirm that replaceState still replaces the entry and doesn't create a new one
after your change, then I'm ok with adding a TODO with a bug number to fix this
issue in a followup CL.

[commit-bot: I haz the power, 2012-08-30 18:11:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10871090/24001

[commit-bot: I haz the power, 2012-08-30 22:31:15]

Change committed as 154287