Regression(r248734) StorageAreaMap objects are getting leaked
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Feb 2020 00:11:23 +0000 (00:11 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Feb 2020 00:11:23 +0000 (00:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=207073
<rdar://problem/59168065>

Reviewed by Darin Adler.

Source/WebCore:

Add test infrastructure for testing this change.

Test: http/tests/storage/storage-map-leaking.html

* storage/StorageNamespace.h:
(WebCore::StorageNamespace::storageAreaMapCountForTesting const):
* storage/StorageNamespaceProvider.h:
* testing/Internals.cpp:
(WebCore::Internals::storageAreaMapCount const):
* testing/Internals.h:
* testing/Internals.idl:

Source/WebKit:

Make sure that StorageAreaMap objects are getting removed from the HashMap
in StorageNamespaceImpl, once they no longer have any users.

* WebProcess/WebStorage/StorageAreaImpl.cpp:
(WebKit::StorageAreaImpl::StorageAreaImpl):
(WebKit::StorageAreaImpl::~StorageAreaImpl):
* WebProcess/WebStorage/StorageAreaMap.cpp:
(WebKit::StorageAreaMap::incrementUseCount):
(WebKit::StorageAreaMap::decrementUseCount):
* WebProcess/WebStorage/StorageAreaMap.h:
* WebProcess/WebStorage/StorageNamespaceImpl.cpp:
(WebKit::StorageNamespaceImpl::destroyStorageAreaMap):
(WebKit::StorageNamespaceImpl::didDestroyStorageAreaMap): Deleted.
* WebProcess/WebStorage/StorageNamespaceImpl.h:
(WebKit::StorageNamespaceImpl::storageType const): Deleted.
(WebKit::StorageNamespaceImpl::storageNamespaceID const): Deleted.
(WebKit::StorageNamespaceImpl::topLevelOrigin const): Deleted.
(WebKit::StorageNamespaceImpl::quotaInBytes const): Deleted.

LayoutTests:

Add layout test coverage.

* TestExpectations:
* http/tests/storage/resources/storage-map-leaking-iframe.html: Added.
* http/tests/storage/storage-map-leaking-expected.txt: Added.
* http/tests/storage/storage-map-leaking.html: Added.
* platform/wk2/TestExpectations:

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

18 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/http/tests/storage/resources/storage-map-leaking-iframe.html [new file with mode: 0644]
LayoutTests/http/tests/storage/storage-map-leaking-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/storage/storage-map-leaking.html [new file with mode: 0644]
LayoutTests/platform/wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/storage/StorageNamespace.h
Source/WebCore/storage/StorageNamespaceProvider.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebStorage/StorageAreaImpl.cpp
Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h
Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp
Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h

index 95a2a30..1e1b754 100644 (file)
@@ -1,3 +1,19 @@
+2020-02-05  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r248734) StorageAreaMap objects are getting leaked
+        https://bugs.webkit.org/show_bug.cgi?id=207073
+        <rdar://problem/59168065>
+
+        Reviewed by Darin Adler.
+
+        Add layout test coverage.
+
+        * TestExpectations:
+        * http/tests/storage/resources/storage-map-leaking-iframe.html: Added.
+        * http/tests/storage/storage-map-leaking-expected.txt: Added.
+        * http/tests/storage/storage-map-leaking.html: Added.
+        * platform/wk2/TestExpectations:
+
 2020-02-05  Jacob Uphoff  <jacob_uphoff@apple.com>
 
         [ macOS ] inspector/animation/lifecycle-css-animation.html is flaky failing
index f8d7434..ec66d7c 100644 (file)
@@ -827,6 +827,9 @@ fast/repaint/placeholder-after-caps-lock-hidden.html [ Skip ]
 # This test currently only works for mac-wk2
 fast/events/inactive-window-no-mouse-event.html [ Skip ]
 
+# This test is only relevant for WebKit2.
+http/tests/storage/storage-map-leaking.html [ Skip ]
+
 # The Web Share API is currently only implemented for modern WebKit on iOS and macOS
 fast/web-share [ Skip ]
 
diff --git a/LayoutTests/http/tests/storage/resources/storage-map-leaking-iframe.html b/LayoutTests/http/tests/storage/resources/storage-map-leaking-iframe.html
new file mode 100644 (file)
index 0000000..62f5a25
--- /dev/null
@@ -0,0 +1,3 @@
+<script>
+localStorage.setItem("foo", "bar");
+</script>
diff --git a/LayoutTests/http/tests/storage/storage-map-leaking-expected.txt b/LayoutTests/http/tests/storage/storage-map-leaking-expected.txt
new file mode 100644 (file)
index 0000000..a25f196
--- /dev/null
@@ -0,0 +1,10 @@
+Make sure that StorageAreaMap objects do no leak.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS StorageAreaMap objects are not leaking
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/storage/storage-map-leaking.html b/LayoutTests/http/tests/storage/storage-map-leaking.html
new file mode 100644 (file)
index 0000000..547d19a
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="/js-test-resources/js-test.js"></script>
+<script>
+description("Make sure that StorageAreaMap objects do no leak.");
+jsTestIsAsync = true;
+localStorage.setItem("foo", "bar");
+
+onload = function() {
+    initialStorageAreaMapCount = internals.storageAreaMapCount;
+
+    document.getElementById("testFrame").remove();
+
+    handle = setInterval(() => {
+        gc()
+        if (internals.storageAreaMapCount < initialStorageAreaMapCount) {
+            testPassed("StorageAreaMap objects are not leaking");
+            clearInterval(handle);
+            finishJSTest();
+        }
+    }, 10);
+}
+</script>
+<iframe id="testFrame" src="http://localhost:8000/storage/resources/storage-map-leaking-iframe.html"></iframe>
+</body>
+</html>
index 906a91d..506ce8e 100644 (file)
@@ -702,6 +702,9 @@ webkit.org/b/115274 http/tests/security/contentSecurityPolicy/report-same-origin
 fast/images/animated-gif-no-layout.html [ Pass ]
 fast/images/gif-loop-count.html [ Pass ]
 
+# Only relevant for WebKit2
+http/tests/storage/storage-map-leaking.html [ Pass ]
+
 # DumpRenderTree does not implement setWillSendRequestHTTPBody
 http/tests/misc/will-send-request-with-client-provided-http-body.html [ Pass ]
 
index 6bd7539..29c9046 100644 (file)
@@ -1,3 +1,23 @@
+2020-02-05  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r248734) StorageAreaMap objects are getting leaked
+        https://bugs.webkit.org/show_bug.cgi?id=207073
+        <rdar://problem/59168065>
+
+        Reviewed by Darin Adler.
+
+        Add test infrastructure for testing this change.
+
+        Test: http/tests/storage/storage-map-leaking.html
+
+        * storage/StorageNamespace.h:
+        (WebCore::StorageNamespace::storageAreaMapCountForTesting const):
+        * storage/StorageNamespaceProvider.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::storageAreaMapCount const):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2020-02-05  Alex Christensen  <achristensen@webkit.org>
 
         Make WKWebView._negotiatedLegacyTLS accurate when loading main resouorce from network or cache
