Allow top-level navigation in extension pop-ups if it only triggers a download.

Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).

Before this CL, blocking of top-level navigations in extension pop-ups
was accomplished by routing them via OpenURL path and blocking (or
rather - silently dropping) CURRENT_TAB navigations via
ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
reasons:

1. This unnecessarily blocked navigations that end-up being treated
   as downloads (i.e. because HTTP response says Content-Disposition:
   attachment).  This was the root cause of the regression raised in
   https://crbug.com/646261

2. There are still some remaining issues in handling of POST requests
   via OpenURL path (e.g. dropping Content-Type header -
   https://crbug.com/648648).

3. In the long-term we want to rely less on process isolation
   accomplished via OpenURL - an exploited renderer process does not
   necessarily have to go through OpenURL path and can instead choose
   to use the regular, renderer-initiated path.

After this CL:

1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
   navigations in extension pop-ups (and background pages).

2. POST navigations do not go through OpenURL path anymore (i.e. this CL
   effectively reverts the essence of r407586).

   In the long-term we want to make all extension navigations to not go
   through OpenURL path, but this seems too risky to merge back to M54,
   so for now we only do #1 above (i.e. avoid OpenURL only for POST
   requests).

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c
Cr-Commit-Position: refs/heads/master@{#421287}

[Łukasz Anforowicz, 2016-09-20 19:17:44]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 19:18:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 19:58:45]

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

[commit-bot: I haz the power, 2016-09-20 19:58:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2016-09-20 20:33:01]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 20:33:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 21:36:28]

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

[commit-bot: I haz the power, 2016-09-20 21:36:29]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-22 21:12:50]

Description was changed from

==========
Allowing CURRENT_TAB navigations in ExtensionViewHost.

CURRENT_TAB navigations might result in a download - in this case
we want to allow them to pass through ExtensionViewHost::OpenURLFromTab
and only block non-downloads (aka actual navigations) later
(e.g. blocking them via
ExtensionViewHost::ShouldTransferNavigation(is_main_frame).

BUG=646261
==========

to

==========
Allow top-level navigation in extension pop-ups if it only triggers a download.

BUG=646261
==========

[Łukasz Anforowicz, 2016-09-22 21:37:32]

Description was changed from

==========
Allow top-level navigation in extension pop-ups if it only triggers a download.

BUG=646261
==========

to

==========
Allow top-level navigation in extension pop-ups if it only triggers a download.

BUG=646261
==========

[Łukasz Anforowicz, 2016-09-23 22:32:39]

Description was changed from

==========
Allow top-level navigation in extension pop-ups if it only triggers a download.

BUG=646261
==========

to

==========
Allow top-level navigation in extension pop-ups if it only triggers a download.

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-09-23 22:35:24]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 22:36:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-09-23 23:23:00]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 23:23:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-09-23 23:30:45]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 23:31:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-24 00:58:30]

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

[commit-bot: I haz the power, 2016-09-24 00:58:31]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-26 16:09:01]

Description was changed from

==========
Allow top-level navigation in extension pop-ups if it only triggers a download.

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).

Before this CL, blocking of top-level navigations in extension pop-ups
was accomplished by routing them via OpenURL path and blocking (or
rather - silently dropping) CURRENT_TAB navigations via
ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
reasons:

1. This unnecessarily blocked navigations that end-up being treated
   as downloads (i.e. because HTTP response says Content-Disposition:
   attachment).  This was the root cause of the regression raised in
   https://crbug.com/646261

2. There are still some remaining issues in handling of POST requests
   via OpenURL path (e.g. dropping Content-Type header -
   https://crbug.com/648648).

3. In the long-term we want to rely less on process isolation
   accomplished via OpenURL - an exploited renderer process does not
   necessarily have to go through OpenURL path and can instead choose
   to use the regular, renderer-initiated path.

After this CL:

1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
   navigations in extension pop-ups (and background pages).

2. POST navigations do not go through OpenURL path anymore (i.e. this CL
   effectively reverts the essence of r407586).

   In the long-term we want to make all extension navigations to not go
   through OpenURL path, but this seems too risky to merge back to M54,
   so for now we only do #1 above (i.e. avoid OpenURL only for POST
   requests).

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-09-26 16:18:46]

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

