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

Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).
(this is relanding a slightly tweaked r421287).

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 (tracked in https://crbug.com/650694) 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 #2
   above (i.e. avoid OpenURL only for POST requests).

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=sky@chromium.org, rdevlin.cronin@chromium.org

Committed: https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7
Cr-Commit-Position: refs/heads/master@{#421645}

[Łukasz Anforowicz, 2016-09-28 17:33:30]

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

[commit-bot: I haz the power, 2016-09-28 17:33:49]

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-28 18:21:20]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

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

[commit-bot: I haz the power, 2016-09-28 19:32:09]

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-28 20:28:46]

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

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

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-09-28 21:00:56]

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

[Łukasz Anforowicz, 2016-09-28 21:00:57]

Charlie, could you PTAL?

This CL is basically the same as what you have already reviewed at
https://codereview.chromium.org/2352083003, but with explicit waiting for the
popup to close - the diff can be seen here:
https://codereview.chromium.org/2377833002/diff2/1:40001/chrome/browser/exten...

The explicit wait makes the test pass on my local Windows machine (it used to
time out before).  One of win10_chromium_x64_rel_ng tryjobs has already came
back green and I am optimistic that the other 2 tryjobs will also be green. 
Some extra information about the test trouble is at https://crbug.com/651106

There should be no diffs (compared to the original CL) outside of
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc.  The
diff between patchset #1 and #2 in
content/browser/web_contents/web_contents_impl.cc is caused by accidentally
rebasing (I've uploaded patchset #1 from a linux box, but uploaded patchset #2
from a windows box that was synced to a slightly different baseline).  Patchset
#3 is caused by me forgetting to upload new test files from the windows machine.

[Łukasz Anforowicz, 2016-09-28 21:07:15]

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 (tracked in https://crbug.com/650694) 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 #2
   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).
(this is relanding a slightly tweaked r421287).

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 (tracked in https://crbug.com/650694) 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 #2
   above (i.e. avoid OpenURL only for POST requests).

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=sky@chromium.org, rdevlin.cronin@chromium.org
==========

[Łukasz Anforowicz, 2016-09-28 21:07:58]

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

[Łukasz Anforowicz, 2016-09-28 21:07:59]

sky@ + rdevlin.cronin@, I hope it is okay to just add you to TBR - you've
already reviewed the original CL, and this CL just adds an explicit waiting to
the test as seen here:
https://codereview.chromium.org/2377833002/diff2/1:40001/chrome/browser/exten...

additionally, the change (compared to the already reviewed CL) is outside of
files reviewed by sky@ in the original CL + I see that rdevlin.cronin@ is OOO
until Thursday, so I hope that a review just by creis@ will be sufficient.

[Charlie Reis, 2016-09-28 21:22:59]

Yep, diff looks good.  RS LGTM for the rest.  Thanks for the test fix!

[Łukasz Anforowicz, 2016-09-28 21:29:25]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-09-28 21:29:49]

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-28 21:36:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-28 21:38:39]

Description was changed from

==========
Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).
(this is relanding a slightly tweaked r421287).

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 (tracked in https://crbug.com/650694) 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 #2
   above (i.e. avoid OpenURL only for POST requests).

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=sky@chromium.org, rdevlin.cronin@chromium.org
==========

to

==========
Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).
(this is relanding a slightly tweaked r421287).

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 (tracked in https://crbug.com/650694) 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 #2
   above (i.e. avoid OpenURL only for POST requests).

BUG=646261
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=sky@chromium.org, rdevlin.cronin@chromium.org

Committed: https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7
Cr-Commit-Position: refs/heads/master@{#421645}
==========

[commit-bot: I haz the power, 2016-09-28 21:38:41]

Patchset 3 (id:??) landed as
https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7
Cr-Commit-Position: refs/heads/master@{#421645}

[Devlin, 2016-09-29 17:05:41]

retroactive lgtm w/ tiny nits that can be fixed in a followup.

https://codereview.chromium.org/2377833002/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/browser_action/popup_with_form/other_page.html
(right):

https://codereview.chromium.org/2377833002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/browser_action/popup_with_form/other_page.html:6:

nit: extra newline here

https://codereview.chromium.org/2377833002/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.html
(right):

https://codereview.chromium.org/2377833002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.html:14:

and here