https://bugs.webkit.org/show_bug.cgi?id=177393 rolled-back the cookie jar change for stateless network sessions that was introduced in https://bugs.webkit.org/show_bug.cgi?id=176386. We should figure out what caused the regressions and again disabled the cookie jar on the stateless session.
I think this was a problem because we tied the decision to use "cookieless" loading to the decision that we did not want to support certificate access. Looking at the code, it was pretty common to not want to allow credential storage while still needing to support cookie access. Alex: It seems like we might want to revisit the decision to only have "stateless" and "stateful" sessions. We might want to support "no certificate with cookies" as a distinct mode.
We might be hitting this during Redirects: DocumentThreadableLoader::redirectReceived { ... m_options.allowCredentials = DoNotAllowStoredCredentials; ... }
<rdar://problem/34613960>
The earlier rollout was <http://trac.webkit.org/changeset/222414>.
It looks like Web Compat requires that we have a “Cookies + Credentials”, “Cookies + No Credentials”, and “Stateless” network sessions.
Created attachment 331262 [details] Patch
Comment on attachment 331262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331262&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:88 > + NSHTTPCookieStorage* m_emptyDenyAllCookieStorage; Let's use one if it's truly stateless. static RetainPtr<NSHTTPCookieStorage> g_emptyCookieStorage; or something like that.
It looks like we'll need to add NSHTTPCookieStorage _initWithIdentifier:private: to CFNetworkSPI.h, too.
Created attachment 331423 [details] Patch
Comment on attachment 331423 [details] Patch Attachment 331423 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6098027 New failing tests: http/tests/resourceLoadStatistics/non-prevalent-resources-can-access-cookies-in-a-third-party-context.html
Created attachment 331433 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
The one new test case that failed wasn't skipped for non-ITP WK2. Will fix.
Created attachment 331436 [details] Patch
Comment on attachment 331436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331436&action=review > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:182 > +- (id) _initWithIdentifier:(NSString*) identifier private:(bool) isPrivate; No space before identifier. Space after NSString > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:188 > +- (void) _setExplicitCookieStorage:(CFHTTPCookieStorageRef) storage; I think this doesn't need to be here because we only call the SPI with performSelector. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:83 > + NSHTTPCookieStorage* emptyDenyAllCookieStorage(); This should be static because it uses no member variables. static NSHTTPCookieStorage *statelessCookieStorage() > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:107 > + static NSHTTPCookieStorage* emptyCookieStorage; static NeverDestroyed<RetainPtr<NSHTTPCookieStorage>> Then we can adopt the alloc'd object and pretend we manage it. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:308 > + [m_task.get() performSelector:NSSelectorFromString(@"_setExplicitCookieStorage:") withObject:(NSObject*)m_session->networkStorageSession().nsCookieStorage()._cookieStorage]; > + m_hasBeenSetToUseEmptyDenyAllCookieStorage = false; I'd make a private member function: setCookiesBlocked(bool)
Created attachment 331443 [details] Patch
Thanks, Alex! Your comments are addressed. I also made the partitioning code into a dedicated function and got rid of the WebCore::StoredCredentialsPolicy::DoNotUse setting when creating the task.
Attachment 331443 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:131: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
The style error is for a comment in code I moved. The comment needs multi-space aligning since it has a truth table.
Comment on attachment 331443 [details] Patch Attachment 331443 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6100499 New failing tests: imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-bitrate.html
Created attachment 331447 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
The test failure does not seem related to me.
Comment on attachment 331443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331443&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:105 > +RetainPtr<NSHTTPCookieStorage>& NetworkDataTaskCocoa::statelessCookieStorage() This is fine. Returning an NSHTTPCookieStorage * would look a little cleaner. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:112 > + return statelessCookieStorage.get(); I think it would be nice to assert that NSHTTPCookieStorage.cookies.count is 0 here. That way we could catch if anything unexpected happens. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:123 > + if (shouldBlock) > + [m_task.get() performSelector:NSSelectorFromString(@"_setExplicitCookieStorage:") withObject:(NSObject*)statelessCookieStorage().get()._cookieStorage]; > + else > + [m_task.get() performSelector:NSSelectorFromString(@"_setExplicitCookieStorage:") withObject:(NSObject*)m_session->networkStorageSession().nsCookieStorage()._cookieStorage]; .get() should be unnecessary. We could also make this simpler: NSHTTPCookieStorage *storage = shouldBlock ? statelessCookieStorage().get(): m_session->networkStorageSession().nsCookieStorage(); [m_task performSelector:NSSelectorFromString(@"_setExplicitCookieStorage:") withObject:(NSObject*)storage._cookieStorage];
Created attachment 331511 [details] Patch for landing
Thanks, Alex! I did all the additional changes you suggested.
Comment on attachment 331511 [details] Patch for landing Clearing flags on attachment: 331511 Committed r227076: <https://trac.webkit.org/changeset/227076>
All reviewed patches have been landed. Closing bug.
Reverted r227076 for reason: This breaks internal builds Committed r227093: <https://trac.webkit.org/changeset/227093>
Comment on attachment 331511 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=331511&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:109 > + statelessCookieStorage.get() = adoptNS([[NSHTTPCookieStorage alloc] _initWithIdentifier:nil private:YES]); It looks like the internal Sierra header incorrectly marks the NSString of _initWithIdentifier:private: as non-nullable. Let's do this: #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wnonnull" #endif statelessCookieStorage.get() = adoptNS([[NSHTTPCookieStorage alloc] _initWithIdentifier:nil private:YES]); #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) #pragma clang diagnostic pop #endif
Created attachment 331565 [details] Patch for landing
Comment on attachment 331565 [details] Patch for landing Clearing flags on attachment: 331565 Committed r227103: <https://trac.webkit.org/changeset/227103>
Why are these comments still in NetworkSessionCocoa::NetworkSessionCocoa()? // FIXME: https://bugs.webkit.org/show_bug.cgi?id=177394 // configuration.HTTPCookieStorage = nil; // configuration.HTTPCookieAcceptPolicy = NSHTTPCookieAcceptPolicyNever;