.: REGRESSION (r39725?): Resources removed from document can not be freed until the...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Aug 2011 21:22:08 +0000 (21:22 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Aug 2011 21:22:08 +0000 (21:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=61006

Patch by Scott Graham <scottmg@chromium.org> on 2011-08-01
Reviewed by Antti Koivisto.

Update exports for test harness.

* Source/autotools/symbols.filter:

Source/WebCore: REGRESSION (r39725?): Resources removed from document can not be freed
until the document is deleted
https://bugs.webkit.org/show_bug.cgi?id=61006

Patch by Scott Graham <scottmg@chromium.org> on 2011-08-01
Reviewed by Antti Koivisto.

Upon completing a load start a Timer to iterate through
CachedResourceLoader's m_documentResources map to check for any items
that have only one reference (thus being the reference in the map
itself). The map should really be weak, but because the
CachedResourceHandle achieves bookkeeping work in addition to
reference counting, this is a simpler and more localized way to free
the used memory while maintaining the other behaviour (when
CachedResource is used as proxy).

With this patch the testcase at
https://bugs.webkit.org/attachment.cgi?id=93850 should no longer
consume 400MB of ram on load. Test added for crash discovered in
previous revision, but no tests for memory usage.

Test: http/tests/inspector/network/disabled-cache-crash.html

* WebCore.exp.in:
* loader/cache/CachedResource.h:
(WebCore::CachedResource::hasOneHandle):
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::CachedResourceLoader):
(WebCore::CachedResourceLoader::loadDone):
(WebCore::CachedResourceLoader::garbageCollectDocumentResourcesTimerFired):
* loader/cache/CachedResourceLoader.h:
* testing/Internals.cpp:
(WebCore::Internals::disableMemoryCache):
* testing/Internals.h:
* testing/Internals.idl:

Source/WebKit2: REGRESSION (r39725?): Resources removed from document can not be freed until the document is deleted
https://bugs.webkit.org/show_bug.cgi?id=61006

Patch by Scott Graham <scottmg@chromium.org> on 2011-08-01
Reviewed by Antti Koivisto.

Update exports for test harness.

* win/WebKit2.def:
* win/WebKit2CFLite.def:

LayoutTests: https://bugs.webkit.org/show_bug.cgi?id=61006

Test for CachedResourceLoader. Not caused by cache-disabling but very
difficult to reproduce when cache is active, so use cache disable in
inspector to exercise code.

Patch by Scott Graham <scottmg@chromium.org> on 2011-08-01
Reviewed by Antti Koivisto.

* http/tests/inspector/network/disabled-cache-crash-expected.txt: Added.
* http/tests/inspector/network/disabled-cache-crash.html: Added.
* platform/gtk/Skipped:
* platform/mac/Skipped:
* platform/qt/Skipped:
* platform/win/Skipped:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@92143 268f45cc-cd09-0410-ab3c-d52691b4dbfc

20 files changed:
ChangeLog
LayoutTests/ChangeLog
LayoutTests/http/tests/inspector/network/disabled-cache-crash-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/inspector/network/disabled-cache-crash.html [new file with mode: 0644]
LayoutTests/platform/gtk/Skipped
LayoutTests/platform/mac/Skipped
LayoutTests/platform/qt/Skipped
LayoutTests/platform/win/Skipped
Source/WebCore/ChangeLog
Source/WebCore/WebCore.exp.in
Source/WebCore/loader/cache/CachedResource.h
Source/WebCore/loader/cache/CachedResourceLoader.cpp
Source/WebCore/loader/cache/CachedResourceLoader.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl
Source/WebKit2/ChangeLog
Source/WebKit2/win/WebKit2.def
Source/WebKit2/win/WebKit2CFLite.def
Source/autotools/symbols.filter

index 3572215..b49a596 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2011-08-01  Scott Graham  <scottmg@chromium.org>
+
+        REGRESSION (r39725?): Resources removed from document can not be freed until the document is deleted
+        https://bugs.webkit.org/show_bug.cgi?id=61006
+
+        Reviewed by Antti Koivisto.
+
+        Update exports for test harness.
+
+        * Source/autotools/symbols.filter:
+
 2011-08-01  Hayato Ito  <hayato@chromium.org>
 
         Add support for getting an element in shadow root by its id into a window.internals object.
index a470cc3..d126c98 100644 (file)
@@ -1,3 +1,20 @@
+2011-08-01  Scott Graham  <scottmg@chromium.org>
+
+        https://bugs.webkit.org/show_bug.cgi?id=61006
+
+        Test for CachedResourceLoader. Not caused by cache-disabling but very
+        difficult to reproduce when cache is active, so use cache disable in
+        inspector to exercise code.
+
+        Reviewed by Antti Koivisto.
+
+        * http/tests/inspector/network/disabled-cache-crash-expected.txt: Added.
+        * http/tests/inspector/network/disabled-cache-crash.html: Added.
+        * platform/gtk/Skipped:
+        * platform/mac/Skipped:
+        * platform/qt/Skipped:
+        * platform/win/Skipped:
+
 2011-08-01  Jochen Eisinger  <jochen@chromium.org>
 
         Require explicit user action to override the policy URL on form submissions.
diff --git a/LayoutTests/http/tests/inspector/network/disabled-cache-crash-expected.txt b/LayoutTests/http/tests/inspector/network/disabled-cache-crash-expected.txt
new file mode 100644 (file)
index 0000000..4b5b780
--- /dev/null
@@ -0,0 +1,3 @@
+CONSOLE MESSAGE: line 20: window 1 ok
+CONSOLE MESSAGE: line 26: window 2 ok
+
diff --git a/LayoutTests/http/tests/inspector/network/disabled-cache-crash.html b/LayoutTests/http/tests/inspector/network/disabled-cache-crash.html
new file mode 100644 (file)
index 0000000..30f976a
--- /dev/null
@@ -0,0 +1,66 @@
+<html>
+<head>
+<script type="text/javascript">
+    if (window.layoutTestController)
+       layoutTestController.setCanOpenWindows();
+
+    var ga = document.createElement('script');
+    ga.type = 'text/javascript';
+    ga.async = true;
+    ga.src = 'script.js';
+    var s = document.getElementsByTagName('script')[0];
+    s.parentNode.insertBefore(ga, s);
+</script>
+<script src="../inspector-test.js"></script>
+<script type="text/javascript">
+
+function openWindow1()
+{
+    window.open(window.location, "_blank");
+    console.log("window 1 ok");
+}
+
+function openWindow2()
+{
+    window.open(window.location, "_blank");
+    console.log("window 2 ok");
+}
+
+function test()
+{
+    if (!window.internals) {
+        console.log("This test cannot be run as window.internals is not available.");
+        return;
+    }
+    NetworkAgent.setCacheDisabled(true, step1);
+    internals.disableMemoryCache(true);
+
+    function step1(msg)
+    {
+        InspectorTest.addSniffer(WebInspector.ConsoleView.prototype, "addMessage", step2);
+        InspectorTest.evaluateInPage("openWindow1()");
+    }
+
+    function step2(msg)
+    {
+        InspectorTest.addSniffer(WebInspector.ConsoleView.prototype, "addMessage", step3);
+        InspectorTest.evaluateInPage("openWindow2()");
+    }
+
+    function step3(msg)
+    {
+        NetworkAgent.setCacheDisabled(false, step4);
+    }
+
+    function step4(msg)
+    {
+        internals.disableMemoryCache(false);
+        InspectorTest.completeTest();
+    }
+}
+</script>
+<head>
+<body onload="runTest()">
+</body>
+</html>
+
index c5b0189..49a8a19 100644 (file)
@@ -1460,6 +1460,7 @@ http/tests/inspector/network/network-size.html
 # https://bugs.webkit.org/show_bug.cgi?id=64097
 http/tests/inspector/network/network-disable-cache-memory.html
 http/tests/inspector/network/network-disable-cache-xhrs.html
+http/tests/inspector/network/disabled-cache-crash.html
 
 # https://bugs.webkit.org/show_bug.cgi?id=61437
 http/tests/inspector/network/network-clear-after-disabled.html
index d15aa41..1c1d750 100644 (file)
@@ -332,6 +332,7 @@ http/tests/inspector/network/network-size-sync.html
 # https://bugs.webkit.org/show_bug.cgi?id=64097
 http/tests/inspector/network/network-disable-cache-memory.html
 http/tests/inspector/network/network-disable-cache-xhrs.html
+http/tests/inspector/network/disabled-cache-crash.html
 
 # https://bugs.webkit.org/show_bug.cgi?id=58515
 compositing/overflow/clip-content-under-overflow-controls.html
index 38f5334..18525da 100644 (file)
@@ -1857,6 +1857,7 @@ http/tests/inspector/network/network-size-sync.html
 # https://bugs.webkit.org/show_bug.cgi?id=64097
 http/tests/inspector/network/network-disable-cache-memory.html
 http/tests/inspector/network/network-disable-cache-xhrs.html
+http/tests/inspector/network/disabled-cache-crash.html
 
 # [Qt] media/video-playbackrate.html fails
 # https://bugs.webkit.org/show_bug.cgi?id=57476
index df0a6b1..ceb8283 100644 (file)
@@ -55,6 +55,7 @@ http/tests/inspector/network/network-size-sync.html
 # https://bugs.webkit.org/show_bug.cgi?id=64097
 http/tests/inspector/network/network-disable-cache-memory.html
 http/tests/inspector/network/network-disable-cache-xhrs.html
+http/tests/inspector/network/disabled-cache-crash.html
 
 # Fails <rdar://problem/5674289>
 media/video-seek-past-end-paused.html
index 96288d5..38abc4c 100644 (file)
@@ -1,3 +1,40 @@
+2011-08-01  Scott Graham  <scottmg@chromium.org>
+
+        REGRESSION (r39725?): Resources removed from document can not be freed
+        until the document is deleted
+        https://bugs.webkit.org/show_bug.cgi?id=61006
+
+        Reviewed by Antti Koivisto.
+
+        Upon completing a load start a Timer to iterate through
+        CachedResourceLoader's m_documentResources map to check for any items
+        that have only one reference (thus being the reference in the map
+        itself). The map should really be weak, but because the
+        CachedResourceHandle achieves bookkeeping work in addition to
+        reference counting, this is a simpler and more localized way to free
+        the used memory while maintaining the other behaviour (when
+        CachedResource is used as proxy).
+
+        With this patch the testcase at
+        https://bugs.webkit.org/attachment.cgi?id=93850 should no longer
+        consume 400MB of ram on load. Test added for crash discovered in
+        previous revision, but no tests for memory usage.
+
+        Test: http/tests/inspector/network/disabled-cache-crash.html
+
+        * WebCore.exp.in:
+        * loader/cache/CachedResource.h:
+        (WebCore::CachedResource::hasOneHandle):
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::CachedResourceLoader):
+        (WebCore::CachedResourceLoader::loadDone):
+        (WebCore::CachedResourceLoader::garbageCollectDocumentResourcesTimerFired):
+        * loader/cache/CachedResourceLoader.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::disableMemoryCache):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2011-08-01  Jochen Eisinger  <jochen@chromium.org>
 
         Never override the policy URL on form submissions.
