Bug 137225 – Add basic caching for Document.cookie API

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

While profiling the load of nytimes.com, I noticed that the site is accessing ~250 times document.cookie, just during page load. Accessing document.cookie is currently slow because we:
- Call WebPlatformStrategies::cookiesForDOM() virtual function
- Send a sync IPC message to the Network process to retrieve the cookies
  * The Network process gets the list of cookies from CFNetwork then serializes the result to send it back to the WebProcess
- We unserialize the cookies into an NSList of cookies
- We filter-out the cookies that are 'httpOnly' and construct a new NSList of cookies
- We create a WTF String out of the cookies NSList

In the case of nytimes.com, it turns out that up to 100 calls to document.cookie() are made in the same event loop iteration. A proposal would be to cache / freeze the cookies until we return to the event loop so that consecutive calls to document.cookie() can be fast.
Doing so seems to be sufficient to achieve a ~76% cache hit for nytimes.com.

[Chris Dumez]

Created attachment 238869 [details]
WIP Patch

I still likely need to invalidate the cache in case of sync XHRs. This case doesn't seem to be covered by existing layout tests so I'll need to add coverage.

[Alexey Proskuryakov]

Comment on attachment 238869 [details]
WIP Patch

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

> Source/WebCore/dom/Document.h:1709
> +    String m_cookiesCache;

It may be worth being more clear that these are DOM cookies (i.e. httponly cookies are not there).

[WebKit Commit Bot]

Attachment 238869 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 238869 [details]
WIP Patch

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

>> Source/WebCore/dom/Document.h:1709
>> +    String m_cookiesCache;
> 
> It may be worth being more clear that these are DOM cookies (i.e. httponly cookies are not there).

Good point, I'll add "DOM" in the names.

[Chris Dumez]

Created attachment 238887 [details]
WIP Patch

[Chris Dumez]

Created attachment 238891 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 238891 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3860
> +    if (!isDOMCookiesCacheValid())
> +        setDOMCookiesCache(cookies(this, cookieURL));
> +
> +    return domCookiesCache();

I think that it's "cookie cache", not "cookies cache".

> Source/WebCore/dom/Document.h:1375
> +    void didDOMCookiesCacheExpiryTimerFired(Timer<Document>&);

Syntax is wrong here "did fired" doesn't parse. Maybe "domCookieCacheExpiryTimerFired"?

> Source/WebCore/dom/Document.h:1376
> +    void didLoadXHRSynchronously() override final;

Can (or should) we catch it more generally, e.g. invalidate the cache when exiting any nested run loop? Another case I have in mind is showModalDialog. Also XSLT stylesheet loading.

> Source/WebCore/dom/ScriptExecutionContext.h:114
> +    // XHR.
> +    virtual void didLoadXHRSynchronously();

This comment doesn't seem helpful.

[Chris Dumez]

Comment on attachment 238891 [details]
Patch

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

>> Source/WebCore/dom/Document.h:1376
>> +    void didLoadXHRSynchronously() override final;
> 
> Can (or should) we catch it more generally, e.g. invalidate the cache when exiting any nested run loop? Another case I have in mind is showModalDialog. Also XSLT stylesheet loading.

I am not sure how to do this but I will try

[Chris Dumez]

Created attachment 238896 [details]
Patch

[Chris Dumez]