index 354070f..71c5db5 100644 (file)
@@ -48,6 +48,8 @@ public:
 
     virtual PAL::SessionID sessionID() const = 0;
     virtual void setSessionIDForTesting(PAL::SessionID) = 0;
+
+    virtual uint64_t storageAreaMapCountForTesting() const { return 0; }
 };
 
 } // namespace WebCore
index 269ece7..0b894be 100644 (file)
@@ -58,7 +58,8 @@ protected:
     StorageNamespace* optionalLocalStorageNamespace() { return m_localStorageNamespace.get(); }
 
 private:
-    StorageNamespace& localStorageNamespace(PAL::SessionID);
+    friend class Internals;
+    WEBCORE_TESTSUPPORT_EXPORT StorageNamespace& localStorageNamespace(PAL::SessionID);
     StorageNamespace& transientLocalStorageNamespace(SecurityOrigin&, PAL::SessionID);
 
     virtual Ref<StorageNamespace> createLocalStorageNamespace(unsigned quota, PAL::SessionID) = 0;
index 5cef510..c0bd9ce 100644 (file)
 #include "SourceBuffer.h"
 #include "SpellChecker.h"
 #include "StaticNodeList.h"
+#include "StorageNamespace.h"
+#include "StorageNamespaceProvider.h"
 #include "StringCallback.h"
 #include "StyleResolver.h"
 #include "StyleRule.h"
