Refactor of ExtensionNavigationThrottle

Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation (this fixes 661324)

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

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

Review-Url: https://codereview.chromium.org/2830893002
Cr-Commit-Position: refs/heads/master@{#473717}
Committed: https://chromium.googlesource.com/chromium/src/+/6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a

[ncarter (slow), 2017-04-19 22:00:26]

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

[commit-bot: I haz the power, 2017-04-19 22:01:08]

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-04-19 22:10:59]

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

[commit-bot: I haz the power, 2017-04-19 22:11:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ncarter (slow), 2017-04-19 22:46:53]

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

[commit-bot: I haz the power, 2017-04-19 22:47:33]

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-04-19 22:59:49]

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

[commit-bot: I haz the power, 2017-04-19 22:59:50]

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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2017-04-21 21:53:39]

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

[commit-bot: I haz the power, 2017-04-21 21:54:16]

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-04-21 22:19:43]

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

[commit-bot: I haz the power, 2017-04-21 22:19: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_...)

[ncarter (slow), 2017-04-24 22:45:42]

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

[commit-bot: I haz the power, 2017-04-24 22:46:11]

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-04-24 23:29:35]

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

[commit-bot: I haz the power, 2017-04-24 23:29:36]

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

[ncarter (slow), 2017-04-25 19:49:53]

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

[commit-bot: I haz the power, 2017-04-25 19:50:23]

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-04-25 20:00:31]

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

[commit-bot: I haz the power, 2017-04-25 20:01:07]

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-04-25 20:42:59]

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

[commit-bot: I haz the power, 2017-04-25 20:43:00]

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-04-26 21:26:38]

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

[commit-bot: I haz the power, 2017-04-26 21:27:07]

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-04-26 22:04:25]

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

[commit-bot: I haz the power, 2017-04-26 22:04:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2017-04-26 22:25:30]

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

[commit-bot: I haz the power, 2017-04-26 22:26:30]

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-04-26 23:21:48]

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

[commit-bot: I haz the power, 2017-04-26 23:21:49]

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

[ncarter (slow), 2017-04-27 19:18:21]

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

[commit-bot: I haz the power, 2017-04-27 19:32:39]

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-04-27 19:38:02]

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

[commit-bot: I haz the power, 2017-04-27 19:38:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2017-04-27 19:58:05]

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

[commit-bot: I haz the power, 2017-04-27 19:58:41]

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-04-27 20:19:37]

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

[commit-bot: I haz the power, 2017-04-27 20:19:38]

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-04-27 20:47:10]

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

[commit-bot: I haz the power, 2017-04-27 20:47:52]

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-04-27 20:49:06]

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

[commit-bot: I haz the power, 2017-04-27 20:49:29]

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-04-27 20:56:13]

Description was changed from

==========
Rewrite extension navigation throttle to support platform app restrictions.

BUG=
==========

to

==========
Refactor of ExtensionNavigationThrottle.

No functional change is intended here, except for the switch to use
url::Origin in the web_accessible_resources check. That should give
more consistent handling of frame-origin "about:blank" ancestors
(these should be pretty rare due to the default extension CSP),
and better handling of extension blob: URLs (which
GURL::GetOrigin() is deficient at).

Control flow is reworked to support adding new reject cases in
a subsequent CL.

BUG=
==========

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

Description was changed from

==========
Refactor of ExtensionNavigationThrottle.

No functional change is intended here, except for the switch to use
url::Origin in the web_accessible_resources check. That should give
more consistent handling of frame-origin "about:blank" ancestors
(these should be pretty rare due to the default extension CSP),
and better handling of extension blob: URLs (which
GURL::GetOrigin() is deficient at).

Control flow is reworked to support adding new reject cases in
a subsequent CL.

BUG=
==========

to

==========
Refactor of ExtensionNavigationThrottle.

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation.

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).~
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=
==========

[ncarter (slow), 2017-04-27 21:05:27]

Description was changed from

==========
Refactor of ExtensionNavigationThrottle.

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation.

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).~
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=
==========

to

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation.

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).~
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=
==========

[ncarter (slow), 2017-04-27 21:05:39]

Description was changed from

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation.

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).~
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=
==========

to

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation.

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=
==========

[ncarter (slow), 2017-04-27 21:08:06]

nick@chromium.org changed reviewers:
+ nasko@chromium.org

[ncarter (slow), 2017-04-27 21:08:07]

