Bug 240981 – Move disk operations when deleting cookies off from the main thread

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alex Christensen]

When it's on the main thread, it causes hangs and lack of responsiveness.

[Alex Christensen]

https://github.com/WebKit/WebKit/pull/1072

[Alex Christensen]

Created attachment 459795 [details]
Patch

[Chris Dumez]

Comment on attachment 459795 [details]
Patch

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

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:155
> +            [nsCookieStorage() setCookie:newCookie];

Isn't this doing a cookie storage operation on the main thread?

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:585
> +        [nsCookieStorage() _saveCookies:makeBlockPtr([completionHandler = WTFMove(completionHandler)] () mutable {

Isn't this doing cookie operations on the main thread?

> Source/WebKit/NetworkProcess/Cookies/WebCookieManager.h:76
> +    void deleteAllCookies(PAL::SessionID, CompletionHandler<void()>&&);

I wish you'd use `CompletionHandler<void()>&&` consistently in your patch. However, you're using `CompletionHandler<void()>` as parameter sometimes.

[Alex Christensen]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 459795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459795&action=review
> 
> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:155
> > +            [nsCookieStorage() setCookie:newCookie];
> 
> Isn't this doing a cookie storage operation on the main thread?
I think setting a cookie doesn't always immediately go to disk on the same thread.  This patch only addresses cookie deleting on the main thread.

> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:585
> > +        [nsCookieStorage() _saveCookies:makeBlockPtr([completionHandler = WTFMove(completionHandler)] () mutable {
> 
> Isn't this doing cookie operations on the main thread?
Ditto.

> > Source/WebKit/NetworkProcess/Cookies/WebCookieManager.h:76
> > +    void deleteAllCookies(PAL::SessionID, CompletionHandler<void()>&&);
> 
> I wish you'd use `CompletionHandler<void()>&&` consistently in your patch.
> However, you're using `CompletionHandler<void()>` as parameter sometimes.
I'm trying to move to without && but there were places where it looked out of place.  The two are functionally equivalent.

[Chris Dumez]

(In reply to Alex Christensen from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 459795 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=459795&action=review
> > 
> > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:155
> > > +            [nsCookieStorage() setCookie:newCookie];
> > 
> > Isn't this doing a cookie storage operation on the main thread?
> I think setting a cookie doesn't always immediately go to disk on the same
> thread.  This patch only addresses cookie deleting on the main thread.
> 
> > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:585
> > > +        [nsCookieStorage() _saveCookies:makeBlockPtr([completionHandler = WTFMove(completionHandler)] () mutable {
> > 
> > Isn't this doing cookie operations on the main thread?
> Ditto.
> 
> > > Source/WebKit/NetworkProcess/Cookies/WebCookieManager.h:76
> > > +    void deleteAllCookies(PAL::SessionID, CompletionHandler<void()>&&);
> > 
> > I wish you'd use `CompletionHandler<void()>&&` consistently in your patch.
> > However, you're using `CompletionHandler<void()>` as parameter sometimes.
> I'm trying to move to without && but there were places where it looked out
> of place.  The two are functionally equivalent.

CompletionHandler<void()>&& communicates to the caller that we want a r-value and they need to WTFMove(). CompletionHandler<void()> doesn't communicate that. It will give a build time error though if they fail to WTFMove() (only because CompletionHandler is not copyable).

[Alex Christensen]

Comment on attachment 459795 [details]
Patch

GitHub PR has more &&

[EWS]

Committed r294947 (251055@main): <https://commits.webkit.org/251055@main>

Reviewed commits have been landed. Closing PR #1072 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/94048443>