Bug 229465 – [WK2] Reuse the same network load when process-swapping on resource response due to COOP

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Reuse the same network load when process-swapping on resource response due to COOP, to avoid going to the server a second time.

[Radar WebKit Bug Importer]

<rdar://problem/82307611>

[Chris Dumez]

Created attachment 436334 [details]
API Test

[Chris Dumez]

Created attachment 436399 [details]
WIP patch

Does not support custom scheme handlers and there are issues with service workers.

[Chris Dumez]

Created attachment 436414 [details]
WIP patch

Should take care of service worker issues.

Still need to support scheme handlers.

[Chris Dumez]

Created attachment 436417 [details]
WIP patch

[Chris Dumez]

Created attachment 436430 [details]
WIP patch

[Chris Dumez]

Created attachment 436432 [details]
WIP patch

[Chris Dumez]

Created attachment 436436 [details]
WIP patch

[Chris Dumez]

Created attachment 436511 [details]
Patch

[Darin Adler]

Comment on attachment 436511 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:1286
> +    return m_networkResourceLoaders.take(resourceLoadIdentifier);

What about if resourceLoadIdentifier is the empty or deleted value? Which level prevents that from happening?

Same question about other places we do hash table access with identifiers.

[Chris Dumez]

Comment on attachment 436511 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2853
> +    auto loader = connection->takeNetworkResourceLoader(resourceLoadIdentifier);

resourceLoadIdentifier is passed here. PrepareLoadForWebProcessTransfer gets called as a result as an IPC from the UIProcess (not the WebProcess), therefore, it is trusted IPC.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:481
> +    if (!resourceLoadIdentifier) {

And before sending the PrepareLoadForWebProcessTransfer IPC, the UIProcess makes sure that resourceLoadIdentifier is valid.

[Chris Dumez]

Comment on attachment 436511 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:513
> +        if (auto existingLoader = networkProcess().takeLoaderAwaitingWebProcessTransfer(*existingLoaderToResume)) {

Here existingLoaderToResume comes from IPC from the WebProcess and is thus untrusted. However, we are using a NetworkResourceLoadIdentifier strongly typed identifier, which would fail decoding over IPC if invalid.

[Darin Adler]

Comment on attachment 436511 [details]
Patch

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

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:481
>> +    if (!resourceLoadIdentifier) {
> 
> And before sending the PrepareLoadForWebProcessTransfer IPC, the UIProcess makes sure that resourceLoadIdentifier is valid.

The above quoted code checks for 0, but not for the deleted value. Is that OK?

[Chris Dumez]

(In reply to Darin Adler from comment #13)
> Comment on attachment 436511 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436511&action=review
> 
> >> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:481
> >> +    if (!resourceLoadIdentifier) {
> > 
> > And before sending the PrepareLoadForWebProcessTransfer IPC, the UIProcess makes sure that resourceLoadIdentifier is valid.
> 
> The above quoted code checks for 0, but not for the deleted value. Is that
> OK?

No it isn't. I'll fix.

[Chris Dumez]

Created attachment 436553 [details]
Patch

[Chris Dumez]

Created attachment 436556 [details]
Patch

[Chris Dumez]

Comment on attachment 436556 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:1286
> +    if (!NetworkResourceLoadMap::MapType::isValidKey(resourceLoadIdentifier))

I added ID validation here.

[Alex Christensen]

Comment on attachment 436556 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.h:620
> +        Ref<NetworkResourceLoader> takeLoader();

There's no benefit in making this return a Ref.  It should return a RefPtr.  Then we don't need an unchecked m_loader.releaseNonNull.  If you want the assertion, ASSERT(m_loader);

> Source/WebKit/NetworkProcess/NetworkProcess.h:627
> +    HashMap<NetworkResourceLoadIdentifier, std::unique_ptr<CachedNetworkResourceLoader>> m_loadersAwaitingWebProcessTransfer;

I think this should be owned by the NetworkSession to compartmentalize things a little bit more.  There isn't a good reason this needs to be process-global.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:217
> +    NetworkResourceLoadParameters m_parameters;

I think I'd prefer to mark m_parameters.identifier as mutable rather than this, which indicates that any member can be mutated.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:95
> +    void setExistingNetworkResourceLoadIdentifierToResume(std::optional<NetworkResourceLoadIdentifier> existingNetworkResourceLoadIdentifierToResume) { m_existingNetworkResourceLoadIdentifierToResume = existingNetworkResourceLoadIdentifierToResume; }

This is kind of a verbose name already, but shouldn't this indicate that it is only for the main resource of the main frame?

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:852
> +    uint64_t mainResourceLoadIdentifier = policyDocumentLoader && policyDocumentLoader->mainResourceLoader() ? policyDocumentLoader->mainResourceLoader()->identifier() : 0;

ResourceLoader::identifier returns an unsigned long.  Would auto work here?
Add FIXME: use a strongly typed identifier.  We don't want to merge that refactoring, but we need to do it.

[Chris Dumez]

Created attachment 436626 [details]
Patch

[Chris Dumez]

Created attachment 436627 [details]
Patch

[Chris Dumez]

Created attachment 436638 [details]
Patch

[EWS]

Committed r281706 (241053@main): <https://commits.webkit.org/241053@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436638 [details].