Fix race when reloading original URL

Fix race when reloading original URL.

Fixes an issue where the browser's last committed entry and the renderer can become out of sync when reloading the original requested URL, which causes the browser to kill the renderer process.

BUG=491861
TBR=jam@chromium.org for chromium.fyi.json

Committed: https://crrev.com/573e8be6d639b7066a270ae99d02e829e10b90dd
Cr-Commit-Position: refs/heads/master@{#338864}

[lfg, 2015-06-22 22:15:51]

lfg@chromium.org changed reviewers:
+ avi@chromium.org

[lfg, 2015-06-22 22:15:52]

Hi Avi, please take a look.

[Charlie Reis, 2015-07-06 20:56:38]

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

[Charlie Reis, 2015-07-06 20:56:39]

I'm not sure about the cause of the crash in the browsertest yet, but here's a
few other comments about the change and test.

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

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2155: for
(int i = 0; i < 1000; i++) {
This doesn't seem like a great strategy for a test.  The loop isn't guaranteed
to test different schedules, so even if the bug exists we'll only fail if we're
lucky.

Can we structure the test so that it deterministically hits the failure without
your fix, without looping?

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2179:
EXPECT_TRUE(shell()->web_contents()->IsLoading());
It seems like we should be able to verify that the last committed entry has the
modified URL and not the original URL at this point.

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2180: //
Wait until there's no more navigations.
nit: Blank line above.

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:457: url =
GetOriginalRequestURL();
Switching the URL here is clever, but it causes problems for WebContentsObserver
methods like DidStartNavigationToPendingEntry, which report that a navigation to
the entry's URL (not the original URL) was started.

That's the reason the ReloadOriginalRequestURL unit test is failing.

I definitely agree that we shouldn't change the NavigationEntry's URL (since it
also refers to the last committed page), but maybe there's a way to get the
observer notifications correct.

[lfg, 2015-07-08 22:09:29]

Please take another look.

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

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2155: for
(int i = 0; i < 1000; i++) {
On 2015/07/06 20:56:39, Charlie Reis wrote:
> This doesn't seem like a great strategy for a test.  The loop isn't guaranteed
> to test different schedules, so even if the bug exists we'll only fail if
we're
> lucky.
> 
> Can we structure the test so that it deterministically hits the failure
without
> your fix, without looping?

Sorry, this was just a test because I wasn't sure why the test was flaky. Turned
out there was a pending message in the message loop that wasn't being handled.

I've changed the strategy a bit, now I'm sending the ExecuteScript, but not
waiting for it, so I don't need to rely on setTimeout anymore. This should make
the test deterministic.

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2179:
EXPECT_TRUE(shell()->web_contents()->IsLoading());
On 2015/07/06 20:56:39, Charlie Reis wrote:
> It seems like we should be able to verify that the last committed entry has
the
> modified URL and not the original URL at this point.

We can't, because we're not running the message loop, we don't know if the
renderer has executed the javascript or not. I've added a check at the end to
make sure we end up in the original URL.

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2180: //
Wait until there's no more navigations.
On 2015/07/06 20:56:39, Charlie Reis wrote:
> nit: Blank line above.

Done.

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:457: url =
GetOriginalRequestURL();
On 2015/07/06 20:56:39, Charlie Reis wrote:
> Switching the URL here is clever, but it causes problems for
WebContentsObserver
> methods like DidStartNavigationToPendingEntry, which report that a navigation
to
> the entry's URL (not the original URL) was started.
> 
> That's the reason the ReloadOriginalRequestURL unit test is failing.
> 
> I definitely agree that we shouldn't change the NavigationEntry's URL (since
it
> also refers to the last committed page), but maybe there's a way to get the
> observer notifications correct.

That seems to be the only issue, so I fixed just that specific case. Cloning is
also not an option, since we don't want a new entry in the session history.

Correct me if I'm wrong, but I think there are no more observable effects
outside of the navigator.

[Charlie Reis, 2015-07-08 23:23:38]

Thanks.  I'm still wary about using two different URLs throughout
NavigateToEntry, though.  Some thoughts below about how we can fix that.

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

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2179:
EXPECT_TRUE(shell()->web_contents()->IsLoading());
On 2015/07/08 22:09:28, lfg wrote:
> On 2015/07/06 20:56:39, Charlie Reis wrote:
> > It seems like we should be able to verify that the last committed entry has
> the
> > modified URL and not the original URL at this point.
> 
> We can't, because we're not running the message loop, we don't know if the
> renderer has executed the javascript or not. I've added a check at the end to
> make sure we end up in the original URL.

Hmm, ok.  It's unfortunate, since that's the part we'd really like to prevent
regression on, but I don't see a great way to test it.

(Maybe a StallDelegate on the original URL that lets us check state before
allowing the navigation to commit?)

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2179:
->ExecuteJavaScriptWithUserGestureForTests(base::UTF8ToUTF16(script));
Please add a comment saying why this has to be done this way (i.e., what
sequence of events do you need for the test to be meaningful?).

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2185: while
(shell()->web_contents()->GetLastCommittedURL() != url)
Does replaceState generate a load stop?  I'm wondering if this would be cleaner
with WaitForLoadStop and then an expectation that we're on |url|.  (I suppose we
can do this loop if needed, but we should have more of an explanation why it's
necessary if so.)

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:236: if
(frame_entry.url().spec().size() > GetMaxURLChars()) {
There's a lot of frame_entry.url() uses in here that are all wrong in this case.

Can we move the reloadOriginalURL check from ConstructCommonNavigationParams to
the top of this method and create a dest_url and dest_referrer to use below? 
We'll have to pass them to RequestNavigation and
ConstructCommonNavigationParams, and they'll trump whatever is in frame_entry.

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:254: navigation_start,
frame_entry.url(), entry.restore_type()));
dest_url

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:261:
frame_tree_node->navigation_request()->common_params().url,
dest_url

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:279: frame_entry.url());
dest_url

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:324:
frame_entry.url().SchemeIs(url::kJavaScriptScheme)) {
dest_url

(Not sure if javascript: URLs can actually be in redirect chains, but we should
be consistent here.)

[lfg, 2015-07-14 18:58:31]

Another round of changes, please take a look.

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

https://codereview.chromium.org/1189413005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2179:
EXPECT_TRUE(shell()->web_contents()->IsLoading());
On 2015/07/08 23:23:37, Charlie Reis wrote:
> Hmm, ok.  It's unfortunate, since that's the part we'd really like to prevent
> regression on, but I don't see a great way to test it.
> 
> (Maybe a StallDelegate on the original URL that lets us check state before
> allowing the navigation to commit?)

I've changed the test to look at the commits instead. We are only indirectly
testing the last committed url, but it may be enough, what do you think?

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2179:
->ExecuteJavaScriptWithUserGestureForTests(base::UTF8ToUTF16(script));
On 2015/07/08 23:23:37, Charlie Reis wrote:
> Please add a comment saying why this has to be done this way (i.e., what
> sequence of events do you need for the test to be meaningful?).

Done.

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2185: while
(shell()->web_contents()->GetLastCommittedURL() != url)
On 2015/07/08 23:23:37, Charlie Reis wrote:
> Does replaceState generate a load stop?  I'm wondering if this would be
cleaner
> with WaitForLoadStop and then an expectation that we're on |url|.  (I suppose
we
> can do this loop if needed, but we should have more of an explanation why it's
> necessary if so.)

