[webkit-reviews] review granted: [Bug 230812] Move Cross-Origin-Opener-Policy handling to the NetworkProcess : [Attachment 439365] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 230812: Move Cross-Origin-Opener-Policy handling to the NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=230812

Attachment 439365: Patch

https://bugs.webkit.org/attachment.cgi?id=439365&action=review




--- Comment #6 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 439365
  --> https://bugs.webkit.org/attachment.cgi?id=439365
Patch

LGTM with some nits below.
I would just make sure to cover all code paths (service worker, cache) now that
we are moving checks to the network process.

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

> Source/WebCore/ChangeLog:28
> +	   context group switch but currently do not involved the network
process. I also had to add

s/involved/involve.

> Source/WebCore/loader/CrossOriginOpenerPolicy.cpp:239
> +//
https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate
-fetch (Step 12.5.6)

13.5.6?

> Source/WebCore/loader/DocumentLoader.cpp:745
>  //
https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate
-fetch (Step 12.5.6)

13.5.6?

> Source/WebCore/loader/NavigationRequester.h:81
> +    return NavigationRequester { WTFMove(*url),
securityOrigin.releaseNonNull(), topOrigin.releaseNonNull(),
WTFMove(*crossOriginOpenerPolicy), WTFMove(*globalFrameIdentifier) };

Probably do not need move for the last two.

> Source/WebCore/loader/ReportingEndpointsCache.cpp:70
> +    return addEndPointsFromReportToHeader(response.url(),
response.httpHeaderField(HTTPHeaderName::ReportTo));

s/EndPoints/Endpoints/

> Source/WebCore/loader/ReportingEndpointsCache.cpp:73
> +void ReportingEndpointsCache::addEndPointsFromReportToHeader(const URL&
responseURL, const String& reportToHeaderValue)

Ditto.

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:326
> +    result.openerURL = *openerURL;

Could use a move

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:77
> +    uint64_t navigationID { 0 };

Should we use an ObjectIdentifier?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1012
> +	   didFailLoading(ResourceError { errorDomainWebKitInternal, 0,
redirectRequest.url(), "Redirection was blocked by
Cross-Origin-Opener-Policy"_s, ResourceError::Type::AccessControl });

It would be nice to have doCrossOriginOpenerHandlingOfResponse return an
optional<ResourceError>.
Do we have a test case for this one?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1324
> +	   didFailLoading(ResourceError { errorDomainWebKitInternal, 0,
response.url(), "Navigation was blocked by Cross-Origin-Opener-Policy"_s,
ResourceError::Type::AccessControl });

return missing.
It might be good to add a test case.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:163
> +    if (!m_loader.doCrossOriginOpenerHandlingOfResponse(response)) {

Do we have tests for that code path?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:439
> +void
NetworkProcessProxy::triggerBrowsingContextGroupSwitchForNavigation(WebPageProx
yIdentifier pageID, uint64_t navigationID, BrowsingContextGroupSwitchDecision
browsingContextGroupSwitchDecision, WebCore::RegistrableDomain responseDomain,
NetworkResourceLoadIdentifier existingNetworkResourceLoadIdentifierToResume)

s/RegistrationDomain/const RegistrationDomain& or RegistrationDomain&&

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:443
> +	   page->triggerBrowsingContextGroupSwitchForNavigation(navigationID,
browsingContextGroupSwitchDecision, responseDomain,
existingNetworkResourceLoadIdentifierToResume);

It would be nice to have a 'cancel' switch for navigation that would just clear
the entry from the pending switch loader map in network process.
As an async callback for instance. Not sure we can make it 100% working though

> Source/WebKit/UIProcess/WebPageProxy.cpp:5676
> +void WebPageProxy::triggerBrowsingContextGroupSwitchForNavigation(uint64_t
navigationID, BrowsingContextGroupSwitchDecision
browsingContextGroupSwitchDecision, WebCore::RegistrableDomain responseDomain,
NetworkResourceLoadIdentifier existingNetworkResourceLoadIdentifierToResume)

const WebCore::RegistrableDomain&