Bug 154957 - Adopt CFNetwork storage partitioning SPI
Summary: Adopt CFNetwork storage partitioning SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks: 154958
  Show dependency treegraph
 
Reported: 2016-03-03 00:55 PST by Andy Estes
Modified: 2016-03-10 17:23 PST (History)
5 users (show)

See Also:


Attachments
Patch (16.78 KB, patch)
2016-03-03 01:45 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (16.69 KB, patch)
2016-03-03 02:10 PST, Andy Estes
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2016-03-03 00:55:29 PST
Adopt CFNetwork storage partitioning SPI
Comment 1 Andy Estes 2016-03-03 00:56:39 PST
rdar://problem/23614620
Comment 2 Andy Estes 2016-03-03 01:45:05 PST
Created attachment 272744 [details]
Patch
Comment 3 Andy Estes 2016-03-03 02:10:51 PST
Created attachment 272747 [details]
Patch
Comment 4 WebKit Commit Bot 2016-03-03 02:12:01 PST
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.
Comment 5 Darin Adler 2016-03-03 09:21:28 PST
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?
Comment 6 Andy Estes 2016-03-03 13:31:36 PST
(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!
Comment 7 Andy Estes 2016-03-03 13:45:58 PST
Committed r197518: <http://trac.webkit.org/changeset/197518>