It only generates a load stop when IsLoading() is true, so in this case we only
get 1 loadstop, but 2 commits.

I've cleaned it up by waiting on the commits instead.

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:236: if
(frame_entry.url().spec().size() > GetMaxURLChars()) {
On 2015/07/08 23:23:37, Charlie Reis wrote:
> There's a lot of frame_entry.url() uses in here that are all wrong in this
case.
> 
> Can we move the reloadOriginalURL check from ConstructCommonNavigationParams
to
> the top of this method and create a dest_url and dest_referrer to use below? 
> We'll have to pass them to RequestNavigation and
> ConstructCommonNavigationParams, and they'll trump whatever is in frame_entry.

Done.

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:254: navigation_start,
frame_entry.url(), entry.restore_type()));
On 2015/07/08 23:23:37, Charlie Reis wrote:
> dest_url

Done.

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:261:
frame_tree_node->navigation_request()->common_params().url,
On 2015/07/08 23:23:37, Charlie Reis wrote:
> dest_url

Done.

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:279: frame_entry.url());
On 2015/07/08 23:23:37, Charlie Reis wrote:
> dest_url

Done.

https://codereview.chromium.org/1189413005/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:324:
frame_entry.url().SchemeIs(url::kJavaScriptScheme)) {
On 2015/07/08 23:23:37, Charlie Reis wrote:
> dest_url
> 
> (Not sure if javascript: URLs can actually be in redirect chains, but we
should
> be consistent here.)

Done.

[Charlie Reis, 2015-07-14 22:23:10]

THanks for the updates!  LGTM with nits.

You can disable the test on the Site Isolation bots in chromium.fyi.json until
my fix for https://crbug.com/317872 lands.  I'll re-enable it after that.  (Just
TBR jam@ for the JSON file change.)

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2148: //
This tests for a race in ReloadOriginalRequest. Not crashing means that the
nit: This tests a race in ReloadOriginalRequest, where a cross-origin reload was
causing an in-flight replaceState to look like a cross-origin navigation, even
though it's in-page.  (The reload should not modify the underlying last
committed entry.)  Not crashing...

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2151: GURL
url(embedded_test_server()->GetURL(
nit: original_url

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2159: //
Redirect.
nit: Redirect so that we can use ReloadOriginalRequest.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2160: GURL
foo_url(embedded_test_server()->GetURL(
nit: redirect_url

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2175: //
renderer to send back a DidCommitProvisionalLoad. Immediatelly after,
nit: Immediately

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2186:
EXPECT_TRUE(shell()->web_contents()->IsLoading());
Also add:
EXPECT_EQ(redirect_url, shell()->web_contents()->GetLastCommittedURL());

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2198:
EXPECT_EQ(url, shell()->web_contents()->GetLastCommittedURL());
Yes, I like this approach.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:149: const Referrer&
dest_referrer,
nit: List these before frame_entry since they take precedence over it.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:62: const Referrer&
dest_referrer,
List these before frame_entry.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:119: const Referrer& dest_referrer,
List these before frame_entry.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:297: const GURL&
dest_url,
nit: List this first, and mention in the comment that dest_url trumps
frame_entry.url() (and why).

[lfg, 2015-07-14 22:47:28]

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2148: //
This tests for a race in ReloadOriginalRequest. Not crashing means that the
On 2015/07/14 22:23:10, Charlie Reis wrote:
> nit: This tests a race in ReloadOriginalRequest, where a cross-origin reload
was
> causing an in-flight replaceState to look like a cross-origin navigation, even
> though it's in-page.  (The reload should not modify the underlying last
> committed entry.)  Not crashing...

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2151: GURL
url(embedded_test_server()->GetURL(
On 2015/07/14 22:23:10, Charlie Reis wrote:
> nit: original_url

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2159: //
Redirect.
On 2015/07/14 22:23:10, Charlie Reis wrote:
> nit: Redirect so that we can use ReloadOriginalRequest.

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2160: GURL
foo_url(embedded_test_server()->GetURL(
On 2015/07/14 22:23:10, Charlie Reis wrote:
> nit: redirect_url

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2175: //
renderer to send back a DidCommitProvisionalLoad. Immediatelly after,
On 2015/07/14 22:23:10, Charlie Reis wrote:
> nit: Immediately

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2186:
EXPECT_TRUE(shell()->web_contents()->IsLoading());
On 2015/07/14 22:23:10, Charlie Reis wrote:
> Also add:
> EXPECT_EQ(redirect_url, shell()->web_contents()->GetLastCommittedURL());

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2198:
EXPECT_EQ(url, shell()->web_contents()->GetLastCommittedURL());
On 2015/07/14 22:23:10, Charlie Reis wrote:
> Yes, I like this approach.

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:149: const Referrer&
dest_referrer,
On 2015/07/14 22:23:10, Charlie Reis wrote:
> nit: List these before frame_entry since they take precedence over it.

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:62: const Referrer&
dest_referrer,
On 2015/07/14 22:23:10, Charlie Reis wrote:
> List these before frame_entry.

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:119: const Referrer& dest_referrer,
On 2015/07/14 22:23:10, Charlie Reis wrote:
> List these before frame_entry.

Done.

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1189413005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:297: const GURL&
dest_url,
On 2015/07/14 22:23:10, Charlie Reis wrote:
> nit: List this first, and mention in the comment that dest_url trumps
> frame_entry.url() (and why).

Done.

[Charlie Reis, 2015-07-14 23:02:24]

LGTM with nit.

One possible concern: I'm worried about other parts of the NavigationEntry that
come from the redirect_url but are used with the original_url, especially the
SiteInstance.  Most client redirects aren't cross-process (yet), but I would
guess that we would load the original_url in the redirect_url's SiteInstance the
way things stand.  That's bad.  That's probably true before this CL lands as
well.

We may want to land this to get the bug fixed in Chrome (where cross-process
redirects are rare) and then revisit that issue separately.

https://codereview.chromium.org/1189413005/diff/200001/testing/buildbot/chrom...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/1189413005/diff/200001/testing/buildbot/chrom...
testing/buildbot/chromium.fyi.json:4030:
"--gtest_filter=-*.PreventSpoofFromSubframeAndReplace:SessionHistoryTest.CrossFrameFormBackForward:SessionHistoryTest.FrameBackForward:*.SupportCrossProcessPostMessageWithMessagePort:DevToolsProtocolTest.NavigationPreservesMessages"
You need to add it here as well.

[lfg, 2015-07-14 23:11:30]

I've created bug 510251 so we don't forget to revisit this issue.

https://codereview.chromium.org/1189413005/diff/200001/testing/buildbot/chrom...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/1189413005/diff/200001/testing/buildbot/chrom...
testing/buildbot/chromium.fyi.json:4030:
"--gtest_filter=-*.PreventSpoofFromSubframeAndReplace:SessionHistoryTest.CrossFrameFormBackForward:SessionHistoryTest.FrameBackForward:*.SupportCrossProcessPostMessageWithMessagePort:DevToolsProtocolTest.NavigationPreservesMessages"
On 2015/07/14 23:02:24, Charlie Reis wrote:
> You need to add it here as well.

Done.

[lfg, 2015-07-15 15:30:54]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-07-15 15:30:54]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1189413005/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-07-15 15:31:12]

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

[commit-bot: I haz the power, 2015-07-15 16:00:22]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-07-15 16:01:18]

Patchset 12 (id:??) landed as
https://crrev.com/573e8be6d639b7066a270ae99d02e829e10b90dd
Cr-Commit-Position: refs/heads/master@{#338864}