Bug 207593 – [WK2][Cocoa] Implement in-WebProcess cookie cache to avoid sync IPC for document.cookie in most cases

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Implement in-WebProcess cookie cache to avoid sync IPC for document.cookie in most cases.

[Chris Dumez]

<rdar://problem/56027027>

[Chris Dumez]

Created attachment 390450 [details]
WIP Patch

[Chris Dumez]

Created attachment 390462 [details]
WIP Patch

[Chris Dumez]

Created attachment 390533 [details]
WIP Patch

[Chris Dumez]

Created attachment 390536 [details]
WIP Patch

[Chris Dumez]

Created attachment 390539 [details]
WIP Patch

[Chris Dumez]

Created attachment 390549 [details]
WIP Patch

Now passing layout tests locally.

[Chris Dumez]

Created attachment 390561 [details]
WIP Patch

[Chris Dumez]

Created attachment 390567 [details]
WIP Patch

With more layout tests.

[Chris Dumez]

Created attachment 390575 [details]
Patch

[Antti Koivisto]

Comment on attachment 390575 [details]
Patch

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

> Source/WTF/wtf/PlatformEnable.h:802
> +#if !defined(ENABLE_COOKIE_CACHE) && PLATFORM(COCOA) && HAVE(CFNETWORK_COOKIE_CHANGE_LISTENER_API)
> +#define ENABLE_COOKIE_CACHE 1
> +#endif

Couldn't this just be a runtime no-op without CFNETWORK_COOKIE_CHANGE_LISTENER_API and avoid most #ifs?

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:617
> +    ASSERT(observers.add(&observer).isNewEntry);

Why is observer only added in debug build?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:131
> +    if (!m_hostsWithCookieListeners.isEmpty()) {
> +        auto hostsWithCookieListeners = copyToVector(m_hostsWithCookieListeners);
> +        unsubscribeFromCookieChangeNotifications(hostsWithCookieListeners);
> +    }

I think it would be nicer to have a separate function or just loop over m_hostsWithCookieListeners here (avoiding the vector copy). It is just call to stopListeningForCookieChangeNotifications for each.

> Source/WebKit/WebProcess/WebPage/WebCookieCache.h:44
> +    bool isFunctional();

isEnabled/isSupported?

[Chris Dumez]

Comment on attachment 390575 [details]
Patch

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

>> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:617
>> +    ASSERT(observers.add(&observer).isNewEntry);
> 
> Why is observer only added in debug build?

ouch, last minute clean up I made :/ Thanks for catching.

[Antti Koivisto]

> ouch, last minute clean up I made :/ Thanks for catching.

Observers don't seem to have much test coverage.

[Chris Dumez]

(In reply to Antti Koivisto from comment #13)
> > ouch, last minute clean up I made :/ Thanks for catching.
> 
> Observers don't seem to have much test coverage.

EWS does not have the CFNetwork SPI needed and thus does not cover my change. Actual layout test bots will run the tests with the cookie cache enabled (at least some of the bots will).

[Chris Dumez]

Created attachment 390687 [details]
Patch

[Chris Dumez]

Created attachment 390688 [details]
Patch

[Chris Dumez]

Created attachment 390691 [details]
Patch

[Chris Dumez]

Created attachment 390695 [details]
Patch

[Chris Dumez]

Created attachment 390925 [details]
Patch

[Chris Dumez]

Created attachment 390926 [details]
Patch

[Chris Dumez]

Created attachment 390988 [details]
Patch

[Chris Dumez]

Created attachment 391004 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391004 [details]
Patch

Clearing flags on attachment: 391004

Committed r256820: <https://trac.webkit.org/changeset/256820>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

The new test http/tests/cookies/document-cookie-after-showModalDialog.html

is a flaky failure. tracking in https://bugs.webkit.org/show_bug.cgi?id=208165