Title: [174454] releases/WebKitGTK/webkit-2.6
Revision
174454
Author
[email protected]
Date
2014-10-08 08:16:23 -0700 (Wed, 08 Oct 2014)

Log Message

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.

Modified Paths

Added Paths

Diff

Modified: releases/WebKitGTK/webkit-2.6/LayoutTests/ChangeLog (174453 => 174454)


--- releases/WebKitGTK/webkit-2.6/LayoutTests/ChangeLog	2014-10-08 14:44:56 UTC (rev 174453)
+++ releases/WebKitGTK/webkit-2.6/LayoutTests/ChangeLog	2014-10-08 15:16:23 UTC (rev 174454)
@@ -1,3 +1,16 @@
+2014-10-01  Chris Dumez  <[email protected]>
+
+        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  <[email protected]>
 
         Stack overflow with enormous SVG filter.

Modified: releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/resources/cookies-test-pre.js (174453 => 174454)


--- releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/resources/cookies-test-pre.js	2014-10-08 14:44:56 UTC (rev 174453)
+++ releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/resources/cookies-test-pre.js	2014-10-08 15:16:23 UTC (rev 174454)
@@ -23,6 +23,11 @@
     }
 }
 
+function registerCookieForCleanup(cookie)
+{
+    cookies.push(cookie);
+}
+
 // Normalize a cookie string
 function normalizeCookie(cookie)
 {

Added: releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache-expected.txt (0 => 174454)


--- releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache-expected.txt	                        (rev 0)
+++ releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache-expected.txt	2014-10-08 15:16:23 UTC (rev 174454)
@@ -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
+

Added: releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html (0 => 174454)


--- releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html	                        (rev 0)
+++ releases/WebKitGTK/webkit-2.6/LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html	2014-10-08 15:16:23 UTC (rev 174454)
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<script src=''></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=''></script>

Modified: releases/WebKitGTK/webkit-2.6/Source/WebCore/ChangeLog (174453 => 174454)


--- releases/WebKitGTK/webkit-2.6/Source/WebCore/ChangeLog	2014-10-08 14:44:56 UTC (rev 174453)
+++ releases/WebKitGTK/webkit-2.6/Source/WebCore/ChangeLog	2014-10-08 15:16:23 UTC (rev 174454)
@@ -1,3 +1,59 @@
+2014-10-01  Chris Dumez  <[email protected]>
+
+        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  <[email protected]>
 
         Stack overflow with enormous SVG filter

Modified: releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/Document.cpp (174453 => 174454)


--- releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/Document.cpp	2014-10-08 14:44:56 UTC (rev 174453)
+++ releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/Document.cpp	2014-10-08 15:16:23 UTC (rev 174454)
@@ -513,6 +513,7 @@
     , 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 @@
 {
     if (ownerDocument) {
         setURL(ownerDocument->url());
-        m_cookieURL = ownerDocument->cookieURL();
+        setCookieURL(ownerDocument->cookieURL());
         setSecurityOrigin(ownerDocument->securityOrigin());
     }
 
@@ -3791,7 +3792,7 @@
     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 @@
     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 @@
     if (cookieURL.isEmpty())
         return;
 
+    invalidateDOMCookieCache();
     setCookies(this, cookieURL, value);
 }
 
@@ -3933,6 +3938,14 @@
     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,7 +4658,7 @@
     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;
@@ -4653,7 +4666,7 @@
 
     // 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 @@
         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 @@
     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)

Modified: releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/Document.h (174453 => 174454)


--- releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/Document.h	2014-10-08 14:44:56 UTC (rev 174453)
+++ releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/Document.h	2014-10-08 15:16:23 UTC (rev 174454)
@@ -881,7 +881,7 @@
     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 @@
     //    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 @@
 
     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 @@
 #endif
 
     Timer<Document> m_didAssociateFormControlsTimer;
+    Timer<Document> m_cookieCacheExpiryTimer;
+    String m_cachedDOMCookies;
     HashSet<RefPtr<Element>> m_associatedFormControls;
     unsigned m_disabledFieldsetElementsCount;
 

Modified: releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/ScriptExecutionContext.cpp (174453 => 174454)


--- releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/ScriptExecutionContext.cpp	2014-10-08 14:44:56 UTC (rev 174453)
+++ releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/ScriptExecutionContext.cpp	2014-10-08 15:16:23 UTC (rev 174454)
@@ -172,6 +172,10 @@
     m_messagePorts.remove(&messagePort);
 }
 
+void ScriptExecutionContext::didLoadResourceSynchronously(const ResourceRequest&)
+{
+}
+
 bool ScriptExecutionContext::canSuspendActiveDOMObjects()
 {
     checkConsistency();

Modified: releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/ScriptExecutionContext.h (174453 => 174454)


--- releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/ScriptExecutionContext.h	2014-10-08 14:44:56 UTC (rev 174453)
+++ releases/WebKitGTK/webkit-2.6/Source/WebCore/dom/ScriptExecutionContext.h	2014-10-08 15:16:23 UTC (rev 174454)
@@ -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 @@
     void createdMessagePort(MessagePort&);
     void destroyedMessagePort(MessagePort&);
 
+    virtual void didLoadResourceSynchronously(const ResourceRequest&);
+
     void ref() { refScriptExecutionContext(); }
     void deref() { derefScriptExecutionContext(); }
 

Modified: releases/WebKitGTK/webkit-2.6/Source/WebCore/loader/ThreadableLoader.cpp (174453 => 174454)


--- releases/WebKitGTK/webkit-2.6/Source/WebCore/loader/ThreadableLoader.cpp	2014-10-08 14:44:56 UTC (rev 174453)
+++ releases/WebKitGTK/webkit-2.6/Source/WebCore/loader/ThreadableLoader.cpp	2014-10-08 15:16:23 UTC (rev 174454)
@@ -66,12 +66,11 @@
 {
     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
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to