Fix in-page logic for allow_universal_access_from_files.

Fix in-page logic for allow_universal_access_from_files.

The previous approach failed if a file URL did a replaceState to another
origin and then back to a file URL.  The new approach checks the frame's
origin instead, which stays as file:// the whole time.

BUG=553418
TEST=See bug comment 11 for repro steps.

Committed: https://crrev.com/dc004e0d2c6f3d7a6815a0ef7a86bc1185c8adac
Cr-Commit-Position: refs/heads/master@{#360917}

[Charlie Reis, 2015-11-19 23:10:13]

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

[Charlie Reis, 2015-11-19 23:10:14]

Alex, can you take a look?

Android Webview uses allow_universal_access_from_files, which means you can do a
replaceState from file to http (and back).  The current logic wasn't allowing
that, because http to file normally looks illegal.

We can fix it by looking at the FTN's origin instead.  I went ahead and exposed
FTN::current_origin, since we're talking about using it in many places.

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

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1396:
last_committed_url.GetOrigin() == url.GetOrigin() ||
I thought about updating this to use committed_origin as well, but that seems
risky-- GetOrigin() returns a GURL, not an Origin.  I don't think they'll agree
in all cases.

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

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3635:
main_test_rfh()->NavigateAndCommitRendererInitiated(0, true, file_url);
I had to tweak TestRenderFrameHost::SendNavigateWithParams to set the
params.origin value to get the origin set on the FTN here.

[alexmos, 2015-11-20 18:07:36]

Tricky stuff, thanks for fixing!

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.h:130: const url::Origin&
current_origin() const {
Is it worth also simplifying tests that use "current_replication_state().origin"
to use this instead?  There are a few in site_per_process_browsertest.cc and
frame_tree_browsertest.cc.  Though it's also ok to do it later.

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

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1396:
last_committed_url.GetOrigin() == url.GetOrigin() ||
On 2015/11/19 23:10:13, Charlie Reis wrote:
> I thought about updating this to use committed_origin as well, but that seems
> risky-- GetOrigin() returns a GURL, not an Origin.  I don't think they'll
agree
> in all cases.

Acknowledged.

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

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3635:
main_test_rfh()->NavigateAndCommitRendererInitiated(0, true, file_url);
On 2015/11/19 23:10:14, Charlie Reis wrote:
> I had to tweak TestRenderFrameHost::SendNavigateWithParams to set the
> params.origin value to get the origin set on the FTN here.

Acknowledged.

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3662: // so
that a file URL would still be in-page.
Would it be useful to reference https://crbug.com/553418 here?

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3672:
main_test_rfh()->NavigateAndCommitRendererInitiated(0, true, url);
Is it ok to set page_id to 0 here (after committing with page_id 0 and 1 above)?

[Charlie Reis, 2015-11-20 19:54:32]

Thanks!  New patch up.

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.h:130: const url::Origin&
current_origin() const {
On 2015/11/20 18:07:36, alexmos wrote:
> Is it worth also simplifying tests that use
"current_replication_state().origin"
> to use this instead?  There are a few in site_per_process_browsertest.cc and
> frame_tree_browsertest.cc.  Though it's also ok to do it later.

Sure, I'll do that in a followup CL.

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

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3662: // so
that a file URL would still be in-page.
On 2015/11/20 18:07:36, alexmos wrote:
> Would it be useful to reference https://crbug.com/553418 here?

Done.

https://codereview.chromium.org/1457053004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3672:
main_test_rfh()->NavigateAndCommitRendererInitiated(0, true, url);
On 2015/11/20 18:07:36, alexmos wrote:
> Is it ok to set page_id to 0 here (after committing with page_id 0 and 1
above)?

Good catch.  It's more correct to say 2 here, though in the fun world of unit
tests, no one's checking.  :)

[Charlie Reis, 2015-11-20 20:04:04]

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

[commit-bot: I haz the power, 2015-11-20 20:05:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1457053004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1457053004/20001

[commit-bot: I haz the power, 2015-11-20 21:15:59]

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

[commit-bot: I haz the power, 2015-11-20 21:16:00]

Dry run: This issue passed the CQ dry run.

[alexmos, 2015-11-20 21:17:26]

LGTM

[Charlie Reis, 2015-11-20 21:21:31]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-11-20 21:22:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1457053004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1457053004/20001

[commit-bot: I haz the power, 2015-11-20 21:28:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-20 21:29:42]

Patchset 2 (id:??) landed as
https://crrev.com/dc004e0d2c6f3d7a6815a0ef7a86bc1185c8adac
Cr-Commit-Position: refs/heads/master@{#360917}