Bug 171748 - [Cocoa] Converting from WebCore::Cookie to NSHTTPCookie always marks cookies as session cookies
Summary: [Cocoa] Converting from WebCore::Cookie to NSHTTPCookie always marks cookies ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-05 14:10 PDT by BJ Burg
Modified: 2017-05-05 16:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.87 KB, patch)
2017-05-05 16:05 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-05-05 14:10:28 PDT
I'm going to temporarily make the assertion wrong so this can be investigated separately from the "secure" flag bug (https://bugs.webkit.org/show_bug.cgi?id=171700). The test fix for that exposed this unexpected failing test assertion.
Comment 1 BJ Burg 2017-05-05 14:15:57 PDT
Committed r216272: <http://trac.webkit.org/changeset/216272>
Comment 2 BJ Burg 2017-05-05 14:16:39 PDT
Un-closing, going to track the fix here too.
Comment 3 BJ Burg 2017-05-05 14:23:24 PDT
Looks like another round tripping issue:

(lldb) po cookie
<NSHTTPCookie version:1 name:"OtherCookieName" value:"OtherCookieValue" expiresDate:(null) created:2017-05-05 21:18:10 +0000 sessionOnly:TRUE domain:".www.w3c.org" partition:"none" path:"/path" isSecure:TRUE>

(lldb) po cookie2.get()
<NSHTTPCookie version:0 name:"OtherCookieName" value:"OtherCookieValue" expiresDate:2017-05-06 00:04:50 +0000 created:2017-05-05 21:18:10 +0000 sessionOnly:FALSE domain:".www.w3c.org" partition:"none" path:"/path" isSecure:FALSE>

Also note that the expires date is getting dropped, afaict. Is that something we are supposed to preserve?
Comment 4 Michael Catanzaro 2017-05-05 14:29:52 PDT
I think it's sessionOnly if it doesn't have an expiresDate... then the expiration date is when you close the browser.
Comment 5 BJ Burg 2017-05-05 14:40:23 PDT
The NS equivalent here is NSHTTPCookieDiscard:

https://developer.apple.com/reference/foundation/nshttpcookiediscard

"String value must be either "TRUE" or "FALSE". This cookie attribute is optional. The default is "FALSE", unless this cookie is version 1 or greater and a value for NSHTTPCookieMaximumAge is not specified, in which case it is assumed to be "TRUE"."
Comment 6 Brady Eidson 2017-05-05 15:08:13 PDT
(In reply to Brian Burg from comment #5)
> The NS equivalent here is NSHTTPCookieDiscard:
> 
> https://developer.apple.com/reference/foundation/nshttpcookiediscard
> 
> "String value must be either "TRUE" or "FALSE". This cookie attribute is
> optional. The default is "FALSE", unless this cookie is version 1 or greater
> and a value for NSHTTPCookieMaximumAge is not specified, in which case it is
> assumed to be "TRUE"."

Ignore the discard thing, I think.
Comment 7 Brady Eidson 2017-05-05 15:15:34 PDT
(In reply to Michael Catanzaro from comment #4)
> I think it's sessionOnly if it doesn't have an expiresDate... then the
> expiration date is when you close the browser.

Michael is right.

If it has an expires date, it's a persistent cookies.
If it doesn't, it's session.
Comment 8 Brady Eidson 2017-05-05 15:17:26 PDT
Yah, the test does the discard thing, but in reality having an expires date is what defines session or not.
Comment 9 Brady Eidson 2017-05-05 15:17:57 PDT
Except the docs say:
"A boolean value that indicates whether the receiver should be discarded at the end of the session (regardless of expiration date)."
lol wut.
Comment 10 Radar WebKit Bug Importer 2017-05-05 15:42:54 PDT
<rdar://problem/32027327>
Comment 11 BJ Burg 2017-05-05 16:05:37 PDT
Created attachment 309232 [details]
Patch
Comment 12 WebKit Commit Bot 2017-05-05 16:45:08 PDT
Comment on attachment 309232 [details]
Patch

Clearing flags on attachment: 309232

Committed r216292: <http://trac.webkit.org/changeset/216292>
Comment 13 WebKit Commit Bot 2017-05-05 16:45:09 PDT
All reviewed patches have been landed.  Closing bug.