Provide default implementation for WebContentsDelegate::OpenURLFromTab.

Provide default implementation for WebContentsDelegate::OpenURLFromTab.

With OOPIF, when a frame in a background page is navigated cross-site
from the renderer, the cross-process transfer involves using
WebContentsDelegate::OpenURLFromTab to reissue the navigation, but the
delegate in this case is extensions::ExtensionHost, which doesn't
implement that method.  As a result, the default delegate
implementation is used, which just returns nullptr and drops the
request.

This CL provides a better default implementation for
WebContentsDelegate::OpenURLFromTab, borrowing it from
Shell::OpenURLFromTab.  This fixes the issue with background pages and
will hopefully prevent this type of issue from happening in other
contexts.

BUG=560510
TEST=ExtensionApiTest.(WindowOpenPopupIframe|ContentScriptExtensionProcess|DontInjectContentScriptsInBackgroundPages) with --isolate-extensions or --site-per-process

Committed: https://crrev.com/35229c1ac03fd2707c8612973f6dd54a5150e9c4
Cr-Commit-Position: refs/heads/master@{#361724}

[alexmos, 2015-11-24 23:38:49]

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

[alexmos, 2015-11-24 23:38:49]

Charlie, can you please take a look?  This is step 1 of the OpenURLFromTab issue
and fixes iframes in background pages.  Step 2 will fix
ExtensionViewHost::OpenURLFromTab to handle CURRENT_TAB (only for transfer URLs)
by using this implementation, which will fix the browser actions bug.

This allows us to reenable three tests in chromium.fyi.json.  I noticed there
are a few more extensions tests passing in --site-per-process unrelated to this
CL, which I'll reenable separately.

[Charlie Reis, 2015-11-25 00:05:28]

Thanks, LGTM with nits.

On 2015/11/24 23:38:49, alexmos wrote:
> Charlie, can you please take a look?  This is step 1 of the OpenURLFromTab
issue
> and fixes iframes in background pages.  Step 2 will fix
> ExtensionViewHost::OpenURLFromTab to handle CURRENT_TAB (only for transfer
URLs)
> by using this implementation, which will fix the browser actions bug.

Seems reasonable.  There's a slight risk that this will make certain navigations
possible that intentionally weren't possible before, but that would be pretty
obscure.  Any features that depended on that for security should have tests. 
I'm ok with adding a default implementation here.

> This allows us to reenable three tests in chromium.fyi.json.  I noticed there
> are a few more extensions tests passing in --site-per-process unrelated to
this
> CL, which I'll reenable separately.

You'll probably want to coordinate with Łukasz, who has started
https://codereview.chromium.org/1466783002/ for that.  (Note that I had a
warning about some flaky tests there.)

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:25: // CURRENT_TAB is the only
one we implement for now.
// This default implementation only handles CURRENT_TAB.

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:30:
load_url_params.source_site_instance = params.source_site_instance;
Let's make sure we cover all the parameters from OpenURLParams (which have
matches in LoadURLParams, like redirect_chain), and in the same order as they're
declared.

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:43:
load_url_params.is_renderer_initiated = true;
I can't figure out why shell.cc had this obscure approach to setting
is_renderer_initiated (added in https://codereview.chromium.org/22904002).  It's
false by default, so it seems like it will always match
params.is_renderer_initiated by the end of this method.  Can we just set it
directly as in the cases above, independent of whether there's a
transferred_global_request_id?

[alexmos, 2015-11-25 19:15:39]

Thanks!

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:25: // CURRENT_TAB is the only
one we implement for now.
On 2015/11/25 00:05:28, Charlie Reis wrote:
> // This default implementation only handles CURRENT_TAB.

Done.

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:30:
load_url_params.source_site_instance = params.source_site_instance;
On 2015/11/25 00:05:28, Charlie Reis wrote:
> Let's make sure we cover all the parameters from OpenURLParams (which have
> matches in LoadURLParams, like redirect_chain), and in the same order as
they're
> declared.

I've added redirect_chain and rearranged them.

I've also added the browser_initiated_post_data translation, but I'm not quite
sure of the cases when it needs to be copied.  I ended up mirroring the
condition from LoadURLInContents in browser_navigator.cc , and also setting
load_type accordingly for posts -- let me know if that makes sense.

