Bug 188852

<rdar://problem/42572559>

Created attachment 347840 [details]
Patch

Created attachment 347842 [details]
Patch

Comment on attachment 347842 [details]
Patch

Attachment 347842 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8949139

New failing tests:
css3/filters/backdrop/add-remove-add-backdrop-filter.html

Created attachment 347851 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

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

Comment on attachment 347842 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        has been granted Storage Access API permissions, allow the ping.

Is there a way to test this change?

> Source/WebKit/NetworkProcess/PingLoad.cpp:64
> +        if (this->loadIsForPrevalentDomain()) {

This check applies to all PingLoad users, like Beacon API loads for instance.
Will we not cancel some loads that we should conduct?

> Source/WebKit/NetworkProcess/PingLoad.cpp:132
> +        if (this->loadIsForPrevalentDomain()) {

loadIsForPrevalentDomain is using m_parameters.request.url(), and it seems we are not updating this URL.
Maybe loadIsForPrevalentDomain should take a const URL& as input parameter.

Created attachment 347866 [details]
Patch

It doesn't seem like the image-only failure for 'css3/filters/backdrop/add-remove-add-backdrop-filter.html' is related to this PingLoad change.

Comment on attachment 347866 [details]
Patch

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

You should be able to create a layout test using the Resource Load Statistics and the Storage Access API test infrastructure.

> Source/WebKit/NetworkProcess/PingLoad.cpp:89
> +        return networkSession->networkStorageSession().shouldBlockCookies(sourceOrigin, url, m_parameters.webPageID, m_parameters.webFrameID);

pageID and frameID should be flipped. I've been wanting to revisit the parameter ordering throughout the call chain. The reason for the current ordering is that the frameID is the main key in the data structure but we should opt for an ordering that's less confusing instead.

> Source/WebKit/NetworkProcess/PingLoad.cpp:133
> +            this->didFinish(ResourceError { String { }, 0, request.url(), "Redirection to prevalent Domain"_s, ResourceError::Type::AccessControl });

Lower case 'd' in domain.

Created attachment 347929 [details]
Patch

Comment on attachment 347929 [details]
Patch

Attachment 347929 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8959290

New failing tests:
http/tests/resourceLoadStatistics/ping-to-prevalent-resource.html

Created attachment 347945 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

The test failure on iOS Sim is probably the event sender. IIRC, the Storage Access API tests that involve taps/clicks are not enabled on iOS for this very reason. :(

(In reply to John Wilander from comment #13)
> The test failure on iOS Sim is probably the event sender. IIRC, the Storage
> Access API tests that involve taps/clicks are not enabled on iOS for this
> very reason. :(

Oh! I'll add a skip on iOS.

Created attachment 347949 [details]
Patch

It is not clear to me whether Beacon API loads should also be blocked in that case.
If that is the case, can we also include a test for it?

Comment on attachment 347949 [details]
Patch

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

Looks good to me. One nit below.

> Source/WebKit/NetworkProcess/PingLoad.cpp:65
> +            this->didFinish(ResourceError { String { }, 0, this->m_parameters.request.url(), "Ping request to prevalent Domain"_s, ResourceError::Type::AccessControl });

Should be a lower case 'd' in my opinion.

Created attachment 347954 [details]
Patch

This will encourage the use of sync xhr.  Can we block sync xhr if we have chosen to block its use?

Auditing hyperlink spec allows to not fetch the resource.
On the other hand, Beacon API spec does not provide the ability to skip the load.

I am not sure we have a way to identify whether a given request is hyperlink auditing vs. beacon API in the network process, maybe we should pass that information from web process.

(In reply to Alex Christensen from comment #19)
> This will encourage the use of sync xhr.  Can we block sync xhr if we have
> chosen to block its use?

Yes, I think we can do so using the same criteria for Ping and Beacon.

Created attachment 347975 [details]
Patch

Created attachment 347977 [details]
Patch

Comment on attachment 347977 [details]
Patch

One of the motivations for the Beacon API was to give people using sync xhr to phone home an alternative that doesn't hurt performance.  If we make that alternative, people will notice that some of their analytics don't arrive and they will use sync xhr instead.  Unless we do the same restrictions to sync xhr I think this is a step in the wrong direction.

(In reply to Alex Christensen from comment #24)
> Comment on attachment 347977 [details]
> Patch
> 
> One of the motivations for the Beacon API was to give people using sync xhr
> to phone home an alternative that doesn't hurt performance.  If we make that
> alternative, people will notice that some of their analytics don't arrive
> and they will use sync xhr instead.  Unless we do the same restrictions to
> sync xhr I think this is a step in the wrong direction.

We should absolutely block sync XHR for them. That’ll help deprecating sync XHR all together, which is good for the web.

Comment on attachment 347977 [details]
Patch

I am still fuzzy of the benefit here.

As per https://html.spec.whatwg.org/multipage/links.html#hyperlink-auditing, the ping is only sent when user clicks the link.
If this ping is blocked and the script really wants to send this information, it has a bunch of options:
- Have a function executed at click time to send the information through web socket/XHR before doing the navigation.
- Use a redirection URL for the navigation to provide the exact same information.
- Send that information as regular XHR/fetch at loading of the next page (work pretty easily for same site navigation)
- Use service worker and trigger a regular fetch in service worker.

Blocking ping loads is ok as per spec.
Blocking sync XHR is probably fine since we want to deprecate it and there is a way for the app to know whether it failed so that it can take some measures.

Blocking sendBeacon on the other hand is not currently allowed by the spec and there is no way for the web developer to know that it was blocked. This might not encourage web developers to use it and they may rely on regular XHR/fetch API instead.

This would be sad since one of the point of sendBeacon is that the beacon requests could be optimized as described in the spec:
- The user agent MAY delay transmission of provided data to optimize network and energy efficiency
- The user agent MUST schedule immediate transmission of all beacon requests when the document visibilityState transitions to hidden

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:209
> +            if (this->isSynchronous() && this->loadIsForPrevalentDomain(this->m_parameters.request.url())) {

There is no need to do this check after network load checker did it.
It could be done just before if (m_networkLoadChecker).

Also, sync XHR redirections are not checked in this patch, see NetworkResourceLoader::willSendRedirectedRequest.

Comment on attachment 347977 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:209
>> +            if (this->isSynchronous() && this->loadIsForPrevalentDomain(this->m_parameters.request.url())) {
> 
> There is no need to do this check after network load checker did it.
> It could be done just before if (m_networkLoadChecker).
> 
> Also, sync XHR redirections are not checked in this patch, see NetworkResourceLoader::willSendRedirectedRequest.

Which check do you mean? The check for synchronicity? Or the prevalence check?

Thanks for catching the redirect case, I'll fix that.

(In reply to Alex Christensen from comment #24)
> Comment on attachment 347977 [details]
> Patch
> 
> One of the motivations for the Beacon API was to give people using sync xhr
> to phone home an alternative that doesn't hurt performance.  If we make that
> alternative, people will notice that some of their analytics don't arrive
> and they will use sync xhr instead.  Unless we do the same restrictions to
> sync xhr I think this is a step in the wrong direction.

I agree. The current patch (now) subjects synchronous XHR to the same checks and blocking as ping or beacon operations.

Created attachment 348015 [details]
Patch

Created attachment 388786 [details]
Patch

Comment on attachment 388786 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +               http/tests/navigation/ping-attribute/area-cross-origin.html

Are we missing some WPT tests?

Comment on attachment 388786 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +               http/tests/navigation/ping-attribute/area-cross-origin.html
> 
> Are we missing some WPT tests?

Yes. Dominic Farolino left a comment adding WPT tests for this in chromium sources so I will coordinate with him.

Comment on attachment 388786 [details]
Patch

Clearing flags on attachment: 388786

Committed r255129: <https://trac.webkit.org/changeset/255129>

All reviewed patches have been landed.  Closing bug.

Reopening since Brent was targeting a different compatibility problem initially.

(In reply to Rob Buis from comment #35)
> Reopening since Brent was targeting a different compatibility problem
> initially.

Oh yes, the referrer patch should have had its own bug.

@Brent - I think  we can create separate bug for referrer refinement as per Comment 36 and close this one? Or it is something still in your radar? Thanks!