Merge r174190 - Add basic caching for Document.cookie API

https://bugs.webkit.org/show_bug.cgi?id=137225 Reviewed by Alexey Proskuryakov. Source/WebCore: 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. This patch thus caches / freezes the cookies until we return to the event loop so that consecutive calls to document.cookie() are extremely fast. Doing so seems to be sufficient to achieve a ~87% cache hit for nytimes.com page load. The cookies cache is invalidated whenever: - document.cookie is set - we return to the event loop - a network resource is loaded synchronously as it may cause cookies to be set before we return to the event loop Test: http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html * dom/Document.cpp: (WebCore::Document::Document): (WebCore::Document::open): (WebCore::Document::cookie): (WebCore::Document::setCookie): (WebCore::Document::setCookieURL): (WebCore::Document::initSecurityContext): (WebCore::Document::setDOMCookieCache): (WebCore::Document::invalidateDOMCookieCache): (WebCore::Document::domCookieCacheExpiryTimerFired): (WebCore::Document::didLoadResourceSynchronously): * dom/Document.h: (WebCore::Document::domCookieCache): (WebCore::Document::isDOMCookieCacheValid): (WebCore::Document::setCookieURL): Deleted. * dom/ScriptExecutionContext.cpp: (WebCore::ScriptExecutionContext::didLoadResourceSynchronously): * dom/ScriptExecutionContext.h: * loader/ThreadableLoader.cpp: (WebCore::ThreadableLoader::loadResourceSynchronously): LayoutTests: Add a layout test to make sure that document.cookie returns updated results after cookies are set via a sync XHR. * http/tests/cookies/sync-xhr-set-cookie-invalidates-cache-expected.txt: Added. * http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html: Added. Canonical link: https://commits.webkit.org/154760.64@webkitgtk/2.6 git-svn-id: https://svn.webkit.org/repository/webkit/releases/WebKitGTK/webkit-2.6@174454 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Oct 8, 2014 · 6e5cb9e · 6e5cb9e
1 parent b458b1b
commit 6e5cb9e
Show file tree

Hide file tree

Showing 10 changed files with 176 additions and 13 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2014-10-01  Chris Dumez  <cdumez@apple.com>
+
+        Add basic caching for Document.cookie API
+        https://bugs.webkit.org/show_bug.cgi?id=137225
+
+        Reviewed by Alexey Proskuryakov.
+
+        Add a layout test to make sure that document.cookie returns updated
+        results after cookies are set via a sync XHR.
+
+        * http/tests/cookies/sync-xhr-set-cookie-invalidates-cache-expected.txt: Added.
+        * http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html: Added.
+
 2014-09-30  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         Stack overflow with enormous SVG filter.

diff --git a/LayoutTests/http/tests/cookies/resources/cookies-test-pre.js b/LayoutTests/http/tests/cookies/resources/cookies-test-pre.js
@@ -23,6 +23,11 @@ function setCookies(cookie)
     }
 }
 
+function registerCookieForCleanup(cookie)
+{
+    cookies.push(cookie);
+}
+
 // Normalize a cookie string
 function normalizeCookie(cookie)
 {

diff --git a/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache-expected.txt b/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache-expected.txt
@@ -0,0 +1,12 @@
+Tests that document.cookie returns the right value after a sync XHR
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS normalizeCookie(document.cookie) is "testKey=testValue"
+PASS xhr.status is 200
+PASS normalizeCookie(document.cookie) is "testKey=testValue; xhrKey=xhrValue"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html b/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<script src='resources/cookies-test-pre.js'></script>
+
+<script>
+description('Tests that document.cookie returns the right value after a sync XHR');
+
+document.cookie = "testKey=testValue";
+shouldBeEqualToString('normalizeCookie(document.cookie)', 'testKey=testValue');
+var xhr = new XMLHttpRequest();
+xhr.open('GET', 'resources/setCookies.cgi', false);
+var cookie = 'xhrKey=xhrValue; path=/';
+xhr.setRequestHeader('SET-COOKIE', cookie);
+xhr.send();
+
+// This is so the cookie gets removed at the end of the test.
+registerCookieForCleanup(cookie);
+
+shouldBe('xhr.status', '200');
+shouldBeEqualToString('normalizeCookie(document.cookie)', 'testKey=testValue; xhrKey=xhrValue');
+
+</script>
+<script src='resources/cookies-test-post.js'></script>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,59 @@
+2014-10-01  Chris Dumez  <cdumez@apple.com>
+
+        Add basic caching for Document.cookie API
+        https://bugs.webkit.org/show_bug.cgi?id=137225
+
+        Reviewed by Alexey Proskuryakov.
+
+        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. This patch
+        thus caches / freezes the cookies until we return to the event
+        loop so that consecutive calls to document.cookie() are extremely fast.
+        Doing so seems to be sufficient to achieve a ~87% cache hit for
+        nytimes.com page load.
+
+        The cookies cache is invalidated whenever:
+        - document.cookie is set
+        - we return to the event loop
+        - a network resource is loaded synchronously as it may cause cookies to
+          be set before we return to the event loop
+
+        Test: http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::Document):
+        (WebCore::Document::open):
+        (WebCore::Document::cookie):
+        (WebCore::Document::setCookie):
+        (WebCore::Document::setCookieURL):
+        (WebCore::Document::initSecurityContext):
+        (WebCore::Document::setDOMCookieCache):
+        (WebCore::Document::invalidateDOMCookieCache):
+        (WebCore::Document::domCookieCacheExpiryTimerFired):
+        (WebCore::Document::didLoadResourceSynchronously):
+        * dom/Document.h:
+        (WebCore::Document::domCookieCache):
+        (WebCore::Document::isDOMCookieCacheValid):
+        (WebCore::Document::setCookieURL): Deleted.
+        * dom/ScriptExecutionContext.cpp:
+        (WebCore::ScriptExecutionContext::didLoadResourceSynchronously):
+        * dom/ScriptExecutionContext.h:
+        * loader/ThreadableLoader.cpp:
+        (WebCore::ThreadableLoader::loadResourceSynchronously):
+
 2014-09-30  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         Stack overflow with enormous SVG filter

diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
@@ -513,6 +513,7 @@ Document::Document(Frame* frame, const URL& url, unsigned documentClasses, unsig
     , m_inputCursor(EmptyInputCursor::create())
 #endif
     , m_didAssociateFormControlsTimer(this, &Document::didAssociateFormControlsTimerFired)
+    , m_cookieCacheExpiryTimer(this, &Document::domCookieCacheExpiryTimerFired)
     , m_disabledFieldsetElementsCount(0)
     , m_hasInjectedPlugInsScript(false)
     , m_renderTreeBeingDestroyed(false)
@@ -2212,7 +2213,7 @@ void Document::open(Document* ownerDocument)
 {
     if (ownerDocument) {
         setURL(ownerDocument->url());
-        m_cookieURL = ownerDocument->cookieURL();
+        setCookieURL(ownerDocument->cookieURL());
         setSecurityOrigin(ownerDocument->securityOrigin());
     }
 
@@ -3791,7 +3792,7 @@ HTMLFrameOwnerElement* Document::ownerElement() const
     return frame()->ownerElement();
 }
 
-String Document::cookie(ExceptionCode& ec) const
+String Document::cookie(ExceptionCode& ec)
 {
     if (page() && !page()->settings().cookieEnabled())
         return String();
@@ -3809,7 +3810,10 @@ String Document::cookie(ExceptionCode& ec) const
     if (cookieURL.isEmpty())
         return String();
 
-    return cookies(this, cookieURL);
+    if (!isDOMCookieCacheValid())
+        setCachedDOMCookies(cookies(this, cookieURL));
+
+    return cachedDOMCookies();
 }
 
 void Document::setCookie(const String& value, ExceptionCode& ec)
@@ -3830,6 +3834,7 @@ void Document::setCookie(const String& value, ExceptionCode& ec)
     if (cookieURL.isEmpty())
         return;
 
+    invalidateDOMCookieCache();
     setCookies(this, cookieURL, value);
 }
 
@@ -3933,6 +3938,14 @@ String Document::lastModified() const
     return String::format("%02d/%02d/%04d %02d:%02d:%02d", date.month() + 1, date.monthDay(), date.fullYear(), date.hour(), date.minute(), date.second());
 }
 
+void Document::setCookieURL(const URL& url)
+{
+    if (m_cookieURL == url)
+        return;
+    m_cookieURL = url;
+    invalidateDOMCookieCache();
+}
+
 static bool isValidNameNonASCII(const LChar* characters, unsigned length)
 {
     if (!isValidNameStart(characters[0]))
@@ -4645,15 +4658,15 @@ void Document::initSecurityContext()
     if (!m_frame) {
         // No source for a security context.
         // This can occur via document.implementation.createDocument().
-        m_cookieURL = URL(ParsedURLString, emptyString());
+        setCookieURL(URL(ParsedURLString, emptyString()));
         setSecurityOrigin(SecurityOrigin::createUnique());
         setContentSecurityPolicy(std::make_unique<ContentSecurityPolicy>(this));
         return;
     }
 
     // In the common case, create the security context from the currently
     // loading URL with a fresh content security policy.
-    m_cookieURL = m_url;
+    setCookieURL(m_url);
     enforceSandboxFlags(m_frame->loader().effectiveSandboxFlags());
 
 #if PLATFORM(IOS)
@@ -4719,7 +4732,7 @@ void Document::initSecurityContext()
         return;
     }
 
-    m_cookieURL = ownerFrame->document()->cookieURL();
+    setCookieURL(ownerFrame->document()->cookieURL());
     // We alias the SecurityOrigins to match Firefox, see Bug 15313
     // https://bugs.webkit.org/show_bug.cgi?id=15313
     setSecurityOrigin(ownerFrame->document()->securityOrigin());
@@ -6137,6 +6150,32 @@ void Document::didAssociateFormControlsTimerFired(Timer<Document>& timer)
     m_associatedFormControls.clear();
 }
 
