Bug 223776 - Refactor NetworkSessionCocoa to prepare for per-WebPageProxy sessions
Summary: Refactor NetworkSessionCocoa to prepare for per-WebPageProxy sessions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-25 16:56 PDT by Brady Eidson
Modified: 2021-03-26 15:06 PDT (History)
2 users (show)

See Also:


Attachments
Patch for EWS (66.30 KB, patch)
2021-03-25 17:01 PDT, Brady Eidson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for EWS (68.10 KB, patch)
2021-03-25 17:47 PDT, Brady Eidson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for EWS (68.15 KB, patch)
2021-03-25 22:59 PDT, Brady Eidson
achristensen: review+
Details | Formatted Diff | Diff
Patch for landing (68.95 KB, patch)
2021-03-26 12:31 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2021-03-25 16:56:13 PDT
Refactor NetworkSessionCocoa to prepare for per-WebPageProxy sessions

For upcoming work, different WKWebViews sharing one logical NetworkSession will need different NSURLSessions to do their networking.

To prepare for that, this patch will do a no-change-in-behavior refactor that accounts for the ability to "look up the set of NSURLSessions by WebPageProxyIdentifier"
Comment 1 Brady Eidson 2021-03-25 17:01:02 PDT
Created attachment 424300 [details]
Patch for EWS
Comment 2 Brady Eidson 2021-03-25 17:43:21 PDT
That makes sense, yay EWS
Comment 3 Brady Eidson 2021-03-25 17:47:21 PDT
Created attachment 424308 [details]
Patch for EWS
Comment 4 Brady Eidson 2021-03-25 22:59:47 PDT
Created attachment 424319 [details]
Patch for EWS
Comment 5 Alex Christensen 2021-03-25 23:25:00 PDT
Comment on attachment 424319 [details]
Patch for EWS

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:159
> +    HashMap<WebPageProxyIdentifier, RefPtr<SessionSet>> m_perPageSessionSets;

I think the value could be made a Ref

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1298
> +    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=177394

You may as well remove this comment and the 2 lines below.  We know why we can't do this, and we typically don't like having commented out code in WebKit.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1307
> +    SessionSet* sessionSet = webPageProxyID ? m_perPageSessionSets.get(webPageProxyID) : nullptr;

If webPageProxyID can be 0 then it should probably be an Optional<WebPageProxyIdentifier>
If you're protecting against strange things happening to the HashTable, you should probably use decltype(m_perPageSessionSets.get)::isValidKey

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1319
> +    return sessionSetForPage(pageID).initializeEphemeralStatelessSessionIfNeeded(isNavigatingToAppBoundDomain, *this);

You call pageID webPageProxyID everywhere else in this patch.  Why not here?
Comment 6 Brady Eidson 2021-03-26 12:17:32 PDT
(In reply to Alex Christensen from comment #5)
> Comment on attachment 424319 [details]
> Patch for EWS

All suggestions taken except:

> If webPageProxyID can be 0 then it should probably be an
> Optional<WebPageProxyIdentifier>
> If you're protecting against strange things happening to the HashTable, you
> should probably use decltype(m_perPageSessionSets.get)::isValidKey

In this patch it can be 0 (and in fact PingLoaders do not necessarily have a proper page context, so they are 0)

In the next patch there will be protection against zero.

ObjectIdentifier<> already has established patterns of "0 means no valid ID", I don't think we need to explore a new direction on that this time around.
Comment 7 Brady Eidson 2021-03-26 12:31:51 PDT
Created attachment 424391 [details]
Patch for landing
Comment 8 EWS 2021-03-26 15:05:40 PDT
Committed r275116: <https://commits.webkit.org/r275116>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424391 [details].
Comment 9 Radar WebKit Bug Importer 2021-03-26 15:06:14 PDT
<rdar://problem/75903385>