Add initial support for navigator.sendBeacon

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Add initial support for navigator.sendBeacon

[Chris Dumez]

<rdar://problem/33547728>

[Chris Dumez]

Created attachment 316828 [details]
WIP Patch

[Build Bot]

Attachment 316828 [details] did not pass style-queue:


ERROR: Source/WebCore/CMakeLists.txt:875:  Alphabetical sorting problem. "Modules/NavigatorBeacon.cpp" should be before "Modules/airplay/WebKitPlaybackTargetAvailabilityEvent.cpp".  [list/order] [5]
Total errors found: 1 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 316864 [details]
WIP Patch

[Chris Dumez]

Created attachment 316872 [details]
WIP Patch

[Chris Dumez]

Created attachment 316880 [details]
Patch

[Sam Weinig]

Comment on attachment 316880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316880&action=review

> Source/WebCore/Modules/beacon/NavigatorBeacon.idl:30
> +    [CallWith=Document, EnabledAtRuntime=BeaconAPI, MayThrowException] boolean sendBeacon(USVString url, optional BodyInit? data = null);

Does sendBeacon not exist for Workers? Can this use CallWith=ScriptExecutionContext?

> Source/WebCore/page/RuntimeEnabledFeatures.h:156
> +    void setBeaconAPIEnabled(bool isEnabled) { m_isBeaconAPIEnabled = isEnabled; }
> +    bool beaconAPIEnabled() const { return m_isBeaconAPIEnabled; }

Can this use Settings instead?

[Chris Dumez]

Comment on attachment 316880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316880&action=review

>> Source/WebCore/Modules/beacon/NavigatorBeacon.idl:30
>> +    [CallWith=Document, EnabledAtRuntime=BeaconAPI, MayThrowException] boolean sendBeacon(USVString url, optional BodyInit? data = null);
> 
> Does sendBeacon not exist for Workers? Can this use CallWith=ScriptExecutionContext?

As far as I can tell, this is not exposed to workers at the moment:
https://w3c.github.io/beacon/#sec-sendBeacon-method

It extended Navigator, not WorkerNavigator:
https://html.spec.whatwg.org/multipage/workers.html#workernavigator

If we do decide to expose to workers at some point, then using CallWith=ScriptExecutionContext would work. For now though, I do not see an obvious reason not to use CallWith=Document.

>> Source/WebCore/page/RuntimeEnabledFeatures.h:156
>> +    bool beaconAPIEnabled() const { return m_isBeaconAPIEnabled; }
> 
> Can this use Settings instead?

It is exposed via the "Experimental Features" menu. This seems to be how those features are implemented at the moment but I'll check if any of them is backed by a setting.

[Chris Dumez]

Comment on attachment 316880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316880&action=review

>>> Source/WebCore/page/RuntimeEnabledFeatures.h:156
>>> +    bool beaconAPIEnabled() const { return m_isBeaconAPIEnabled; }
>> 
>> Can this use Settings instead?
> 
> It is exposed via the "Experimental Features" menu. This seems to be how those features are implemented at the moment but I'll check if any of them is backed by a setting.

Also, I don't get why we want a setting here. RuntimeEnabled is how we've been doing experimental features so far, no? Did we change our policy?

[Sam Weinig]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 316880 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316880&action=review
> 
> >>> Source/WebCore/page/RuntimeEnabledFeatures.h:156
> >>> +    bool beaconAPIEnabled() const { return m_isBeaconAPIEnabled; }
> >> 
> >> Can this use Settings instead?
> > 
> > It is exposed via the "Experimental Features" menu. This seems to be how those features are implemented at the moment but I'll check if any of them is backed by a setting.
> 
> Also, I don't get why we want a setting here. RuntimeEnabled is how we've
> been doing experimental features so far, no? Did we change our policy?

Subresource Integrity uses Settings and is an Experimental Feature. 