[Łukasz Anforowicz, 2016-09-26 16:18:47]

Charlie, could you take a look please?  This CL implements the option we
discussed on Friday.  I've verified that this CL alone is sufficient to fix the
regression in M54 (i.e. we can just merge this CL; the Content-Type fix can
probably stay in M55).

[Charlie Reis, 2016-09-26 17:27:51]

On 2016/09/26 16:18:47, Łukasz Anforowicz wrote:
> Charlie, could you take a look please?  This CL implements the option we
> discussed on Friday.  I've verified that this CL alone is sufficient to fix
the
> regression in M54 (i.e. we can just merge this CL; the Content-Type fix can
> probably stay in M55).

Great!  LGTM with minor nits.

(Certainly touches a lot of files for a small functional change!  I see there's
not really a way around that, so we'll just need to reassure TPMs it's a fairly
targeted behavior change.)

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:840:
LOG(INFO) << "Waiting until popup navigation stops...";
Did you mean to leave these LOG(INFO) statements in?  I'm ok with it if you
think it will help diagnose test failures, though I'm not sure if we need all of
them.

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:927:
"form.submit();"
This function looks the same as TestPopupNavigationViaGet except for these 3
lines of script.  Maybe let the rest share code, given that they're already
written as helper functions?

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:957:
false /* == expecting_navigation_success */);
nit: No need for the ==

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1035:
#if !defined(OS_CHROMEOS)
nit: Add a comment explaining why this is skipped on ChromeOS.

[Łukasz Anforowicz, 2016-09-26 18:27:06]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 18:27:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-09-26 18:30:23]

Thanks for the feedback - I've addressed the comments in the latest patchset.

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:840:
LOG(INFO) << "Waiting until popup navigation stops...";
On 2016/09/26 17:27:50, Charlie Reis (slow) wrote:
> Did you mean to leave these LOG(INFO) statements in?  I'm ok with it if you
> think it will help diagnose test failures, though I'm not sure if we need all
of
> them.

When I was working on the tests, I had them hang (i.e. when navigation is
blocked, then downloads never complete).  Having the extra logging helped to
figure out which part of the test was hanging - I think this will also be true
for cases where the tests time out on a try bot.

That said, maybe some of the logging statements are not really needed - for
example I've removed the one talking about opening the pop-up.

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:927:
"form.submit();"
On 2016/09/26 17:27:50, Charlie Reis (slow) wrote:
> This function looks the same as TestPopupNavigationViaGet except for these 3
> lines of script.  Maybe let the rest share code, given that they're already
> written as helper functions?

Done.

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:957:
false /* == expecting_navigation_success */);
On 2016/09/26 17:27:50, Charlie Reis (slow) wrote:
> nit: No need for the ==

Done.

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1035:
#if !defined(OS_CHROMEOS)
On 2016/09/26 17:27:50, Charlie Reis (slow) wrote:
> nit: Add a comment explaining why this is skipped on ChromeOS.

Done.

I've also felt that maybe !defined(OS_CHROMEOS) is a bit too fragile - I was
able to replace this condition with a more direct test for download shelf
feature support (I am keeping my fingers crossed that this will also pass on
ChromeOS trybots).

[Łukasz Anforowicz, 2016-09-26 18:31:27]

lukasza@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Łukasz Anforowicz, 2016-09-26 18:31:28]

+rdevlin.cronin - PTAL at:
  chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
  chrome/browser/extensions/extension_view_host.cc
  chrome/browser/extensions/extension_view_host.h
  chrome/browser/extensions/process_management_browsertest.cc

[Charlie Reis, 2016-09-26 19:00:48]

Thanks!

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1035:
#if !defined(OS_CHROMEOS)
On 2016/09/26 18:30:23, Łukasz Anforowicz wrote:
> On 2016/09/26 17:27:50, Charlie Reis (slow) wrote:
> > nit: Add a comment explaining why this is skipped on ChromeOS.
> 
> Done.
> 
> I've also felt that maybe !defined(OS_CHROMEOS) is a bit too fragile - I was
> able to replace this condition with a more direct test for download shelf
> feature support (I am keeping my fingers crossed that this will also pass on
> ChromeOS trybots).

