Ad Click Attribution should be disabled in ephemeral sessions and conversion requests should use an ephemeral, stateless session.
<rdar://problem/49918702>
Created attachment 367830 [details] Patch
Comment on attachment 367830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367830&action=review Seems mostly conceptually good to me. > Source/WebCore/loader/ResourceLoader.cpp:721 > + if (m_options.storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse > + || m_options.storedCredentialsPolicy == StoredCredentialsPolicy::EphemeralStatelessCookieless) This could just be if it's not equal to ::Use. > Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:183 > - return completionHandler("No stored Ad Click Attribution data.\n"_s); > + return completionHandler("\nNo stored Ad Click Attribution data.\n"_s); You probably want to put the first newline where this string is used. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:593 > + NetworkSession* networkSession; May as well initialize this to nullptr here, even though we know it will be initialized later. Ideally we would have C++17's if-with-initializer. I sent an email to webkit-dev. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366 > + if (!shouldBlockCookies) This seems like a good job for the operator || > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:984 > + initializeEphemeralStatelessCookielessSession(configuration); We should probably lazily initialize this. Hopefully most users will never need to even allocate this. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1010 > + configuration.allowsCellularAccess = YES; This needs to come from NetworkSessionCreationParameters or from the other sessions. There's an internal client that needs to disallow cellular access and it should disallow it for this, too.
(In reply to Alex Christensen from comment #3) > Comment on attachment 367830 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367830&action=review > > Seems mostly conceptually good to me. > > > Source/WebCore/loader/ResourceLoader.cpp:721 > > + if (m_options.storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse > > + || m_options.storedCredentialsPolicy == StoredCredentialsPolicy::EphemeralStatelessCookieless) > > This could just be if it's not equal to ::Use. True. Will fix. > > Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:183 > > - return completionHandler("No stored Ad Click Attribution data.\n"_s); > > + return completionHandler("\nNo stored Ad Click Attribution data.\n"_s); > > You probably want to put the first newline where this string is used. The reason I added this is after dumping the /cookies/resources/echo-cookies.php frame, I don't get a newline. And in the case of empty maps, I only output this line. > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:593 > > + NetworkSession* networkSession; > > May as well initialize this to nullptr here, even though we know it will be > initialized later. Ideally we would have C++17's if-with-initializer. I > sent an email to webkit-dev. OK, I'll initialize to nullptr. > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366 > > + if (!shouldBlockCookies) > > This seems like a good job for the operator || Ah, yes. Will fix. > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:984 > > + initializeEphemeralStatelessCookielessSession(configuration); > > We should probably lazily initialize this. Hopefully most users will never > need to even allocate this. OK, I'll try to do that. But if Ad Click Attribution is adopted, this session will be used quite a lot. > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1010 > > + configuration.allowsCellularAccess = YES; > > This needs to come from NetworkSessionCreationParameters or from the other > sessions. There's an internal client that needs to disallow cellular access > and it should disallow it for this, too. OK. Hopefully I can pick it up when I do my lazy initialization. Thanks!
Created attachment 367848 [details] Patch
Created attachment 367849 [details] Patch
(In reply to John Wilander from comment #4) > (In reply to Alex Christensen from comment #3) > > Comment on attachment 367830 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=367830&action=review > > > > Seems mostly conceptually good to me. > > > > > Source/WebCore/loader/ResourceLoader.cpp:721 > > > + if (m_options.storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse > > > + || m_options.storedCredentialsPolicy == StoredCredentialsPolicy::EphemeralStatelessCookieless) > > > > This could just be if it's not equal to ::Use. > > True. Will fix. > > > > Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:183 > > > - return completionHandler("No stored Ad Click Attribution data.\n"_s); > > > + return completionHandler("\nNo stored Ad Click Attribution data.\n"_s); > > > > You probably want to put the first newline where this string is used. > > The reason I added this is after dumping the > /cookies/resources/echo-cookies.php frame, I don't get a newline. And in the > case of empty maps, I only output this line. I don't see a way to make the expect file to look nice with out this leading newline. Thoughts? > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:593 > > > + NetworkSession* networkSession; > > > > May as well initialize this to nullptr here, even though we know it will be > > initialized later. Ideally we would have C++17's if-with-initializer. I > > sent an email to webkit-dev. > > OK, I'll initialize to nullptr. > > > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366 > > > + if (!shouldBlockCookies) > > > > This seems like a good job for the operator || > > Ah, yes. Will fix. I did it with || in both places. > > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:984 > > > + initializeEphemeralStatelessCookielessSession(configuration); > > > > We should probably lazily initialize this. Hopefully most users will never > > need to even allocate this. > > OK, I'll try to do that. But if Ad Click Attribution is adopted, this > session will be used quite a lot. Can I really lazy initialize this since I need the configuration parameters that's sent to the constructor? > > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1010 > > > + configuration.allowsCellularAccess = YES; > > > > This needs to come from NetworkSessionCreationParameters or from the other > > sessions. There's an internal client that needs to disallow cellular access > > and it should disallow it for this, too. > > OK. Hopefully I can pick it up when I do my lazy initialization. I've changed to set this according to the parameters. But no last initialization yet. Please advise.
Comment on attachment 367849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367849&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:218 > + m_task = [cocoaSession.m_ephemeralStatelessCookielessSession dataTaskWithRequest:nsRequest]; To lazily initialize m_ephemeralStatelessCookielessSession, just put the call to initializeEphemeralStatelessCookielessSession before this line. Then inside initializeEphemeralStatelessCookielessSession early return if m_ephemeralStatelessCookielessSession is non null.
(In reply to Alex Christensen from comment #8) > Comment on attachment 367849 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367849&action=review > > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:218 > > + m_task = [cocoaSession.m_ephemeralStatelessCookielessSession dataTaskWithRequest:nsRequest]; > > To lazily initialize m_ephemeralStatelessCookielessSession, just put the > call to initializeEphemeralStatelessCookielessSession before this line. > Then inside initializeEphemeralStatelessCookielessSession early return if > m_ephemeralStatelessCookielessSession is non null. But initializeEphemeralStatelessCookielessSession() takes an NSURLSessionConfiguration. Do you mean I should instead pick up the configuration from one of the other, already existing sessions?
Right. From m_statelessSession
(In reply to Alex Christensen from comment #10) > Right. From m_statelessSession Got it. Thanks!
Created attachment 367852 [details] Patch for landing
Comment on attachment 367852 [details] Patch for landing Clearing flags on attachment: 367852 Committed r244475: <https://trac.webkit.org/changeset/244475>
All reviewed patches have been landed. Closing bug.
Thanks for landing it for me, Chris! If there's any problem with the patch, send it to me, not Chris. He landed it for me because the commit queue is currently broken.