The reason to use Settings is that I would like us to not have concepts that are so similar, Settings and RuntimeEnabledFeatures.  Settings, if implemented via Settings.in also gives you automatic generation of InternalSettingsGenerated so it can be accessed from tests.

[Sam Weinig]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 316880 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316880&action=review
> 
> >>> Source/WebCore/page/RuntimeEnabledFeatures.h:156
> >>> +    bool beaconAPIEnabled() const { return m_isBeaconAPIEnabled; }
> >> 
> >> Can this use Settings instead?
> > 
> > It is exposed via the "Experimental Features" menu. This seems to be how those features are implemented at the moment but I'll check if any of them is backed by a setting.
> 
> Also, I don't get why we want a setting here. RuntimeEnabled is how we've
> been doing experimental features so far, no? Did we change our policy?

We don't have a policy, and I have been heavily encouraging others to use Settings in other bugs.

[Chris Dumez]

(In reply to Sam Weinig from comment #10)
> (In reply to Chris Dumez from comment #9)
> > Comment on attachment 316880 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=316880&action=review
> > 
> > >>> Source/WebCore/page/RuntimeEnabledFeatures.h:156
> > >>> +    bool beaconAPIEnabled() const { return m_isBeaconAPIEnabled; }
> > >> 
> > >> Can this use Settings instead?
> > > 
> > > It is exposed via the "Experimental Features" menu. This seems to be how those features are implemented at the moment but I'll check if any of them is backed by a setting.
> > 
> > Also, I don't get why we want a setting here. RuntimeEnabled is how we've
> > been doing experimental features so far, no? Did we change our policy?
> 
> Subresource Integrity uses Settings and is an Experimental Feature. 
> 
> The reason to use Settings is that I would like us to not have concepts that
> are so similar, Settings and RuntimeEnabledFeatures.  Settings, if
> implemented via Settings.in also gives you automatic generation of
> InternalSettingsGenerated so it can be accessed from tests.

Ok, I'll look at SRI and match what is done there.

[Chris Dumez]

Created attachment 316903 [details]
Patch

[Chris Dumez]

Comment on attachment 316903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316903&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4133
> +                push(@implContent, "    putDirect(vm, vm.propertyNames->iteratorSymbol, this->globalObject()->arrayPrototype()->getDirect(vm, vm.propertyNames->builtinNames().valuesPrivateName()), DontEnum);\n");

@Sam: Mixing RuntimeEnabled and EnabledBySetting on the first interface did not build without this change. The issue is that the RuntimeEnabled code uses globalObject() getter, while the EnabledBySetting code relies on an extra parameter named globalObject being passed to finishCreation(). Both names conflict. I did the easy fix to using this-> but it is not clear to me why the EnabledBySetting code needs this extra parameter and why it cannot rely on globalObject() getter.

[Sam Weinig]

(In reply to Chris Dumez from comment #14)
> Comment on attachment 316903 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316903&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4133
> > +                push(@implContent, "    putDirect(vm, vm.propertyNames->iteratorSymbol, this->globalObject()->arrayPrototype()->getDirect(vm, vm.propertyNames->builtinNames().valuesPrivateName()), DontEnum);\n");
> 
> @Sam: Mixing RuntimeEnabled and EnabledBySetting on the first interface did
> not build without this change. The issue is that the RuntimeEnabled code
> uses globalObject() getter, while the EnabledBySetting code relies on an
> extra parameter named globalObject being passed to finishCreation(). Both
> names conflict. I did the easy fix to using this-> but it is not clear to me
> why the EnabledBySetting code needs this extra parameter and why it cannot
> rely on globalObject() getter.

I doesn't. I'll fix that.

[Sam Weinig]

Comment on attachment 316903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316903&action=review

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:44
> +        return Exception { TypeError, ASCIILiteral("Beacons can only be sent over HTTP(S)") };

Do we use the "HTTP(S)" in other exception text, or anywhere else?

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:47
> +    // We do not allow sending beacons from detached documents.

This should either explain why or be removed.

> Source/WebCore/loader/PingLoader.cpp:272
> +    startPingLoad(frame, request, ShouldFollowRedirects::Yes);

What happens to a ping load that wants to follow redirects when the page is closed? Does the NetworkProcess handle this without consulting the WebProcess? What does that mean for content extensions that would like to block some of those redirects?

> Source/WebCore/loader/PingLoader.h:62
> +using BodyInit = Variant<RefPtr<Blob>, RefPtr<JSC::ArrayBufferView>, RefPtr<JSC::ArrayBuffer>, RefPtr<DOMFormData>, RefPtr<URLSearchParams>, String>;

At some point we should put this in a header.  You can use FetchBody::Init, which should be the same.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:349
> +    m_testRunner->setBeaconAPIEnabled(true);

Why not enable this in the normal preferences? Why use the weird ones in the bundle?

[Chris Dumez]

Comment on attachment 316903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316903&action=review

>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:44
>> +        return Exception { TypeError, ASCIILiteral("Beacons can only be sent over HTTP(S)") };
> 
> Do we use the "HTTP(S)" in other exception text, or anywhere else?

I do not think so. Do you have a better proposal? Do you prefer "Beacons can only be sent over HTTP or HTTPS" ?

>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:47
>> +    // We do not allow sending beacons from detached documents.
> 
> This should either explain why or be removed.

OK.

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4133
>>> +                push(@implContent, "    putDirect(vm, vm.propertyNames->iteratorSymbol, this->globalObject()->arrayPrototype()->getDirect(vm, vm.propertyNames->builtinNames().valuesPrivateName()), DontEnum);\n");
>> 
>> @Sam: Mixing RuntimeEnabled and EnabledBySetting on the first interface did not build without this change. The issue is that the RuntimeEnabled code uses globalObject() getter, while the EnabledBySetting code relies on an extra parameter named globalObject being passed to finishCreation(). Both names conflict. I did the easy fix to using this-> but it is not clear to me why the EnabledBySetting code needs this extra parameter and why it cannot rely on globalObject() getter.
> 
> I doesn't. I'll fix that.

Thanks.

>> Source/WebCore/loader/PingLoader.cpp:272
>> +    startPingLoad(frame, request, ShouldFollowRedirects::Yes);
> 
> What happens to a ping load that wants to follow redirects when the page is closed? Does the NetworkProcess handle this without consulting the WebProcess? What does that mean for content extensions that would like to block some of those redirects?

Yes, PingLoad takes care of following the redirects for us on NetworkProcess side, without involving the WebProcess. I believe this means content extensions would not be able to block those redirects.

>> Source/WebCore/loader/PingLoader.h:62
>> +using BodyInit = Variant<RefPtr<Blob>, RefPtr<JSC::ArrayBufferView>, RefPtr<JSC::ArrayBuffer>, RefPtr<DOMFormData>, RefPtr<URLSearchParams>, String>;
> 
> At some point we should put this in a header.  You can use FetchBody::Init, which should be the same.

Agreed.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:349
>> +    m_testRunner->setBeaconAPIEnabled(true);
> 
> Why not enable this in the normal preferences? Why use the weird ones in the bundle?

I'll take a look. I merely followed an existing (and apparently bad) pattern.

[Chris Dumez]

Comment on attachment 316903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316903&action=review

>>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:349
>>> +    m_testRunner->setBeaconAPIEnabled(true);
>> 
>> Why not enable this in the normal preferences? Why use the weird ones in the bundle?
> 
> I'll take a look. I merely followed an existing (and apparently bad) pattern.

Do you means a WKPreferencesSetXXXEnabled() call in TestController::resetPreferencesToConsistentValues()?

[Chris Dumez]

Created attachment 316923 [details]
Patch

[Build Bot]

Comment on attachment 316923 [details]
Patch

Attachment 316923 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4237439

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/mode-no-cors-worker.html
imported/w3c/web-platform-tests/fetch/api/basic/mode-no-cors.html

[Build Bot]

Created attachment 316928 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 316929 [details]
Patch

[Chris Dumez]

Comment on attachment 316929 [details]
Patch

Clearing flags on attachment: 316929

Committed r220121: <http://trac.webkit.org/changeset/220121>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Comment on attachment 316929 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316929&action=review

> Source/WebCore/loader/PingLoader.cpp:193
> +bool PingLoader::sendBeacon(Frame& frame, Document& document, const URL& url, std::optional<BodyInit>&& data)

I am not sure we should continue improving PingLoader.

If we were to go through DocumentThreadableLoader or CachedResourceLoader, we would have the following:
- consistent fetch algorithm with headers properly set
- worker support (if going through DTL, although I am not sure there is a good reason for that)
- beacon interception by service worker
- some sort of preflighting (if going through DTL although keepAlive would need some specific treatment)

Maybe in the short term we could do:
- add a keepAlive flag (specified in fetch spec) at CachedResourceRequest and/or ResourceRequest level
- make CachedResource or CachedResourceLoader call createPingHandle in case this flag is true

At some point, we might consider implementing the 65ko limit, which would mean that ping handle might need to communicate back that it is finished to some WebCore object.
This is close to the behavior of CachedResource or preloads.
It seems natural for CachedResourceLoader or related siblings to do this 65ko bookkeeping.

I also wonder what happens when the WebProcess is terminated, like a tab is closed.
In that case, the network process might abort the network load which is probably not expected to happen for beacon.
Maybe the NetworkProcess needs to know that the request should be kept alive.

[youenn fablet]

Support of beacon API in Worker was removed.
See https://github.com/w3c/beacon/issues/37

[Sam Weinig]

(In reply to youenn fablet from comment #25)
> Comment on attachment 316929 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316929&action=review
> 
> > Source/WebCore/loader/PingLoader.cpp:193
> > +bool PingLoader::sendBeacon(Frame& frame, Document& document, const URL& url, std::optional<BodyInit>&& data)
> 
> I am not sure we should continue improving PingLoader.
> 
> If we were to go through DocumentThreadableLoader or CachedResourceLoader,
> we would have the following:
> - consistent fetch algorithm with headers properly set
> - worker support (if going through DTL, although I am not sure there is a
> good reason for that)
> - beacon interception by service worker
> - some sort of preflighting (if going through DTL although keepAlive would
> need some specific treatment)
> 
> Maybe in the short term we could do:
> - add a keepAlive flag (specified in fetch spec) at CachedResourceRequest
> and/or ResourceRequest level
> - make CachedResource or CachedResourceLoader call createPingHandle in case
> this flag is true
> 
> At some point, we might consider implementing the 65ko limit, which would
> mean that ping handle might need to communicate back that it is finished to
> some WebCore object.
> This is close to the behavior of CachedResource or preloads.
> It seems natural for CachedResourceLoader or related siblings to do this
> 65ko bookkeeping.
> 
> I also wonder what happens when the WebProcess is terminated, like a tab is
> closed.
> In that case, the network process might abort the network load which is
> probably not expected to happen for beacon.
> Maybe the NetworkProcess needs to know that the request should be kept alive.

I agree the long term plan should be to have a consistent loader that matches the fetch algorithm. Unfortunately, we are not there. 

It seems like a great project for someone to take on to design a loading pipeline based on Fetch, but I am not sure adding features like beacon should be predicated on it.

[youenn fablet]

New features should use WebKit fetch primitive when the spec says that fetch is to be used. WebKit fetch is CachedResouceLoader/DTL.

If WebKit fetch is not sufficient, we can either make an exception or try to upgrade it.
In that particular case, a limited upgrade might not be a lot of work, I would give it a try.