@@ -2531,6 +2533,15 @@ bool Internals::isDocumentAlive(uint64_t documentIdentifier) const
     return Document::allDocumentsMap().contains(makeObjectIdentifier<DocumentIdentifierType>(documentIdentifier));
 }
 
+uint64_t Internals::storageAreaMapCount() const
+{
+    auto* page = contextDocument() ? contextDocument()->page() : nullptr;
+    if (!page)
+        return 0;
+
+    return page->storageNamespaceProvider().localStorageNamespace(page->sessionID()).storageAreaMapCountForTesting();
+}
+
 uint64_t Internals::elementIdentifier(Element& element) const
 {
     return element.document().identifierForElement(element).toUInt64();
index 4af131b..53175c5 100644 (file)
@@ -424,6 +424,8 @@ public:
     uint64_t documentIdentifier(const Document&) const;
     bool isDocumentAlive(uint64_t documentIdentifier) const;
 
+    uint64_t storageAreaMapCount() const;
+
     uint64_t elementIdentifier(Element&) const;
     uint64_t frameIdentifier(const Document&) const;
     uint64_t pageIdentifier(const Document&) const;
index 171e577..e9e8be3 100644 (file)
@@ -734,6 +734,8 @@ enum CompositingPolicy {
     unsigned long long documentIdentifier(Document document);
     boolean isDocumentAlive(unsigned long long documentIdentifier);
 
+    readonly attribute unsigned long long storageAreaMapCount;
+
     unsigned long long elementIdentifier(Element element);
     unsigned long long frameIdentifier(Document document);
     unsigned long long pageIdentifier(Document document);
index 49475ec..5dd655d 100644 (file)
@@ -1,3 +1,30 @@
+2020-02-05  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r248734) StorageAreaMap objects are getting leaked
+        https://bugs.webkit.org/show_bug.cgi?id=207073
+        <rdar://problem/59168065>
+
+        Reviewed by Darin Adler.
+
+        Make sure that StorageAreaMap objects are getting removed from the HashMap
+        in StorageNamespaceImpl, once they no longer have any users.
+
+        * WebProcess/WebStorage/StorageAreaImpl.cpp:
+        (WebKit::StorageAreaImpl::StorageAreaImpl):
+        (WebKit::StorageAreaImpl::~StorageAreaImpl):
+        * WebProcess/WebStorage/StorageAreaMap.cpp:
+        (WebKit::StorageAreaMap::incrementUseCount):
+        (WebKit::StorageAreaMap::decrementUseCount):
+        * WebProcess/WebStorage/StorageAreaMap.h:
+        * WebProcess/WebStorage/StorageNamespaceImpl.cpp:
+        (WebKit::StorageNamespaceImpl::destroyStorageAreaMap):
+        (WebKit::StorageNamespaceImpl::didDestroyStorageAreaMap): Deleted.
+        * WebProcess/WebStorage/StorageNamespaceImpl.h:
+        (WebKit::StorageNamespaceImpl::storageType const): Deleted.
+        (WebKit::StorageNamespaceImpl::storageNamespaceID const): Deleted.
+        (WebKit::StorageNamespaceImpl::topLevelOrigin const): Deleted.
+        (WebKit::StorageNamespaceImpl::quotaInBytes const): Deleted.
+
 2020-02-05  Per Arne Vollan  <pvollan@apple.com>
 
         [iOS] Do not create sandbox reports when the UI process cannot issue extensions to diagnostics service
index 955d562..8a547ea 100644 (file)
@@ -46,10 +46,13 @@ StorageAreaImpl::StorageAreaImpl(StorageAreaMap& storageAreaMap)
     : m_identifier(Identifier::generate())
     , m_storageAreaMap(makeWeakPtr(storageAreaMap))
 {
+    storageAreaMap.incrementUseCount();
 }
 
 StorageAreaImpl::~StorageAreaImpl()
 {
+    if (m_storageAreaMap)
+        m_storageAreaMap->decrementUseCount();
 }
 
 unsigned StorageAreaImpl::length()
index bdacb3a..4687e03 100644 (file)
@@ -364,4 +364,15 @@ void StorageAreaMap::disconnect()
     m_mapID = { };
 }
 
