Make browser_handles_top_level_requests synchronous.

Make browser_handles_top_level_requests synchronous.

Currently if the browser_handles_top_level_requests renderer pref
is set the top level navigations are unconditionally ignore at
the renderer level and re-initiated from the browser.
This change makes it possible to ignore only certain navigations
and let the others continue uninterrupted.
The change is needed to implement Android url interception where
for certain http(s):// URLs we want to handle it in an external
application.

BUG=121956
TEST=None

[mkosiba (inactive), 2012-04-04 20:09:13]

This is the change I was talking about in the
WebContentsDelegate::OpenURLFromTab chromium-dev thread.

There don't seem to be any unittests explicitly associated with
WebContentsDelegate methods so I'd appreciate some suggestions on where to add a
test case or two for this.

[Charlie Reis, 2012-04-04 20:23:22]

https://chromiumcodereview.appspot.com/9978015/diff/1/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:2345: return
WebKit::WebNavigationPolicyIgnore;
Darin, can you take a look at this?  Adding synchronous IPCs to navigation makes
me nervous, but it could also be useful for letting the browser make all
navigation-related decisions in the future.

[mkosiba (inactive), 2012-04-04 20:40:45]

https://chromiumcodereview.appspot.com/9978015/diff/1/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:2345: return
WebKit::WebNavigationPolicyIgnore;
On 2012/04/04 20:23:22, creis wrote:
> Darin, can you take a look at this?  Adding synchronous IPCs to navigation
makes
> me nervous, but it could also be useful for letting the browser make all
> navigation-related decisions in the future.

The alternative is to change
FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction to not call the
WebKit callback immediately. This would introduce a bunch of new states in the
render_view though.

[Charlie Reis, 2012-04-05 17:06:53]

https://chromiumcodereview.appspot.com/9978015/diff/13001/chrome/browser/exte...
File chrome/browser/external_tab/external_tab_container_win.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/chrome/browser/exte...
chrome/browser/external_tab/external_tab_container_win.cc:416: bool
is_content_intiated,
nit: initiated

https://chromiumcodereview.appspot.com/9978015/diff/13001/chrome/browser/exte...
chrome/browser/external_tab/external_tab_container_win.cc:420: // and re-issuing
every navigation.
This TODO seems critical.  We can't treat every link click as a
browser-initiated navigation.  That would introduce process swaps for every type
of cross-site navigation, which would break script connections between tabs in
real cases.  We're currently very careful about when we use process swaps for
this reason-- see the Caveats section of this page for details:
http://dev.chromium.org/developers/design-documents/process-models

(Note: as mentioned below, I'm not sure when this class is used, so I'm not sure
how often this problem will arise.)

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/browser/tab...
File content/browser/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/browser/tab...
content/browser/tab_contents/tab_contents.cc:2330: transition_type);
I'm not very familiar with ExternalTabContainers.  In which situations is
delegate_ one?

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/public/brow...
content/public/browser/web_contents_delegate.h:88: bool is_content_intiated,
nit: initiated

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
File content/renderer/render_view_impl.cc (left):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
content/renderer/render_view_impl.cc:2328: if (is_content_initiated) {
I thought this was here so it didn't send browser-initiated navigations right
back to the browser.  It looks like ExternalTabContainer will now intercept
those and cause us to ignore them, so I'm puzzled if this works.  Do we get here
for browser-initiated navigations?

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
content/renderer/render_view_impl.cc:5169: #else
This also makes me nervous.  I'd rather have a well-tested path that works for
both Android and desktop Chrome if possible, than the possibility of
Android-specific bugs from the ifdef.

[groby-ooo-7-16, 2012-04-05 22:51:19]

Mostly nits - trust creis/darin, not me. This is not a code area I'm very
familiar with.

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1176: disposition,
source_frame_id,
nit: collapse last two argument lines into one. Save a line :)

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/public/brow...
content/public/browser/web_contents_delegate.h:88: bool is_content_intiated,
nit: Switch to enum, if possible

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
content/renderer/render_view_impl.cc:5169: #else
Specifically, unless I misread the intent - this doesn't start considering
http/https. It stops treating non-http/https as local navigations.

On 2012/04/05 17:06:53, creis wrote:
> This also makes me nervous.  I'd rather have a well-tested path that works for
> both Android and desktop Chrome if possible, than the possibility of
> Android-specific bugs from the ifdef.

[mkosiba (inactive), 2012-04-10 17:58:37]

