Bug 209992 - [ macOS ] TestWebKitAPI.WKHTTPCookieStore.WithoutProcessPoolDuplicates is failing
Summary: [ macOS ] TestWebKitAPI.WKHTTPCookieStore.WithoutProcessPoolDuplicates is fai...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-03 16:36 PDT by Sihui Liu
Modified: 2020-04-08 12:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.23 KB, patch)
2020-04-03 16:42 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (2.54 KB, patch)
2020-04-07 11:49 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (2.58 KB, patch)
2020-04-07 15:30 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2020-04-03 16:36:40 PDT
...
Comment 1 Sihui Liu 2020-04-03 16:42:34 PDT
Created attachment 395417 [details]
Patch
Comment 2 Alexey Proskuryakov 2020-04-03 22:30:11 PDT
Comment on attachment 395417 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:641
> +            [cookieStore deleteCookie:cookie completionHandler:nil];

What makes it so that we don’t need to wait for completion?
Comment 3 Sihui Liu 2020-04-07 11:49:14 PDT
Created attachment 395716 [details]
Patch
Comment 4 Sihui Liu 2020-04-07 11:50:29 PDT
(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 395417 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395417&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:641
> > +            [cookieStore deleteCookie:cookie completionHandler:nil];
> 
> What makes it so that we don’t need to wait for completion?

We need to wait for the completion. Updated the patch.
Comment 5 Alexey Proskuryakov 2020-04-07 14:53:29 PDT
Comment on attachment 395716 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:649
> +                if (++deletedCount == cookies.count)

Are all of these completion handlers called on the same thread? I thought that they just go on a dispatch queue, and can be executed simultaneously, in which case this should use an atomic operation instead of a plain "++".

Also, please take cookies.count function call out of the loop. This array should be thread safe, but there is no need to recalculate its size.
Comment 6 Sihui Liu 2020-04-07 15:13:27 PDT
Comment on attachment 395716 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:649
>> +                if (++deletedCount == cookies.count)
> 
> Are all of these completion handlers called on the same thread? I thought that they just go on a dispatch queue, and can be executed simultaneously, in which case this should use an atomic operation instead of a plain "++".
> 
> Also, please take cookies.count function call out of the loop. This array should be thread safe, but there is no need to recalculate its size.

All completion handlers will be called on the main runloop. Will move cookies.count out of the loop.
Comment 7 Sihui Liu 2020-04-07 15:30:36 PDT
Created attachment 395749 [details]
Patch
Comment 8 Radar WebKit Bug Importer 2020-04-08 09:23:38 PDT
<rdar://problem/61458442>
Comment 9 Geoffrey Garen 2020-04-08 09:47:02 PDT
Comment on attachment 395749 [details]
Patch

r=me
Comment 10 EWS 2020-04-08 12:25:36 PDT
Committed r259745: <https://trac.webkit.org/changeset/259745>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395749 [details].