Don't recreate feature policy on fragment navigation

Don't recreate feature policy on fragment navigation.

This looks like just an optimization for now: the policy is currently recreated on every navigation from the parent policy and the document's own headers, and this eliminates that work when the navigation is just within the page.

It will be more important soon, though, once per-frame policies are implemented. At that point, this also will prevent the policy from changing, if the embedding iframe attributes have been updated since the document became active.

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

[iclelland, 2017-02-24 19:00:38]

Description was changed from

==========
Don't recreate feature policy on fragment navigation
==========

to

==========
Don't recreate feature policy on fragment navigation
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[iclelland, 2017-02-24 19:00:40]

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

[commit-bot: I haz the power, 2017-02-24 19:01:14]

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

[iclelland, 2017-02-24 19:10:28]

Description was changed from

==========
Don't recreate feature policy on fragment navigation
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't recreate feature policy on fragment navigation.

This looks like just an optimization for now: the policy is currently recreated
on every navigation from the parent policy and the document's own headers, and
this eliminates that work when the navigation is just within the page.

It will be more important soon, though, once per-frame policies are implemented.
At that point, this also will prevent the policy from changing, if the embedding
iframe attributes have been updated since the document became active.

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

[iclelland, 2017-02-24 19:10:28]

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

[commit-bot: I haz the power, 2017-02-24 19:27:04]

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

[commit-bot: I haz the power, 2017-02-24 19:27:06]

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

[alexmos, 2017-02-24 21:21:12]

https://codereview.chromium.org/2707263013/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2707263013/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:688: if (!did_navigate)
A couple of concerns with this.  First, I think you meant "if (did_navigate)",
as the negation will skip the ResetFeaturePolicy() on almost every navigation,
including normal cross-site navigations.