(In reply to comment #8)
> (From update of attachment 238891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238891&action=review
> 
> >> Source/WebCore/dom/Document.h:1376
> >> +    void didLoadXHRSynchronously() override final;
> > 
> > Can (or should) we catch it more generally, e.g. invalidate the cache when exiting any nested run loop? Another case I have in mind is showModalDialog. Also XSLT stylesheet loading.
> 
> I am not sure how to do this but I will try

Alexey, how about invalidating the Document's cookie cache when ThreadableLoader::loadResourceSynchronously() is called? This will cover the sync XHR case and more cases where network resources are loaded synchronously. This will hopefully be safer.

[Alexey Proskuryakov]

Good idea!

[Chris Dumez]

Created attachment 238939 [details]
Patch

[Geoffrey Garen]

Comment on attachment 238939 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6197
> +    // The cookies cache is valid until we go back to the event loop.

"cookie cache"

> Source/WebCore/dom/Document.cpp:6198
> +    m_cookieCacheExpiryTimer.startOneShot(0);

It would be nice not to rely on this race to expire the cache. It's possible for a network resource and a zero-delay JavaScript timer to run in the next runloop iteration before this invalidation event. I wonder if there's a low-level networking API we can use to invalidate the cache instead. For example, is the document notified when a subresource loads?

[Chris Dumez]

(In reply to comment #13)
> (From update of attachment 238939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238939&action=review
> 
> > Source/WebCore/dom/Document.cpp:6197
> > +    // The cookies cache is valid until we go back to the event loop.
> 
> "cookie cache"
> 
> > Source/WebCore/dom/Document.cpp:6198
> > +    m_cookieCacheExpiryTimer.startOneShot(0);
> 
> It would be nice not to rely on this race to expire the cache. It's possible for a network resource and a zero-delay JavaScript timer to run in the next runloop iteration before this invalidation event. I wonder if there's a low-level networking API we can use to invalidate the cache instead. For example, is the document notified when a subresource loads?

Knowing about sub-resources loaded for the current WebProcess sounds uncomplicated. The issue is knowing about the sub-resources loaded for other WebProcesses as they can alter the cookies as well.

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 238939 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=238939&action=review
> > 
> > > Source/WebCore/dom/Document.cpp:6197
> > > +    // The cookies cache is valid until we go back to the event loop.
> > 
> > "cookie cache"
> > 
> > > Source/WebCore/dom/Document.cpp:6198
> > > +    m_cookieCacheExpiryTimer.startOneShot(0);
> > 
> > It would be nice not to rely on this race to expire the cache. It's possible for a network resource and a zero-delay JavaScript timer to run in the next runloop iteration before this invalidation event. I wonder if there's a low-level networking API we can use to invalidate the cache instead. For example, is the document notified when a subresource loads?
> 
> Knowing about sub-resources loaded for the current WebProcess sounds uncomplicated. The issue is knowing about the sub-resources loaded for other WebProcesses as they can alter the cookies as well.

Ideally, we would just get a notification when cookies are changed. Invalidating the cookie cache for all resources loads would be less optimal. However, I was trying to find an acceptable short-term solution to caching Document.cookie.

[Alexey Proskuryakov]

What is the use case for caring about cookies that changed from another tab? I'm not sure if it's even correct to respect those changes before the next run loop iteration.

[Chris Dumez]

(In reply to comment #16)
> What is the use case for caring about cookies that changed from another tab? I'm not sure if it's even correct to respect those changes before the next run loop iteration.

Oh OK, then you would want to keep both right?
1. Invalidate in a setTimeout(0) (as in my patch)
+
2. Invalidate if a sub resource load finishes for the current WebProcess

[Alexey Proskuryakov]

I liked the approach with loadResourceSynchronously more, even though there is probably no difference in practice. Not everything loaded by a page is a subresource - notably, pings are not. So, resource loading feels like the wrong level to implement this at.

[Chris Dumez]

(In reply to comment #18)
> I liked the approach with loadResourceSynchronously more, even though there is probably no difference in practice. Not everything loaded by a page is a subresource - notably, pings are not. So, resource loading feels like the wrong level to implement this at.

This is what my last patch iteration does (invalidate cache in loadResourceSynchronously).

[Chris Dumez]

Created attachment 238951 [details]
Patch

[Chris Dumez]

Created attachment 238977 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 238977 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3858
> +        setDOMCookieCache(cookies(this, cookieURL));

setDOMCookieCache sounds like it's taking some kind of CookieCache object, which it is not. Maybe setCachedDOMCookies would be less confusing?

> Source/WebCore/dom/Document.cpp:3860
> +    return domCookieCache();

Ditto. cachedDOMCookies?

> Source/WebCore/dom/Document.cpp:3988
> +    m_cookieURL = url;
> +    invalidateDOMCookieCache();

I'm wondering if we ever call this function with the same value repeatedly. It may not matter in practice, we probably don't do that between document.cookie.calls too often, if ever.

> Source/WebCore/dom/Document.cpp:6197
> +    // The cookie cache is valid until we go back to the event loop.

Should probably say "at most", as sync loading can also invalidate it.

> LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html:18
> +<script src='resources/cookies-test-post.js'></script>

I'm somewhat concerned that this may not clean up the cookies before the next test. Could you please double-check that?

[Chris Dumez]

Comment on attachment 238977 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:3858
>> +        setDOMCookieCache(cookies(this, cookieURL));
> 
> setDOMCookieCache sounds like it's taking some kind of CookieCache object, which it is not. Maybe setCachedDOMCookies would be less confusing?

Right, done.

>> Source/WebCore/dom/Document.cpp:3860
>> +    return domCookieCache();
> 
> Ditto. cachedDOMCookies?

Done.

>> Source/WebCore/dom/Document.cpp:3988
>> +    invalidateDOMCookieCache();
> 
> I'm wondering if we ever call this function with the same value repeatedly. It may not matter in practice, we probably don't do that between document.cookie.calls too often, if ever.

I verified and we definitely call setCookieURL() with the same URL repeatedly so I added a check. Cache hit is up from 76% to 87% on nytimes.com load now.

>> Source/WebCore/dom/Document.cpp:6197
>> +    // The cookie cache is valid until we go back to the event loop.
> 
> Should probably say "at most", as sync loading can also invalidate it.

Done.

>> LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html:18
>> +<script src='resources/cookies-test-post.js'></script>
> 
> I'm somewhat concerned that this may not clean up the cookies before the next test. Could you please double-check that?

cookies-test-post.js seems to be clearing the cookies at the end:
// Make sure that we do not leak any cookies.
clearCookies();

[Chris Dumez]

Created attachment 239059 [details]
Patch

[Chris Dumez]

Comment on attachment 239059 [details]
Patch

Actually, my new test causes http/tests/misc/empty-cookie.html to be flaky. I will investigate and fix the test.

[Chris Dumez]

Created attachment 239063 [details]
Patch

[Chris Dumez]

Ok, I updated my layout test to fix the flakiness.

[WebKit Commit Bot]

Comment on attachment 239063 [details]
Patch

Clearing flags on attachment: 239063

Committed r174190: <http://trac.webkit.org/changeset/174190>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.