Don't leave aborted URLs in the omnibox unless we're on a new tab.

Don't leave aborted URLs in the omnibox unless we're on the NTP.

BUG=355537
TEST=See bug for repro steps.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265044

[Charlie Reis, 2014-04-11 22:28:01]

John, see comment 17 on the bug for the rationale here.  Hope it's ok to ask the
embedder for its notion of the NTP via ContentBrowserClient.

[jam, 2014-04-11 22:36:57]

On 2014/04/11 22:28:01, Charlie Reis wrote:
> John, see comment 17 on the bug for the rationale here.  Hope it's ok to ask
the
> embedder for its notion of the NTP via ContentBrowserClient.

NTP is a chrome concept, so we shouldn't put this in the content api.

[Charlie Reis, 2014-04-11 22:44:25]

On 2014/04/11 22:36:57, jam wrote:
> On 2014/04/11 22:28:01, Charlie Reis wrote:
> > John, see comment 17 on the bug for the rationale here.  Hope it's ok to ask
> the
> > embedder for its notion of the NTP via ContentBrowserClient.
> 
> NTP is a chrome concept, so we shouldn't put this in the content api.

How about something like ShouldPreserveAbortedURLs(const GURL&
last_committed_url)?  We can implement that as an NTP check in chrome-land.

[jam, 2014-04-11 23:25:48]

On 2014/04/11 22:44:25, Charlie Reis wrote:
> On 2014/04/11 22:36:57, jam wrote:
> > On 2014/04/11 22:28:01, Charlie Reis wrote:
> > > John, see comment 17 on the bug for the rationale here.  Hope it's ok to
ask
> > the
> > > embedder for its notion of the NTP via ContentBrowserClient.
> > 
> > NTP is a chrome concept, so we shouldn't put this in the content api.
> 
> How about something like ShouldPreserveAbortedURLs(const GURL&
> last_committed_url)?  We can implement that as an NTP check in chrome-land.

i had thought about that, but it seemed like obfuscating a chrome concept in the
content api :)

let me understand this better: when you preserve this aborted url, is that for
the download case (i.e. the linked bug), or is it for the case when a user hits
escape before the url loads? or if the url is incorrect?

[Charlie Reis, 2014-04-11 23:31:21]

On 2014/04/11 23:25:48, jam wrote:
> On 2014/04/11 22:44:25, Charlie Reis wrote:
> > On 2014/04/11 22:36:57, jam wrote:
> > > On 2014/04/11 22:28:01, Charlie Reis wrote:
> > > > John, see comment 17 on the bug for the rationale here.  Hope it's ok to
> ask
> > > the
> > > > embedder for its notion of the NTP via ContentBrowserClient.
> > > 
> > > NTP is a chrome concept, so we shouldn't put this in the content api.
> > 
> > How about something like ShouldPreserveAbortedURLs(const GURL&
> > last_committed_url)?  We can implement that as an NTP check in chrome-land.
> 
> i had thought about that, but it seemed like obfuscating a chrome concept in
the
> content api :)
> 
> let me understand this better: when you preserve this aborted url, is that for
> the download case (i.e. the linked bug), or is it for the case when a user
hits
> escape before the url loads? or if the url is incorrect?

We can't currently distinguish between downloads, user hitting stop, invalid
URLs, and 204 responses.  All of those produce identical net::ERR_ABORTED
notifications, and this continues to treat them the same.

Peter suggested we adopt Firefox's behavior of leaving all of them around in new
tabs and clearing them otherwise, which (IMHO) is a bit saner and much easier
than distinguishing the cases in the network stack.

[jam, 2014-04-11 23:42:33]

On 2014/04/11 23:31:21, Charlie Reis wrote:
> On 2014/04/11 23:25:48, jam wrote:
> > On 2014/04/11 22:44:25, Charlie Reis wrote:
> > > On 2014/04/11 22:36:57, jam wrote:
> > > > On 2014/04/11 22:28:01, Charlie Reis wrote:
> > > > > John, see comment 17 on the bug for the rationale here.  Hope it's ok
to
> > ask
> > > > the
> > > > > embedder for its notion of the NTP via ContentBrowserClient.
> > > > 
> > > > NTP is a chrome concept, so we shouldn't put this in the content api.
> > > 
> > > How about something like ShouldPreserveAbortedURLs(const GURL&
> > > last_committed_url)?  We can implement that as an NTP check in
chrome-land.
> > 
> > i had thought about that, but it seemed like obfuscating a chrome concept in
> the
> > content api :)
> > 
> > let me understand this better: when you preserve this aborted url, is that
for
> > the download case (i.e. the linked bug), or is it for the case when a user
> hits
> > escape before the url loads? or if the url is incorrect?
> 
> We can't currently distinguish between downloads, user hitting stop, invalid
> URLs, and 204 responses.  All of those produce identical net::ERR_ABORTED
> notifications, and this continues to treat them the same.
> 
> Peter suggested we adopt Firefox's behavior of leaving all of them around in
new
> tabs and clearing them otherwise, which (IMHO) is a bit saner and much easier
> than distinguishing the cases in the network stack.

