Bug 175192 – [Beacon] Update sendBeacon to use the CachedResourceLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Update sendBeacon to use the CachedResourceLoader and use the normal loading code path. This avoids some code duplication.

[Radar WebKit Bug Importer]

<rdar://problem/33725923>

[Chris Dumez]

Created attachment 317251 [details]
WIP Patch

[youenn fablet]

Comment on attachment 317251 [details]
WIP Patch

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

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

I would tend to remove PingLoader::sendBeacon and call requestBeaconResource directly in NavigatorBeacon.
That will make it clear that we use fetch algorithm.
We could also try to do some renaming, like CachedResourceRequest -> FetchRequest, for the same reason.

> Source/WebCore/loader/cache/CachedResource.cpp:265
> +    }

Looks good to me for now.
In the future, we might want to consider using loadResource and pass the keepAlive information to the network process.
That will enable us to fully implement keepAlive, which can also be triggered by JS/Fetch API outside of the beacon API.

[Chris Dumez]

Grumpf, this is breaking LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-keepalive-quota.html now that keepalive CachedResource uses a PingHandle...

Not sure how to fix this one yet since PingHandle currently does not provide any feedback.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Grumpf, this is breaking
> LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-
> keepalive-quota.html now that keepalive CachedResource uses a PingHandle...
> 
> Not sure how to fix this one yet since PingHandle currently does not provide
> any feedback.

I think part of the problem is that while sendBeacon does not have a completion handler (and therefore can use PingHandle as is), the Fetch API itself cannot because it has a completion handler.

[youenn fablet]