index 115f00d..1d726fa 100644 (file)
@@ -217,6 +217,7 @@ __ZN7WebCore11MemoryCache13setCapacitiesEjjj
 __ZN7WebCore11MemoryCache14evictResourcesEv
 __ZN7WebCore11MemoryCache19getOriginsWithCacheERN3WTF7HashSetINS1_6RefPtrINS_14SecurityOriginEEENS_18SecurityOriginHashENS1_10HashTraitsIS5_EEEE
 __ZN7WebCore11MemoryCache25removeResourcesWithOriginEPNS_14SecurityOriginE
+__ZN7WebCore11MemoryCache11setDisabledEb
 __ZN7WebCore11RenderLayer19scrollRectToVisibleERKNS_7IntRectERKNS_15ScrollAlignmentES6_
 __ZN7WebCore11globalPointERK8_NSPointP8NSWindow
 __ZN7WebCore11memoryCacheEv
index 99cf303..632eb25 100644 (file)
@@ -182,6 +182,7 @@ public:
     CachedMetadata* cachedMetadata(unsigned dataTypeID) const;
 
     bool canDelete() const { return !hasClients() && !m_request && !m_preloadCount && !m_handleCount && !m_resourceToRevalidate && !m_proxyResource; }
+    bool hasOneHandle() const { return m_handleCount == 1; }
 
     bool isExpired() const;
 