I've left out the user_gesture -> has_user_gesture translation. The
LoadURLParams flag is only declared and used for Android, so I'm not sure it's
needed here, but let me know if you think it's better to include it.

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:43:
load_url_params.is_renderer_initiated = true;
On 2015/11/25 00:05:28, Charlie Reis wrote:
> I can't figure out why shell.cc had this obscure approach to setting
> is_renderer_initiated (added in https://codereview.chromium.org/22904002). 
It's
> false by default, so it seems like it will always match
> params.is_renderer_initiated by the end of this method.  Can we just set it
> directly as in the cases above, independent of whether there's a
> transferred_global_request_id?

Done.  Same seems to be true for transferred_global_request_id (the default is
GlobalRequestID(), so no reason to only set it if it's not GlobalRequestID(). 
I'm setting both directly now.

I think the source of this may be just that it was copied over from
LoadURLInContents in browser_navigator.cc, and there this logic was added in
https://chromiumcodereview.appspot.com/10830144 (which you reviewed :) ).

[Charlie Reis, 2015-11-25 19:34:25]

LGTM.

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:30:
load_url_params.source_site_instance = params.source_site_instance;
On 2015/11/25 19:15:39, alexmos wrote:
> On 2015/11/25 00:05:28, Charlie Reis wrote:
> > Let's make sure we cover all the parameters from OpenURLParams (which have
> > matches in LoadURLParams, like redirect_chain), and in the same order as
> they're
> > declared.
> 
> I've added redirect_chain and rearranged them.
> 
> I've also added the browser_initiated_post_data translation, but I'm not quite
> sure of the cases when it needs to be copied.  I ended up mirroring the
> condition from LoadURLInContents in browser_navigator.cc , and also setting
> load_type accordingly for posts -- let me know if that makes sense.
> 

Seems right.


> I've left out the user_gesture -> has_user_gesture translation. The
> LoadURLParams flag is only declared and used for Android, so I'm not sure it's
> needed here, but let me know if you think it's better to include it.

That would only affect WebContentsDelegates on Android that don't override
OpenURLFromTab.  Interestingly, WebContentsDelegateAndroid already overrides
OpenURLFromTab and doesn't copy over user_gesture.  Not sure if that's a bug on
their part, but at least it's fail closed, and it probably means we don't need
to worry about it here.

https://codereview.chromium.org/1473423002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.cc:43:
load_url_params.is_renderer_initiated = true;
On 2015/11/25 19:15:39, alexmos wrote:
> On 2015/11/25 00:05:28, Charlie Reis wrote:
> > I can't figure out why shell.cc had this obscure approach to setting
> > is_renderer_initiated (added in https://codereview.chromium.org/22904002). 
> It's
> > false by default, so it seems like it will always match
> > params.is_renderer_initiated by the end of this method.  Can we just set it
> > directly as in the cases above, independent of whether there's a
> > transferred_global_request_id?
> 
> Done.  Same seems to be true for transferred_global_request_id (the default is
> GlobalRequestID(), so no reason to only set it if it's not GlobalRequestID(). 
> I'm setting both directly now.
> 
> I think the source of this may be just that it was copied over from
> LoadURLInContents in browser_navigator.cc, and there this logic was added in
> https://chromiumcodereview.appspot.com/10830144 (which you reviewed :) ). 

Ah, yes, nice find.  Specifically:
https://codereview.chromium.org/10830144/diff/2003/chrome/browser/ui/browser_...
That seems to have gone through contortions due to the old variations on LoadURL
/ LoadURLFromRenderer / TransferURL.  So we could have simplified that logic
further at the time.

[alexmos, 2015-11-25 19:48:30]

Thanks!  Nasko points out that it may be worthwhile to have a unified place
somewhere to convert OpenURLParams into LoadURLParams.  That would avoid the
duplicate logic between  browser_navigator.cc and the new function here, and I
see that BlimpEngineSession::OpenURLFromTab is also doing this kind of copying
and can probably use this conversion function as well.

I think I'll land this as is (to not get into modifying Blimp here) and try this
out in a separate CL.

[alexmos, 2015-11-25 19:48:42]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-11-25 19:50:13]

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

[commit-bot: I haz the power, 2015-11-25 20:10:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-25 20:12:02]

Description was changed from

==========
Provide default implementation for WebContentsDelegate::OpenURLFromTab.

With OOPIF, when a frame in a background page is navigated cross-site
from the renderer, the cross-process transfer involves using
WebContentsDelegate::OpenURLFromTab to reissue the navigation, but the
delegate in this case is extensions::ExtensionHost, which doesn't
implement that method.  As a result, the default delegate
implementation is used, which just returns nullptr and drops the
request.

This CL provides a better default implementation for
WebContentsDelegate::OpenURLFromTab, borrowing it from
Shell::OpenURLFromTab.  This fixes the issue with background pages and
will hopefully prevent this type of issue from happening in other
contexts.

BUG=560510
TEST=ExtensionApiTest.(WindowOpenPopupIframe|ContentScriptExtensionProcess|DontInjectContentScriptsInBackgroundPages)
with --isolate-extensions or --site-per-process
==========

to

==========
Provide default implementation for WebContentsDelegate::OpenURLFromTab.

With OOPIF, when a frame in a background page is navigated cross-site
from the renderer, the cross-process transfer involves using
WebContentsDelegate::OpenURLFromTab to reissue the navigation, but the
delegate in this case is extensions::ExtensionHost, which doesn't
implement that method.  As a result, the default delegate
implementation is used, which just returns nullptr and drops the
request.

This CL provides a better default implementation for
WebContentsDelegate::OpenURLFromTab, borrowing it from
Shell::OpenURLFromTab.  This fixes the issue with background pages and
will hopefully prevent this type of issue from happening in other
contexts.

BUG=560510
TEST=ExtensionApiTest.(WindowOpenPopupIframe|ContentScriptExtensionProcess|DontInjectContentScriptsInBackgroundPages)
with --isolate-extensions or --site-per-process

Committed: https://crrev.com/35229c1ac03fd2707c8612973f6dd54a5150e9c4
Cr-Commit-Position: refs/heads/master@{#361724}
==========

[commit-bot: I haz the power, 2015-11-25 20:12:03]

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