Anantha,
   Could you please take a look at the changes I've made to
external_tab_container? I added you to the review based on git log, if you're
not the right person to review the code please delegate as required. 
   I tried to be as non-invasive as possible but I'd be glad to improve the
current implementation. In order to do that I'd need to know whether the
automation code depends on every navigation being browser initiated. If not, I'd
also need to know how to see if my changes actually work - I tried running ui
tests on the win trybot but that doesn't seem to exercise this code path.

Thanks!
    Martin

https://chromiumcodereview.appspot.com/9978015/diff/13001/chrome/browser/exte...
File chrome/browser/external_tab/external_tab_container_win.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/chrome/browser/exte...
chrome/browser/external_tab/external_tab_container_win.cc:416: bool
is_content_intiated,
On 2012/04/05 17:06:53, creis wrote:
> nit: initiated

Done.

https://chromiumcodereview.appspot.com/9978015/diff/13001/chrome/browser/exte...
chrome/browser/external_tab/external_tab_container_win.cc:420: // and re-issuing
every navigation.
On 2012/04/05 17:06:53, creis wrote:
> This TODO seems critical.  We can't treat every link click as a
> browser-initiated navigation.  That would introduce process swaps for every
type
> of cross-site navigation, which would break script connections between tabs in
> real cases.  We're currently very careful about when we use process swaps for
> this reason-- see the Caveats section of this page for details:
> http://dev.chromium.org/developers/design-documents/process-models
> 
> (Note: as mentioned below, I'm not sure when this class is used, so I'm not
sure
> how often this problem will arise.)

This is exactly how browser_handles_top_level_requests works now - every link
click is forced to be a browser-initiated navigation (take a look at the code in
decidePolicyForNavigation).

The reason I'm not making more changes here is because I'm assuming
ExternalTabContainer clients are depending on this behavior (pessimistic
assumption - I don't know this for a fact)

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1176: disposition,
source_frame_id,
On 2012/04/05 22:51:19, groby wrote:
> nit: collapse last two argument lines into one. Save a line :)

Would this be OK according to the code formatting guide?

  *result = delegate_->ShouldIgnoreNavigation(validated_url, referrer,
      disposition, source_frame_id, is_content_initiated);

I thought the only alternative is:

  *result = delegate_->ShouldIgnoreNavigation(
      validated_url, referrer, disposition, source_frame_id,
      is_content_initiated);

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/browser/tab...
File content/browser/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/browser/tab...
content/browser/tab_contents/tab_contents.cc:2330: transition_type);
On 2012/04/05 17:06:53, creis wrote:
> I'm not very familiar with ExternalTabContainers.  In which situations is
> delegate_ one?

Neither am I. I don't even have a Windows machine to test this out on.
This is what I found on one of the mailing list:


ExternalTabContainer is an abstraction that replaces Browser in the case 
where the host is not a Chrome window.  Chrome Frame uses this to host 
Chrome-rendered content in a plugin HWND, generally inside IE. 

Form searching the code I see it being used in automation_provider_win.cc. I'm
adding ananta@ who seems to have authored a lot of the code in
ExternalTabContainer.

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/public/brow...
content/public/browser/web_contents_delegate.h:88: bool is_content_intiated,
On 2012/04/05 17:06:53, creis wrote:
> nit: initiated

Done.

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/public/brow...
content/public/browser/web_contents_delegate.h:88: bool is_content_intiated,
On 2012/04/05 22:51:19, groby wrote:
> nit: Switch to enum, if possible

is_content_initiated is already defined as a bool on NavigationState.

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
File content/renderer/render_view_impl.cc (left):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
content/renderer/render_view_impl.cc:2328: if (is_content_initiated) {
On 2012/04/05 17:06:53, creis wrote:
> I thought this was here so it didn't send browser-initiated navigations right
> back to the browser.  It looks like ExternalTabContainer will now intercept
> those and cause us to ignore them, so I'm puzzled if this works.  Do we get
here
> for browser-initiated navigations?

The change is here so that Chrome on *Android* doesn't send navigations right
back to the browser (sorry, I should have made this clear). I don't intend to
change the way ExternalTabContainer works in this CL.

The way this currently works is:
1) user clicks on a link/some action takes place
2) the navigation comes in to render_view_impl::decidePolicyForNavigation
3) since the navigation was not initiated via WebFrame::LoadRequest
is_content_initiated == true
4) we cancel the navigation and send a message to the browser
5) navigation is re-initiated browser side,
6) navigation is now initiated via WebFrame::LoadRequest, so
is_content_initiated == false,
7) since !is_content_initiated we skip over this if block and continue to
loading the page

