Expose the initiating origin/URL of a navigation in the Blink public API

Expose a navigation's requestor Document's URL.

BUG=651895

[dcheng, 2016-10-12 05:23:51]

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

[commit-bot: I haz the power, 2016-10-12 05:24:06]

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

[dcheng, 2016-10-12 05:27:58]

Description was changed from

==========
Expose the initiating origin/URL of a navigation in the Blink public API

BUG=651895
==========

to

==========
Expose the initiating origin/URL of a navigation in the Blink public API

BUG=651895
==========

[dcheng, 2016-10-12 05:27:58]

dcheng@chromium.org changed reviewers:
+ mkwst@chromium.org

[dcheng, 2016-10-12 05:34:27]

mkwst@, does this patch make sense to you? The motivation is that we'd like to
be able to attribute navigations to the origin that actually initiated the
navigation (which could be cross-origin, in another window, etc).

The reason for plumbing through the URL as well is to help in cases where we're
loading a data URL: otherwise, the origin would just be a unique origin, which
isn't very useful.

If this patch makes sense, I'll go ahead and add some unit tests. Note that
"originDocument" isn't completely accurate right now: for example,
HTMLFrameOwnerElement::loadOrRedirectSubframe passes the Document, without
considering if there's a current (JS) context at the top of the stack. However,
this might be good enough for now: if it can access the frame element, it has to
be same-origin.

[commit-bot: I haz the power, 2016-10-12 06:58:25]

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

[commit-bot: I haz the power, 2016-10-12 06:58:26]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-10-12 13:43:15]

Is this different from the `requestorOrigin` we have on WebURLRequest?

[dcheng, 2016-10-12 16:10:42]

On 2016/10/12 13:43:15, Mike West wrote:
> Is this different from the `requestorOrigin` we have on WebURLRequest?

Hmm, I must have read the code incorrectly originally. This is actually pretty
close to what I wanted.

However, requestorOrigin seems to have some (surprising to me) behavior. For
example, it has
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
will clobber the security origin on the URL for top-level navigation requests...
but most of the time, FrameTypeTopLevel isn't set until after FrameLoadRequest
is created. What are the desired semantics here?

In addition, some places can mutate the RequestorOrigin as it passes through:
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...
will clobber the security origin as well.

Finally, we'd want to plumb through the requestor's URL as well for the data URL
case: would that be acceptable overhead for every request? It could be pretty
heavyweight... this is one of the reasons I leaned towards adding it to
WebDataSource instead of WebURLRequest.

[Mike West, 2016-10-13 09:56:16]

On 2016/10/12 at 16:10:42, dcheng wrote:
> However, requestorOrigin seems to have some (surprising to me) behavior. For
example, it has
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
will clobber the security origin on the URL for top-level navigation requests...
but most of the time, FrameTypeTopLevel isn't set until after FrameLoadRequest
is created. What are the desired semantics here?

This triggers only if there's no `originDocument`, so it basically maps to a
browser-initiated navigation from something like the omnibox:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr....
We should be putting a `nullptr` in here rather than a unique origin. That's
somewhat related to clamy@'s patch https://codereview.chromium.org/2405483002,
as we currently require an origin to be sent up to the browser during
navigation. Once we make that optional, we can drop it here.

> In addition, some places can mutate the RequestorOrigin as it passes through:
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...
will clobber the security origin as well.

I think this boils down to the same bug as above.
 
> Finally, we'd want to plumb through the requestor's URL as well for the data
URL case: would that be acceptable overhead for every request? It could be
pretty heavyweight... this is one of the reasons I leaned towards adding it to
WebDataSource instead of WebURLRequest.

Hrm. If you need to distinguish `data:` navigators from sandboxed frame
navigators, do you need the whole URL? Perhaps a bool would be enough?

[dcheng, 2016-10-13 17:31:01]