+void Document::setCachedDOMCookies(const String& cookies)
+{
+    ASSERT(!isDOMCookieCacheValid());
+    m_cachedDOMCookies = cookies;
+    // The cookie cache is valid at most until we go back to the event loop.
+    m_cookieCacheExpiryTimer.startOneShot(0);
+}
+
+void Document::invalidateDOMCookieCache()
+{
+    m_cookieCacheExpiryTimer.stop();
+    m_cachedDOMCookies = String();
+}
+
+void Document::domCookieCacheExpiryTimerFired(Timer<Document>&)
+{
+    invalidateDOMCookieCache();
+}
+
+void Document::didLoadResourceSynchronously(const ResourceRequest&)
+{
+    // Synchronous resources loading can set cookies so we invalidate the cookies cache
+    // in this case, to be safe.
+    invalidateDOMCookieCache();
+}
+
 void Document::ensurePlugInsInjectedScript(DOMWrapperWorld& world)
 {
     if (m_hasInjectedPlugInsScript)

diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
@@ -881,7 +881,7 @@ class Document : public ContainerNode, public TreeScope, public ScriptExecutionC
     void setTitleElement(const StringWithDirection&, Element* titleElement);
     void removeTitle(Element* titleElement);
 
-    String cookie(ExceptionCode&) const;
+    String cookie(ExceptionCode&);
     void setCookie(const String&, ExceptionCode&);
 
     String referrer() const;
@@ -904,7 +904,7 @@ class Document : public ContainerNode, public TreeScope, public ScriptExecutionC
     //    inherits its cookieURL but not its URL.
     //
     const URL& cookieURL() const { return m_cookieURL; }
-    void setCookieURL(const URL& url) { m_cookieURL = url; }
+    void setCookieURL(const URL&);
 
     // The firstPartyForCookies is used to compute whether this document
     // appears in a "third-party" context for the purpose of third-party
@@ -1362,6 +1362,14 @@ class Document : public ContainerNode, public TreeScope, public ScriptExecutionC
 
     void didAssociateFormControlsTimerFired(Timer<Document>&);
 
+    // DOM Cookies caching.
+    const String& cachedDOMCookies() const { return m_cachedDOMCookies; }
+    void setCachedDOMCookies(const String&);
+    bool isDOMCookieCacheValid() const { return m_cookieCacheExpiryTimer.isActive(); }
+    void invalidateDOMCookieCache();
+    void domCookieCacheExpiryTimerFired(Timer<Document>&);
+    void didLoadResourceSynchronously(const ResourceRequest&) override final;
+
     unsigned m_referencingNodeCount;
 
     std::unique_ptr<StyleResolver> m_styleResolver;
@@ -1694,6 +1702,8 @@ class Document : public ContainerNode, public TreeScope, public ScriptExecutionC
 #endif
 
     Timer<Document> m_didAssociateFormControlsTimer;
+    Timer<Document> m_cookieCacheExpiryTimer;
+    String m_cachedDOMCookies;
     HashSet<RefPtr<Element>> m_associatedFormControls;
     unsigned m_disabledFieldsetElementsCount;
 

diff --git a/Source/WebCore/dom/ScriptExecutionContext.cpp b/Source/WebCore/dom/ScriptExecutionContext.cpp
@@ -172,6 +172,10 @@ void ScriptExecutionContext::destroyedMessagePort(MessagePort& messagePort)
     m_messagePorts.remove(&messagePort);
 }
 
+void ScriptExecutionContext::didLoadResourceSynchronously(const ResourceRequest&)
+{
+}
+
 bool ScriptExecutionContext::canSuspendActiveDOMObjects()
 {
     checkConsistency();

diff --git a/Source/WebCore/dom/ScriptExecutionContext.h b/Source/WebCore/dom/ScriptExecutionContext.h
@@ -30,6 +30,7 @@
 
 #include "ActiveDOMObject.h"
 #include "DOMTimer.h"
+#include "ResourceRequest.h"
 #include "ScheduledAction.h"
 #include "SecurityContext.h"
 #include "Supplementable.h"
@@ -110,6 +111,8 @@ class ScriptExecutionContext : public SecurityContext, public Supplementable<Scr
     void createdMessagePort(MessagePort&);
     void destroyedMessagePort(MessagePort&);
 
+    virtual void didLoadResourceSynchronously(const ResourceRequest&);
+
     void ref() { refScriptExecutionContext(); }
     void deref() { derefScriptExecutionContext(); }
 

diff --git a/Source/WebCore/loader/ThreadableLoader.cpp b/Source/WebCore/loader/ThreadableLoader.cpp
@@ -66,12 +66,11 @@ void ThreadableLoader::loadResourceSynchronously(ScriptExecutionContext* context
 {
     ASSERT(context);
 
-    if (context->isWorkerGlobalScope()) {
+    if (context->isWorkerGlobalScope())
         WorkerThreadableLoader::loadResourceSynchronously(toWorkerGlobalScope(context), request, client, options);
-        return;
-    }
-
-    DocumentThreadableLoader::loadResourceSynchronously(*toDocument(context), request, client, options);
+    else
+        DocumentThreadableLoader::loadResourceSynchronously(*toDocument(context), request, client, options);
+    context->didLoadResourceSynchronously(request);
 }
 
 } // namespace WebCore