thanks for the explanation. so as i understand it, what's safe is that we only
leave the failed url around if the user is not on an attacker-controllable page.
NTP happens to be not attacker-controllable. but really, it's because it's
implemented using webui, none of which is attacker-controllable. so can we do
this for all webui pages? this would remove the need to add yet another method
to CBC

[Charlie Reis, 2014-04-11 23:45:08]

On 2014/04/11 23:42:33, jam wrote:
> On 2014/04/11 23:31:21, Charlie Reis wrote:
> > On 2014/04/11 23:25:48, jam wrote:
> > > On 2014/04/11 22:44:25, Charlie Reis wrote:
> > > > On 2014/04/11 22:36:57, jam wrote:
> > > > > On 2014/04/11 22:28:01, Charlie Reis wrote:
> > > > > > John, see comment 17 on the bug for the rationale here.  Hope it's
ok
> to
> > > ask
> > > > > the
> > > > > > embedder for its notion of the NTP via ContentBrowserClient.
> > > > > 
> > > > > NTP is a chrome concept, so we shouldn't put this in the content api.
> > > > 
> > > > How about something like ShouldPreserveAbortedURLs(const GURL&
> > > > last_committed_url)?  We can implement that as an NTP check in
> chrome-land.
> > > 
> > > i had thought about that, but it seemed like obfuscating a chrome concept
in
> > the
> > > content api :)
> > > 
> > > let me understand this better: when you preserve this aborted url, is that
> for
> > > the download case (i.e. the linked bug), or is it for the case when a user
> > hits
> > > escape before the url loads? or if the url is incorrect?
> > 
> > We can't currently distinguish between downloads, user hitting stop, invalid
> > URLs, and 204 responses.  All of those produce identical net::ERR_ABORTED
> > notifications, and this continues to treat them the same.
> > 
> > Peter suggested we adopt Firefox's behavior of leaving all of them around in
> new
> > tabs and clearing them otherwise, which (IMHO) is a bit saner and much
easier
> > than distinguishing the cases in the network stack.
> 
> thanks for the explanation. so as i understand it, what's safe is that we only
> leave the failed url around if the user is not on an attacker-controllable
page.
> NTP happens to be not attacker-controllable. but really, it's because it's
> implemented using webui, none of which is attacker-controllable. so can we do
> this for all webui pages? this would remove the need to add yet another method
> to CBC

Interesting.  I'm not entirely opposed to that idea, but the NTP itself isn't
WebUI anymore, so it wouldn't help there.  :)

[jam, 2014-04-14 05:26:50]

On 2014/04/11 23:45:08, Charlie Reis wrote:
> On 2014/04/11 23:42:33, jam wrote:
> > On 2014/04/11 23:31:21, Charlie Reis wrote:
> > > On 2014/04/11 23:25:48, jam wrote:
> > > > On 2014/04/11 22:44:25, Charlie Reis wrote:
> > > > > On 2014/04/11 22:36:57, jam wrote:
> > > > > > On 2014/04/11 22:28:01, Charlie Reis wrote:
> > > > > > > John, see comment 17 on the bug for the rationale here.  Hope it's
> ok
> > to
> > > > ask
> > > > > > the
> > > > > > > embedder for its notion of the NTP via ContentBrowserClient.
> > > > > > 
> > > > > > NTP is a chrome concept, so we shouldn't put this in the content
api.
> > > > > 
> > > > > How about something like ShouldPreserveAbortedURLs(const GURL&
> > > > > last_committed_url)?  We can implement that as an NTP check in
> > chrome-land.
> > > > 
> > > > i had thought about that, but it seemed like obfuscating a chrome
concept
> in
> > > the
> > > > content api :)
> > > > 
> > > > let me understand this better: when you preserve this aborted url, is
that
> > for
> > > > the download case (i.e. the linked bug), or is it for the case when a
user
> > > hits
> > > > escape before the url loads? or if the url is incorrect?
> > > 
> > > We can't currently distinguish between downloads, user hitting stop,
invalid
> > > URLs, and 204 responses.  All of those produce identical net::ERR_ABORTED
> > > notifications, and this continues to treat them the same.
> > > 
> > > Peter suggested we adopt Firefox's behavior of leaving all of them around
in
> > new
> > > tabs and clearing them otherwise, which (IMHO) is a bit saner and much
> easier
> > > than distinguishing the cases in the network stack.
> > 
> > thanks for the explanation. so as i understand it, what's safe is that we
only
> > leave the failed url around if the user is not on an attacker-controllable
> page.
> > NTP happens to be not attacker-controllable. but really, it's because it's
> > implemented using webui, none of which is attacker-controllable. so can we
do
> > this for all webui pages? this would remove the need to add yet another
method
> > to CBC
> 
> Interesting.  I'm not entirely opposed to that idea, but the NTP itself isn't
> WebUI anymore, so it wouldn't help there.  :)

