Bug 230015

Summary: Replace PrivateClickMeasurementNetworkLoader::start with an implementation that doesn't need NetworkSession
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: REOPENED ---    
Severity: Normal CC: webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=230317
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
wilander: review+, ews-feeder: commit-queue-
Patch
none
Patch
none
Patch ews-feeder: commit-queue-

Description Alex Christensen 2021-09-07 12:40:46 PDT
Replace PrivateClickMeasurementNetworkLoader::start with an implementation that doesn't need NetworkSession
Comment 1 Alex Christensen 2021-09-07 12:44:31 PDT
Created attachment 437541 [details]
Patch
Comment 2 Alex Christensen 2021-09-07 12:53:54 PDT
Created attachment 437544 [details]
Patch
Comment 3 Alex Christensen 2021-09-07 13:01:43 PDT
Created attachment 437546 [details]
Patch
Comment 4 Alex Christensen 2021-09-07 13:17:01 PDT
Created attachment 437552 [details]
Patch
Comment 5 John Wilander 2021-09-07 13:27:22 PDT
Comment on attachment 437546 [details]
Patch

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

We should check that we're not leaving behind an ephemeral, stateless session that is no longer used. I believe we use it for more things but we should make sure.

See inline questions. r=me as long as EWS is happy.

> Source/WebKit/ChangeLog:8
> +        This is needed to run this code in a process that doesn't have NetworkSession, which is a class tied closely to WKWebsiteDataStore.

With this change, do we still have guarantees that PCM will not be enabled in ephemeral sessions?

> Source/WebKit/ChangeLog:11
> +        2. Does not allow redirects.

Did we have this configuration before? I'm for it until we are proven otherwise but I want to keep track of any functional changes.

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/cocoa/PrivateClickMeasurementNetworkLoaderCocoa.mm:88
> +        configuration._shouldSkipPreferredClientCertificateLookup = YES;

What does this imply?
Comment 6 Alex Christensen 2021-09-07 13:32:10 PDT
(In reply to John Wilander from comment #5)
> Comment on attachment 437546 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437546&action=review
> 
> We should check that we're not leaving behind an ephemeral, stateless
> session that is no longer used. I believe we use it for more things but we
> should make sure.
> 
> See inline questions. r=me as long as EWS is happy.
> 
> > Source/WebKit/ChangeLog:8
> > +        This is needed to run this code in a process that doesn't have NetworkSession, which is a class tied closely to WKWebsiteDataStore.
> 
> With this change, do we still have guarantees that PCM will not be enabled
> in ephemeral sessions?
Yes, that is currently done in a function named pcmStoreDirectory.

> > Source/WebKit/ChangeLog:11
> > +        2. Does not allow redirects.
> 
> Did we have this configuration before? I'm for it until we are proven
> otherwise but I want to keep track of any functional changes.
Yes, PrivateClickMeasurementNetworkLoader::willSendRedirectedRequest called cancel.

> > Source/WebKit/NetworkProcess/PrivateClickMeasurement/cocoa/PrivateClickMeasurementNetworkLoaderCocoa.mm:88
> > +        configuration._shouldSkipPreferredClientCertificateLookup = YES;
> 
> What does this imply?
This is what we do for all our sessions in WebKit now.  It makes it so that CFNetwork doesn't look in the keychain for client certificates if the server requests one.
Comment 7 Alex Christensen 2021-09-07 15:10:36 PDT
Created attachment 437561 [details]
Patch
Comment 8 Alex Christensen 2021-09-07 15:14:39 PDT
http://trac.webkit.org/r282110
Comment 9 Radar WebKit Bug Importer 2021-09-07 15:15:54 PDT
<rdar://problem/82841918>
Comment 10 Alex Christensen 2021-09-07 18:35:43 PDT
Reopening to attach new patch.
Comment 11 Alex Christensen 2021-09-07 18:35:44 PDT
Created attachment 437576 [details]
Patch
Comment 12 Alex Christensen 2021-09-07 18:42:38 PDT
Removed extra "static" and committed to r282121