index be19bee..777d8be 100644 (file)
@@ -86,6 +86,7 @@ static CachedResource* createResource(CachedResource::Type type, ResourceRequest
 CachedResourceLoader::CachedResourceLoader(Document* document)
     : m_document(document)
     , m_requestCount(0)
+    , m_garbageCollectDocumentResourcesTimer(this, &CachedResourceLoader::garbageCollectDocumentResourcesTimerFired)
     , m_autoLoadImages(true)
     , m_loadFinishing(false)
     , m_allowStaleResources(false)
@@ -572,6 +573,33 @@ void CachedResourceLoader::loadDone()
     if (frame())
         frame()->loader()->loadDone();
     performPostLoadActions();
+
+    if (!m_garbageCollectDocumentResourcesTimer.isActive())
+        m_garbageCollectDocumentResourcesTimer.startOneShot(0);
+}
+
+// Garbage collecting m_documentResources is a workaround for the
+// CachedResourceHandles on the RHS being strong references. Ideally this
+// would be a weak map, however CachedResourceHandles perform additional
+// bookkeeping on CachedResources, so instead pseudo-GC them -- when the
+// reference count reaches 1, m_documentResources is the only reference, so
+// remove it from the map.
+void CachedResourceLoader::garbageCollectDocumentResourcesTimerFired(Timer<CachedResourceLoader>* timer)
+{
+    ASSERT_UNUSED(timer, timer == &m_garbageCollectDocumentResourcesTimer);
+
+    typedef Vector<String, 10> StringVector;
+    StringVector resourcesToDelete;
+
+    for (DocumentResourceMap::iterator it = m_documentResources.begin(); it != m_documentResources.end(); ++it) {
+        if (it->second->hasOneHandle()) {
+            resourcesToDelete.append(it->first);
+            it->second->setOwningCachedResourceLoader(0);
+        }
+    }
+
+    for (StringVector::const_iterator it = resourcesToDelete.begin(); it != resourcesToDelete.end(); ++it)
+        m_documentResources.remove(*it);
 }
 
 void CachedResourceLoader::performPostLoadActions()
