Bug 171483

Summary: Updates to _WKWebsiteDataStoreConfiguration cookie storage location SPI
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, bfulgham, commit-queue, ggaren, mitz, thorton
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Brady Eidson 2017-04-29 23:15:49 PDT 
Updates to _WKWebsiteDataStoreConfiguration cookie storage location SPI

These include:
-Point to a file instead of a directory
-Proper sandboxing
-Proper testing
Comment 1 Brady Eidson 2017-04-29 23:16:10 PDT 
<rdar://problem/31906397>
Comment 2 Brady Eidson 2017-04-29 23:22:41 PDT 
Created attachment 308686 [details]
Patch
Comment 3 Brady Eidson 2017-04-29 23:56:22 PDT 
Created attachment 308687 [details]
Patch
Comment 4 Brady Eidson 2017-04-30 10:52:20 PDT 
Created attachment 308694 [details]
Patch
Comment 5 Geoffrey Garen 2017-04-30 11:01:42 PDT 
Comment on attachment 308694 [details]
Patch

r=me
Comment 6 Brady Eidson 2017-04-30 11:30:10 PDT 
Created attachment 308695 [details]
Patch
Comment 7 Brady Eidson 2017-04-30 11:36:44 PDT 
Created attachment 308696 [details]
Patch
Comment 8 Brady Eidson 2017-04-30 11:45:31 PDT 
Created attachment 308697 [details]
Patch
Comment 9 Brady Eidson 2017-04-30 12:54:14 PDT 
The mac-debug failures are happening on the test bots without this patch.

The ios-sim failures are happening on the test bots without this patch, too.

cq+'ing
Comment 10 WebKit Commit Bot 2017-04-30 13:24:08 PDT 
Comment on attachment 308697 [details]
Patch

Clearing flags on attachment: 308697

Committed r215991: <http://trac.webkit.org/changeset/215991>
Comment 11 WebKit Commit Bot 2017-04-30 13:24:10 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 12 mitz 2017-04-30 13:29:06 PDT 
Comment on attachment 308697 [details]
Patch

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

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

Using less-than-or-equal in a deployment version check is almost always wrong. I think in this case you meant __MAC_OS_X_VERSION_MIN_REQUIRED < 101300.

> Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:214
> +#if (__MAC_OS_X_VERSION_MIN_REQUIRED <= 101200)

Ditto.
Comment 13 Brady Eidson 2017-04-30 13:34:47 PDT 
(In reply to mitz from comment #12)
> Comment on attachment 308697 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308697&action=review
> 
> > Source/WebCore/platform/spi/cf/CFNetworkSPI.h:175
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <= 101200)
> 
> Using less-than-or-equal in a deployment version check is almost always
> wrong. I think in this case you meant __MAC_OS_X_VERSION_MIN_REQUIRED <
> 101300.
> 
> > Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:214
> > +#if (__MAC_OS_X_VERSION_MIN_REQUIRED <= 101200)
> 
> Ditto.

Can you describe why it is wrong?

Is this documented anywhere?

(First I've heard of it)
Comment 14 mitz 2017-04-30 13:35:42 PDT 
(In reply to WebKit Commit Bot from comment #10)
> Comment on attachment 308697 [details]
> Patch
> 
> Clearing flags on attachment: 308697
> 
> Committed r215991: <http://trac.webkit.org/changeset/215991>

This also broke builds that use the Apple-internal SDK because _CFHTTPCookieStorageFlushCookieStores is annotated as deprecated in that SDK.
Comment 15 Brady Eidson 2017-04-30 13:41:29 PDT 
(In reply to mitz from comment #14)
> (In reply to WebKit Commit Bot from comment #10)
> > Comment on attachment 308697 [details]
> > Patch
> > 
> > Clearing flags on attachment: 308697
> > 
> > Committed r215991: <http://trac.webkit.org/changeset/215991>
> 
> This also broke builds that use the Apple-internal SDK because
> _CFHTTPCookieStorageFlushCookieStores is annotated as deprecated in that SDK.

On which OS?
Comment 16 Brady Eidson 2017-04-30 13:42:41 PDT 
(In reply to Brady Eidson from comment #15)
> (In reply to mitz from comment #14)
> > (In reply to WebKit Commit Bot from comment #10)
> > > Comment on attachment 308697 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 308697
> > > 
> > > Committed r215991: <http://trac.webkit.org/changeset/215991>
> > 
> > This also broke builds that use the Apple-internal SDK because
> > _CFHTTPCookieStorageFlushCookieStores is annotated as deprecated in that SDK.
> 
> On which OS?

I see the problem.

I'll just have disable the deprecation warnings there.
Comment 17 mitz 2017-04-30 13:43:44 PDT 
(In reply to Brady Eidson from comment #13)
> (In reply to mitz from comment #12)
> > Comment on attachment 308697 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=308697&action=review
> > 
> > > Source/WebCore/platform/spi/cf/CFNetworkSPI.h:175
> > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <= 101200)
> > 
> > Using less-than-or-equal in a deployment version check is almost always
> > wrong. I think in this case you meant __MAC_OS_X_VERSION_MIN_REQUIRED <
> > 101300.
> > 
> > > Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:214
> > > +#if (__MAC_OS_X_VERSION_MIN_REQUIRED <= 101200)
> > 
> > Ditto.
> 
> Can you describe why it is wrong?

It’s wrong because API availability intervals are half-open, they include the left endpoint and exclude the right endpoint. For example, the interval of macOS versions v in which WKWebView’s certificateChain property is available but not deprecated is 10.11 <= v < 10.12.

> Is this documented anywhere?

I believe that the semantics of availability annotations are documented in Availability.h as well as in the clang documentation. Everything else follows from that.

> (First I've heard of it)

It’s also easy to look for prior examples in the source code (as long as there aren’t many similar mistakes).
Comment 18 Brady Eidson 2017-04-30 13:55:54 PDT 
Followups in https://trac.webkit.org/changeset/215994/webkit