+mmenke@, +pasko@

Any thoughts on this? I can think of two solutions to this problem:

1. Don't prerender pages that would trigger a browser switch
2. Prerender those pages, but don't count them as "real" navigations for BrowserSwitcher

I would personally prefer the first approach, since it would involve less "magic"  on the BrowserSwitcher side. However, it does mean Preload needs to be aware our feature exists.

I don't even know what a BrowserSwitcher is, and that doc is access restricted.

[pasko]:  Do we even still support prerendering?

BrowserSwitcher is the internal name for Legacy Browser Support (https://support.google.com/chrome/a/answer/3019558?hl=en). Current a Chrome extensions that we want to build in as core Chrome feature. It allows the user to define a set of hosts that should be opened in a different browser than Chrome for compatibility reasons. So in those cases the navigation should be prevented and the url loaded in the alternative browser. Prerendering urls could cause issues with some one-time urls that are only valid for a single get request and require to be rendered in the other browser.  

Ah, I had thought we removed that years ago, back when we removed Chrome Custom Tab support (IE Chrome Tab support?  Not sure what it was called).

mmenke: I should have provided more context. This feature isn't in Chrome right now, but it's available as an extension. We are working to bake those features into Chrome, but it's still super early in development. Legacy Browser (LBS) support isn't the same as 'IE Tab' (which is third-party), since LBS launches a complete instance of IE in a different window.

We were thinking of implementing LBS like this:

1. Listen to DidStartNavigation() [1]
2. Check the navigation's URL against an administrator-controlled list
3. If it matches, close the tab and open the URL in IE instead

I was working on a prototype for the feature, and this is the issue I noticed (and why I opened the bug):

1. Admin sets 'example.com' to open in IE
2. User types 'e' in the omnibar
3. Omnibar offers to autocomplete 'e' to 'example.com' (e.g. because of history)
4. Prerender starts prerendering the URL 'example.com'
5. Prerender triggers a DidStartNavigation() event
6. Tab closes, and IE opens with 'example.com' (all the user did was hit 'e'!)

We don't want (6) to happen unless the users starts a 'real' navigation (e.g. by hitting Enter).

This is where the solutions proposed in Comment 1 come in. For instance, Prerender could look at the URL it wants to prerender ('example.com'), and see that BrowserSwitcher would open it in IE. This means there's not much point in pre-rendering it (since it won't be used by Chrome anyways).

Another possible solution I overlooked earlier: there might already be a different event than DidStartNavigation(), which doesn't get triggered for prerenders. Listening to that instead could be acceptable.

[1] https://cs.chromium.org/chromium/src/content/public/browser/web_contents_observer.h?l=168&rcl=a3314d3c8731bc47db2cb59e6b9038015ed9626c

"IE tab" was first party, where we supported opening Chrome tabs in IE - we had a weird pseudo network stack which implemented IE's network stack primitives on top of our network stack (Or the other way around?).  I think we discontinued it 5 years ago?  I'm pretty sure it also included the ability to open certain URLs only in IE.  It's possible I'm misremembering, and it was the other way around (open IE tabs in chrome, only for certain URLs).

Anyhow, I'll defer to pasko here.

It's been years since I'd worked on this code (is prerender not removed yet?), but it should be possible for your code to detect if a WebContents is a prerender and, if so, rather than launching IE in the DidStartNavigation event, abort the prerender. See chrome/browser/prerender/prerender_final_status.h for the various existing prerender-aborting triggers.

Sorry, late to the party. Another sorry: cannot access the doc.

Prerender code is still used, but only to fuel nostate-prefetch (same heuristics to fire, triggers a navigation, fetches a few subresources and shuts down, all invisibly). There is some cleanup TBD to simplify the prerender code an maybe rename.

Hooking browser switcher on navigation level is indeed making it difficult for prerender code, maybe there could be a nav flag introduced that would disable this feature for prerenders? Or let the prerenders know that this navigation would make a browser switch?

Not a navigation expert here, but doing a switch triggered by navigation may reveal other problems. Like renderer-initiated navigations would make some checks/side-effects on the Document/Frame side, but then out of the blue their expectations are overridden by browser side BrowserSwitcher. Have you chatted with nasko@ and clamy@ regarding the safety of this approach?

+nasko@, +clamy@, what do you think of pasko@'s comment on side-effects & safety?

We're trying to add a hook on navigation that can close the tab, and open the URL in IE instead.