nasko, please review

[commit-bot: I haz the power, 2017-04-27 21:15:43]

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

[commit-bot: I haz the power, 2017-04-27 21:15:44]

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

[ncarter (slow), 2017-04-27 22:59:33]

Description was changed from

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation.

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=
==========

to

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation.

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=661324
==========

[ncarter (slow), 2017-04-27 22:59:57]

Description was changed from

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation.

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=661324
==========

to

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation (this fixes 661324)

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=661324
==========

[nasko, 2017-04-27 23:14:06]

LGTM

https://codereview.chromium.org/2830893002/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/process_manager_browsertest.cc (left):

https://codereview.chromium.org/2830893002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:870:
!content::IsBrowserSideNavigationEnabled()) {
Nice to see this difference gone! Thanks!

https://codereview.chromium.org/2830893002/diff/220001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/220001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:92: std::string
resource_path = url.path();
nit: We can skip the local variable here, as it seems to be used only once as an
input parameter below.

[ncarter (slow), 2017-04-27 23:19:41]

https://codereview.chromium.org/2830893002/diff/220001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/220001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:92: std::string
resource_path = url.path();
On 2017/04/27 23:14:06, nasko wrote:
> nit: We can skip the local variable here, as it seems to be used only once as
an
> input parameter below.

Done.

[ncarter (slow), 2017-04-27 23:19:56]

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

[ncarter (slow), 2017-04-27 23:19:57]

Devlin, please review

[ncarter (slow), 2017-04-28 16:59:38]

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

[commit-bot: I haz the power, 2017-04-28 17:00:41]

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

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

[commit-bot: I haz the power, 2017-04-28 18:21:16]

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-04-28 21:45:02]

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

[commit-bot: I haz the power, 2017-04-28 21:45:53]

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-04-28 22:06:56]

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

[commit-bot: I haz the power, 2017-04-28 22:06:57]

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

[Devlin, 2017-04-28 22:13:03]

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:38: bool
url_has_extension_scheme = url.SchemeIs(kExtensionScheme);
Is it worth a brief comment explaining the significance of
url.SchemeIs(kExtensionScheme) vs Origin(url).scheme() == kExtensionScheme?

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:85: // PlzNavigate in
url_request_util::AllowCrossRendererResourceLoad.
"and for non-PlzNavigate" - non-PlzNavigate what?  Main resources?  All
resources?

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:86: std::string
owner_extension_id = guest->owner_host();
Can this be a const std::string&?

(Or even just inline the call in GetByID())

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:90: std::string
partition_domain, partition_id;
one variable per line

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:91: bool in_memory;
initialize

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:121: // case of sandboxed
extension resources.
Can you expand on this a bit to include briefly why sandboxed extension
resources need this separate check?

[ncarter (slow), 2017-04-28 23:05:41]

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

[commit-bot: I haz the power, 2017-04-28 23:06: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, 2017-04-29 00:11:04]

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

[commit-bot: I haz the power, 2017-04-29 00:11:05]

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-01 21:12:16]

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

[commit-bot: I haz the power, 2017-05-01 21:13:03]

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-01 21:24:30]

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

[commit-bot: I haz the power, 2017-05-01 21:25:40]

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-01 21:50:38]

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:38: bool
url_has_extension_scheme = url.SchemeIs(kExtensionScheme);
On 2017/04/28 22:13:02, Devlin wrote:
> Is it worth a brief comment explaining the significance of
> url.SchemeIs(kExtensionScheme) vs Origin(url).scheme() == kExtensionScheme?

Done. I eliminated the is_nested_url variable and added comments on lines 47 and
51.

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:85: // PlzNavigate in
url_request_util::AllowCrossRendererResourceLoad.
On 2017/04/28 22:13:02, Devlin wrote:
> "and for non-PlzNavigate" - non-PlzNavigate what?  Main resources?  All
> resources?

Done.

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:86: std::string
owner_extension_id = guest->owner_host();
On 2017/04/28 22:13:02, Devlin wrote:
> Can this be a const std::string&?
> 
> (Or even just inline the call in GetByID())

Done.

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:90: std::string
partition_domain, partition_id;
On 2017/04/28 22:13:02, Devlin wrote:
> one variable per line

Done.

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:91: bool in_memory;
On 2017/04/28 22:13:02, Devlin wrote:
> initialize

Done.

