Prevent a possible crash on omnibox navigation.

Prevent a possible crash on omnibox navigation.

If the user enters a word that triggers the alternate nav infobar (i.e. is a
search by default but also happens to be an intranet hostname), and the omnibox
navigation synchronously triggers the extension system to navigate a background
page, the background page navigation could be mistaken for the omnibox
navigation, leading to a crash when trying to show the infobar.

This could also goof up shortcut backend statistics, in theory, if the
background page navigation succeeded but the omnibox navigation did not.

This CL also adds a different CHECK to verify that the navigations that we do
pay attention to are the ones we want.  I'll convert this to a DCHECK in a
couple of weeks if it doesn't turn up anything in the field.

BUG=363105
TEST=none

Committed: https://crrev.com/8215378f588fa67251c277e326437188efda1dcc
Cr-Commit-Position: refs/heads/master@{#312950}

[Peter Kasting, 2015-01-22 19:45:55]

pkasting@chromium.org changed reviewers:
+ vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-01-22 21:59:09]

lgtm either way

https://codereview.chromium.org/865313002/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_navigation_observer.cc (right):

https://codereview.chromium.org/865313002/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_navigation_observer.cc:98: if
(!InfoBarService::FromWebContents(web_contents))
Using InfoBarservice for unwanted web-contents looks confusing.
It would be nicer to have more direct check.

Maybe opposite?

if (match_.destination_url != ...) {
...
}
and CHECK for InfoBarService...

[Peter Kasting, 2015-01-22 22:02:26]

https://codereview.chromium.org/865313002/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_navigation_observer.cc (right):

https://codereview.chromium.org/865313002/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_navigation_observer.cc:98: if
(!InfoBarService::FromWebContents(web_contents))
On 2015/01/22 21:59:09, Vitaly Buka wrote:
> Using InfoBarservice for unwanted web-contents looks confusing.
> It would be nicer to have more direct check.
> 
> Maybe opposite?
> 
> if (match_.destination_url != ...) {
> ...
> }
> and CHECK for InfoBarService...

There's no theoretical reason why an extension background page couldn't load the
same page the user is trying to load, so there's a chance (although it seems
very unlikely) that doing things that way would break.

[Peter Kasting, 2015-01-22 22:02:30]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-01-22 22:03:31]

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

[commit-bot: I haz the power, 2015-01-22 23:16:04]

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

[commit-bot: I haz the power, 2015-01-22 23:16:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2015-01-23 02:15:12]

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

[Peter Kasting, 2015-01-23 02:15:12]

Charlie, can you glance quickly at this patch?  I primarily want to be sure that
the new overload I added (for DidFailProvisionalLoad()) is doing the right thing
to detect cases when the pending entry we saw earlier is not going to be
committed.  I assume any failed provisional load that happens to our
WebContents' main frame between "entry pending" and "entry committed" means the
pending entry will be discarded, and there aren't other ways for the pending
entry to be discarded that I'm missing.

Secondarily, if you're aware of a case where an omnibox navigation attempt
produces a pending entry whose virtual URL doesn't match the original omnibox
destination URL, let me know.  The original URL has already gone through fixup,
and using the virtual URL instead of the URL accounts for cases that the browser
rewrites, e.g. chrome://history/ -> chrome://chrome/history or view-source:
URLs.

[Charlie Reis, 2015-01-23 06:34:28]

On 2015/01/23 02:15:12, Peter Kasting wrote:
> Charlie, can you glance quickly at this patch?  I primarily want to be sure
that
> the new overload I added (for DidFailProvisionalLoad()) is doing the right
thing
> to detect cases when the pending entry we saw earlier is not going to be
> committed.  I assume any failed provisional load that happens to our
> WebContents' main frame between "entry pending" and "entry committed" means
the
> pending entry will be discarded,

This isn't quite true-- there are cases that we leave the pending entry around
after DidFailProvisionalLoad, such as a navigation that fails in a new tab. 
This lets the user edit the URL and try again.  See
NavigatorImpl::DidFailProvisionalLoadWithError and this comment:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

> and there aren't other ways for the pending
> entry to be discarded that I'm missing.

There's lots of ways for the pending entry to be discarded, though most should
probably generate DidFailProvisionalLoad.  I'm not 100% sure what happens on
server redirects (i.e., if we replace the pending entry) or if there are races
where one navigation might interrupt another without causing
DidFailProvisionalLoad from the old one (e.g., if the old renderer process has
crashed).

That said, it could be nice to aim for a guarantee here, using something like
WebContentsObserverSanityChecker to find any cases we miss.


> Secondarily, if you're aware of a case where an omnibox navigation attempt
> produces a pending entry whose virtual URL doesn't match the original omnibox
> destination URL, let me know.  The original URL has already gone through
fixup,
> and using the virtual URL instead of the URL accounts for cases that the
browser
> rewrites, e.g. chrome://history/ -> chrome://chrome/history or view-source:
> URLs.