index 644f51c..7d0d2ee 100644 (file)
@@ -30,6 +30,7 @@
 #include "CachedResourceHandle.h"
 #include "CachePolicy.h"
 #include "ResourceLoadPriority.h"
+#include "Timer.h"
 #include <wtf/Deque.h>
 #include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
@@ -117,6 +118,7 @@ private:
     void notifyLoadedFromMemoryCache(CachedResource*);
     bool canRequest(CachedResource::Type, const KURL&, bool forPreload = false);
 
+    void garbageCollectDocumentResourcesTimerFired(Timer<CachedResourceLoader>*);
     void performPostLoadActions();
     
     HashSet<String> m_validatedURLs;
@@ -133,6 +135,8 @@ private:
     };
     Deque<PendingPreload> m_pendingPreloads;
 
+    Timer<CachedResourceLoader> m_garbageCollectDocumentResourcesTimer;
+
     //29 bits left
     bool m_autoLoadImages : 1;
     bool m_loadFinishing : 1;
index 6ef5ea2..6680ec5 100644 (file)
@@ -32,6 +32,7 @@
 #include "Element.h"
 #include "ExceptionCode.h"
 #include "InspectorController.h"
+#include "MemoryCache.h"
 #include "NodeRenderingContext.h"
 #include "Page.h"
 #include "RenderObject.h"
@@ -147,6 +148,11 @@ String Internals::shadowPseudoId(Element* element, ExceptionCode& ec)
     return element->shadowPseudoId().string();
 }
 
