Bug 191480 – Add ability to configure document.cookie lifetime cap through user defaults

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

We should have a way to configure the lifetime cap introduced in https://bugs.webkit.org/show_bug.cgi?id=189933.

[John Wilander]

<rdar://problem/45240871>

[John Wilander]

Created attachment 354373 [details]
Patch

[John Wilander]

I hit the same build error locally that the ios-sim bot is seeing. There seems to be a problem with our derived code. The message has been updated to transfer an optional Seconds parameter instead of a boolean but for some reason it's not picked up.

I had to wipe my WebKit Build directory to make it work. :(

[Chris Dumez]

Comment on attachment 354373 [details]
Patch

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

> Source/WebCore/platform/network/NetworkStorageSession.h:195
> +    std::optional<Seconds> m_shouldCapLifetimeForClientSideCookies { };

a m_shouldCapLifetimeForClientSideCookies named variable that is no a boolean seems super weird. This variable and getter / setter probably need better names now?

[John Wilander]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 354373 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354373&action=review
> 
> > Source/WebCore/platform/network/NetworkStorageSession.h:195
> > +    std::optional<Seconds> m_shouldCapLifetimeForClientSideCookies { };
> 
> a m_shouldCapLifetimeForClientSideCookies named variable that is no a
> boolean seems super weird. This variable and getter / setter probably need
> better names now?

True. Will fix.

[Chris Dumez]

Comment on attachment 354373 [details]
Patch

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

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:118
> +    void updateClientSideCookiesAgeCap();

Why is this public?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:334
> +        m_memoryStore->updateClientSideCookiesAgeCap();

Which we should call something like:
m_memoryStore->didCreateNetworkProcess();

And let its implementation decide what it needs to do. This would likely allow these update*() methods to be private.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:842
> +void WebResourceLoadStatisticsStore::setClientSideCookiesAgeCap(std::optional<Seconds> seconds, CompletionHandler<void()>&& completionHandler)

Do we really need this setter? Can't the memory store call setShouldCapLifetimeForClientSideCookies() on the websiteDataStore by itself?

[John Wilander]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 354373 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354373&action=review
> 
> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:118
> > +    void updateClientSideCookiesAgeCap();
> 
> Why is this public?

It's called in WebResourceLoadStatisticsStore::didCreateNetworkProcess().

> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:334
> > +        m_memoryStore->updateClientSideCookiesAgeCap();
> 
> Which we should call something like:
> m_memoryStore->didCreateNetworkProcess();
> 
> And let its implementation decide what it needs to do. This would likely
> allow these update*() methods to be private.

True. I can change that.

> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:842
> > +void WebResourceLoadStatisticsStore::setClientSideCookiesAgeCap(std::optional<Seconds> seconds, CompletionHandler<void()>&& completionHandler)
> 
> Do we really need this setter? Can't the memory store call
> setShouldCapLifetimeForClientSideCookies() on the websiteDataStore by itself?

The memory store doesn't have a pointer to the WebsiteDataStore.

[John Wilander]

Created attachment 354392 [details]
Patch

[Chris Dumez]

Comment on attachment 354392 [details]
Patch

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

> Source/WebCore/platform/network/NetworkStorageSession.h:106
> +    WEBCORE_EXPORT void setShouldCapLifetimeForClientSideCookies(std::optional<Seconds>);

Bad name for setter.

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:284
> +            if (!cookie.expiresDate || cookie.expiresDate.timeIntervalSinceNow > cappedLifetime.value().seconds()) {

cappedLifetime->seconds() is shorter.

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:286
> +                RetainPtr<NSDate> dateInAWeek = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cappedLifetime.value().seconds()]);

ditto.

> Source/WebKit/NetworkProcess/NetworkProcess.h:161
> +    void setShouldCapLifetimeForClientSideCookies(PAL::SessionID, std::optional<Seconds>, uint64_t contextId);

Bad name.

> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:82
> +    SetShouldCapLifetimeForClientSideCookies(PAL::SessionID sessionID, std::optional<Seconds> seconds, uint64_t contextId)

Bad name.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:418
> +void NetworkProcessProxy::setShouldCapLifetimeForClientSideCookies(PAL::SessionID sessionID, std::optional<Seconds> seconds, CompletionHandler<void()>&& completionHandler)

Bad name

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:430
> +    send(Messages::NetworkProcess::SetShouldCapLifetimeForClientSideCookies(sessionID, seconds, callbackId), 0);

Bad name

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:82
> +    void setShouldCapLifetimeForClientSideCookies(PAL::SessionID, std::optional<Seconds>, CompletionHandler<void()>&&);

Bad name

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:840
> +void WebResourceLoadStatisticsStore::setClientSideCookiesAgeCap(std::optional<Seconds> seconds, CompletionHandler<void()>&& completionHandler)

Why do we have this method? Cannot the memory store call setShouldCapLifetimeForClientSideCookies() on the WebsiteDataStore directly? (getting the WebSiteDataStore from the WebResourceLoadStatisticsStore).

This method confuses me as setters on the WebResourceLoadStatisticsStore() usually forward the call to the memory store.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:139
> +    void setShouldCapLifetimeForClientSideCookies(std::optional<Seconds>, CompletionHandler<void()>&&);

Bad name.

[John Wilander]

Created attachment 354411 [details]
Patch

[John Wilander]

Created attachment 354415 [details]
Patch

[John Wilander]

Non-Cocoa build fix.

[Chris Dumez]

Comment on attachment 354415 [details]
Patch

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

r=me with comment.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:143
> +    WebsiteDataStore* getWebsiteDataStore() { return m_websiteDataStore.get(); }

We NEVER use 'get' prefix for getters in WebKit. This should be named websiteDataStore().

[John Wilander]

Created attachment 354433 [details]
Patch for landing

[John Wilander]

Thanks, Chris! Have a nice weekend.

[WebKit Commit Bot]

Comment on attachment 354433 [details]
Patch for landing

Clearing flags on attachment: 354433

Committed r238063: <https://trac.webkit.org/changeset/238063>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.