The way ExternalTabContainer works with the change is:
1) user clicks on link
2) the navigation comes in to render_view_impl::decidePolicyForNavigation
3) is_content_initiated == true for the same reasons
4) we send a synchronous IPC to ExternalTabContainer
5) in ExternalTabContainer we see content_initiated == true so we start a new
browser-initiated navigation (the new navigation request will wait in the
message queue until we've finished processing the current navigation) and tell
the render to ignore the current one
6) the new browser-initiated navigation comes in, is_content_initiated == false
7) we send a synchronous message to ExternalTabContainer, but since
is_content_initiated == false the navigation proceeds as normal

Now, what I've described under 7) isn't what the code does
(ExternalTabContainer::ShouldIgnoreNavigation should finish with return
is_content_initiated instead of always returning  true). I'll change the code to
do that, but this also means that (contrary to what I expected) running ui tests
on the win trybot doesn't excercise this code...

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/13001/content/renderer/re...
content/renderer/render_view_impl.cc:5169: #else
On 2012/04/05 22:51:19, groby wrote:
> Specifically, unless I misread the intent - this doesn't start considering
> http/https. It stops treating non-http/https as local navigations.
> 
> On 2012/04/05 17:06:53, creis wrote:
> > This also makes me nervous.  I'd rather have a well-tested path that works
for
> > both Android and desktop Chrome if possible, than the possibility of
> > Android-specific bugs from the ifdef.
> 

Agreed. My bad for sticking this in to the CL - this actually is a hack to work
around our current lack of a platform_util:OpenExternal implementation. I'll
remove this bit.

[ananta, 2012-04-10 19:45:45]

https://chromiumcodereview.appspot.com/9978015/diff/16001/chrome/browser/exte...
File chrome/browser/external_tab/external_tab_container_win.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/16001/chrome/browser/exte...
chrome/browser/external_tab/external_tab_container_win.cc:423:
source->GetController().LoadURL(url, referrer, transition_type,
In ChromeFrame's case we want the top level URL request to be handled by IE,
which is achieved via the RenderViewImpl::OpenURL call in the renderer. This
code as written would just reissue the navigation in the chrome browser which is
incorrect.

https://chromiumcodereview.appspot.com/9978015/diff/16001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/16001/content/renderer/re...
content/renderer/render_view_impl.cc:2343: &ignore_navigation));
Would not do the right thing for ChromeFrame as mentioned in the comment in
ExternalTabContainer.

[mkosiba (inactive), 2012-04-11 13:49:35]