+void Internals::disableMemoryCache(bool disabled)
+{
+    WebCore::memoryCache()->setDisabled(disabled);
+}
+
 #if ENABLE(INSPECTOR)
 void Internals::setInspectorResourcesDataSizeLimits(Document* document, int maximumResourcesContentSize, int maximumSingleResourceContentSize, ExceptionCode& ec)
 {
index 9d3fc53..84fb401 100644 (file)
@@ -55,6 +55,7 @@ public:
     String shadowPseudoId(Element*, ExceptionCode&);
     PassRefPtr<Element> createShadowContentElement(Document*, ExceptionCode&);
     Element* getElementByIdInShadowRoot(Node* shadowRoot, const String& id, ExceptionCode&);
+    void disableMemoryCache(bool disabled);
 
 #if ENABLE(INSPECTOR)
     void setInspectorResourcesDataSizeLimits(Document*, int maximumResourcesContentSize, int maximumSingleResourceContentSize, ExceptionCode&);
index ff65204..088d694 100644 (file)
@@ -37,6 +37,7 @@ module window {
         DOMString shadowPseudoId(in Element element) raises (DOMException);
         Element createShadowContentElement(in Document document) raises(DOMException);
         Element getElementByIdInShadowRoot(in Node shadowRoot, in DOMString id) raises(DOMException);
+        void disableMemoryCache(in boolean disabled);
 
         void setInspectorResourcesDataSizeLimits(in Document document, in long maximumResourcesContentSize, in long maximumSingleResourceContentSize) raises(DOMException);
 
index 85ab41f..5e1a761 100644 (file)
@@ -1,3 +1,15 @@
+2011-08-01  Scott Graham  <scottmg@chromium.org>
+
+        REGRESSION (r39725?): Resources removed from document can not be freed until the document is deleted
+        https://bugs.webkit.org/show_bug.cgi?id=61006
+
+        Reviewed by Antti Koivisto.
+
+        Update exports for test harness.
+
+        * win/WebKit2.def:
+        * win/WebKit2CFLite.def:
+
 2011-08-01  Hayato Ito  <hayato@chromium.org>
 
         Add support for getting an element in shadow root by its id into a window.internals object.
index 59a301e..3ffdb7e 100644 (file)
@@ -154,8 +154,10 @@ EXPORTS
         ?getElementById@TreeScope@WebCore@@QBEPAVElement@2@ABVAtomicString@WTF@@@Z
         ?isPreloaded@CachedResourceLoader@WebCore@@QBE_NABVString@WTF@@@Z
         ?jsStringSlowCase@WebCore@@YA?AVJSValue@JSC@@PAVExecState@3@AAV?$HashMap@PAVStringImpl@WTF@@V?$Weak@VJSString@JSC@@@JSC@@UStringHash@2@U?$HashTraits@PAVStringImpl@WTF@@@2@U?$HashTraits@V?$Weak@VJSString@JSC@@@JSC@@@2@@WTF@@PAVStringImpl@6@@Z
+        ?memoryCache@WebCore@@YAPAVMemoryCache@1@XZ
         ?page@Document@WebCore@@QBEPAVPage@2@XZ
         ?removeShadowRoot@Element@WebCore@@QAEXXZ
+        ?setDisabled@MemoryCache@WebCore@@QAEX_N@Z
         ?setDOMException@WebCore@@YAXPAVExecState@JSC@@H@Z
         ?setResourcesDataSizeLimitsFromInternals@InspectorController@WebCore@@QAEXHH@Z
         ?shadowRoot@Element@WebCore@@QBEPAVShadowRoot@2@XZ
index 9dda751..b3fd56b 100644 (file)
@@ -148,8 +148,10 @@ EXPORTS
         ?toJS@WebCore@@YA?AVJSValue@JSC@@PAVExecState@3@PAVJSDOMGlobalObject@1@PAVClientRect@1@@Z
         ?updateLayoutIgnorePendingStylesheets@Document@WebCore@@QAEXXZ
         ?jsStringSlowCase@WebCore@@YA?AVJSValue@JSC@@PAVExecState@3@AAV?$HashMap@PAVStringImpl@WTF@@V?$Weak@VJSString@JSC@@@JSC@@UStringHash@2@U?$HashTraits@PAVStringImpl@WTF@@@2@U?$HashTraits@V?$Weak@VJSString@JSC@@@JSC@@@2@@WTF@@PAVStringImpl@6@@Z
+        ?memoryCache@WebCore@@YAPAVMemoryCache@1@XZ
         ?page@Document@WebCore@@QBEPAVPage@2@XZ
         ?removeShadowRoot@Element@WebCore@@QAEXXZ
+        ?setDisabled@MemoryCache@WebCore@@QAEX_N@Z
         ?setDOMException@WebCore@@YAXPAVExecState@JSC@@H@Z
         ?setResourcesDataSizeLimitsFromInternals@InspectorController@WebCore@@QAEXHH@Z
         ?shadowRoot@Element@WebCore@@QBEPAVShadowRoot@2@XZ
index f80bc3d..cbd646a 100644 (file)
@@ -35,6 +35,8 @@ _ZN7WebCore10ClientRectC1Ev;
 _ZN7WebCore10ClientRectC1ERKNS_7IntRectE;
 _ZN7WebCore11EventTarget17toGeneratedStreamEv;
 _ZN7WebCore11EventTarget8toStreamEv;
+_ZN7WebCore11MemoryCache11setDisabledEb;
+_ZN7WebCore11memoryCacheEv;
 _ZN7WebCore12JSDOMWrapper34virtualFunctionToPreventWeakVtableEv;
 _ZN7WebCore12RenderObject23absoluteBoundingBoxRectEb;
 _ZN7WebCore13createWrapperEPN3JSC9ExecStateEPNS_17JSDOMGlobalObjectEPNS_4NodeE;