Do not create pending NavigationEntry when a transient one is present.

Do not create pending NavigationEntry when a transient one is present.

When an interestitial page is displayed, network requests from the existing
RenderFrameHost are blocked. However, this still allows the renderer to
send a DidStartProvisionalLoad IPC. This CL adds check whether when
processing the IPC a transient entry is present and skips creating a
pending entry. Otherwise the pending entry creation deletes the transient
entry and leades to URL spoof.

BUG=600046
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[nasko, 2016-04-12 21:49:11]

Description was changed from

==========
Do not create pending NavigationEntry when a transient one is present.

When an interestitial page is displayed, network requests from the existing
RenderFrameHost are blocked. However, this still allows the renderer to
send a DidStartProvisionalLoad IPC. This CL adds check whether when
processing the IPC a transient entry is present and skips creating a
pending entry. Otherwise the pending entry creation deletes the transient
entry and leades to URL spoof.

BUG=600046
==========

to

==========
Do not create pending NavigationEntry when a transient one is present.

When an interestitial page is displayed, network requests from the existing
RenderFrameHost are blocked. However, this still allows the renderer to
send a DidStartProvisionalLoad IPC. This CL adds check whether when
processing the IPC a transient entry is present and skips creating a
pending entry. Otherwise the pending entry creation deletes the transient
entry and leades to URL spoof.

BUG=600046
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-04-12 21:52:44]

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

[nasko, 2016-04-12 21:52:44]

Hey Charlie,
Can you review this CL for me? It fixes the URL spoof bug and creates a new test
file in chrome/ where we can host navigation related tests.
Thanks in advance!
Nasko

[Charlie Reis, 2016-04-13 06:03:37]

Great!  I'm glad you were able to get a test to work.  Mostly nits, and one
question about the fix that we should think about.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
File chrome/browser/chrome_navigation_browsertest.cc (right):

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:34: net::EmbeddedTestServer*
https_server_expired() {
nit: expired_https_server

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:71: // process is trying to
navigate.
Let's list the bug number here.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:78:
embedded_test_server()->GetURL("/window_open_and_navigate.html");
Looks like you forgot to add this file, which probably explains the test
failures?

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:103:
EXPECT_EQ(new_web_contents->GetLastCommittedURL(), initial_url);
nit: Reverse order (expected, actual), here and below.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:123: nav_observer.Wait();
Sanity check: We're not depending on this returning before some later event
happens, right?  I made that mistake in some of my tests and they were flaky,
since the message loop might not quit until more events happen.

Hopefully this is just using DidStartNavigationObserver because that's the last
thing we could observe.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:125: 
nit: No blank line here (or move the EXPECTs into the nested braces), since the
comment describing them is in the block above.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:126:
EXPECT_EQ(new_web_contents->GetVisibleURL(), error_url);
Can you add an EXPECT for new_web_contents->GetController().GetTransientEntry()?

https://codereview.chromium.org/1881023003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1881023003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1031: bool has_transient_entry =
!!controller_->GetTransientEntry();
I still think this is correct, but I just want to make sure we're doing the
right thing.

Suppose URL A causes an interstitial in a new tab, and it has a
renderer-initiated pending entry.  Then renderer-initiated URL B gets here.  We
skip the block below because there's an interstitial, so we still have the
pending entry for A.  If the interstitial gets dismissed, the renderer-initiated
pending entry could be displayed, and it would be for A rather than B?  (Or
maybe we'd get a DidFail and cancel it?)

I'm mainly wondering if we need to discard the pending entry (and not the
transient entry) in this case, or if just skipping the block below is
sufficient.

[Charlie Reis, 2016-04-13 17:27:22]

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/OWNERS
File chrome/browser/OWNERS (right):

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/OWNERS#n...
chrome/browser/OWNERS:24: per-file
chrome_navigation_browsertest.cc=nasko@chromium.org
Can you add a similar set of lines for
site_per_process_interactive_browsertest.cc?  I noticed that one in Alex's
https://codereview.chromium.org/1881863003/, and we're probably sufficient
owners for that as well.  (Maybe also Nick for both?)

[nasko, 2016-04-13 18:45:30]

This PS addresses all the comments but one. I will do more follow up to ensure
correct behavior when multiple navigations are initiated during the
interstitial.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/OWNERS
File chrome/browser/OWNERS (right):

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/OWNERS#n...
chrome/browser/OWNERS:24: per-file
chrome_navigation_browsertest.cc=nasko@chromium.org
On 2016/04/13 17:27:22, Charlie Reis wrote:
> Can you add a similar set of lines for
> site_per_process_interactive_browsertest.cc?  I noticed that one in Alex's
> https://codereview.chromium.org/1881863003/, and we're probably sufficient
> owners for that as well.  (Maybe also Nick for both?)

Done.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
File chrome/browser/chrome_navigation_browsertest.cc (right):

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:34: net::EmbeddedTestServer*
https_server_expired() {
On 2016/04/13 06:03:37, Charlie Reis wrote:
> nit: expired_https_server

Done.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:71: // process is trying to
navigate.
On 2016/04/13 06:03:37, Charlie Reis wrote:
> Let's list the bug number here.

Done.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:78:
embedded_test_server()->GetURL("/window_open_and_navigate.html");
On 2016/04/13 06:03:37, Charlie Reis wrote:
> Looks like you forgot to add this file, which probably explains the test
> failures?

Yeah, noticed later, but didn't have a chance to upload a new patch. The last PS
should have it.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:103:
EXPECT_EQ(new_web_contents->GetLastCommittedURL(), initial_url);
On 2016/04/13 06:03:37, Charlie Reis wrote:
> nit: Reverse order (expected, actual), here and below.

Done.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:123: nav_observer.Wait();
On 2016/04/13 06:03:37, Charlie Reis wrote:
> Sanity check: We're not depending on this returning before some later event
> happens, right?  I made that mistake in some of my tests and they were flaky,
> since the message loop might not quit until more events happen.

No dependency on any other events. I did also discover this behavior when I was
reviewing one of Ken's CLs, so I'm acutely aware now :). Thanks for pointing it
out!

> Hopefully this is just using DidStartNavigationObserver because that's the
last
> thing we could observe.

Yes, I need to wait to see the DidStartProvisionalLoad from the navigation.
Because we block the network requests while the interstitial is showing, there
are no other navigation events that will happen from the same RFH. It is also
the reason I couldn't just use TestNavigationObserver, as it expects to see a
commit/end of navigation, which can't happen here.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:125: 
On 2016/04/13 06:03:37, Charlie Reis wrote:
> nit: No blank line here (or move the EXPECTs into the nested braces), since
the
> comment describing them is in the block above.

Done.

https://codereview.chromium.org/1881023003/diff/40001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:126:
EXPECT_EQ(new_web_contents->GetVisibleURL(), error_url);
On 2016/04/13 06:03:37, Charlie Reis wrote:
> Can you add an EXPECT for
new_web_contents->GetController().GetTransientEntry()?

Done.

[Charlie Reis, 2016-04-14 23:02:45]

Thanks, LGTM!

https://codereview.chromium.org/1881023003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1881023003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1031: bool has_transient_entry =
!!controller_->GetTransientEntry();
On 2016/04/13 06:03:37, Charlie Reis wrote:
> I still think this is correct, but I just want to make sure we're doing the
> right thing.
> 
> Suppose URL A causes an interstitial in a new tab, and it has a
> renderer-initiated pending entry.  Then renderer-initiated URL B gets here. 
We
> skip the block below because there's an interstitial, so we still have the
> pending entry for A.  If the interstitial gets dismissed, the
renderer-initiated
> pending entry could be displayed, and it would be for A rather than B?  (Or
> maybe we'd get a DidFail and cancel it?)
> 
> I'm mainly wondering if we need to discard the pending entry (and not the
> transient entry) in this case, or if just skipping the block below is
> sufficient.

From your email, it sounds like we'll hang if we proceed after trying to
navigate to B.  I'm not too concerned, then.

[nasko, 2016-04-15 02:17:19]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2016-04-15 02:17:32]

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

