Bug 110269 – Optionally partition cache to prevent using cache for tracking

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Vicki Pfau]

It is possible to use cache items for tracking a user. In the event that third-party data blocking is enabled, we should have an approach for preventing cache items from being used to track users across different domains.

This approach is to partition the cache such that different domains' subresources cannot access the cache items created by other domains' subresources. E.g., if a.com in a top frame loads an image from b.com, c.com in a top frame cannot access the same cache item for the image from b.com.

<rdar://problem/8830560>

[Vicki Pfau]

Created attachment 189189 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/cache/partitioned-cache-expected.txt', u'LayoutTests/http/tests/cache/partitioned-cache.html', u'LayoutTests/http/tests/cache/resources/echo-cookie.cgi', u'LayoutTests/http/tests/cache/resources/partitioned-cache-loader.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/DOMURL.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/inspector/InspectorResourceAgent.cpp', u'Source/WebCore/loader/DocumentLoader.h', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResource.h', u'Source/WebCore/loader/cache/CachedResourceLoader.cpp', u'Source/WebCore/loader/cache/MemoryCache.cpp', u'Source/WebCore/loader/cache/MemoryCache.h', u'Source/WebCore/page/Page.cpp', u'Source/WebCore/page/SecurityOrigin.cpp', u'Source/WebCore/page/SecurityOrigin.h', u'Source/WebCore/platform/PublicSuffix.h', u'Source/WebCore/platform/mac/PublicSuffixMac.mm', u'Source/WebCore/platform/mac/WebCoreSystemInterface.h', u'Source/WebCore/platform/mac/WebCoreSystemInterface.mm', u'Source/WebCore/platform/network/ResourceRequestBase.h', u'Source/WebCore/platform/network/mac/ResourceRequestMac.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebSystemInterface.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebCoreSupport/mac/WebSystemInterface.mm', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj', u'Tools/TestWebKitAPI/Tests/mac/PublicSuffix.mm', u'WebKitLibraries/ChangeLog', u'WebKitLibraries/WebKitSystemInterface.h', u'WebKitLibraries/libWebKitSystemInterfaceLion.a', u'WebKitLibraries/libWebKitSystemInterfaceMountainLion.a']" exit_code: 1
Source/WebCore/loader/cache/MemoryCache.cpp:706:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 37 files


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

[Maciej Stachowiak]

Comment on attachment 189189 [details]
Patch

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

> Source/WebCore/platform/PublicSuffix.h:34
> +String highLevelDomain(const String& domain);

I suggest that instead of "high level domain" we name this concept "top privately controlled domain". That's a little verbose, but I think it is more clear.

[Adam Barth]

Comment on attachment 189189 [details]
Patch

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

Please add an ifdef so that ports that don't want to partition the MemoryCache do not need to compile in code for partitioning the memory cache.

> Source/WebCore/loader/cache/MemoryCache.cpp:69
> +#if PLATFORM(MAC)

This is the wrong ifdef to use here.  We should instead have a USE or ENABLE macro for the public suffix list.

> Source/WebCore/platform/PublicSuffix.h:33
> +bool isPublicSuffix(const String& domain);

Please put this header behind a USE or ENABLE ifdef so that ports that don't wish to depend on the public suffix list do not need to.

> Source/WebCore/platform/network/ResourceRequestBase.h:194
> +        String m_cachePartition;

It's not appropriate to add apple-mac specific state to ResourceRequestBase.  Please add it to ResourceRequestMac instead.

[Adam Barth]

To clarify, we need two separate ENABLE or USE macro: one for the public suffix list and one for partitioning the MemoryCache.  Ideally the public suffix list macro would have the #error I requested in bug 105590, but that's less important in this patch than it was for the patch in bug 105590.

[Maciej Stachowiak]

Comment on attachment 189189 [details]
Patch

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

>> Source/WebCore/platform/network/ResourceRequestBase.h:194
>> +        String m_cachePartition;
> 
> It's not appropriate to add apple-mac specific state to ResourceRequestBase.  Please add it to ResourceRequestMac instead.

We'll ultimately need it in ResourceRequestCF as well, for the CFNetwork back end. Adam, would you recommend implementing this member and its accessors separately for both, or putting it behind the cache partitioning enable flag you suggested?

