Bug 177394 – Figure out how to disable all cookies in the stateless network storage session

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

We might be hitting this during Redirects:

DocumentThreadableLoader::redirectReceived
{
...
    m_options.allowCredentials = DoNotAllowStoredCredentials;
...
}

[Radar WebKit Bug Importer]

<rdar://problem/34613960>

[Brent Fulgham]

The earlier rollout was <http://trac.webkit.org/changeset/222414>.

[Brent Fulgham]

It looks like Web Compat requires that we have a “Cookies + Credentials”, “Cookies + No Credentials”, and “Stateless” network sessions.

[John Wilander]

Created attachment 331262 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

It looks like we'll need to add NSHTTPCookieStorage _initWithIdentifier:private: to CFNetworkSPI.h, too.

[John Wilander]

Created attachment 331423 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

The one new test case that failed wasn't skipped for non-ITP WK2. Will fix.

[John Wilander]

Created attachment 331436 [details]
Patch

[Alex Christensen]

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)

[John Wilander]

Created attachment 331443 [details]
Patch

[John Wilander]

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.

[EWS Watchlist]

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.

[John Wilander]

The style error is for a comment in code I moved. The comment needs multi-space aligning since it has a truth table.

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

The test failure does not seem related to me.

[Alex Christensen]

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];

[John Wilander]

Created attachment 331511 [details]
Patch for landing

[John Wilander]

Thanks, Alex! I did all the additional changes you suggested.

[WebKit Commit Bot]

Comment on attachment 331511 [details]
Patch for landing

Clearing flags on attachment: 331511

Committed r227076: <https://trac.webkit.org/changeset/227076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

Reverted r227076 for reason:

This breaks internal builds

Committed r227093: <https://trac.webkit.org/changeset/227093>

[Alex Christensen]

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

[John Wilander]

Created attachment 331565 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 331565 [details]
Patch for landing

Clearing flags on attachment: 331565

Committed r227103: <https://trac.webkit.org/changeset/227103>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Why are these comments still in NetworkSessionCocoa::NetworkSessionCocoa()?

    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=177394
    // configuration.HTTPCookieStorage = nil;
    // configuration.HTTPCookieAcceptPolicy = NSHTTPCookieAcceptPolicyNever;