FWIW, the old implementation uses the webRequest extension API [1], which is backed by a NetworkDelegate [2]. This seems to work fine for our use-case. Maybe we should just keep that approach instead of listening for navigation events.

[1] https://github.com/LegacyBrowserSupport/legacy-browser-support/blob/8caa623692b94dc0154074ce904de8f60ee8a404/chrome_extension/js/extension_logic.js#L539
[2] https://cs.chromium.org/chromium/src/net/base/network_delegate_impl.h?l=37&rcl=15a1df62f1b42f8d88bb30f28c6225f6302c2ec6

pasko: That sounds bizarre. By that, do you mean that no-state prefetch makes the rest of the stack think it's actually loading a page and everything?

There seems to still be code for canceling prerenders in response to print attempts and whatnot? It's even hooked up to some Mojo thing. I take it that's unused now?

Also how does prerender fetches currently work with WebRequest API - it can also block or redirect loading of resources. Does prerendered resources then get discarded or does this API hook earlier in the loading process and prevents that completely. I am asking this because what Nicolas wants to achieve is currently implemented as an extension that uses a blocking WebRequest hook on OnBeforeRequest that selectively redirects loads to an extension internal page and in the same time invokes the alt browser in the background.

> pasko: That sounds bizarre. By that, do you mean that no-state prefetch makes the rest of the stack think it's actually loading a page and everything?

yes. I thought that re-creating a parallel stack would be harder to support. Now we have only one specialty for network service that uploads subresource data to renderers and discards it. This was a tradeoff to try to keep the network service simpler.

Code for cancelling prerenders in various cases triggered by JS execution still exists because removing it was not the highest priority :/

re #12: Quick answer: Offhand I don't know what WebRequest API is, but I can check if prerender respects it. Since we are reusing essentially the normal navigation and loading machinery, it is likely that this is supported.

More detailed answer: TBD :)

ah, oh, I misunderstood, webRequest is an extension API! This would nicely remove browser switches from prerender navigations indeed. Extensions are not attached to prerenders AFAICT, so they would go below radar for the BrowserSwitcher, it seems.

Just to clarify, are you still considering the solution outlined in 5? In that case, IIRC prerender allows to check whether a WebContents is actually a prerender. If that's the case, in step 3 of your solution you can just ask the prerender to stop and not open the URL. I would MUCH prefer we go with a kill of the prerender at the WebContentObserver level rather than rely on the WebRequest API code hooking or not in prerenders (which might be impacted in non-trivial ways by servicification).

Neither my @google.com not @chromium.org account can access the document. nicolaso: can you please fix it?

clamy: thanks! good to know what the easier path is. I do not fully understand how we would kill the prerender reliably - it could have departed to IE already?

On the other hand: if the browser is to make the LBS decision at the _start_ of the navigation (i.e. before redirects - by only looking at the URL?), then it'd be better to just avoid starting the prerender. We can stick the logic around the place ChromeOmniboxClient::DoPrerender() is called: 

https://codesearch.chromium.org/chromium/src/chrome/browser/ui/omnibox/chrome_omnibox_client.cc?q=file:chrome_omnibox_client.cc+DoPrerender&sq=package:chromium&g=0&l=480

> Neither my @google.com not @chromium.org account can access the document. nicolaso: can you please fix it?

Ahh, I forgot to share it with people outside our team. The link should work now. The high-level design is still in its early stages, though.

> then it'd be better to just avoid starting the prerender. We can stick the logic around the place ChromeOmniboxClient::DoPrerender() is called:

Hm, that sounds like it could work. As long as we have a |BrowserContext|, we can read BrowserSwitcher prefs/policies and compare them with the URL. This would use the |BrowserSwitcherSitelist| attached to the current BrowserContext [1].

Are we sure this will remain the only entry point for prerender?

> Just to clarify, are you still considering the solution outlined in 5?

We haven't committed to anything, but considering Comment 17, it seems like a good path to follow.

Specifically, on the BrowserSwitcher side, we could still:

> 1. Listen to DidStartNavigation()
> 2. Check the navigation's URL against an administrator-controlled list
> 3. If it matches, close the tab and open the URL in IE instead

And on the Prerender side:

> avoid starting the prerender. We can stick the logic around the place ChromeOmniboxClient::DoPrerender() is called

This would run our code only on non-prerenders. There's also no reason to start a prerender on something that would open in IE.

