Bug 154957 – Adopt CFNetwork storage partitioning SPI

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

Adopt CFNetwork storage partitioning SPI

[Andy Estes]

rdar://problem/23614620

[Andy Estes]

Created attachment 272744 [details]
Patch

[Andy Estes]

Created attachment 272747 [details]
Patch

[WebKit Commit Bot]

Attachment 272747 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:99:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 272747 [details]
Patch

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

> Source/WebCore/page/Settings.h:358
> +    static bool gCookieStoragePartitioningEnabled;

These "g" prefixes sure seem peculiar.

> Source/WebCore/platform/network/NetworkStorageSession.h:84
> +String cookieStoragePartition(const URL& firstPartyForCookies, const URL&);

Might want a name for the second URL to somehow say that it’s the actual resource URL or whatever.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:126
> +String cookieStoragePartition(const URL& firstPartyForCookies, const URL& url)
> +{
> +    if (!Settings::cookieStoragePartitioningEnabled())
> +        return emptyString();
> +
> +    String firstPartyHost = firstPartyForCookies.host();
> +#if ENABLE(PUBLIC_SUFFIX_LIST)
> +    firstPartyHost = topPrivatelyControlledDomain(firstPartyHost);
> +#endif
> +
> +    String host = url.host();
> +    if (!host.endsWithIgnoringASCIICase(firstPartyHost))
> +        return firstPartyHost;
> +
> +    ASSERT(host.length() >= firstPartyHost.length());
> +    unsigned suffixOffset = host.length() - firstPartyHost.length();
> +    if (suffixOffset > 0 && host[suffixOffset - 1] != '.')
> +        return firstPartyHost;
> +
> +    return emptyString();
> +}

The combination of policy and string algorithms seems a bit unpleasant to me; hard to see that both halves of the function’s job are done correctly. Maybe the predicate that takes two host strings and does the suffix check could be a separate helper function. It would return a boolean and take two StringView, not const String&.

Not sure if empty string is better than null string for the "no partition" magic value. Annoying to have to even consider that question. Optional<String> would be better, if String didn’t already have two obvious special values.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:67
> +#if HAVE(CFNETWORK_STORAGE_PARTITIONING)

I like to use blank lines inside the #if/#endif so they don’t seem grouped with the first/last functions inside more than with the rest of what’s inside.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:89
> +    // FIXME: Stop creating a new NSHTTPCookieStorage object each time we want to query the cookie jar.

A thousand times yes.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:97
> +    // completionHandler is called synchronously.

That is a peculiar way to design a method. Is the synchrony guaranteed?

The comment should use active voice for greater clarity:

    // The _getCookiesForURL: method calls the completionHandler synchronously.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:184
> +        filteredCookies = applyPartitionToCookies(partition, filteredCookies.autorelease());

What’s the benefit of calling autorelease() here instead of get()? I think maybe there is no benefit.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:179
> +        [(NSMutableURLRequest *)nsRequest _setProperty:storagePartition forKey:@"__STORAGE_PARTITION_IDENTIFIER"];

Why is this cast to mutable safe? The code just above for !shouldContentSniff goes out of its way to make a mutable copy if needed. Why isn’t that kind of logic needed here?

[Andy Estes]

(In reply to comment #5)
> Comment on attachment 272747 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272747&action=review
> 
> > Source/WebCore/platform/network/NetworkStorageSession.h:84
> > +String cookieStoragePartition(const URL& firstPartyForCookies, const URL&);
> 
> Might want a name for the second URL to somehow say that it’s the actual
> resource URL or whatever.

Sure. I'll call it resource.

> 
> > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:126
> > +String cookieStoragePartition(const URL& firstPartyForCookies, const URL& url)
> > +{
> > +    if (!Settings::cookieStoragePartitioningEnabled())
> > +        return emptyString();
> > +
> > +    String firstPartyHost = firstPartyForCookies.host();
> > +#if ENABLE(PUBLIC_SUFFIX_LIST)
> > +    firstPartyHost = topPrivatelyControlledDomain(firstPartyHost);
> > +#endif
> > +
> > +    String host = url.host();
> > +    if (!host.endsWithIgnoringASCIICase(firstPartyHost))
> > +        return firstPartyHost;
> > +
> > +    ASSERT(host.length() >= firstPartyHost.length());
> > +    unsigned suffixOffset = host.length() - firstPartyHost.length();
> > +    if (suffixOffset > 0 && host[suffixOffset - 1] != '.')
> > +        return firstPartyHost;
> > +
> > +    return emptyString();
> > +}
> 
> The combination of policy and string algorithms seems a bit unpleasant to
> me; hard to see that both halves of the function’s job are done correctly.
> Maybe the predicate that takes two host strings and does the suffix check
> could be a separate helper function. It would return a boolean and take two
> StringView, not const String&.

Agreed. I'll split this up into two functions.

> 
> Not sure if empty string is better than null string for the "no partition"
> magic value. Annoying to have to even consider that question.
> Optional<String> would be better, if String didn’t already have two obvious
> special values.

I don't think we want to create empty partition names in the cookie jar, so I'd still have to check for empty strings if I used another value for "no partition" (for instance, I'd get an empty string if firstPartyForCookie's host was empty for some reason).

> 
> > Source/WebCore/platform/network/mac/CookieJarMac.mm:67
> > +#if HAVE(CFNETWORK_STORAGE_PARTITIONING)
> 
> I like to use blank lines inside the #if/#endif so they don’t seem grouped
> with the first/last functions inside more than with the rest of what’s
> inside.

Sure.

> 
> > Source/WebCore/platform/network/mac/CookieJarMac.mm:97
> > +    // completionHandler is called synchronously.
> 
> That is a peculiar way to design a method. Is the synchrony guaranteed?

Yeah, I was also surprised. I actually don't know if it's guaranteed, so I'll add some verification logic. I think I'll also file a bug asking for the cookies array to be a return value if the method is intended to remain synchronous.

> 
> The comment should use active voice for greater clarity:
> 
>     // The _getCookiesForURL: method calls the completionHandler
> synchronously.

Done.

> 
> > Source/WebCore/platform/network/mac/CookieJarMac.mm:184
> > +        filteredCookies = applyPartitionToCookies(partition, filteredCookies.autorelease());
> 
> What’s the benefit of calling autorelease() here instead of get()? I think
> maybe there is no benefit.

Yeah, no benefit. I'll use get().

> 
> > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:179
> > +        [(NSMutableURLRequest *)nsRequest _setProperty:storagePartition forKey:@"__STORAGE_PARTITION_IDENTIFIER"];
> 
> Why is this cast to mutable safe? The code just above for
> !shouldContentSniff goes out of its way to make a mutable copy if needed.
> Why isn’t that kind of logic needed here?

Yeah, I'll just follow the pattern from !shouldContentSniff. I think it was safe in this specific case, but I shouldn't rely on that.

Thanks for the review!

[Andy Estes]

Committed r197518: <http://trac.webkit.org/changeset/197518>