But also to skip fragments and other in-page navigations, "if
(!is_navigation_within_page)" seems to be the better thing to check. 
|did_navigate| returns false only for auto_subframe and ignored navigations, but
I think it's possible to navigate to a hash on the same page for other types of
navigations. (E.g., from comment on NAVIGATION_TYPE_NEW_PAGE: "cases like
navigations to a hash on the same page are NEW_PAGE").

(I also ran this by creis@ to double check, and he agreed.)

[iclelland, 2017-02-28 23:57:43]

On 2017/02/24 21:21:12, alexmos wrote:
>
https://codereview.chromium.org/2707263013/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/navigator_impl.cc (right):
> 
>
https://codereview.chromium.org/2707263013/diff/1/content/browser/frame_host/...
> content/browser/frame_host/navigator_impl.cc:688: if (!did_navigate)
> A couple of concerns with this.  First, I think you meant "if (did_navigate)",
> as the negation will skip the ResetFeaturePolicy() on almost every navigation,
> including normal cross-site navigations.

I'm definitely not happy that that got past me, or that there are essentially no
tests for it. I'll switch it around, but I'm going to try to put some kind of
test to ensure that it's doing what it should before I submit this again.

> 
> But also to skip fragments and other in-page navigations, "if
> (!is_navigation_within_page)" seems to be the better thing to check. 
> |did_navigate| returns false only for auto_subframe and ignored navigations,
but
> I think it's possible to navigate to a hash on the same page for other types
of
> navigations. (E.g., from comment on NAVIGATION_TYPE_NEW_PAGE: "cases like
> navigations to a hash on the same page are NEW_PAGE").
> 
> (I also ran this by creis@ to double check, and he agreed.)

That makes a lot of sense, I'll update the condition. Thanks for checking!

[iclelland, 2017-03-03 15:07:16]

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

[commit-bot: I haz the power, 2017-03-03 15:07:26]

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

[iclelland, 2017-03-03 15:26:31]

alexmos, can you PTAL again?

https://codereview.chromium.org/2707263013/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2707263013/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:688: if (!did_navigate)
On 2017/02/24 21:21:12, alexmos wrote:
> A couple of concerns with this.  First, I think you meant "if (did_navigate)",
> as the negation will skip the ResetFeaturePolicy() on almost every navigation,
> including normal cross-site navigations.
> 
> But also to skip fragments and other in-page navigations, "if
> (!is_navigation_within_page)" seems to be the better thing to check. 
> |did_navigate| returns false only for auto_subframe and ignored navigations,
but
> I think it's possible to navigate to a hash on the same page for other types
of
> navigations. (E.g., from comment on NAVIGATION_TYPE_NEW_PAGE: "cases like
> navigations to a hash on the same page are NEW_PAGE").
> 
> (I also ran this by creis@ to double check, and he agreed.)

Thanks for that -- I've updated the condition, to include both |did_navigate|
and !is_navigation_within_page, and added tests to ensure that the policy is not
reset on fragment navigation.

[commit-bot: I haz the power, 2017-03-03 15:51:02]

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

[commit-bot: I haz the power, 2017-03-03 15:51:03]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-03-04 01:33:25]

LGTM, thanks for adding these tests!  Is there a more general feature policy bug
number that you could reference here?

https://codereview.chromium.org/2707263013/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/2707263013/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:1260:
main_test_rfh()->SendNavigate(0, true, kUrl2);
Would it work to just use contents()->NavigateAndCommit(kUrl2) instead of these
three steps?  Same for other test.

[iclelland, 2017-03-04 05:42:36]

Description was changed from

==========
Don't recreate feature policy on fragment navigation.

This looks like just an optimization for now: the policy is currently recreated
on every navigation from the parent policy and the document's own headers, and
this eliminates that work when the navigation is just within the page.

It will be more important soon, though, once per-frame policies are implemented.
At that point, this also will prevent the policy from changing, if the embedding
iframe attributes have been updated since the document became active.

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

to

==========
Don't recreate feature policy on fragment navigation.

This looks like just an optimization for now: the policy is currently recreated
on every navigation from the parent policy and the document's own headers, and
this eliminates that work when the navigation is just within the page.

It will be more important soon, though, once per-frame policies are implemented.
At that point, this also will prevent the policy from changing, if the embedding
iframe attributes have been updated since the document became active.

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

[iclelland, 2017-03-04 05:43:10]

https://codereview.chromium.org/2707263013/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/2707263013/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:1260:
main_test_rfh()->SendNavigate(0, true, kUrl2);
On 2017/03/04 01:33:25, alexmos wrote:
> Would it work to just use contents()->NavigateAndCommit(kUrl2) instead of
these
> three steps?  Same for other test.

It absolutely does. Thanks, done. (Validated manually that it still catches the
errors if the condition is wrong)

[iclelland, 2017-03-04 05:43:23]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-03-04 05:43:24]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2707263013/#ps40001
(title: "Simplify test navigation")

[commit-bot: I haz the power, 2017-03-04 05:43:37]

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-03-04 07:05:16]

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

[commit-bot: I haz the power, 2017-03-04 07:05:17]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[iclelland, 2017-03-04 13:24:33]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2017-03-04 13:24:37]

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-03-04 14:01:48]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488633873745810,
"parent_rev": "8e679697d635baa92d4927b2b4e838f125266af1", "commit_rev":
"f46f88a56a012c3633a76662cc2f1735c42d9174"}

[commit-bot: I haz the power, 2017-03-04 14:06:03]

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

[commit-bot: I haz the power, 2017-03-04 14:06:04]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[iclelland, 2017-03-05 03:57:50]

On 2017/03/04 14:06:04, commit-bot: I haz the power wrote:
> Prior attempt to commit was detected, but we were not able to check whether
the
> issue was successfully committed. Please check Git history manually and
re-check
> CQ or close this issue as needed.

Closing issue, patch appears to have been successfully committed as
https://crrev.com/f46f88a56a012c3633a76662cc2f1735c42d9174