ExtensionNavigationThrottle: Enforce the same rules on redirect as we

ExtensionNavigationThrottle: Enforce the same rules on redirect as we
would on the initial navigation.

TEST=ExtensionResourceRequestPolicyTest.IframeNavigateToInaccessibleViaServerRedirect
BUG=721568
Review-Url: https://codereview.chromium.org/2881733006
Cr-Commit-Position: refs/heads/master@{#474044}
Committed: https://chromium.googlesource.com/chromium/src/+/7a719cdbf12822eb9a7be0879273e6b716b9273b

[ncarter (slow), 2017-05-12 23:31:04]

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

[commit-bot: I haz the power, 2017-05-12 23:32:21]

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, 2017-05-13 00:47:00]

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

[commit-bot: I haz the power, 2017-05-13 00:47:01]

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_...)

[ncarter (slow), 2017-05-17 20:15:02]

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

[commit-bot: I haz the power, 2017-05-17 20:16:44]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-05-17 20:16:50]

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

[ncarter (slow), 2017-05-17 20:16:50]

Devlin, please review

[ncarter (slow), 2017-05-17 21:15:05]

hmm, the EditingTest.RichTextMoveByLine failure on cros may be legit.

[ncarter (slow), 2017-05-17 21:15:49]

On 2017/05/17 21:15:05, ncarter (slow) wrote:
> hmm, the EditingTest.RichTextMoveByLine failure on cros may be legit.

or not.

[commit-bot: I haz the power, 2017-05-17 21:53:57]

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

[commit-bot: I haz the power, 2017-05-17 21:53:58]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-18 15:22:44]

https://codereview.chromium.org/2881733006/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_navigation_throttle_unittest.cc
(right):

https://codereview.chromium.org/2881733006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_navigation_throttle_unittest.cc:87:
NavigationThrottle::ThrottleCheckResult expected_redirect_result =
This is a bit strange, because now |expected_result| isn't always the actual
expected result.  I would suggest either:
a) passing in a bool "expect_succeed", and then constructing a
direct_navigation_expected_result = success ? PROCEED : BLOCK_REQUEST and a
redirect_navigation_expected_result = success ? PROCEED : CANCEL, or
b) passing in both a direct and redirect expected result.

Since it looks like the expected result is currently always either proceed or
block, a) would probably be sufficient and is a little less verbose.

https://codereview.chromium.org/2881733006/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2881733006/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:165: // BLOCK_REQUEST is on
redirect does not work without PlzNavigate, so
s/is on/on

Should this be a TODO?  And should we be returning BLOCK_REQUEST when
PlzNavigate is enabled?

https://codereview.chromium.org/2881733006/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.h (right):

https://codereview.chromium.org/2881733006/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.h:26: ThrottleCheckResult
WillStartOrRedirectRequest();
Should this be private?

[ncarter (slow), 2017-05-22 22:54:45]

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

[commit-bot: I haz the power, 2017-05-22 22:55:18]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-05-22 22:55:59]

https://codereview.chromium.org/2881733006/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_navigation_throttle_unittest.cc
(right):

https://codereview.chromium.org/2881733006/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_navigation_throttle_unittest.cc:87:
NavigationThrottle::ThrottleCheckResult expected_redirect_result =
On 2017/05/18 15:22:44, Devlin (OOO Monday 22.May) wrote:
> This is a bit strange, because now |expected_result| isn't always the actual
> expected result.  I would suggest either:
> a) passing in a bool "expect_succeed", and then constructing a
> direct_navigation_expected_result = success ? PROCEED : BLOCK_REQUEST and a
> redirect_navigation_expected_result = success ? PROCEED : CANCEL, or
> b) passing in both a direct and redirect expected result.
> 
> Since it looks like the expected result is currently always either proceed or
> block, a) would probably be sufficient and is a little less verbose.

Really, it's just a limitation of the throttle system in non-plznavigate that we
need to round the BLOCKs down to CANCELs currently.

My hope is that in the future, we'll be able to use the same errors for
WillRedirect as we do for WillStartRequest.

I've added some TODO comments w/ bug links to clarify what's going on.

https://codereview.chromium.org/2881733006/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2881733006/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:165: // BLOCK_REQUEST is on
redirect does not work without PlzNavigate, so
On 2017/05/18 15:22:44, Devlin (OOO Monday 22.May) wrote:
> s/is on/on
> 
> Should this be a TODO?  And should we be returning BLOCK_REQUEST when
> PlzNavigate is enabled?

Added a TODO.

The least risky thing to do, I think, is to explore the switch to non-CANCEL
results once PlzNavigate is fully shipped, which is really soon.

https://codereview.chromium.org/2881733006/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.h (right):

https://codereview.chromium.org/2881733006/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.h:26: ThrottleCheckResult
WillStartOrRedirectRequest();
On 2017/05/18 15:22:44, Devlin (OOO Monday 22.May) wrote:
> Should this be private?

Done.

[commit-bot: I haz the power, 2017-05-22 23:05:51]

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

[commit-bot: I haz the power, 2017-05-22 23:05:52]

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

[ncarter (slow), 2017-05-22 23:29:53]

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

[commit-bot: I haz the power, 2017-05-22 23:30:31]

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, 2017-05-23 00:24:11]

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

[commit-bot: I haz the power, 2017-05-23 00:24:12]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-23 17:11:33]

lgtm

[ncarter (slow), 2017-05-23 20:44:48]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2017-05-23 20:45:38]

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, 2017-05-23 20:51:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495572288084990,
"parent_rev": "d2a2b5263102748b3884a1c19b1faaed9e8ae58a", "commit_rev":
"7a719cdbf12822eb9a7be0879273e6b716b9273b"}

[commit-bot: I haz the power, 2017-05-23 20:51:45]

Description was changed from

==========
ExtensionNavigationThrottle: Enforce the same rules on redirect as we
would on the initial navigation.

TEST=ExtensionResourceRequestPolicyTest.IframeNavigateToInaccessibleViaServerRedirect
BUG=721568
==========

to

==========
ExtensionNavigationThrottle: Enforce the same rules on redirect as we
would on the initial navigation.

TEST=ExtensionResourceRequestPolicyTest.IframeNavigateToInaccessibleViaServerRedirect
BUG=721568

Review-Url: https://codereview.chromium.org/2881733006
Cr-Commit-Position: refs/heads/master@{#474044}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a719cdbf12822eb9a7be0879273...
==========

[commit-bot: I haz the power, 2017-05-23 20:51:46]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7a719cdbf12822eb9a7be0879273...