Cool, I like it.

[commit-bot: I haz the power, 2016-09-26 19:50:02]

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

[commit-bot: I haz the power, 2016-09-26 19:50:03]

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

[Devlin, 2016-09-26 20:57:06]

Mostly all nits

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:828:
void VerifyNavigationResult(WebContents* popup,
Could this be private?

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:840:
LOG(INFO) << "Waiting until popup navigation stops...";
nit: remove debugging logs

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:848: <<
"Navigation to " << target_url.spec()
I think you can avoid the .spec() calls here - log streams can handle printing
GURLs.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:872:
void TestPopupNavigationViaGet(GURL target_url,
const GURL&s

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:909:
"setTimeout("
\n

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:910: " 
function(){ window.domAutomationController.send(true); },"
space after function()
\n

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911: " 
0);";
no indentation

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:914:
bool ignored;
init ignored

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:933: //
Test verifies if an extension pop-up can be navigated to a web page.
s/Test verifies if/Tests that (and add the expected result, so here "that an
extension popup can't be navigated to a web page").

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:937:
false /* expecting_navigation_success */);
with how frequent this bool is used, maybe make an enum for readability?

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:941:
IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, WebpageViaPost) {
Browsertests are expensive.  Can we combine the ViaGet/ViaPosts into one test
per scenario?

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/process_management_browsertest.cc (right):

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/process_management_browsertest.cc:284: "  function(){
window.domAutomationController.send(true); },"
same formatting nits as other test's string script

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/process_management_browsertest.cc:288: bool ignored;
init

https://codereview.chromium.org/2352083003/diff/140001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json
(right):

https://codereview.chromium.org/2352083003/diff/140001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json:2:
"name": "Popup with iframe",
not an iframe, right?

[Łukasz Anforowicz, 2016-09-26 22:05:11]

Devlin, can you take another look please?

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1035:
#if !defined(OS_CHROMEOS)
On 2016/09/26 19:00:48, Charlie Reis (slow) wrote:
> On 2016/09/26 18:30:23, Łukasz Anforowicz wrote:
> > On 2016/09/26 17:27:50, Charlie Reis (slow) wrote:
> > > nit: Add a comment explaining why this is skipped on ChromeOS.
> > 
> > Done.
> > 
> > I've also felt that maybe !defined(OS_CHROMEOS) is a bit too fragile - I was
> > able to replace this condition with a more direct test for download shelf
> > feature support (I am keeping my fingers crossed that this will also pass on
> > ChromeOS trybots).
> 
> Cool, I like it.

Actually, this didn't work...  I've built CrOS browser_tests and the test indeed
doesn't trigger the download shelf on this OS - there is a download notification
in bottom right screen corner instead.  Surprisingly,
browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF) returns true,
so I think I need to go back to !defined(OS_CHROMEOS) as the condition here.

Visibility of the shelf is not critical for this test, but it still feels fairly
important / integral to the expected behavior for the extension described in
https://crbug.com/646261.  I am therefore hesitant to remove this verification
just because it doesn't work for ChromeOS.  I hope the ifdef approach here is
okay.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:828:
void VerifyNavigationResult(WebContents* popup,
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> Could this be private?

Actually, after addressing Charlie's feedback there is only one called of
VerifyNavigationResult, so I've decided to just inline this method.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:840:
LOG(INFO) << "Waiting until popup navigation stops...";
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> nit: remove debugging logs

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:848: <<
"Navigation to " << target_url.spec()
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> I think you can avoid the .spec() calls here - log streams can handle printing
> GURLs.

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:872:
void TestPopupNavigationViaGet(GURL target_url,
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> const GURL&s

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:909:
"setTimeout("
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> \n

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:910: " 
function(){ window.domAutomationController.send(true); },"
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> space after function()
> \n

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911: " 
0);";
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> no indentation

I don't understand.  The arguments of setTimeout should be aligned, so that they
begin in the same column right?  In other words, "0" has to be in the same
column as "f" from "function", right?

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:914:
bool ignored;
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> init ignored

Done (+ renamed the variable to be more descriptive).

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:933: //
Test verifies if an extension pop-up can be navigated to a web page.
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> s/Test verifies if/Tests that (and add the expected result, so here "that an
> extension popup can't be navigated to a web page").

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:937:
false /* expecting_navigation_success */);
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> with how frequent this bool is used, maybe make an enum for readability?

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:941:
IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, WebpageViaPost) {
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> Browsertests are expensive.  Can we combine the ViaGet/ViaPosts into one test
> per scenario?

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/process_management_browsertest.cc (right):

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/process_management_browsertest.cc:284: "  function(){
window.domAutomationController.send(true); },"
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> same formatting nits as other test's string script

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/process_management_browsertest.cc:288: bool ignored;
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> init