https://codereview.chromium.org/2830893002/diff/240001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:121: // case of sandboxed
extension resources.
On 2017/04/28 22:13:02, Devlin wrote:
> Can you expand on this a bit to include briefly why sandboxed extension
> resources need this separate check?

Done.

https://codereview.chromium.org/2830893002/diff/300001/chrome/browser/extensi...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://codereview.chromium.org/2830893002/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/extension_browsertest.cc:538: content::WebContents**
newtab_result) {
I did some robustness/readability cleanup here while trying to chase down one
PlzNavigate failure.

https://codereview.chromium.org/2830893002/diff/300001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/300001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:65: return
content::NavigationThrottle::PROCEED;
Since the previous patch set, I added this PROCEED case, to not regress the
error page behavior (these get "This site can’t be reached" / ERR_FAILED,
instead of ERR_BLOCKED, whose error text suggests that it was blocked by an
extension, which is particularly confusing in this case)

https://codereview.chromium.org/2830893002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://codereview.chromium.org/2830893002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_browsertest.cc:547:
EXPECT_EQ(contents->GetMainFrame()->GetSiteInstance(),
This is incidental cleanup, not actually necessary. Looking for a SiteInstance
swap is technically more correct, since two extensions might be assigned the
same a renderer. However, this didn't turn out to be the reason the test failed.

https://codereview.chromium.org/2830893002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/window_open_apitest.cc (right):

https://codereview.chromium.org/2830893002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/window_open_apitest.cc:254:
IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, WindowOpenInvalidExtension) {
Also incidental cleanup (this test had been failing under PlzNavigate, and I
thought it might be flake). These changes aren't actually necessary, but seem
worth including.

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:65: return
content::NavigationThrottle::PROCEED;
Note that this PROCEED case is new vs. the previous patch set. This is to fix
the WindowOpenInvalidExtension browsertest under PlzNavigate, and it also
preserves the text content of the ERR_FAILED error page that results in this
case (at least, it preserves it when neterror.js is not blocked by CSP!). The
text on ERR_FAILED is more appropriate.

[commit-bot: I haz the power, 2017-05-01 21:57:04]

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

[commit-bot: I haz the power, 2017-05-01 21:57:04]

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

[Devlin, 2017-05-01 22:04:28]

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:65: return
content::NavigationThrottle::PROCEED;
On 2017/05/01 21:50:37, ncarter wrote:
> Note that this PROCEED case is new vs. the previous patch set. This is to fix
> the WindowOpenInvalidExtension browsertest under PlzNavigate, and it also
> preserves the text content of the ERR_FAILED error page that results in this
> case (at least, it preserves it when neterror.js is not blocked by CSP!). The
> text on ERR_FAILED is more appropriate.

Hmm... is there no way we could surface a useful error while keeping the check
here?  This just strikes me as unfortunate that it seems to amount to:
// We know this is unsafe, but we have another check that will hopefully catch
this.
return PROCEED;

If that other check ever changes, we're in bad shape.

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:148: // Cancel navigations
to nested URLs, to match the main frame behavior.
Is this difference (CANCEL vs BLOCK_REQUEST) visible to script?  (We want to
make sure there's no way of detecting if an extension is installed by looking at
the result of a request to a non-web-accessible-resource)

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

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:148: // Cancel navigations
to nested URLs, to match the main frame behavior.
On 2017/05/01 22:04:28, Devlin wrote:
> Is this difference (CANCEL vs BLOCK_REQUEST) visible to script?  (We want to
> make sure there's no way of detecting if an extension is installed by looking
at
> the result of a request to a non-web-accessible-resource)

Nested URLs (blob and filesystem) are classified as local only schemes -- blink
won't actually let you use them in any way, unless you're already in the context
of that origin:

% location.href = "blob:chrome-extension://lalalala/foo.html"
"blob:chrome-extension://lalalala/foo.html"
% (index):1 Not allowed to load local resource:
blob:chrome-extension://lalalala/foo.html

So in browsertests, these cases are exercisable only via --disable-web-security.

===

There are two reasons we might use CANCEL instead of BLOCK: 
 (1) when neterror.js executes in the context of an extension CSP, it shows a
blank page with 8+ CSP errors. I don't know if these CSP errors bubble (as
securitypolicyviolation events) to the parent document or not -- they might. You
can see this for yourself if you run
ExtensionBrowserTest.WindowOpenInvalidExtension. However, we really just ought
to fix the interaction of neterror.js with CSP.
 (2) When we're seeing something that a well-behaved renderer should never do,
or to a target URL that's inherently unsafe, CANCEL is a safer option than
BLOCK, because it suppresses the commit of the potentially dangerous URL (if we
commit the error page, we might still transfer it to an extensioney process). On
the other hand, CANCEL is the more developer-hostile behavior, since navigations
just disappear with no explanation.

FWIW, I floated the idea with Nasko of having the navigation throttles
optionally produce console messages, and he was on board with it.

[Devlin, 2017-05-02 01:52:10]

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:148: // Cancel navigations
to nested URLs, to match the main frame behavior.
On 2017/05/01 22:42:53, ncarter wrote:
> On 2017/05/01 22:04:28, Devlin wrote:
> > Is this difference (CANCEL vs BLOCK_REQUEST) visible to script?  (We want to
> > make sure there's no way of detecting if an extension is installed by
looking
> at
> > the result of a request to a non-web-accessible-resource)
> 
> Nested URLs (blob and filesystem) are classified as local only schemes --
blink
> won't actually let you use them in any way, unless you're already in the
context
> of that origin:
> 
> % location.href = "blob:chrome-extension://lalalala/foo.html"
> "blob:chrome-extension://lalalala/foo.html"
> % (index):1 Not allowed to load local resource:
> blob:chrome-extension://lalalala/foo.html
> 
> So in browsertests, these cases are exercisable only via
--disable-web-security.
> 
> ===
> 
> There are two reasons we might use CANCEL instead of BLOCK: 
>  (1) when neterror.js executes in the context of an extension CSP, it shows a
> blank page with 8+ CSP errors. I don't know if these CSP errors bubble (as
> securitypolicyviolation events) to the parent document or not -- they might.
You
> can see this for yourself if you run
> ExtensionBrowserTest.WindowOpenInvalidExtension. However, we really just ought
> to fix the interaction of neterror.js with CSP.
>  (2) When we're seeing something that a well-behaved renderer should never do,
> or to a target URL that's inherently unsafe, CANCEL is a safer option than
> BLOCK, because it suppresses the commit of the potentially dangerous URL (if
we
> commit the error page, we might still transfer it to an extensioney process).
On
> the other hand, CANCEL is the more developer-hostile behavior, since
navigations
> just disappear with no explanation.
> 
> FWIW, I floated the idea with Nasko of having the navigation throttles
> optionally produce console messages, and he was on board with it.

