Bug 175192 - [Beacon] Update sendBeacon to use the CachedResourceLoader
Summary: [Beacon] Update sendBeacon to use the CachedResourceLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 147885
  Show dependency treegraph
 
Reported: 2017-08-04 09:32 PDT by Chris Dumez
Modified: 2017-08-04 16:09 PDT (History)
10 users (show)

See Also:


Attachments
WIP Patch (13.92 KB, patch)
2017-08-04 09:39 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (989.38 KB, application/zip)
2017-08-04 10:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.07 MB, application/zip)
2017-08-04 10:53 PDT, Build Bot
no flags Details
WIP Patch (22.40 KB, patch)
2017-08-04 11:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (28.41 KB, patch)
2017-08-04 12:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.49 KB, patch)
2017-08-04 13:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.34 KB, patch)
2017-08-04 14:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.29 KB, patch)
2017-08-04 15:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-08-04 09:32:37 PDT
Update sendBeacon to use the CachedResourceLoader and use the normal loading code path. This avoids some code duplication.
Comment 1 Radar WebKit Bug Importer 2017-08-04 09:33:06 PDT
<rdar://problem/33725923>
Comment 2 Chris Dumez 2017-08-04 09:39:07 PDT
Created attachment 317251 [details]
WIP Patch
Comment 3 youenn fablet 2017-08-04 10:12:28 PDT
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.
Comment 4 Chris Dumez 2017-08-04 10:32:11 PDT
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.
Comment 5 Chris Dumez 2017-08-04 10:34:18 PDT
(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.
Comment 6 youenn fablet 2017-08-04 10:36:36 PDT
(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.
Comment 7 youenn fablet 2017-08-04 10:37:06 PDT
Or we can trigger an error whenever trying to do a keep alive fetch API load.
Comment 8 youenn fablet 2017-08-04 10:37:48 PDT
(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.
Comment 9 Build Bot 2017-08-04 10:49:20 PDT
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
Comment 10 Build Bot 2017-08-04 10:49:22 PDT
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
Comment 11 Build Bot 2017-08-04 10:53:11 PDT
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
Comment 12 Build Bot 2017-08-04 10:53:13 PDT
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
Comment 13 Chris Dumez 2017-08-04 11:03:41 PDT
Created attachment 317259 [details]
WIP Patch
Comment 14 Chris Dumez 2017-08-04 11:40:39 PDT
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.
Comment 15 youenn fablet 2017-08-04 12:18:40 PDT
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.
Comment 16 Chris Dumez 2017-08-04 12:34:31 PDT
Created attachment 317268 [details]
Patch
Comment 17 Chris Dumez 2017-08-04 12:41:49 PDT
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?
Comment 18 youenn fablet 2017-08-04 12:51:47 PDT
(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.
Comment 19 Chris Dumez 2017-08-04 13:35:16 PDT
Created attachment 317275 [details]
Patch
Comment 20 Chris Dumez 2017-08-04 13:38:44 PDT
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.
Comment 21 youenn fablet 2017-08-04 14:04:59 PDT
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...
Comment 22 Chris Dumez 2017-08-04 14:07:41 PDT
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".
Comment 23 Chris Dumez 2017-08-04 14:08:43 PDT
Created attachment 317283 [details]
Patch
Comment 24 youenn fablet 2017-08-04 15:19:02 PDT
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?
Comment 25 Chris Dumez 2017-08-04 15:26:08 PDT
(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).
Comment 26 Chris Dumez 2017-08-04 15:28:19 PDT
Created attachment 317302 [details]
Patch
Comment 27 WebKit Commit Bot 2017-08-04 16:09:28 PDT
Comment on attachment 317302 [details]
Patch

Clearing flags on attachment: 317302

Committed r220303: <http://trac.webkit.org/changeset/220303>
Comment 28 WebKit Commit Bot 2017-08-04 16:09:30 PDT
All reviewed patches have been landed.  Closing bug.