Bug 148574 – Use new CFNetwork cookie jar SPI only on El Capitan

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Use new CFNetwork cookie jar SPI only on El Capitan

<rdar://problem/22460752>

[Brady Eidson]

Created attachment 260156 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 260156 [details]
Patch v1

r-

This won't build on El Capitan without internal headers.  You need a change like this to CFNetworkSPI.h:

diff --git a/Source/WebCore/platform/spi/cf/CFNetworkSPI.h b/Source/WebCore/platform/spi/cf/CFNetworkSPI.h
index ac8f087..34621e1 100644
--- a/Source/WebCore/platform/spi/cf/CFNetworkSPI.h
+++ b/Source/WebCore/platform/spi/cf/CFNetworkSPI.h
@@ -144,6 +144,14 @@ EXTERN_C CFArrayRef _CFHTTPParsedCookiesWithResponseHeaderFields(CFAllocatorRef
 + (void)_setSharedHTTPCookieStorage:(NSHTTPCookieStorage *)storage;
 @end
 #endif
+
+#if !USE(APPLE_INTERNAL_SDK) && ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 90000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100))
+@interface NSHTTPCookie ()
++ (NSArray *)_parsedCookiesWithResponseHeaderFields:(NSDictionary *)headerFields forURL:(NSURL *)aURL;
+@end
 #endif
 
+#endif
+
+
 #endif // CFNetworkSPI_h

[David Kilzer (:ddkilzer)]

Comment on attachment 260156 [details]
Patch v1

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

> Source/WebCore/platform/network/mac/CookieJarMac.mm:123
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

Also, this needs to include a PLATFORM(MAC) check to fix the build failure on iOS:

/Volumes/Data/EWS/WebKit/Source/WebCore/platform/network/mac/CookieJarMac.mm:123:5: error: '__MAC_OS_X_VERSION_MIN_REQUIRED' is not defined, evaluates to 0 [-Werror,-Wundef]
#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
    ^
1 error generated.

#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

[Brady Eidson]

Created attachment 260157 [details]
Patch v2

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 260156 [details]
> Patch v1
> 
> r-
> 
> This won't build on El Capitan without internal headers.  You need a change
> like this to CFNetworkSPI.h:
> 

Yup, we already went over this in the radar. Needs the forward declare, but this one isn't quite right.

[Brady Eidson]

(In reply to comment #3)
> Comment on attachment 260156 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260156&action=review
> 
> > Source/WebCore/platform/network/mac/CookieJarMac.mm:123
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
> 
> Also, this needs to include a PLATFORM(MAC) check to fix the build failure
> on iOS:
> 
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/network/mac/CookieJarMac.mm:
> 123:5: error: '__MAC_OS_X_VERSION_MIN_REQUIRED' is not defined, evaluates to
> 0 [-Werror,-Wundef]
> #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
>     ^
> 1 error generated.
> 
> #if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

This is sad and always catches me by surprise - iOS uses both cookiejarmac and cookiejarcf  :(

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #3)
> > Comment on attachment 260156 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=260156&action=review
> > 
> > > Source/WebCore/platform/network/mac/CookieJarMac.mm:123
> > > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
> > 
> > Also, this needs to include a PLATFORM(MAC) check to fix the build failure
> > on iOS:
> > 
> > /Volumes/Data/EWS/WebKit/Source/WebCore/platform/network/mac/CookieJarMac.mm:
> > 123:5: error: '__MAC_OS_X_VERSION_MIN_REQUIRED' is not defined, evaluates to
> > 0 [-Werror,-Wundef]
> > #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
> >     ^
> > 1 error generated.
> > 
> > #if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
> 
> This is sad and always catches me by surprise - iOS uses both cookiejarmac
> and cookiejarcf  :(


Actually, this is SUPREMELY surprising.  How is this possible?  iOS gets this exact same symbol twice - Once from CookieJarMac and once from CookieJarCFNet, yet it uses the CFNet version  o_O

[Brady Eidson]

Created attachment 260158 [details]
Patch v3

This adds the PLATFORM(MAC), but it's weird that it's needed.

We need an answer to why that builds on iOS, and how it's possible that iOS gets the same symbol twice and chooses the right one.  =/

[David Kilzer (:ddkilzer)]

Comment on attachment 260157 [details]
Patch v2

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

r=me with the macro fixes.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:123
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

This still needs to be changed to prevent an iOS build failure:

#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

Apparently this file is compiled on iOS but not used.

> Source/WebCore/platform/spi/cf/CFNetworkSPI.h:132
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

This should really be:

#if !USE(APPLE_INTERNAL_SDK) || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

[David Kilzer (:ddkilzer)]

Comment on attachment 260158 [details]
Patch v3

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

r=me but consider the macro change.

> Source/WebCore/platform/spi/cf/CFNetworkSPI.h:132
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

This should probably be:

#if !USE(APPLE_INTERNAL_SDK) || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Created attachment 260158 [details]
> Patch v3
> 
> This adds the PLATFORM(MAC), but it's weird that it's needed.
> 
> We need an answer to why that builds on iOS, and how it's possible that iOS
> gets the same symbol twice and chooses the right one.  =/

I know why.  It's because the EWS builder has the NSURLConnection path defined (so WTF_USE_CFNETWORK set to 0), while builds against an internal iOS SDK have WTF_USE_CFNETOWRK set to 1.  (This needs to be fixed; I'll explain the details in radar.)

Otherwise there would be linker errors with duplicate symbols.

[David Kilzer (:ddkilzer)]

Comment on attachment 260158 [details]
Patch v3

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

r=me

>> Source/WebCore/platform/spi/cf/CFNetworkSPI.h:132
>> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
> 
> This should probably be:
> 
> #if !USE(APPLE_INTERNAL_SDK) || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

Comment withdrawn after taking to Brady in person.  The !USE(APPLE_INTERNAL_SDK) doesn't matter here.

[Brady Eidson]

Since EWS ran and since I was actually back at my computer, landed myself instead of waiting on the CQ.

https://trac.webkit.org/changeset/189101

[Antti Koivisto]

Why no tests?