[commit-bot: I haz the power, 2016-04-15 02:27:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-15 02:27:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nasko, 2016-04-15 03:10:21]

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

[nasko, 2016-04-15 03:10:22]

Hey Lei,
Can you do an OWNERS review of chrome/ for me?
Thanks in advance!
Nasko

[Lei Zhang, 2016-04-15 04:50:38]

lgtm

https://codereview.chromium.org/1881023003/diff/60001/chrome/browser/OWNERS
File chrome/browser/OWNERS (right):

https://codereview.chromium.org/1881023003/diff/60001/chrome/browser/OWNERS#n...
chrome/browser/OWNERS:23: per-file
chrome_navigation_browsertest.cc=alexmos@chromium.org
sort - elf before navigation

https://codereview.chromium.org/1881023003/diff/60001/chrome/browser/chrome_n...
File chrome/browser/chrome_navigation_browsertest.cc (right):

https://codereview.chromium.org/1881023003/diff/60001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:85: content::WebContents*
new_web_contents = nullptr;
declare where first used

[nasko, 2016-04-15 05:14:46]

https://codereview.chromium.org/1881023003/diff/60001/chrome/browser/OWNERS
File chrome/browser/OWNERS (right):

https://codereview.chromium.org/1881023003/diff/60001/chrome/browser/OWNERS#n...
chrome/browser/OWNERS:23: per-file
chrome_navigation_browsertest.cc=alexmos@chromium.org
On 2016/04/15 04:50:38, Lei Zhang wrote:
> sort - elf before navigation

Done.

https://codereview.chromium.org/1881023003/diff/60001/chrome/browser/chrome_n...
File chrome/browser/chrome_navigation_browsertest.cc (right):

https://codereview.chromium.org/1881023003/diff/60001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:85: content::WebContents*
new_web_contents = nullptr;
On 2016/04/15 04:50:38, Lei Zhang wrote:
> declare where first used

Done.

[nasko, 2016-04-15 05:14:50]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-04-15 05:14:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1881023003/#ps80001
(title: "Fixes for review by Lei.")

[commit-bot: I haz the power, 2016-04-15 05:15:02]

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

[commit-bot: I haz the power, 2016-04-15 07:09:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-15 07:10:47]

Description was changed from

==========
Do not create pending NavigationEntry when a transient one is present.

When an interestitial page is displayed, network requests from the existing
RenderFrameHost are blocked. However, this still allows the renderer to
send a DidStartProvisionalLoad IPC. This CL adds check whether when
processing the IPC a transient entry is present and skips creating a
pending entry. Otherwise the pending entry creation deletes the transient
entry and leades to URL spoof.

BUG=600046
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Do not create pending NavigationEntry when a transient one is present.

When an interestitial page is displayed, network requests from the existing
RenderFrameHost are blocked. However, this still allows the renderer to
send a DidStartProvisionalLoad IPC. This CL adds check whether when
processing the IPC a transient entry is present and skips creating a
pending entry. Otherwise the pending entry creation deletes the transient
entry and leades to URL spoof.

BUG=600046
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-04-15 07:10:48]

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