Done.

https://codereview.chromium.org/2352083003/diff/140001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json
(right):

https://codereview.chromium.org/2352083003/diff/140001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json:2:
"name": "Popup with iframe",
On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> not an iframe, right?

Ooops.  Done.

[Łukasz Anforowicz, 2016-09-26 22:05:17]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 22:06:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 23:22:06]

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

[commit-bot: I haz the power, 2016-09-26 23:22:10]

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

[Devlin, 2016-09-27 15:57:28]

extensions lgtm

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911: " 
0);";
On 2016/09/26 22:05:11, Łukasz Anforowicz wrote:
> On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> > no indentation
> 
> I don't understand.  The arguments of setTimeout should be aligned, so that
they
> begin in the same column right?  In other words, "0" has to be in the same
> column as "f" from "function", right?

Whoops, misread this for some reason.  The 0 should line with the 'f', but both
should be indented 4, rather than 2.  (Also for the process_manager_browsertest
one.)

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:877:
content::WebContentsDestroyedWatcher popup_destruction_watcher(popup);
Is this used?

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:921:
EXPECT_TRUE(actions_bar->HidePopup());
nit: Just inline actions_bar

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:965: //
GET should automagically start working for downloads.
Should there be a TODO: Enable the test for Get once these are resolved?

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/process_management_browsertest.cc (right):

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_management_browsertest.cc:290:
content::WebContentsDestroyedWatcher tab_destruction_watcher(web_contents);
used?

[Łukasz Anforowicz, 2016-09-27 16:33:14]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 16:33:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-09-27 17:00:50]

Thanks for the review.

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:911: " 
0);";
On 2016/09/27 15:57:28, Devlin (catching up) wrote:
> On 2016/09/26 22:05:11, Łukasz Anforowicz wrote:
> > On 2016/09/26 20:57:05, Devlin (catching up) wrote:
> > > no indentation
> > 
> > I don't understand.  The arguments of setTimeout should be aligned, so that
> they
> > begin in the same column right?  In other words, "0" has to be in the same
> > column as "f" from "function", right?
> 
> Whoops, misread this for some reason.  The 0 should line with the 'f', but
both
> should be indented 4, rather than 2.  (Also for the
process_manager_browsertest
> one.)

Got it - done.

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:877:
content::WebContentsDestroyedWatcher popup_destruction_watcher(popup);
On 2016/09/27 15:57:28, Devlin (catching up) wrote:
> Is this used?