[Maciej Stachowiak]

(In reply to comment #5)
> To clarify, we need two separate ENABLE or USE macro: one for the public suffix list and one for partitioning the MemoryCache.  Ideally the public suffix list macro would have the #error I requested in bug 105590, but that's less important in this patch than it was for the patch in bug 105590.

Do you think the third-party data blocking feature should have a feature flag too (in which case perhaps cache partitioning should be merged into it)?

[Adam Barth]

> We'll ultimately need it in ResourceRequestCF as well, for the CFNetwork back end. Adam, would you recommend implementing this member and its accessors separately for both, or putting it behind the cache partitioning enable flag you suggested?

I'd start with it separately for both.  If folks besides Apple become interested in this feature, we can re-evaluate how to structure the code.

> Do you think the third-party data blocking feature should have a feature flag too (in which case perhaps cache partitioning should be merged into it)?

I'd prefer if cache partitioning had a separate feature flag.  Unlike third-party storage blocking, the cache partitioning feature is not supported by the consensus of the WebKit project.

[Maciej Stachowiak]

(In reply to comment #8)
> > We'll ultimately need it in ResourceRequestCF as well, for the CFNetwork back end. Adam, would you recommend implementing this member and its accessors separately for both, or putting it behind the cache partitioning enable flag you suggested?
> 
> I'd start with it separately for both.  If folks besides Apple become interested in this feature, we can re-evaluate how to structure the code.
> 
> > Do you think the third-party data blocking feature should have a feature flag too (in which case perhaps cache partitioning should be merged into it)?
> 
> I'd prefer if cache partitioning had a separate feature flag.  Unlike third-party storage blocking, the cache partitioning feature is not supported by the consensus of the WebKit project.

Sounds like a reasonable approach, at least until we have a chance to check if other ports have an interest in third-party data blocking in general or the cache partitioning aspect in particular.

(FWIW if any non-Apple ports are planning on offering third-party data blocking as an end-user feature, I would recommend including cache partitioning support. The goal for the third-party data blocking feature is to prevent cross-site tracking in a third-party context for at minimum all the techniques <http://samy.pl/evercookie/>. We have evidence that the http-cache-based techniques are used in the wild. So the effectiveness of the feature is fairly limited without addressing the cache tracking vectors in some way. That being said, it may still be better than only blocking cookies.)

[Adam Barth]

I don't believe it's possible to stop cooperative tracking with technical means:

http://crypto.stanford.edu/sameorigin/sameorigin.pdf

Tor has gone pretty far down this rabbit hole, and I wouldn't say they've succeeded.  As a simple example, you also need to worry about TLS session IDs and HTTP Keep-Alive:

https://trac.torproject.org/projects/tor/ticket/4099

One way to understand the Keep-Alive-based techniques is that they tag clients with a unique destination port.  You can imagine how you might do something similar on an IPv6 network by tagging clients with a unique destination IP address by having tracker.com always resolve to a novel IPv6 address.

This list goes on effectively forever.  I can understand your desire to "do something" about this issue, but trying to solve this problem by partitioning the cache is just adding code complexity and degrading performance without any real privacy benefit.

[Vicki Pfau]

(In reply to comment #5)
> To clarify, we need two separate ENABLE or USE macro: one for the public suffix list and one for partitioning the MemoryCache.  Ideally the public suffix list macro would have the #error I requested in bug 105590, but that's less important in this patch than it was for the patch in bug 105590.

It's worth noting that the public suffix list itself is no longer in any webkit patch, so I fail to see how it's relevant for this #error to exist: this can easily be used (as is done in this patch) to call into a port-specific implementation of the public suffix list thus obviating the source of your complaint in the bug.

[Adam Barth]

I also objected (and continue to object) to the public suffix list use in WebCore.  However, I don't intend to block this patch over this issue.

[Adam Barth]

Here's some suggested text for the #error:

"The public suffix list is harmful to the web. Please do not enable it for the Chromium port."

[Maciej Stachowiak]

(In reply to comment #13)
> Here's some suggested text for the #error:
> 
> "The public suffix list is harmful to the web. Please do not enable it for the Chromium port."

Can you please suggest a #error that is less judgmental? Like "Please do not enable the public suffix list for the Chromium port." or even "The Chromium port has a policy against use of the public suffix list." It seems a little overly demanding to state as fact that it's "harmful to the web" when clearly not everyone agrees with that.

[Maciej Stachowiak]

(In reply to comment #10)
> I don't believe it's possible to stop cooperative tracking with technical means:
> 
> http://crypto.stanford.edu/sameorigin/sameorigin.pdf
> 
> Tor has gone pretty far down this rabbit hole, and I wouldn't say they've succeeded.  As a simple example, you also need to worry about TLS session IDs and HTTP Keep-Alive:
> 
> https://trac.torproject.org/projects/tor/ticket/4099
> 
> One way to understand the Keep-Alive-based techniques is that they tag clients with a unique destination port.  You can imagine how you might do something similar on an IPv6 network by tagging clients with a unique destination IP address by having tracker.com always resolve to a novel IPv6 address.
> 
> This list goes on effectively forever.  I can understand your desire to "do something" about this issue, but trying to solve this problem by partitioning the cache is just adding code complexity and degrading performance without any real privacy benefit.

There's also behavioral fingerprinting (e.g. trying to identify a user from their pattern and timing of mouse actions) which we're not even trying to prevent. I acknowledge that you can't completely prevent tracking. We are ok with just blocking the more obvious techniques. Other approaches are likely to be harder to use, less long-term persistent, and/or less reliable (though not all are). And using them may also be a more obvious indicator of bad faith. Using the http cache though is trivial and persistent essentially forever, even across relaunches of the browser.

(As a side not though, cache partitioning doesn't measurably degrade performance; we tested page load speed on scenarios that clearly benefit from caching and this particular approach is within the noise perf-wise. I can't argue with the code complexity claim, though.)

[Adam Barth]

I don't feel strongly about the text of the #error, and I think it would even be OK to land this patch without the #error.

[Vicki Pfau]

I'm preparing the #ifdef'd version for posting right now, but it's a huge mess of #ifdef blocks that actually INCREASES the complexity of the code. I'm hoping there's some middle ground that can be reached with fewer #ifdefs, but I'm not entirely sure, as it required changing the signatures of several functions and a function or two that are entirely behind #ifdefs.

As we're putting the cache partitioning code behind a USE flag, I thought it still made sense to put the cachePartition() getter on ResourceRequest and not ResourceRequestMac, as they share the same header regardless. (I did move it from ResourceRequestBase.)

[Adam Barth]

Comment on attachment 189189 [details]
Patch

IMHO, you should pass the cachePartition as an argument to memoryCache() so that you get a different object instance for each cache partition.  Alternatively, we can pass the CachedResourceRequest to resourceForURL (presumably renamed).

But yes, you should think about how best to structure your change so that you don't impose complexity on the entire project for this Apple-only feature.

[Maciej Stachowiak]

(In reply to comment #18)
> (From update of attachment 189189 [details])
> IMHO, you should pass the cachePartition as an argument to memoryCache() so that you get a different object instance for each cache partition.  Alternatively, we can pass the CachedResourceRequest to resourceForURL (presumably renamed).

Of these two options, I suspect the latter will be cleaner (though more functions that just resourceForURL will have to take a structured object of some kind instead of a URL). Having multiple MemoryCache objects would complexify the memory cache pruning logic by requiring it to deal with multiple MemoryCaches instead of a singleton, so would actually spread complexity beyond Jeffrey's patch.

Keeping the signatures consistent is probably the best way, and not having this separate partition parameter threaded all over the code seems nicer in any case.

[Vicki Pfau]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 189189 [details] [details])
> > IMHO, you should pass the cachePartition as an argument to memoryCache() so that you get a different object instance for each cache partition.  Alternatively, we can pass the CachedResourceRequest to resourceForURL (presumably renamed).
> 
> Of these two options, I suspect the latter will be cleaner (though more functions that just resourceForURL will have to take a structured object of some kind instead of a URL). Having multiple MemoryCache objects would complexify the memory cache pruning logic by requiring it to deal with multiple MemoryCaches instead of a singleton, so would actually spread complexity beyond Jeffrey's patch.
> 
> Keeping the signatures consistent is probably the best way, and not having this separate partition parameter threaded all over the code seems nicer in any case.

The issue with passing the CachedResourceRequest is that not everything that calls resourceForURL has a CachedResourceRequest in the first place; there are places where we'd need to fabricate them. I don't think that'll be a huge problem, though; I can try that and see how disgusting it gets. It'll probably be nicer than the wall of #ifdefs I have right now. Although I'm not sure why we'd want to use a CachedResourceRequest instead of just a ResourceRequest? It seems like if we're going from just a URL to something structured, we might just want a ResourceRequest, or perhaps something even more minimal (as I don't think we need the HTTP headers floating around, for example).

[Maciej Stachowiak]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 189189 [details] [details] [details])
> > > IMHO, you should pass the cachePartition as an argument to memoryCache() so that you get a different object instance for each cache partition.  Alternatively, we can pass the CachedResourceRequest to resourceForURL (presumably renamed).
> > 
> > Of these two options, I suspect the latter will be cleaner (though more functions that just resourceForURL will have to take a structured object of some kind instead of a URL). Having multiple MemoryCache objects would complexify the memory cache pruning logic by requiring it to deal with multiple MemoryCaches instead of a singleton, so would actually spread complexity beyond Jeffrey's patch.
> > 
> > Keeping the signatures consistent is probably the best way, and not having this separate partition parameter threaded all over the code seems nicer in any case.
> 
> The issue with passing the CachedResourceRequest is that not everything that calls resourceForURL has a CachedResourceRequest in the first place; there are places where we'd need to fabricate them. I don't think that'll be a huge problem, though; I can try that and see how disgusting it gets. It'll probably be nicer than the wall of #ifdefs I have right now. Although I'm not sure why we'd want to use a CachedResourceRequest instead of just a ResourceRequest? It seems like if we're going from just a URL to something structured, we might just want a ResourceRequest, or perhaps something even more minimal (as I don't think we need the HTTP headers floating around, for example).

Anything that contains both the URL and the cache partition ID (when in use) would be sufficient.

[Adam Barth]

> As a side not though, cache partitioning doesn't measurably degrade performance; we tested page load speed on scenarios that clearly benefit from caching and this particular approach is within the noise perf-wise. I can't argue with the code complexity claim, though.

I'm having trouble reconciling your statements about performance on this bug with your statements about MemoryCache performance on this thread:

https://lists.webkit.org/pipermail/webkit-dev/2012-October/022444.html

If you go forward with this patch, do you plan to rip out the shared memory cache?

[Maciej Stachowiak]

(In reply to comment #22)
> > As a side not though, cache partitioning doesn't measurably degrade performance; we tested page load speed on scenarios that clearly benefit from caching and this particular approach is within the noise perf-wise. I can't argue with the code complexity claim, though.
> 
> I'm having trouble reconciling your statements about performance on this bug with your statements about MemoryCache performance on this thread:
> 
> https://lists.webkit.org/pipermail/webkit-dev/2012-October/022444.html
> 
> If you go forward with this patch, do you plan to rip out the shared memory cache?

We don't have a shared memory cache currently. We likely will add one. The scenario we test where it helps includes a mix of front pages and deep pages from sites. We believe the primary benefit is for pages served from the same domain being able to share cache. On this set around 20% of encoded cache memory is duplicated between processes.

Meanwhile, testing of cache partitioning shows that it affects the cache hit rate by only 0.01%, and around 0.1% in samples of actual user browsing.

You may find this surprising but there's no a priori reason both of these things shouldn't be true, as processes and domains slice your set of web pages differently.

I can publish the list of URLs we use if you like, though copyright precludes sharing the specific snapshotted versions that we use in our testing. We use this same set of pages for both memory testing in a membuster-type harness and speed testing in a PLT-type harness. It is much more recent than the original PLT set.

[Adam Barth]

We're veering a bit off-topic for this bug, but I believe the issues you're seeing with MemoryCache hit rate in WebKit2 are likely related to using a poor process model.  We've studied this issue in detail and have shared some of our findings:

http://www.chromium.org/developers/design-documents/process-models

The model we've found best balances the trade-offs is Process-per-site-instance.  Using a process-per-site-instance, we get good MemoryCache hit rates, intuitive fate sharing, and a natural partition of the MemoryCache.

Rather than hacking a shared memory cache and a partitioned memory cache into WebCore, I would encourage you to simply use a more sophisticated process model in WebKit2.

[Maciej Stachowiak]

(In reply to comment #24)
> We're veering a bit off-topic for this bug, but I believe the issues you're seeing with MemoryCache hit rate in WebKit2 are likely related to using a poor process model.  We've studied this issue in detail and have shared some of our findings:
> 
> http://www.chromium.org/developers/design-documents/process-models
> 
> The model we've found best balances the trade-offs is Process-per-site-instance.  Using a process-per-site-instance, we get good MemoryCache hit rates, intuitive fate sharing, and a natural partition of the MemoryCache.
> 
> Rather than hacking a shared memory cache and a partitioned memory cache into WebCore, I would encourage you to simply use a more sophisticated process model in WebKit2.

Cache partitioning for better tracking privacy can't be achieved solely by doing a process split like the Chromium process-per-site model, for at least three reasons I can think of:

(a) To be effective for tracking mitigation, cache partitioning has to go through to the shared disk cache and to any memory cache layers that are shared between processes, even if they do not use cached memory. 

(b) The Caveats to the Chromium process-per-site model (e.g. no process swap for renderer-initiated navigation, process cap) make it less than completely reliable as a way to isolate even just WebCore memory caches for different top-level domains.

(c) We also want the API to work correctly for WebKit1 clients and for WebKit2 clients using a single WebProcess.

The Chromium process-per-site-instance model may nonetheless turn out to be the best way to split processes. Thanks for the reference. Do you have a reference for the details of how it does subdomain grouping? Maybe we should use that for cache partitioning instead of the public suffix list, if the algorithm used is less problematic.

----

I agree that shared memory for the memory cache is a separate topic and should be discussed in a more relevant bug and/or on webkit-dev. I only gave a data dump because you brought it up.

[Maciej Stachowiak]

(In reply to comment #25)

> 
> (a) To be effective for tracking mitigation, cache partitioning has to go through to the shared disk cache and to any memory cache layers that are shared between processes, even if they do not use cached memory. 

Meant to say: "even if they do not use shared memory".

[Adam Barth]

As I mentioned in IRC, I think it's fine to land this patch with the ifdefs mentioned above.  Based on this discussion, it seems unlikely that anyone besides Apple will be interested in turning the ifdefs on.

[Vicki Pfau]

Created attachment 189604 [details]
Patch

[Adam Barth]

Comment on attachment 189604 [details]
Patch

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

> Source/WebCore/loader/cache/MemoryCache.cpp:65
> +static String partitionName(String domain)

String -> const String&

> Source/WebCore/loader/cache/MemoryCache.cpp:67
> +    DEFINE_STATIC_LOCAL(String, nullPartition, (""));

There's no reason to define a new static for the empty string.  You can just use emptyString()

> Source/WebCore/page/Page.cpp:1182
> +    if (m_settings->storageBlockingPolicy() == SecurityOrigin::BlockAllStorage)
> +        memoryCache()->setDisabled(true);
> +    else
> +        memoryCache()->setDisabled(false);

Should this be guarded by USE(CACHE_PARTITIONING)?  I don't understand how this part relates to the rest of our patch.

> Source/WebCore/page/SecurityOrigin.cpp:450
> +    return host();

This is wrong for non-HTTP/HTTPS schemes.

[Vicki Pfau]

(In reply to comment #29)
> (From update of attachment 189604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189604&action=review
> 
> > Source/WebCore/loader/cache/MemoryCache.cpp:65
> > +static String partitionName(String domain)
> 
> String -> const String&
> 
> > Source/WebCore/loader/cache/MemoryCache.cpp:67
> > +    DEFINE_STATIC_LOCAL(String, nullPartition, (""));
> 
> There's no reason to define a new static for the empty string.  You can just use emptyString()

Didn't know there was an emptyString() function. Good to know.
 
> > Source/WebCore/page/Page.cpp:1182
> > +    if (m_settings->storageBlockingPolicy() == SecurityOrigin::BlockAllStorage)
> > +        memoryCache()->setDisabled(true);
> > +    else
> > +        memoryCache()->setDisabled(false);
> 
> Should this be guarded by USE(CACHE_PARTITIONING)?  I don't understand how this part relates to the rest of our patch.

This should probably be part of a separate patch, actually. I don't think it should be behind USE(CACHE_PARTITIONING), but it might want to be behind some #ifdef. The idea is that if someone wants to block all storage from a third party, cache counts, so it should be disabled outright. But that's unrelated to the direct intent of this patch.

> > Source/WebCore/page/SecurityOrigin.cpp:450
> > +    return host();
> 
> This is wrong for non-HTTP/HTTPS schemes.

I originally had a check here for HTTP/HTTPS, but I wasn't sure what other schemes would be relevant/different. What would it be for FTP, for example?

[Adam Barth]

> > > Source/WebCore/page/SecurityOrigin.cpp:450
> > > +    return host();
> > 
> > This is wrong for non-HTTP/HTTPS schemes.
> 
> I originally had a check here for HTTP/HTTPS, but I wasn't sure what other schemes would be relevant/different. What would it be for FTP, for example?

I'm not sure what the best check is here.  isHierarchical is a good start, but that's still going to give you the wrong answer for safari-extension URLs.

Generally speaking, the question you need to answer is whether the public suffix list applies to a given scheme, which isn't a question that WebCore is going to be able to answer because it's ignorant of such things.

[Build Bot]

Comment on attachment 189604 [details]
Patch

Attachment 189604 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16701247

[Vicki Pfau]

(In reply to comment #31)
> > > > Source/WebCore/page/SecurityOrigin.cpp:450
> > > > +    return host();
> > > 
> > > This is wrong for non-HTTP/HTTPS schemes.
> > 
> > I originally had a check here for HTTP/HTTPS, but I wasn't sure what other schemes would be relevant/different. What would it be for FTP, for example?
> 
> I'm not sure what the best check is here.  isHierarchical is a good start, but that's still going to give you the wrong answer for safari-extension URLs.
> 
> Generally speaking, the question you need to answer is whether the public suffix list applies to a given scheme, which isn't a question that WebCore is going to be able to answer because it's ignorant of such things.

Since we don't have the KURL anymore at this point, perhaps it just makes sense to just check for HTTP/HTTPS, as these are the likely the only relevant cases, anyway.

[Build Bot]

Comment on attachment 189604 [details]
Patch

Attachment 189604 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16711132

[Maciej Stachowiak]

Comment on attachment 189604 [details]
Patch

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

r- since Adam had a bunch of comments and I had one minor one. I think it's probably ready to go once you address the current round of comments.

> Source/WTF/wtf/Platform.h:1071
> +#define WTF_USE_CACHE_PARTITIONING 1
> +#define WTF_USE_PUBLIC_SUFFIX_LIST 1

These should both be ENABLE flags, not USE. USE is about making use of a particular external library or API, ENABLE is about optional pieces of functionality in WebCore itself.

[Maciej Stachowiak]

Comment on attachment 189604 [details]
Patch

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

r- since Adam had a bunch of comments and I had one minor one. I think it's probably ready to go once you address the current round of comments.

> Source/WTF/wtf/Platform.h:1071
> +#define WTF_USE_CACHE_PARTITIONING 1
> +#define WTF_USE_PUBLIC_SUFFIX_LIST 1

These should both be ENABLE flags, not USE. USE is about making use of a particular external library or API, ENABLE is about optional pieces of functionality in WebCore itself.

[Vicki Pfau]

Created attachment 189836 [details]
Patch

[Adam Barth]

Thanks for addressing my concerns.  I'm still not super happy about this patch, but I appreciate your taking my feedback into account.

[Maciej Stachowiak]

Comment on attachment 189836 [details]
Patch

Looks good to me, and it seems like you've addressed all previous comments. But don't you need to turn on the feature flags for CACHE_PARTITIONING and PUBLIC_SUFFIX_LIST by default on Mac?

[Vicki Pfau]

(In reply to comment #39)
> (From update of attachment 189836 [details])
> Looks good to me, and it seems like you've addressed all previous comments. But don't you need to turn on the feature flags for CACHE_PARTITIONING and PUBLIC_SUFFIX_LIST by default on Mac?

I did that but it looks like I forgot to put the updated version of Platform.h in this patch (this one still has it as though they were USE flags).

[Maciej Stachowiak]

The right way to turn on the feature for the Mac port it is using FeatureDefines.xconfig, not Platform.h.

[Vicki Pfau]

Committed r143986: <http://trac.webkit.org/changeset/143986>