Okay, I think that makes sense, as long as there's no way (excluding a bug) for
a renderer to cause a navigation that results in a cancellation.  Thanks for the
explanation!

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

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:65: return
content::NavigationThrottle::PROCEED;
On 2017/05/01 22:04:28, Devlin wrote:
> On 2017/05/01 21:50:37, ncarter wrote:
> > Note that this PROCEED case is new vs. the previous patch set. This is to
fix
> > the WindowOpenInvalidExtension browsertest under PlzNavigate, and it also
> > preserves the text content of the ERR_FAILED error page that results in this
> > case (at least, it preserves it when neterror.js is not blocked by CSP!).
The
> > text on ERR_FAILED is more appropriate.
> 
> Hmm... is there no way we could surface a useful error while keeping the check
> here?  This just strikes me as unfortunate that it seems to amount to:
> // We know this is unsafe, but we have another check that will hopefully catch
> this.
> return PROCEED;
> 
> If that other check ever changes, we're in bad shape.

Note that the PROCEED logic just matches the original behavior for main frame
navigations. So this behavior has existed for some time.

Having said that, I agree: we ought to block this here; let me tackle this in a
follow-on CL. I think there may be a PlzNavigate bug, and possibly a CSP bug,
confounding this. The ExtensionApiTest.WindowOpenInvalidExtension is sensitive
to both of these, and I'm still digging into what's going on there.