On 2016/10/13 09:56:16, Mike West wrote:
> On 2016/10/12 at 16:10:42, dcheng wrote:
> > However, requestorOrigin seems to have some (surprising to me) behavior. For
> example, it has
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
> will clobber the security origin on the URL for top-level navigation
requests...
> but most of the time, FrameTypeTopLevel isn't set until after FrameLoadRequest
> is created. What are the desired semantics here?
> 
> This triggers only if there's no `originDocument`, so it basically maps to a
> browser-initiated navigation from something like the omnibox:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr....
> We should be putting a `nullptr` in here rather than a unique origin. That's
> somewhat related to clamy@'s patch https://codereview.chromium.org/2405483002,
> as we currently require an origin to be sent up to the browser during
> navigation. Once we make that optional, we can drop it here.
> 
> > In addition, some places can mutate the RequestorOrigin as it passes
through:
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...
> will clobber the security origin as well.
> 
> I think this boils down to the same bug as above.

It's a little different. The requestor origin could be a data URL, and we'd lose
track of that here.

>  
> > Finally, we'd want to plumb through the requestor's URL as well for the data
> URL case: would that be acceptable overhead for every request? It could be
> pretty heavyweight... this is one of the reasons I leaned towards adding it to
> WebDataSource instead of WebURLRequest.
> 
> Hrm. If you need to distinguish `data:` navigators from sandboxed frame
> navigators, do you need the whole URL? Perhaps a bool would be enough?

It's also a matter of distinguishing different data URLs from each other, so
just knowing the data URL isn't quite enough. If you have time tomorrow, a quick
chat might help figure out how to move forward.

[Mike West, 2016-10-24 14:01:51]

dcheng@: We talked last week (sorry for the delay!), but I'm not sure if you're
waiting on me for something here or not.

[dcheng, 2016-10-24 15:17:10]

On 2016/10/24 14:01:51, Mike West wrote:
> dcheng@: We talked last week (sorry for the delay!), but I'm not sure if
you're
> waiting on me for something here or not.

No, I'm not: I haven't had time to update the CL. I will reping when it's ready
for review =)

[dcheng, 2016-11-12 04:50:09]

Description was changed from

==========
Expose the initiating origin/URL of a navigation in the Blink public API

BUG=651895
==========

to

==========
Expose the URL of the requestor Document of a navigation.

BUG=651895
==========

[dcheng, 2016-11-12 05:58:41]

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

[commit-bot: I haz the power, 2016-11-12 05:58:50]

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

[dcheng, 2016-11-12 06:02:13]

OK, I updated this to just retain a pointer to the requestor document: the URL
won't be created / copied until it's read out by something in //content, which
we can do in a loading callback like didCommitProvisionalLoad().

An alternative I considered is plumbing through the requestor Document /
SecurityContext on ResourceRequest. However, this is impossible today, due to
layering issues (SecurityContext is a core/dom concept, but ResourceRequest
lives in platform).

https://codereview.chromium.org/2416523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/EmptyClients.cpp (right):

https://codereview.chromium.org/2416523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/EmptyClients.cpp:148: Document*
requestorDocument) {
I could plumb this through here as well, but I opted to only expose it in
WebDataSource, since that's the only place it's needed.

[dcheng, 2016-11-12 06:02:40]

Description was changed from

==========
Expose the URL of the requestor Document of a navigation.

BUG=651895
==========

to

==========
Expose a navigation's requestor Document's URL.

BUG=651895
==========

[commit-bot: I haz the power, 2016-11-12 06:19:02]

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

[commit-bot: I haz the power, 2016-11-12 06:19:03]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dcheng, 2016-11-12 06:31:37]

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

[commit-bot: I haz the power, 2016-11-12 06:31:47]

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

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

[commit-bot: I haz the power, 2016-11-12 08:01:22]

Dry run: This issue passed the CQ dry run.

[kuzminruslan, 2016-11-12 08:51:56]

kuzminruslan@icloud.com changed reviewers:
+ kuzminruslan@icloud.com

[kuzminruslan, 2016-11-12 08:51:57]

https://codereview.chromium.org/2416523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2416523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:683:
m_webFrame->client()->didCreateDataSource(m_webFrame, ds);
i am not sure, but perhaps skiped if(m_webFrame->client()) besides in code exist
examples code - m_webFrame->setClient(0);

[dcheng, 2016-11-12 18:14:56]

