Implement navigator.sendBeacon()

Implement navigator.sendBeacon()

This provides support for the Beacon API,

 https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/Beacon/Overview.html

allowing async transmission of data using navigator.sendBeacon().

R=abarth@chromium.org,mkwst@chromium.org
BUG=360603
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173837

[abarth-chromium, 2014-04-10 18:25:14]

https://codereview.chromium.org/232053005/diff/60001/Source/modules/beacon/Na...
File Source/modules/beacon/NavigatorBeacon.h (right):

https://codereview.chromium.org/232053005/diff/60001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.h:24: class NavigatorBeacon FINAL : public
NoBaseWillBeGarbageCollectedFinalized<NavigatorBeacon>, public
WillBeHeapSupplement<Navigator> {
You shouldn't need to instantiate this object at all.  You can drop all these
base classes.

https://codereview.chromium.org/232053005/diff/60001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.h:29: static NavigatorBeacon&
from(Navigator&);
You should be able to drop this function too.

https://codereview.chromium.org/232053005/diff/60001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/232053005/diff/60001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:21: BeaconAPI status=experimental
You can remove the "API" from the name.  Everything here is an API.

[sof, 2014-05-05 16:07:49]

I think this is now ready for consideration, please take a look (or suggest
someone even better placed to do so & not too busy.)

Arbitrarily picked 16k as the default limit on beacon transmission per document
+ followed Mozilla in having sendBeacon() be CSP connect-src controllable.

[sof, 2014-05-08 05:34:31]

ping?

[abarth-chromium, 2014-05-08 13:15:25]

https://codereview.chromium.org/232053005/diff/70001/Source/core/loader/Beaco...
File Source/core/loader/BeaconLoader.cpp (right):

https://codereview.chromium.org/232053005/diff/70001/Source/core/loader/Beaco...
Source/core/loader/BeaconLoader.cpp:35: request.clearHTTPOrigin();
Why?  I'd just remove this logic.  We include Origin headers on all POST
requests.

https://codereview.chromium.org/232053005/diff/70001/Source/core/loader/PingL...
File Source/core/loader/PingLoader.cpp (right):

https://codereview.chromium.org/232053005/diff/70001/Source/core/loader/PingL...
Source/core/loader/PingLoader.cpp:56: 
Extra blank line

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.cpp:42: bool
NavigatorBeacon::isAllowedAccess(ExecutionContext* context, const KURL& url,
ExceptionState& exceptionState)
s/isAllowedAccess/canSendBeacon/  ?

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.cpp:69: if (settings) {
We usually combine these lines.

I don't think we need to handle the case when m_navigator.frame() is zero
because we check isAllowedAccess before calling this function.

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
File Source/modules/beacon/NavigatorBeacon.h (right):

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.h:16: #define BLINK_MAX
???

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.h:26: class NavigatorBeacon FINAL : public
NoBaseWillBeGarbageCollectedFinalized<NavigatorBeacon>, public
WillBeHeapSupplement<Navigator> {
Are you use this class needs to be finalized?

[abarth-chromium, 2014-05-08 13:15:40]

lgtm

[sof, 2014-05-08 20:47:07]

Many thanks for the review, abarth.

https://codereview.chromium.org/232053005/diff/70001/Source/core/loader/Beaco...
File Source/core/loader/BeaconLoader.cpp (right):

https://codereview.chromium.org/232053005/diff/70001/Source/core/loader/Beaco...
Source/core/loader/BeaconLoader.cpp:35: request.clearHTTPOrigin();
On 2014/05/08 13:15:25, abarth wrote:
> Why?  I'd just remove this logic.  We include Origin headers on all POST
> requests.

ok, no strong reason for filtering it out here. Removed.

(If you recall why POSTs are Origin-decorated, I'd be interested to hear. git
history is a bit chaotic for loader code.)

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.cpp:42: bool
NavigatorBeacon::isAllowedAccess(ExecutionContext* context, const KURL& url,
ExceptionState& exceptionState)
On 2014/05/08 13:15:25, abarth wrote:
> s/isAllowedAccess/canSendBeacon/  ?

That captures purpose more precisely, I like it. Adopted.

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.cpp:69: if (settings) {
On 2014/05/08 13:15:25, abarth wrote:
> We usually combine these lines.
> 
> I don't think we need to handle the case when m_navigator.frame() is zero
> because we check isAllowedAccess before calling this function.

Yes, removed the null check.

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
File Source/modules/beacon/NavigatorBeacon.h (right):

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.h:16: #define BLINK_MAX
On 2014/05/08 13:15:25, abarth wrote:
> ???

Sorry, that shouldn't have made into this patch. Removed.

https://codereview.chromium.org/232053005/diff/70001/Source/modules/beacon/Na...
Source/modules/beacon/NavigatorBeacon.h:26: class NavigatorBeacon FINAL : public
NoBaseWillBeGarbageCollectedFinalized<NavigatorBeacon>, public
WillBeHeapSupplement<Navigator> {
On 2014/05/08 13:15:25, abarth wrote:
> Are you use this class needs to be finalized?

No, nothing needs to be finalized from this class or the supplement. Thanks,
simplified to GarbageCollected<>.

[abarth-chromium, 2014-05-09 03:16:09]

On 2014/05/08 20:47:07, sof wrote:
> (If you recall why POSTs are Origin-decorated, I'd be interested to hear. git
> history is a bit chaotic for loader code.)

It's a (perhaps ill-conceived) CSRF mitigation.

[sof, 2014-05-09 06:48:59]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-09 06:50:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/232053005/90001

[Mike West, 2014-05-09 06:59:26]

Nice work, Sigbjorn!

One note about the CSP integration: my understanding is that Beacon requests do
follow redirects. Is that the case?

If so, I don't think the checks you're doing are sufficient. Would you mind
looking at the case in which a beacon is redirected to a page that the active
policy would block?

[Mike West, 2014-05-09 07:02:09]

https://codereview.chromium.org/232053005/diff/90001/Source/core/loader/Beaco...
File Source/core/loader/BeaconLoader.cpp (right):

https://codereview.chromium.org/232053005/diff/90001/Source/core/loader/Beaco...
Source/core/loader/BeaconLoader.cpp:70: // Q: not in the spec; should there be
some default?
Nit: We ususally stick with FIXME, even for questions. It makes it easier to
grep the codebase for things we're unsure about.

[sof, 2014-05-09 07:03:26]

The CQ bit was unchecked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-09 07:04:07]

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

[sof, 2014-05-09 07:50:08]

On 2014/05/09 06:59:26, Mike West wrote:
> Nice work, Sigbjorn!
> 
> One note about the CSP integration: my understanding is that Beacon requests
do
> follow redirects. Is that the case?
> 
> If so, I don't think the checks you're doing are sufficient. Would you mind
> looking at the case in which a beacon is redirected to a page that the active
> policy would block?

I left that redirect check out for CSP, yes. Once the fetch goes ahead, we're
operating outside of an execution context / document, so reliably holding onto a
well-formed ContentSecurityPolicy becomes troublesome.

Will see what I can do.

[sof, 2014-05-09 15:40:43]

On 2014/05/09 07:50:08, sof wrote:
> On 2014/05/09 06:59:26, Mike West wrote:
> > Nice work, Sigbjorn!
> > 
> > One note about the CSP integration: my understanding is that Beacon requests
> do
> > follow redirects. Is that the case?
> > 
> > If so, I don't think the checks you're doing are sufficient. Would you mind
> > looking at the case in which a beacon is redirected to a page that the
active
> > policy would block?
> 
> I left that redirect check out for CSP, yes. Once the fetch goes ahead, we're
> operating outside of an execution context / document, so reliably holding onto
a
> well-formed ContentSecurityPolicy becomes troublesome.
> 
> Will see what I can do.

This brings about something new, I believe: how to summarily cancel a ping
resource request.

A ping (which is the resource type of beacons also) resource request ends up
being represented by a 'detached resource handler' on the Chromium side. Which
upon receiving a cancel request isn't cancelled, but detached, so as to drive
the request to conclusion independently of document navigation, see
ResourceLoader::CancelRequestInternal().

If the CSP check fails on redirect, we don't want that behaviour, but a real
abort, so cancel()ing the load doesn't have the desired effect -- it runs into
an ASSERT when the deferred handler is detached, resumed and attempts to follow
the redirect.

Need to address that, but I'm not sure how (first time I've seen this Chromium
loader code is today..) Relevant bug for this deferred handling logic is
crbug.com/286186. Suggestions?

Anyway, I've updated the CL with a new patchset, which support CSP checking on
redirect. Without addressing the cancellation of ping requests on the Chromium
side, it crashes in debug builds on the added test, so it clearly cannot land in
its current state.

Somewhat of a (dark) corner case this, overall.

[sof, 2014-05-09 15:43:02]

@davidben: if you've got suggestions on how to proceed with what #15 (tries to)
detail, I'd be very interested to hear. Thanks!

[davidben, 2014-05-09 16:29:40]

On 2014/05/09 15:43:02, sof wrote:
> @davidben: if you've got suggestions on how to proceed with what #15 (tries
to)
> detail, I'd be very interested to hear. Thanks!

So, the weird quirk where cancels are ignored is honestly kinda silly and I
think just shouldn't be there. I think it was added to work around some prefetch
thing where Blink would cancel the prefetches when navigating away and people
wanted it to outlive it? (I just copied that over from the original version of
the detachable resource thing when I rewrote it.)

That said, does doing a CSP redirect check in the renderer actually work? Even
without that cancellation quirk, the renderer may be gone by the time the
redirect occurs. I think if CSP redirect checks are to happen reliably for
beacons, we'd have to staple a representation to the resource request and have
the browser enforce it.

(Should CSP even constrain redirects here? I'm really unclear on the point of
CSP when it's limiting things other than scripts and equivalent. Presumably you
could always exfiltrate data by doing a normal navigation or something. The
redirect checks on form-action complicate
https://code.google.com/p/chromium/issues/detail?id=368813 too and is another
case where we'll probably need to push CSP evaluation to the browser.)

[sof, 2014-05-09 19:12:04]

On 2014/05/09 16:29:40, David Benjamin wrote:
> On 2014/05/09 15:43:02, sof wrote:
> > @davidben: if you've got suggestions on how to proceed with what #15 (tries
> to)
> > detail, I'd be very interested to hear. Thanks!
> 
> So, the weird quirk where cancels are ignored is honestly kinda silly and I
> think just shouldn't be there. I think it was added to work around some
prefetch
> thing where Blink would cancel the prefetches when navigating away and people
> wanted it to outlive it? (I just copied that over from the original version of
> the detachable resource thing when I rewrote it.)
> 

I see, that's useful to know; thanks. I haven't evaluated the effect of turning
that Detach() into a Cancel(), or looked at the testcases on the prefetch bugs
it addressed in detail.

> That said, does doing a CSP redirect check in the renderer actually work? Even
> without that cancellation quirk, the renderer may be gone by the time the
> redirect occurs. I think if CSP redirect checks are to happen reliably for
> beacons, we'd have to staple a representation to the resource request and have
> the browser enforce it.
> 

You're completely right, the checking cannot be done reliably on the renderer
side. Ouch.

> (Should CSP even constrain redirects here? I'm really unclear on the point of
> CSP when it's limiting things other than scripts and equivalent. Presumably
you
> could always exfiltrate data by doing a normal navigation or something. The
> redirect checks on form-action complicate
> https://code.google.com/p/chromium/issues/detail?id=368813 too and is another
> case where we'll probably need to push CSP evaluation to the browser.)

I think sendBeacon() falls under what connect-src tries to restrict, even if no
spec requires this right now. Gecko does support it, including redirects (tested
with FF31 just now.)

[sof, 2014-05-10 07:32:47]

How about not supporting CSP redirects here, initially? CSP checking in the
browser process is needed to do this right.

[abarth-chromium, 2014-05-10 13:49:54]

On 2014/05/10 07:32:47, sof wrote:
> How about not supporting CSP redirects here, initially? CSP checking in the
> browser process is needed to do this right.

I think it's fine to defer to a later CL.  However, we'll probably need to
discuss more whether we want to enable this feature by default with that part
working right.

[sof, 2014-05-10 14:20:44]

On 2014/05/10 13:49:54, abarth wrote:
> On 2014/05/10 07:32:47, sof wrote:
> > How about not supporting CSP redirects here, initially? CSP checking in the
> > browser process is needed to do this right.
> 
> I think it's fine to defer to a later CL.  However, we'll probably need to
> discuss more whether we want to enable this feature by default with that part
> working right.

Yes, the experiences gained from having it available as an experimental feature
will separately help that discussion.

mkwst: what's your assessment; suggestions for how to proceed wrt CSP support?

[Mike West, 2014-05-10 17:35:18]

On 2014/05/10 14:20:44, sof wrote:
> On 2014/05/10 13:49:54, abarth wrote:
> > On 2014/05/10 07:32:47, sof wrote:
> > > How about not supporting CSP redirects here, initially? CSP checking in
the
> > > browser process is needed to do this right.
> > 
> > I think it's fine to defer to a later CL.  However, we'll probably need to
> > discuss more whether we want to enable this feature by default with that
part
> > working right.
> 
> Yes, the experiences gained from having it available as an experimental
feature
> will separately help that discussion.
> 
> mkwst: what's your assessment; suggestions for how to proceed wrt CSP support?

Yes, please just land this. Sorry my comments were unclear.

I don't think redirects should block this feature. You should land it as-is; I'd
just like you to take a look at redirects at some point in the future. If you
file a bug for yourself, I'm perfectly happy. :)

[Mike West, 2014-05-10 17:36:13]

On 2014/05/10 17:35:18, Mike West wrote:
> On 2014/05/10 14:20:44, sof wrote:
> > On 2014/05/10 13:49:54, abarth wrote:
> > > On 2014/05/10 07:32:47, sof wrote:
> > > > How about not supporting CSP redirects here, initially? CSP checking in
> the
> > > > browser process is needed to do this right.
> > > 
> > > I think it's fine to defer to a later CL.  However, we'll probably need to
> > > discuss more whether we want to enable this feature by default with that
> part
> > > working right.
> > 
> > Yes, the experiences gained from having it available as an experimental
> feature
> > will separately help that discussion.
> > 
> > mkwst: what's your assessment; suggestions for how to proceed wrt CSP
support?
> 
> Yes, please just land this. Sorry my comments were unclear.
> 
> I don't think redirects should block this feature. You should land it as-is;
I'd
> just like you to take a look at redirects at some point in the future. If you
> file a bug for yourself, I'm perfectly happy. :)

(And we should talk about moving bits of CSP to the browser: I have that on my
list for unrelated reasons. It'd be nice if we could work together on it.)

[sof, 2014-05-11 14:35:14]

On 2014/05/10 17:36:13, Mike West wrote:
> On 2014/05/10 17:35:18, Mike West wrote:
> > On 2014/05/10 14:20:44, sof wrote:
> > > On 2014/05/10 13:49:54, abarth wrote:
> > > > On 2014/05/10 07:32:47, sof wrote:
> > > > > How about not supporting CSP redirects here, initially? CSP checking
in
> > the
> > > > > browser process is needed to do this right.
> > > > 
> > > > I think it's fine to defer to a later CL.  However, we'll probably need
to
> > > > discuss more whether we want to enable this feature by default with that
> > part
> > > > working right.
> > > 
> > > Yes, the experiences gained from having it available as an experimental
> > feature
> > > will separately help that discussion.
> > > 
> > > mkwst: what's your assessment; suggestions for how to proceed wrt CSP
> support?
> > 
> > Yes, please just land this. Sorry my comments were unclear.
> > 
> > I don't think redirects should block this feature. You should land it as-is;
> I'd
> > just like you to take a look at redirects at some point in the future. If
you
> > file a bug for yourself, I'm perfectly happy. :)
> 
> (And we should talk about moving bits of CSP to the browser: I have that on my
> list for unrelated reasons. It'd be nice if we could work together on it.)

Thanks for clarifying, created
https://code.google.com/p/chromium/issues/detail?id=372197 + reverted the
experimental redirect changes.

If I can contribute to moving CSP enforcement, great. Perhaps there'll be a bit
of time to talk about it next week?

[sof, 2014-05-11 14:35:24]

https://codereview.chromium.org/232053005/diff/90001/Source/core/loader/Beaco...
File Source/core/loader/BeaconLoader.cpp (right):

https://codereview.chromium.org/232053005/diff/90001/Source/core/loader/Beaco...
Source/core/loader/BeaconLoader.cpp:70: // Q: not in the spec; should there be
some default?
On 2014/05/09 07:02:10, Mike West wrote:
> Nit: We ususally stick with FIXME, even for questions. It makes it easier to
> grep the codebase for things we're unsure about.

Thanks, done.

[sof, 2014-05-11 14:35:39]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-11 14:35:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/232053005/130001

[commit-bot: I haz the power, 2014-05-11 14:51:51]

Change committed as 173837