[ncarter (slow), 2017-05-02 23:53:15]

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/320001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:65: return
content::NavigationThrottle::PROCEED;
On 2017/05/02 20:14:15, ncarter wrote:
> On 2017/05/01 22:04:28, Devlin wrote:
> > On 2017/05/01 21:50:37, ncarter wrote:
> > > Note that this PROCEED case is new vs. the previous patch set. This is to
> fix
> > > the WindowOpenInvalidExtension browsertest under PlzNavigate, and it also
> > > preserves the text content of the ERR_FAILED error page that results in
this
> > > case (at least, it preserves it when neterror.js is not blocked by CSP!).
> The
> > > text on ERR_FAILED is more appropriate.
> > 
> > Hmm... is there no way we could surface a useful error while keeping the
check
> > here?  This just strikes me as unfortunate that it seems to amount to:
> > // We know this is unsafe, but we have another check that will hopefully
catch
> > this.
> > return PROCEED;
> > 
> > If that other check ever changes, we're in bad shape.
> 
> Note that the PROCEED logic just matches the original behavior for main frame
> navigations. So this behavior has existed for some time.
> 
> Having said that, I agree: we ought to block this here; let me tackle this in
a
> follow-on CL. I think there may be a PlzNavigate bug, and possibly a CSP bug,
> confounding this. The ExtensionApiTest.WindowOpenInvalidExtension is sensitive
> to both of these, and I'm still digging into what's going on there.
> 

My prev comment was incorrect (since the PROCEED is in fact new for subframes,
which the unittests show). I'll figure out a way forward and ping this review
when I have a new CL ready.

[ncarter (slow), 2017-05-11 19:22:20]

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

[commit-bot: I haz the power, 2017-05-11 19:23:36]

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-11 20:27:21]

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

[commit-bot: I haz the power, 2017-05-11 20:28:16]

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-11 20:29:09]

Remove unnecessary line.

[ncarter (slow), 2017-05-11 20:29:10]

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

[commit-bot: I haz the power, 2017-05-11 20:29:48]

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-11 20:40:09]

Nasko: PTAL.

I've restored the blocking of invalid/disabled extensions so that it applies to
the main frame, with a TODO about using a better value than
NET_ERR_BLOCKED_BY_CLIENT once that's possible. There are new unittests too.

I'd like to TBR devlin here with your approval.

[commit-bot: I haz the power, 2017-05-11 21:47:43]

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

[commit-bot: I haz the power, 2017-05-11 21:47:44]

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

[nasko, 2017-05-11 22:14:06]

On 2017/05/11 20:40:09, ncarter wrote:
> Nasko: PTAL.
> 
> I've restored the blocking of invalid/disabled extensions so that it applies
to
> the main frame, with a TODO about using a better value than
> NET_ERR_BLOCKED_BY_CLIENT once that's possible. There are new unittests too.
> 
> I'd like to TBR devlin here with your approval.

LGTM on the code and SGTM to TBR Devlin to make progress.

[Devlin, 2017-05-12 01:19:42]

lgtm

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
File chrome/browser/extensions/extension_navigation_throttle_unittest.cc
(right):

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/extension_navigation_throttle_unittest.cc:194: // Test
blob and filesystem URLs with disabled/invalid extensions.
nit: s/invalid/nonexistent?  invalid seems like it refers to the id not matching
[a-p]^32

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
File chrome/browser/extensions/window_open_apitest.cc (right):

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/window_open_apitest.cc:278: std::string document_body
= "uninitialized";
is this just to allow contrast with the expected value of " "?

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/window_open_apitest.cc:285: EXPECT_EQ(" ",
document_body);
This looks like it's failing because the result is actually " \n".  I wonder if
this would be cleaner as something like:
EXPECT_TRUE(base::TrimWhitespace(document_body, base::TRIM_ALL).empty());
?

https://codereview.chromium.org/2830893002/diff/400001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/400001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:81: if
(!has_webview_permission)
optional nit: has_webview_permission could be inlined here since it's pretty
simple.

[ncarter (slow), 2017-05-12 17:27:59]

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

[commit-bot: I haz the power, 2017-05-12 17:29:23]

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-12 17:32:19]

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
File chrome/browser/extensions/extension_navigation_throttle_unittest.cc
(right):

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/extension_navigation_throttle_unittest.cc:194: // Test
blob and filesystem URLs with disabled/invalid extensions.
On 2017/05/12 01:19:41, Devlin (catching up) wrote:
> nit: s/invalid/nonexistent?  invalid seems like it refers to the id not
matching
> [a-p]^32

Done.

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
File chrome/browser/extensions/window_open_apitest.cc (right):

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/window_open_apitest.cc:278: std::string document_body
= "uninitialized";
On 2017/05/12 01:19:42, Devlin (catching up) wrote:
> is this just to allow contrast with the expected value of " "?