+void StorageAreaMap::incrementUseCount()
+{
+    ++m_useCount;
+}
+
+void StorageAreaMap::decrementUseCount()
+{
+    if (!--m_useCount)
+        m_namespace.destroyStorageAreaMap(*this);
+}
+
 } // namespace WebKit
index 7c81219..c0bd375 100644 (file)
@@ -70,6 +70,9 @@ public:
 
     void disconnect();
 
+    void incrementUseCount();
+    void decrementUseCount();
+
 private:
     void didSetItem(uint64_t mapSeed, const String& key, bool quotaError);
     void didRemoveItem(uint64_t mapSeed, const String& key);
@@ -97,6 +100,7 @@ private:
     uint64_t m_currentSeed { 0 };
     unsigned m_quotaInBytes;
     WebCore::StorageType m_type;
+    uint64_t m_useCount { 0 };
     bool m_hasPendingClear { false };
 };
 
index fd0f122..391383d 100644 (file)
@@ -95,7 +95,7 @@ PAL::SessionID StorageNamespaceImpl::sessionID() const
     return WebProcess::singleton().sessionID();
 }
 
-void StorageNamespaceImpl::didDestroyStorageAreaMap(StorageAreaMap& map)
+void StorageNamespaceImpl::destroyStorageAreaMap(StorageAreaMap& map)
 {
     m_storageAreaMaps.remove(map.securityOrigin().data());
 }
index 7fb00d2..c6f9f8a 100644 (file)
@@ -40,7 +40,7 @@ namespace WebKit {
 class StorageAreaMap;
 class WebPage;
 
-class StorageNamespaceImpl : public WebCore::StorageNamespace {
+class StorageNamespaceImpl final : public WebCore::StorageNamespace {
 public:
     using Identifier = StorageNamespaceIdentifier;
 
@@ -58,7 +58,7 @@ public:
     unsigned quotaInBytes() const { return m_quotaInBytes; }
     PAL::SessionID sessionID() const override;
 
-    void didDestroyStorageAreaMap(StorageAreaMap&);
+    void destroyStorageAreaMap(StorageAreaMap&);
 
     void setSessionIDForTesting(PAL::SessionID) override;
 
@@ -66,6 +66,7 @@ private:
     StorageNamespaceImpl(WebCore::StorageType, Identifier, const Optional<WebCore::PageIdentifier>&, WebCore::SecurityOrigin* topLevelOrigin, unsigned quotaInBytes);
 
     Ref<WebCore::StorageArea> storageArea(const WebCore::SecurityOriginData&) override;
+    uint64_t storageAreaMapCountForTesting() const final { return m_storageAreaMaps.size(); }
 
     // FIXME: This is only valid for session storage and should probably be moved to a subclass.
     Ref<WebCore::StorageNamespace> copy(WebCore::Page&) override;