[1] https://cs.chromium.org/chromium/src/chrome/browser/browser_switcher/browser_switcher_sitelist.h?l=24&rcl=e8aaa5ec61a503fca6be890eaf9be4c0c66d4faa

> I do not fully understand how we would kill the prerender reliably - it could have departed to IE already?

The way prerender features normally worked, one would kill the prerender *before* triggering logic to depart to IE or whichever and presumably decline to run that logic. You can, on the UI thread, check if a WebContents is a prerender. See callers of PrerenderContents::Destroy.

If no-state prefetch uses the same machinery, that would probably work too.

It's true that we could have redirects. In that case, you could implement a NavigationThrottle that would check navigation starts and redirects and cancel the navigation if it's part of a prerender and matching URLs that need to be open in another browser.

re #19: yeah indeed: the IE-departure logic running on UI thread can check for nullptr-ness of prerender::PrerenderContents::FromWebContents()

re #18: there is actually one other entry point that is still alive: in prerender_link_manager.cc - indeed not ideal. We can check BrowserSwitcherSitelist::ShouldRedirect in PrerenderManager::AddPrerender() - this would cover all entry points.

re #20: yeah, ignoring HTTP redirects sounded odd to me as well, asked for clarification in the doc

Thinking about it some more, have you considered using the InterceptNavigationDelegate? It's used on Android to show the intent picker and send an intent to an app instead of navigating, and on desktop to open special kinds of windows for navigations. I think it would suit your use case pretty well, and you wouldn't need to add a new WebContentsObserver or NavigationThrottle. I know that the InterceptNavigationthrottle which backs the InterceptNavigationDelegate has the knowledge of whether a navigation is a prerender or not. I'm not sure if it cancels the prerender if the navigation needs to be intercepted, or just ignore prerenders, but I don't think the behavior should be too hard to modify to match what you want.

Also as a side note: I see that in your plan you plan to close the tab when you detect that the URL should be opened in another browser. Other navigation interceptors tend to leave the tab open but cancel the navigation IIRC. I think it'd be a better user experience to match what they are doing.

>InterceptNavigationDelegate

That sounds like it would take a lot of work, since the class only works on Android right now. It really looks like it's only meant to be used from the Java side [1].

[1] https://cs.chromium.org/chromium/src/components/navigation_interception/intercept_navigation_delegate.h?l=28&rcl=740a775ca3b928cc46e4ae2aa195c0a09acec4cf

Mmh re-reading the code you just need to use the InterceptNavigationThrottle itself: https://cs.chromium.org/chromium/src/components/navigation_interception/intercept_navigation_throttle.h?sq=package:chromium&dr&g=0&l=37

This one is used by components in desktop as well, and you will avoid having to code another throttle (not to mention that there's a bit of subtlety around the destruction of objects when you cancel a navigation that InterceptNavigationThrottle already takes care of). In particular, The PlatformAppRedirector, which is backed by the InterceptNavigationThrottle should be very similar to what you want. See https://cs.chromium.org/chromium/src/chrome/browser/apps/platform_apps/platform_app_navigation_redirector.cc?sq=package:chromium&dr&g=0&l=118.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/995e77706df44b70134f735f44b9b196e5c5b413

commit 995e77706df44b70134f735f44b9b196e5c5b413
Author: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Date: Thu Sep 13 16:02:13 2018

[BrowserSwitcher] Add BrowserSwitcherNavigationThrottle

This lets us listen to navigations/redirects and launch an alternative
browser if the URL is whitelisted for BrowserSwitcher.

Bug:  876802 ,  876752 
Change-Id: I1c2a87ed3e918d0c5443d58d113e5edec431e627
Reviewed-on: https://chromium-review.googlesource.com/1211746
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591029}
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/BUILD.gn
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/alternative_browser_driver.cc
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/alternative_browser_launcher.cc
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/alternative_browser_launcher.h
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/alternative_browser_launcher_unittest.cc
[add] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/browser_switcher_navigation_throttle.cc
[add] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/browser_switcher_navigation_throttle.h
[add] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/browser_switcher_navigation_throttle_unittest.cc
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/browser_switcher_service.cc
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/browser_switcher_service.h
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/browser_switcher_sitelist.cc
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/browser_switcher_sitelist.h
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/browser_switcher/browser_switcher_sitelist_unittest.cc
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/prerender/prerender_final_status.cc
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/browser/prerender/prerender_final_status.h
[modify] https://crrev.com/995e77706df44b70134f735f44b9b196e5c5b413/chrome/test/BUILD.gn