Bug 229465 - [WK2] Reuse the same network load when process-swapping on resource response due to COOP
Summary: [WK2] Reuse the same network load when process-swapping on resource response ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 229203
Blocks: 228755 229559
  Show dependency treegraph
 
Reported: 2021-08-24 13:47 PDT by Chris Dumez
Modified: 2021-08-27 11:09 PDT (History)
11 users (show)

See Also:


Attachments
API Test (4.44 KB, patch)
2021-08-24 14:43 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (70.03 KB, patch)
2021-08-25 10:06 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (70.89 KB, patch)
2021-08-25 13:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (71.66 KB, patch)
2021-08-25 13:38 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (74.07 KB, patch)
2021-08-25 15:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (69.64 KB, patch)
2021-08-25 15:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (70.66 KB, patch)
2021-08-25 16:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (79.43 KB, patch)
2021-08-26 08:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (79.37 KB, patch)
2021-08-26 13:08 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (79.38 KB, patch)
2021-08-26 13:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.08 KB, patch)
2021-08-27 08:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.08 KB, patch)
2021-08-27 08:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.07 KB, patch)
2021-08-27 10:04 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 2021-08-24 13:47:39 PDT
Reuse the same network load when process-swapping on resource response due to COOP, to avoid going to the server a second time.
Comment 1 Radar WebKit Bug Importer 2021-08-24 13:48:03 PDT
<rdar://problem/82307611>
Comment 2 Chris Dumez 2021-08-24 14:43:24 PDT
Created attachment 436334 [details]
API Test
Comment 3 Chris Dumez 2021-08-25 10:06:58 PDT
Created attachment 436399 [details]
WIP patch

Does not support custom scheme handlers and there are issues with service workers.
Comment 4 Chris Dumez 2021-08-25 13:25:45 PDT
Created attachment 436414 [details]
WIP patch

Should take care of service worker issues.

Still need to support scheme handlers.
Comment 5 Chris Dumez 2021-08-25 13:38:40 PDT
Created attachment 436417 [details]
WIP patch
Comment 6 Chris Dumez 2021-08-25 15:06:56 PDT
Created attachment 436430 [details]
WIP patch
Comment 7 Chris Dumez 2021-08-25 15:50:24 PDT
Created attachment 436432 [details]
WIP patch
Comment 8 Chris Dumez 2021-08-25 16:31:48 PDT
Created attachment 436436 [details]
WIP patch
Comment 9 Chris Dumez 2021-08-26 08:01:44 PDT
Created attachment 436511 [details]
Patch
Comment 10 Darin Adler 2021-08-26 12:44:39 PDT
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.
Comment 11 Chris Dumez 2021-08-26 12:50:17 PDT
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.
Comment 12 Chris Dumez 2021-08-26 12:51:44 PDT
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.
Comment 13 Darin Adler 2021-08-26 13:00:35 PDT
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?
Comment 14 Chris Dumez 2021-08-26 13:02:46 PDT
(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.
Comment 15 Chris Dumez 2021-08-26 13:08:16 PDT
Created attachment 436553 [details]
Patch
Comment 16 Chris Dumez 2021-08-26 13:21:11 PDT
Created attachment 436556 [details]
Patch
Comment 17 Chris Dumez 2021-08-26 13:21:57 PDT
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.
Comment 18 Alex Christensen 2021-08-26 17:30:52 PDT
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.
Comment 19 Chris Dumez 2021-08-27 08:03:14 PDT
Created attachment 436626 [details]
Patch
Comment 20 Chris Dumez 2021-08-27 08:08:44 PDT
Created attachment 436627 [details]
Patch
Comment 21 Chris Dumez 2021-08-27 10:04:57 PDT
Created attachment 436638 [details]
Patch
Comment 22 EWS 2021-08-27 11:09:04 PDT
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].