On 2016/11/12 08:51:57, kuzminruslan wrote:
>
https://codereview.chromium.org/2416523002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/2416523002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:683:
> m_webFrame->client()->didCreateDataSource(m_webFrame, ds);
> i am not sure, but perhaps skiped if(m_webFrame->client()) besides in code
exist
> examples code - m_webFrame->setClient(0);

It's actually not necessary: you can't get to the FrameLoaderClient if the frame
is detached--and if the frame is attached, it must have a WebFrameClient.

[Mike West, 2016-11-14 13:58:30]

This looks pretty reasonable, thanks for taking a second pass after our
conversation. Would you mind adding some tests to verify the behavior?

[dcheng, 2016-11-23 08:30:23]

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

[commit-bot: I haz the power, 2016-11-23 08:30:34]

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

[dcheng, 2016-11-23 08:32:59]

PTAL

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:649: // TODO(dcheng):
This doesn't really belong here.
I'm not sure what the best place to put this is; it's a bit out of place here,
but it also seems to be the best current location for "clean up resources"...

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:76: Document*
requestorDocument = nullptr) {
The default arg made it easier to add this argument, but I'm happy to make it
explicit (and update the tests) instead.

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebDataSource.h (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebDataSource.h:54: // datasource. Will return an
empty URL once this datasource commits.
Unfortunately, I realized that the previous implementation would keep Documents
live indefinitely. So I changed it to a more complex interface =(

[commit-bot: I haz the power, 2016-11-23 10:38:58]

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

[commit-bot: I haz the power, 2016-11-23 10:38:59]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-23 11:11:20]

LGTM % test questions.

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:76: Document*
requestorDocument = nullptr) {
On 2016/11/23 at 08:32:59, dcheng wrote:
> The default arg made it easier to add this argument, but I'm happy to make it
explicit (and update the tests) instead.

Looks like we only call it in ~5 places, mostly tests. If you don't mind making
it explicit, I'd prefer that.

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1692:
frameLoadRequest.clientRedirect(), frameLoadRequest.originDocument());
How will this eventually work in a oopif world?

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10689: // The main frame
should have no requestor URL.
Should the main frame have a requestor URL during a navigation? Can you add a
test to verify that behavior?

[dcheng, 2016-11-23 14:40:51]

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

[commit-bot: I haz the power, 2016-11-23 14:41:10]

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

[dcheng, 2016-11-23 14:41:44]

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:76: Document*
requestorDocument = nullptr) {
On 2016/11/23 11:11:20, Mike West (slow) wrote:
> On 2016/11/23 at 08:32:59, dcheng wrote:
> > The default arg made it easier to add this argument, but I'm happy to make
it
> explicit (and update the tests) instead.
> 
> Looks like we only call it in ~5 places, mostly tests. If you don't mind
making
> it explicit, I'd prefer that.

Done.

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1692:
frameLoadRequest.clientRedirect(), frameLoadRequest.originDocument());
On 2016/11/23 11:11:20, Mike West (slow) wrote:
> How will this eventually work in a oopif world?

We'd still have a DocumentLoader in the original renderer. When we swap from
local to remote, content can read off the requestor URL then...

It's not great, but I can't really think of anything better =/

In my ideal world, content would just track this: whenever we initiate a
navigation, content/browser would take a note of the URL of the requestor frame
at the time. It'd require aggressively copying the (potentially) large URL
though. I'm a bit torn about what the best approach is atm... I think this would
be longer in the long run, but we'd want to figure out a way to defer the URL
copy until actual commit type.

(I'll talk to creis@ and nasko@ more about this today)

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2416523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10689: // The main frame
should have no requestor URL.
On 2016/11/23 11:11:20, Mike West (slow) wrote:
> Should the main frame have a requestor URL during a navigation? Can you add a
> test to verify that behavior?

Done.

[commit-bot: I haz the power, 2016-11-23 14:42:54]

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

[commit-bot: I haz the power, 2016-11-23 14:42:55]

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

[dcheng, 2016-11-23 14:57:21]

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

[commit-bot: I haz the power, 2016-11-23 14:57:32]

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

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

[commit-bot: I haz the power, 2016-11-23 16:13:43]

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

[Mike West, 2016-11-28 07:12:33]

Ok. LGTM. I trust that you'll come up with a good plan for the future. :)