Removed; EXPECT_TRUE here suffices to make sure the value is set.

https://codereview.chromium.org/2830893002/diff/400001/chrome/browser/extensi...
chrome/browser/extensions/window_open_apitest.cc:285: EXPECT_EQ(" ",
document_body);
On 2017/05/12 01:19:42, Devlin (catching up) wrote:
> This looks like it's failing because the result is actually " \n".  I wonder
if
> this would be cleaner as something like:
> EXPECT_TRUE(base::TrimWhitespace(document_body, base::TRIM_ALL).empty());
> ?

Did the trim() in js. Done.

https://codereview.chromium.org/2830893002/diff/400001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2830893002/diff/400001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:81: if
(!has_webview_permission)
On 2017/05/12 01:19:42, Devlin (catching up) wrote:
> optional nit: has_webview_permission could be inlined here since it's pretty
> simple.

This is old code, and I know that alexmos has a pending CL that touches it, so
I'm letting it be.

[ncarter (slow), 2017-05-12 17:32:28]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2017-05-12 17:32:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2830893002/#ps440001
(title: "Fix comment.")

[commit-bot: I haz the power, 2017-05-12 17:34:55]

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-12 19:48:23]

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

[commit-bot: I haz the power, 2017-05-12 19:48:25]

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-12 22:19:06]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2017-05-12 22:19:59]

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-12 23:38:56]

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

[commit-bot: I haz the power, 2017-05-12 23:38:58]

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-15 23:22:07]

Description was changed from

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation (this fixes 661324)

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=661324
==========

to

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation (this fixes 661324)

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

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

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

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

[commit-bot: I haz the power, 2017-05-15 23:23:26]

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-15 23:59:47]

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

[commit-bot: I haz the power, 2017-05-15 23:59:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-16 20:13:14]

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

[commit-bot: I haz the power, 2017-05-16 20:14:38]

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-16 23:14:42]

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

[commit-bot: I haz the power, 2017-05-16 23:14:43]

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 19:02:58]

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

[commit-bot: I haz the power, 2017-05-17 19:05:31]

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 19:07:19]

nick@chromium.org changed reviewers:
+ pkasting@chromium.org

[ncarter (slow), 2017-05-17 19:07:20]

+pkasting: please look at toolbar_model_unittest.cc. My changes to the
ExtensionNavigationThrottle required that this test use a valid extension ID,
since the throttle now blocks navigations to invalid extension IDs, and this
test seems to want to simulate a successful navigation.

[Peter Kasting, 2017-05-17 23:13:59]

LGTM

https://codereview.chromium.org/2830893002/diff/500001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/2830893002/diff/500001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:102: // valid.
Nit: I would extend this comment to talk about why having a valid ID matters.

[commit-bot: I haz the power, 2017-05-17 23:26:35]

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

[commit-bot: I haz the power, 2017-05-17 23:26:37]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-05-22 21:14:07]

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

[ncarter (slow), 2017-05-22 21:14:29]

https://codereview.chromium.org/2830893002/diff/500001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/2830893002/diff/500001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:102: // valid.
On 2017/05/17 23:13:59, Peter Kasting wrote:
> Nit: I would extend this comment to talk about why having a valid ID matters.

Done.

[commit-bot: I haz the power, 2017-05-22 21:14:32]

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 21:14:38]

The CQ bit was unchecked by nick@chromium.org

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

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2017-05-22 21:14:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, nasko@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2830893002/#ps520001
(title: "\")

[commit-bot: I haz the power, 2017-05-22 21:15:35]

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-22 22:00:45]

CQ is committing da patch.

Bot data: {"patchset_id": 520001, "attempt_start_ts": 1495487685812620,
"parent_rev": "55585f6f526153baaced20d2b684eb8b2ee418e7", "commit_rev":
"6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a"}

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

Description was changed from

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation (this fixes 661324)

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

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

to

==========
Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation (this fixes 661324)

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

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

Review-Url: https://codereview.chromium.org/2830893002
Cr-Commit-Position: refs/heads/master@{#473717}
Committed:
https://chromium.googlesource.com/chromium/src/+/6cfe5c7f23e1447422a21ddc7b6a...
==========

[commit-bot: I haz the power, 2017-05-22 22:01:04]

Committed patchset #27 (id:520001) as
https://chromium.googlesource.com/chromium/src/+/6cfe5c7f23e1447422a21ddc7b6a...