https://chromiumcodereview.appspot.com/9978015/diff/16001/chrome/browser/exte...
File chrome/browser/external_tab/external_tab_container_win.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/16001/chrome/browser/exte...
chrome/browser/external_tab/external_tab_container_win.cc:423:
source->GetController().LoadURL(url, referrer, transition_type,
On 2012/04/10 19:45:45, ananta wrote:
> In ChromeFrame's case we want the top level URL request to be handled by IE,
> which is achieved via the RenderViewImpl::OpenURL call in the renderer. This
> code as written would just reissue the navigation in the chrome browser which
is
> incorrect.

Ok, should be better now - any ideas on how to see if my changes work?

[ananta, 2012-04-11 23:08:05]

https://chromiumcodereview.appspot.com/9978015/diff/22001/chrome/browser/exte...
File chrome/browser/external_tab/external_tab_container_win.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/22001/chrome/browser/exte...
chrome/browser/external_tab/external_tab_container_win.cc:376: return (NULL ==
OpenUrlInExtenalHost(source, url, disposition, referrer));
The OpenUrlInExtenalHost function will return NULL in case the request is
pending, i.e waiting for IE to connect to chrome on the automation  channel. The
right thing to do would be to return true for all content initiated navigations.

[mkosiba (inactive), 2012-04-12 10:12:52]

https://chromiumcodereview.appspot.com/9978015/diff/22001/chrome/browser/exte...
File chrome/browser/external_tab/external_tab_container_win.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/22001/chrome/browser/exte...
chrome/browser/external_tab/external_tab_container_win.cc:376: return (NULL ==
OpenUrlInExtenalHost(source, url, disposition, referrer));
On 2012/04/11 23:08:05, ananta wrote:
> The OpenUrlInExtenalHost function will return NULL in case the request is
> pending, i.e waiting for IE to connect to chrome on the automation  channel.
The
> right thing to do would be to return true for all content initiated
navigations.

Done.

[ananta, 2012-04-13 17:52:13]

LGTM

[Charlie Reis, 2012-04-13 18:21:52]

On 2012/04/04 20:23:22, creis wrote:
>
https://chromiumcodereview.appspot.com/9978015/diff/1/content/renderer/render...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9978015/diff/1/content/renderer/render...
> content/renderer/render_view_impl.cc:2345: return
> WebKit::WebNavigationPolicyIgnore;
> Darin, can you take a look at this?  Adding synchronous IPCs to navigation
makes
> me nervous, but it could also be useful for letting the browser make all
> navigation-related decisions in the future.

Martin, have you talked with Darin about the synchronous IPC during navigation? 
I'm still not sure if it's safe or not.  If he's ok with it, I'll look again at
the rest.

[darin (slow to review), 2012-04-16 15:47:11]

https://chromiumcodereview.appspot.com/9978015/diff/26001/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/26001/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1164: 
This creates a dead-lock.  It is not valid to handle synchronous IPCs messages,
sent by the renderer's main thread, on the browser's main thread.

I think you should also look hard at avoiding this synchronous IPC altogether.

[mkosiba (inactive), 2012-04-16 16:15:07]

https://chromiumcodereview.appspot.com/9978015/diff/26001/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9978015/diff/26001/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1164: 
On 2012/04/16 15:47:11, darin wrote:
> This creates a dead-lock.  It is not valid to handle synchronous IPCs
messages,
> sent by the renderer's main thread, on the browser's main thread.
Is is a guaranteed deadlock or only under certain conditions? We've had a change
similar to this one in Chrome on Android for a while and it seems to work ok...

> 
> I think you should also look hard at avoiding this synchronous IPC altogether.


For the URL overriding in Chrome on Android we effectively need to 'park' the
navigation while we check (on the UI thread) whether we want to cancel it or
not. The only alternative I can think of is to modify the
decidePolicyForNavigation flow to be asynchronous (WebKit supports it in
FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction). Doing that
would probably add some 'interesting' states for the renderer and navigation
code.

[darin (slow to review), 2012-04-16 16:25:37]

As I wrote to you privately, what you are trying to do is akin to how
downloads are handled or even ShellExecute integration for unknown
URL schemes.  You should be able to implement this like those.  You
don't need to introduce synchronous IPCs from renderer to browser.

-Darin

On Mon, Apr 16, 2012 at 9:15 AM, <mkosiba@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/9978015/diff/**
>
26001/content/browser/**renderer_host/render_view_**host_impl.cc<https://chromiumcodereview.appspot.com/9978015/diff/26001/content/browser/renderer_host/render_view_host_impl.cc>
> File content/browser/renderer_host/**render_view_host_impl.cc (right):
>
> https://chromiumcodereview.**appspot.com/9978015/diff/**
> 26001/content/browser/**renderer_host/render_view_**
>
host_impl.cc#newcode1164<https://chromiumcodereview.appspot.com/9978015/diff/26001/content/browser/renderer_host/render_view_host_impl.cc#newcode1164>
> content/browser/renderer_host/**render_view_host_impl.cc:1164:
> On 2012/04/16 15:47:11, darin wrote:
>
>> This creates a dead-lock.  It is not valid to handle synchronous IPCs
>>
> messages,
>
>> sent by the renderer's main thread, on the browser's main thread.
>>
> Is is a guaranteed deadlock or only under certain conditions? We've had
> a change similar to this one in Chrome on Android for a while and it
> seems to work ok...
>
>
>
>  I think you should also look hard at avoiding this synchronous IPC
>>
> altogether.
>
> For the URL overriding in Chrome on Android we effectively need to
> 'park' the navigation while we check (on the UI thread) whether we want
> to cancel it or not. The only alternative I can think of is to modify
> the decidePolicyForNavigation flow to be asynchronous (WebKit supports
> it in FrameLoaderClientImpl::**dispatchDecidePolicyForNavigat**ionAction).
> Doing that would probably add some 'interesting' states for the renderer
> and navigation code.
>
>
https://chromiumcodereview.**appspot.com/9978015/<https://chromiumcodereview....
>

[mkosiba (inactive), 2012-04-24 08:59:04]

Abandoning this approach based on conversations with Darin. Chrome on Android
will be using a ResourceThrottle instead of browser_handles_top_level_requests.