Ooops - thanks for catching this.  My mistake - we used to pass popup =
popup_destruction_watcher.web_contents() to the verification method - I stopped
doing this in the latest patchset... :-(  Fixed.

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:921:
EXPECT_TRUE(actions_bar->HidePopup());
On 2016/09/27 15:57:28, Devlin (catching up) wrote:
> nit: Just inline actions_bar

Done.

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:965: //
GET should automagically start working for downloads.
On 2016/09/27 15:57:28, Devlin (catching up) wrote:
> Should there be a TODO: Enable the test for Get once these are resolved?

Done.

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/process_management_browsertest.cc (right):

https://codereview.chromium.org/2352083003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_management_browsertest.cc:290:
content::WebContentsDestroyedWatcher tab_destruction_watcher(web_contents);
On 2016/09/27 15:57:28, Devlin (catching up) wrote:
> used?

Not used and not needed - removed in the latest patchset.

[Łukasz Anforowicz, 2016-09-27 17:07:33]

lukasza@chromium.org changed reviewers:
+ sky@chromium.org

[Łukasz Anforowicz, 2016-09-27 17:07:34]

sky@ - can you please do an OWNERS review for:
    chrome/browser/prerender/prerender_contents.cc
    chrome/renderer/chrome_content_renderer_client.cc

This CL basically reverts essence of r407586, but at the same time ensures that
transfer logic will preserve desired semantics (i.e. blocking top-level
navigations in extension pop-ups and background pages).

[Łukasz Anforowicz, 2016-09-27 17:07:42]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 17:07:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-09-27 18:45:15]

LGTM

[commit-bot: I haz the power, 2016-09-27 18:45:39]

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

[commit-bot: I haz the power, 2016-09-27 18:45:40]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-27 18:55:17]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-09-27 18:55:17]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2352083003/#ps220001
(title: "Changed bug reference (in a TODO comment) to a fresh bug.")

[commit-bot: I haz the power, 2016-09-27 18:55:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 19:04:40]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-09-27 19:09:04]

Description was changed from

==========
Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).

Before this CL, blocking of top-level navigations in extension pop-ups
was accomplished by routing them via OpenURL path and blocking (or
rather - silently dropping) CURRENT_TAB navigations via
ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
reasons:

1. This unnecessarily blocked navigations that end-up being treated
   as downloads (i.e. because HTTP response says Content-Disposition:
   attachment).  This was the root cause of the regression raised in
   https://crbug.com/646261

2. There are still some remaining issues in handling of POST requests
   via OpenURL path (e.g. dropping Content-Type header -
   https://crbug.com/648648).

3. In the long-term we want to rely less on process isolation
   accomplished via OpenURL - an exploited renderer process does not
   necessarily have to go through OpenURL path and can instead choose
   to use the regular, renderer-initiated path.

After this CL:

1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
   navigations in extension pop-ups (and background pages).

2. POST navigations do not go through OpenURL path anymore (i.e. this CL
   effectively reverts the essence of r407586).

   In the long-term we want to make all extension navigations to not go
   through OpenURL path, but this seems too risky to merge back to M54,
   so for now we only do #1 above (i.e. avoid OpenURL only for POST
   requests).

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).

Before this CL, blocking of top-level navigations in extension pop-ups
was accomplished by routing them via OpenURL path and blocking (or
rather - silently dropping) CURRENT_TAB navigations via
ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
reasons:

1. This unnecessarily blocked navigations that end-up being treated
   as downloads (i.e. because HTTP response says Content-Disposition:
   attachment).  This was the root cause of the regression raised in
   https://crbug.com/646261

2. There are still some remaining issues in handling of POST requests
   via OpenURL path (e.g. dropping Content-Type header -
   https://crbug.com/648648).

3. In the long-term we want to rely less on process isolation
   accomplished via OpenURL - an exploited renderer process does not
   necessarily have to go through OpenURL path and can instead choose
   to use the regular, renderer-initiated path.

After this CL:

1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
   navigations in extension pop-ups (and background pages).

2. POST navigations do not go through OpenURL path anymore (i.e. this CL
   effectively reverts the essence of r407586).

   In the long-term we want to make all extension navigations to not go
   through OpenURL path, but this seems too risky to merge back to M54,
   so for now we only do #1 above (i.e. avoid OpenURL only for POST
   requests).

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c
Cr-Commit-Position: refs/heads/master@{#421287}
==========

[commit-bot: I haz the power, 2016-09-27 19:09:05]

Patchset 12 (id:??) landed as
https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c
Cr-Commit-Position: refs/heads/master@{#421287}

[Łukasz Anforowicz, 2016-09-27 22:22:19]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2375623003/ by lukasza@chromium.org.

The reason for reverting is: It seems that tests introduced in this CL are
failing on some non-CQ bots:
https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds...
(and also 4654 and 4655). .