[WKHTTPCookieStore getAllCookies] returns inconsistent creation time

[Sihui Liu]

Calling getAllCookies multiple times would return different creation time/expiration time for the same cookie.

[Sihui Liu]

<rdar://problem/34684214>

[Sihui Liu]

Created attachment 339171 [details]
Patch

[Chris Dumez]

Comment on attachment 339171 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:335
> +    NSMutableDictionary *cookieProperties = [[NSMutableDictionary alloc] init];

Doesn't this leak? Should probably use RetainPtr and adoptNS().

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:357
> +    [cookies release];

Should probably use RetainPtr instead of manual release.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:359
> +    sleep(5_s);

Do we need to sleep for that long? We do not like slow tests in general.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:367
> +    ASSERT_EQ(cookies.count, 1u);

I believe this is reversed:
ASSERT_EQ(1U, cookies.count);

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:371
> +    [cookies release];

Should probably use RetainPtr instead of manual release.

[Geoffrey Garen]

Comment on attachment 339171 [details]
Patch

r- I agree with Chris. Let's discuss Cocoa retain/release in more detail when we have a chance.

[Sihui Liu]

Created attachment 339212 [details]
Patch

[Geoffrey Garen]

Comment on attachment 339212 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:348
> +        *cookiesPtr = [nsCookies retain];

RetainPtr is a smart pointer <https://en.wikipedia.org/wiki/Smart_pointer> that automatically retains and releases the pointer it holds. The act of assignment is an implicit retain of the new value and release of the old value, so please remove this explicit retain, which is an extra retain, and therefore a memory leak.

Also, you can just lambda capture &cookies and use "cookies" by name, rather than capturing cookiesPtr = &cookies. I think that's simpler.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:356
> +    sleep(1_s);

What is the resolution of creation time? If it's milliseconds, then you can reduce this sleep to something like 10ms.

It's good to make tests as fast as possible because we have tens of thousands of them.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:359
> +        *cookiesPtr = [nsCookies retain];

Ditto.

[Sihui Liu]

Created attachment 339240 [details]
Patch

[Sihui Liu]

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 339212 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339212&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:348
> > +        *cookiesPtr = [nsCookies retain];
> 
> RetainPtr is a smart pointer <https://en.wikipedia.org/wiki/Smart_pointer>
> that automatically retains and releases the pointer it holds. The act of
> assignment is an implicit retain of the new value and release of the old
> value, so please remove this explicit retain, which is an extra retain, and
> therefore a memory leak.
> 
> Also, you can just lambda capture &cookies and use "cookies" by name, rather
> than capturing cookiesPtr = &cookies. I think that's simpler.
> 
Done. Lambda capture seems to pass non-static params by value. But I realized we don't need a cookies variable here...

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:356
> > +    sleep(1_s);
> 
> What is the resolution of creation time? If it's milliseconds, then you can
> reduce this sleep to something like 10ms.
> 
> It's good to make tests as fast as possible because we have tens of
> thousands of them.
> 
It's measured in seconds.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:359
> > +        *cookiesPtr = [nsCookies retain];
> 
> Ditto.
Done.

[Geoffrey Garen]

Comment on attachment 339240 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:351
> +    RetainPtr<NSNumber> creationTime = nil;
> +    [globalCookieStore getAllCookies:[creationTimePtr = &creationTime](NSArray<NSHTTPCookie *> *cookies) {
> +        ASSERT_EQ(1u, cookies.count);
> +        *creationTimePtr = [cookies objectAtIndex:0].properties[@"Created"];
> +        gotFlag = true;
> +    }];

I think it should work to do "[globalCookieStore getAllCookies:[&](...) { ... creationTime = ... }". Then you capture the creationTime RetainPtr by reference, and assign to it.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:359
> +        RetainPtr<NSNumber> creationTime2 = [cookies objectAtIndex:0].properties[@"Created"];

You can use NSNumber * here instead of RetainPtr<NSNumber> because objectAtIndex returns an autoreleased value, which remains live within this scope. 

<https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAutoreleasePools.html#//apple_ref/doc/uid/20000047-CJBFBEDI>

[Sihui Liu]

Created attachment 339803 [details]
Patch

[Geoffrey Garen]

Comment on attachment 339803 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 339803 [details]
Patch

Clearing flags on attachment: 339803

Committed r231491: <https://trac.webkit.org/changeset/231491>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 185434

[Geoffrey Garen]

If we need a CF change to get the behavior we want, then we should conditionalize this new behavior and exclude it from OS's that lack the change we need.

[Sihui Liu]

Created attachment 341302 [details]
Patch

[Geoffrey Garen]

Comment on attachment 341302 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 341302 [details]
Patch

Clearing flags on attachment: 341302

Committed r232191: <https://trac.webkit.org/changeset/232191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.