(In reply to Chris Dumez from comment #4)
> Grumpf, this is breaking
> LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-
> keepalive-quota.html now that keepalive CachedResource uses a PingHandle...
> 
> Not sure how to fix this one yet since PingHandle currently does not provide
> any feedback.

For now, we will not be able to resolve the promise for any keep-alive fetch.
That seems ok to me.
Can we just rebase request-keepalive-quota.html since we are not supporting the tested functionality anyway? It would be good to not skip it if possible.

[youenn fablet]

Or we can trigger an error whenever trying to do a keep alive fetch API load.

[youenn fablet]

(In reply to youenn fablet from comment #7)
> Or we can trigger an error whenever trying to do a keep alive fetch API load.

At FetchLoader level.

[Build Bot]

Comment on attachment 317251 [details]
WIP Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-keepalive-quota.html
http/wpt/beacon/connect-src-beacon-blocked.sub.html

[Build Bot]

Created attachment 317256 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Build Bot]

Comment on attachment 317251 [details]
WIP Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-keepalive-quota.html
http/wpt/beacon/connect-src-beacon-blocked.sub.html

[Build Bot]

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

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

[Chris Dumez]

Created attachment 317259 [details]
WIP Patch

[Chris Dumez]

Comment on attachment 317259 [details]
WIP Patch

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

> Source/WebCore/loader/cache/CachedResource.cpp:262
> +    if (m_options.keepAlive && type() == CachedResource::Beacon) {

@Youenn, are you OK with this approach for now or do you want me to throw an exception and fetch/keepalive as you suggested earlier?

I'll look into improving this when I add support for keepalive quota limitation since I will need it for beacon.

[youenn fablet]

Comment on attachment 317259 [details]
WIP Patch

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

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:72
> +    // FIXME: We should restrict the size of payloads.

This does not pertain really here.
Maybe add this FIXME in CachedResourceLoader instead?

>> Source/WebCore/loader/cache/CachedResource.cpp:262
>> +    if (m_options.keepAlive && type() == CachedResource::Beacon) {
> 
> @Youenn, are you OK with this approach for now or do you want me to throw an exception and fetch/keepalive as you suggested earlier?
> 
> I'll look into improving this when I add support for keepalive quota limitation since I will need it for beacon.

This approach seems fine for now.

[Chris Dumez]

Created attachment 317268 [details]
Patch

[Chris Dumez]

Comment on attachment 317268 [details]
Patch

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

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:49
> +static Ref<FormData> extractBody(FetchBody::Init& bodyInit, Document& document, String& mimeType)

It feels like I should be using the FetchRequest class but I guess this would be a lot more effort?

[youenn fablet]

(In reply to Chris Dumez from comment #17)
> Comment on attachment 317268 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317268&action=review
> 
> > Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:49
> > +static Ref<FormData> extractBody(FetchBody::Init& bodyInit, Document& document, String& mimeType)
> 
> It feels like I should be using the FetchRequest class but I guess this
> would be a lot more effort?

Right, you can probably reuse/adapt FetchBody, by doing something like: FetchBody::extract(...).bodyForInternalRequest().
Either in this patch or as a follow-up.

[Chris Dumez]

Created attachment 317275 [details]
Patch

[Chris Dumez]

Comment on attachment 317275 [details]
Patch

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

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:71
> +    document.cachedResourceLoader().requestBeaconResource({ fetchRequest->internalRequest(), fetchRequest->fetchOptions() });

@Youeen, since you know the Fetch code best, do you think it would be viable for me to use FetchLoader in some way now that I rely on FetchRequest? Using FetchRequest makes the sendBeacon code look a lot closer to spec and gets rid of a lot of code duplication. However, it still feels a bit weird to extra its internalRequest to use the CachedResourceLoader.
Also, I think that I would get the CSP check for free if I could use the FetchLoader somehow.

[youenn fablet]

Comment on attachment 317275 [details]
Patch

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

>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:71
>> +    document.cachedResourceLoader().requestBeaconResource({ fetchRequest->internalRequest(), fetchRequest->fetchOptions() });
> 
> @Youeen, since you know the Fetch code best, do you think it would be viable for me to use FetchLoader in some way now that I rely on FetchRequest? Using FetchRequest makes the sendBeacon code look a lot closer to spec and gets rid of a lot of code duplication. However, it still feels a bit weird to extra its internalRequest to use the CachedResourceLoader.
> Also, I think that I would get the CSP check for free if I could use the FetchLoader somehow.

Technically using FetchLoader would be the most spec compliant option.
But then you are tied to using RawResource and this patch currently needs to distinguish between the two for various things like priority, whether to use ping handle...

Might be best to keep the CachedResource::Beacon approach for now.
If we are successful in fetch keepAlive implementation, it should be quick to move to FetchLoader.

It would also be good if we could somehow rationalize how we are doing the CSP check, currently it is done in various loader clients.

> Source/WebCore/loader/SubresourceLoader.cpp:450
> +    case CachedResource::Beacon:

At the moment, we should add ASSERT_NOT_REACHED here.

> LayoutTests/http/wpt/beacon/headers/header-content-type-same-origin.html:80
> +testContentTypeHeader(stringToArrayBufferView("123"), "application/x-www-form-urlencoded", "ArrayBufferView"); // FIXME: It is unclear what the Content-Type should be here.

Or even if a Content-Type should be present...

[Chris Dumez]

Comment on attachment 317275 [details]
Patch

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

>>> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:71
>>> +    document.cachedResourceLoader().requestBeaconResource({ fetchRequest->internalRequest(), fetchRequest->fetchOptions() });
>> 
>> @Youeen, since you know the Fetch code best, do you think it would be viable for me to use FetchLoader in some way now that I rely on FetchRequest? Using FetchRequest makes the sendBeacon code look a lot closer to spec and gets rid of a lot of code duplication. However, it still feels a bit weird to extra its internalRequest to use the CachedResourceLoader.
>> Also, I think that I would get the CSP check for free if I could use the FetchLoader somehow.
> 
> Technically using FetchLoader would be the most spec compliant option.
> But then you are tied to using RawResource and this patch currently needs to distinguish between the two for various things like priority, whether to use ping handle...
> 
> Might be best to keep the CachedResource::Beacon approach for now.
> If we are successful in fetch keepAlive implementation, it should be quick to move to FetchLoader.
> 
> It would also be good if we could somehow rationalize how we are doing the CSP check, currently it is done in various loader clients.

Agreed.

>> Source/WebCore/loader/SubresourceLoader.cpp:450
>> +    case CachedResource::Beacon:
> 
> At the moment, we should add ASSERT_NOT_REACHED here.

OK.

>> LayoutTests/http/wpt/beacon/headers/header-content-type-same-origin.html:80
>> +testContentTypeHeader(stringToArrayBufferView("123"), "application/x-www-form-urlencoded", "ArrayBufferView"); // FIXME: It is unclear what the Content-Type should be here.
> 
> Or even if a Content-Type should be present...

Yes. FYI, I don't think the FetchBody code setting any Content-Type header but I guess some lower level code ends up falling back to "application/x-www-form-urlencoded".

[Chris Dumez]

Created attachment 317283 [details]
Patch

[youenn fablet]

Comment on attachment 317283 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:121
> +    case CachedResource::Beacon:

Shouldn't it be Seconds::infinity() since we might only care when the load is finished in the future?

[Chris Dumez]

(In reply to youenn fablet from comment #24)
> Comment on attachment 317283 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317283&action=review
> 
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:121
> > +    case CachedResource::Beacon:
> 
> Shouldn't it be Seconds::infinity() since we might only care when the load
> is finished in the future?

This parameter is only used by NetworkResourceLoader. A value greater than 0_s will buffer the received data for the given duration before sending it back to the WebProcess.

In the beacon case, PingLoad is used instead of NetworkResourceLoader. PingLoad does not use maximumBufferingTime because ping load do not expect any data in the response from the server. Therefore, no buffering is needed.

As a result, the value use does not really matter. Not sure which one is better, infinity or 0_s (in theory since it does not matter in practice).

[Chris Dumez]

Created attachment 317302 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 317302 [details]
Patch

Clearing flags on attachment: 317302

Committed r220303: <http://trac.webkit.org/changeset/220303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.