Bug 171483 - Updates to _WKWebsiteDataStoreConfiguration cookie storage location SPI
Summary: Updates to _WKWebsiteDataStoreConfiguration cookie storage location SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-29 23:15 PDT by Brady Eidson
Modified: 2017-04-30 13:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (28.65 KB, patch)
2017-04-29 23:22 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (30.18 KB, patch)
2017-04-29 23:56 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (30.50 KB, patch)
2017-04-30 10:52 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (31.85 KB, patch)
2017-04-30 11:30 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (32.06 KB, patch)
2017-04-30 11:36 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (32.07 KB, patch)
2017-04-30 11:45 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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