Those are the main examples of virtual URLs that I'm aware of, so that sounds
reasonable to me.

https://codereview.chromium.org/865313002/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_navigation_observer.cc (right):

https://codereview.chromium.org/865313002/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_navigation_observer.cc:74:
content::NotificationService::AllSources());
I'm guessing we can't enumerate the possible tabs to listen to, either?  That
would be one alternative to returning early below.  However, I'm guessing that
the navigation might occur in a tab that hasn't been created yet (e.g.,
Alt+Enter), hence the need for AllSources.

[Peter Kasting, 2015-01-23 07:14:16]

On 2015/01/23 06:34:28, Charlie Reis (slow until 1-29) wrote:
> On 2015/01/23 02:15:12, Peter Kasting wrote:
> > Charlie, can you glance quickly at this patch?  I primarily want to be sure
> that
> > the new overload I added (for DidFailProvisionalLoad()) is doing the right
> thing
> > to detect cases when the pending entry we saw earlier is not going to be
> > committed.  I assume any failed provisional load that happens to our
> > WebContents' main frame between "entry pending" and "entry committed" means
> the
> > pending entry will be discarded,
> 
> This isn't quite true-- there are cases that we leave the pending entry around
> after DidFailProvisionalLoad, such as a navigation that fails in a new tab. 
> This lets the user edit the URL and try again.  See
> NavigatorImpl::DidFailProvisionalLoadWithError and this comment:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

So, in this case, if the user hits enter again, does that mean there won't be a
new NAV_ENTRY_PENDING notification?

The omnibox navigation observer relies on the idea that there's always a
"pending"/"committed" sequence to successful navigations.  Prerendering can
break that right now, but if this can also break it, that's a (minor) problem.

It would be nice to ensure this case files NAV_ENTRY_PENDING again when the load
is restarted, or else that there's some more reliable series of notifications
that we can use instead.

> > and there aren't other ways for the pending
> > entry to be discarded that I'm missing.
> 
> There's lots of ways for the pending entry to be discarded, though most should
> probably generate DidFailProvisionalLoad.  I'm not 100% sure what happens on
> server redirects (i.e., if we replace the pending entry) or if there are races
> where one navigation might interrupt another without causing
> DidFailProvisionalLoad from the old one (e.g., if the old renderer process has
> crashed).
> 
> That said, it could be nice to aim for a guarantee here, using something like
> WebContentsObserverSanityChecker to find any cases we miss.

My hope on a redirect, if it replaces the pending entry, would be that it does
_not_ call DidFailProvisionalLoad for the old entry, as the old entry didn't
actually "fail", and my observer here wants to follow redirects (and will,
assuming DidFailProvisionalLoad isn't called).

>
https://codereview.chromium.org/865313002/diff/20001/chrome/browser/ui/omnibo...
> File chrome/browser/ui/omnibox/omnibox_navigation_observer.cc (right):
> 
>
https://codereview.chromium.org/865313002/diff/20001/chrome/browser/ui/omnibo...
> chrome/browser/ui/omnibox/omnibox_navigation_observer.cc:74:
> content::NotificationService::AllSources());
> I'm guessing we can't enumerate the possible tabs to listen to, either?  That
> would be one alternative to returning early below.  However, I'm guessing that
> the navigation might occur in a tab that hasn't been created yet (e.g.,
> Alt+Enter), hence the need for AllSources.

Alt-enter, as well as middle-clicking an item in the omnibox dropdown.  Both
create a new tab during the actual navigation so we don't know what it will be
ahead of time, hence AllSources.

[Peter Kasting, 2015-01-23 21:41:50]

On 2015/01/23 07:14:16, Peter Kasting wrote:
> So, in this case, if the user hits enter again, does that mean there won't be
a
> new NAV_ENTRY_PENDING notification?

I tested this.  We get NAV_ENTRY_PENDING again.  What seems to happen is that
the old pending entry is preserved, but when the user hits enter again, we
replace it with a new, identical pending entry.  So things work out OK.

> My hope on a redirect, if it replaces the pending entry, would be that it does
> _not_ call DidFailProvisionalLoad for the old entry, as the old entry didn't
> actually "fail", and my observer here wants to follow redirects (and will,
> assuming DidFailProvisionalLoad isn't called).

I tried loading "google.com/mail" (which should redirect at least once) and
DidFailProvisionalLoad() wasn't called at any time before
NavigationEntryCommitted().  So I think this is OK, unless I wasn't testing the
right case.

It seems like this patch is probably close to correct, and if there are errors,
they're both rare and "err on the side of canceling the observer", which is the
safer route.  So I'm going to go ahead and commit.

[Peter Kasting, 2015-01-23 21:41:54]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:42:39]

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

[commit-bot: I haz the power, 2015-01-23 22:36:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-23 22:37:41]

Patchset 2 (id:??) landed as
https://crrev.com/8215378f588fa67251c277e326437188efda1dcc
Cr-Commit-Position: refs/heads/master@{#312950}