ah yes, didn't think that suggestion through fully :)

ok doing this through a call to chrome i guess is the only solution. I prefer we
do this through a WebContentsDelegate call instead of ContentBrowserClient
though. ContentBrowserClient is meant as a last resort (i.e. when there's no
delegate interface for an object, or when a call is needed by multiple different
objects in content). In this case, this only matters for WebContents, and just
the ones for tabs, so implementing it by Browser class is enough.

[Peter Kasting, 2014-04-14 19:59:25]

I am worried about the behavior proposed here; see comment on bug.

[Charlie Reis, 2014-04-18 01:23:53]

Ok, I've revised this to support the middle click case (initial navigation of an
unmodified blank tab).  It's more subtle but I think I've worked through the
implications and tests.

There's one issue not covered here, where we need to clear a browser-initiated
pending entry if it fails and the tab is later modified.  That exists today and
is confusing but not a security problem, so I'll address it in a second CL.

PTAL.

https://codereview.chromium.org/232463007/diff/50001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.h (right):

https://codereview.chromium.org/232463007/diff/50001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.h:99: bool
IsUnmodifiedBlankTab() const;
I abstracted this out so I could use it in both GetVisibleEntry and DidFail.

https://codereview.chromium.org/232463007/diff/50001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/232463007/diff/50001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1049:
EXPECT_EQ(1, delegate->navigation_state_change_count());
We're now triggering a state change in DidFail so that the omnibox updates when
we clear the entry.

https://codereview.chromium.org/232463007/diff/50001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2887:
TEST_F(NavigationControllerTest, ShowRendererURLAfterFailUntilModified) {
This is similar to the test above but checks the DidFail case, making sure the
URL stays visible until the underlying page is modified.

[Peter Kasting, 2014-04-18 21:17:40]

As long as middle-clicking and alt-enter in the address bar work "as users
expect", I'm good.  I won't formally review, since I don't really understand the
functional level here.

[Charlie Reis, 2014-04-18 21:48:15]

On 2014/04/18 21:17:40, Peter Kasting wrote:
> As long as middle-clicking and alt-enter in the address bar work "as users
> expect", I'm good.  I won't formally review, since I don't really understand
the
> functional level here.

Yes, those should be handled correctly now.  Thanks for mentioning it.

Nasko, would you be able to review the details since John's OOO until Tuesday? 
I'll need to wait for his approval on the public API changes, but you'd be a
good reviewer for some of the navigation changes.

[jam, 2014-04-21 07:31:08]

lgtm for content/public with nit

https://codereview.chromium.org/232463007/diff/50001/content/public/browser/w...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/232463007/diff/50001/content/public/browser/w...
content/public/browser/web_contents_delegate.h:177: // preserved in the omnibox.
 Defaults to false.
nit: avoid mentioning omnibox in content since that's a chrome feature. explain
it instead in terms of content-level objects, i.e. pending entry

[nasko, 2014-04-21 16:43:07]

LGTM

[Charlie Reis, 2014-04-21 17:28:59]

Thanks.

https://codereview.chromium.org/232463007/diff/50001/content/public/browser/w...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/232463007/diff/50001/content/public/browser/w...
content/public/browser/web_contents_delegate.h:177: // preserved in the omnibox.
 Defaults to false.
On 2014/04/21 07:31:08, jam wrote:
> nit: avoid mentioning omnibox in content since that's a chrome feature.
explain
> it instead in terms of content-level objects, i.e. pending entry

Done.

https://codereview.chromium.org/232463007/diff/70001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/232463007/diff/70001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2920:
FrameHostMsg_DidFailProvisionalLoadWithError(0, params));
Also fixed this based on Peter's comment in
https://codereview.chromium.org/243493002/.

[Charlie Reis, 2014-04-21 17:29:07]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-04-21 17:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/232463007/90001

[commit-bot: I haz the power, 2014-04-21 19:41:40]

Change committed as 265044