Attachment 362504 Details for Bug 194709 – Patch

[patch] Patch

bug-194709-20190220093335.patch (text/plain), 11.74 KB, created by Sihui Liu on 2019-02-20 09:33:36 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Sihui Liu

Created: 2019-02-20 09:33:36 PST

Size: 11.74 KB

patch

obsolete

>Subversion Revision: 241761
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index a6da53fe8e04ad79dd06c117c410a040b2b97e76..5f2d6a1a72972fcbac7a1e97c60744884e7d2e65 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2019-02-20  Sihui Liu  <sihui_liu@apple.com>
>+
>+        IndexedDB: IDBDatabase and IDBTransaction are leaked in layout tests
>+        https://bugs.webkit.org/show_bug.cgi?id=194709
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When connection to IDB server is closed, IDBTransaction would abort without notifying IDBDatabase, so 
>+        IDBDatabase didn't clear its reference to IDBTransaction which created a reference cycle. 
>+
>+        Also IDBTransaction didn't clear its reference to IDBRequest in this case and it led to another reference cycle
>+        between IDBOpenDBRequest and IDBTransaction.
>+
>+        Test: storage/indexeddb/IDBObject-leak.html
>+
>+        * Modules/indexeddb/IDBDatabase.cpp:
>+        (WebCore::IDBDatabase::connectionToServerLost):
>+        * Modules/indexeddb/IDBTransaction.cpp:
>+        (WebCore::IDBTransaction::IDBTransaction):
>+        (WebCore::IDBTransaction::~IDBTransaction):
>+        (WebCore::IDBTransaction::connectionClosedFromServer):
>+        * Modules/indexeddb/IDBTransaction.h:
>+        * testing/Internals.cpp:
>+        (WebCore::Internals::numberOfIDBTransactions const):
>+        * testing/Internals.h:
>+        * testing/Internals.idl:
>+
> 2019-02-19  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r241722.
>diff --git a/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp b/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp
>index 1847439190a8b79bef706432fd2f9d7556907365..0a095725fd2fbd324b957118924149eba26364e7 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp
>@@ -264,7 +264,8 @@ void IDBDatabase::connectionToServerLost(const IDBError& error)
>     m_closePending = true;
>     m_closedInServer = true;
> 
>-    for (auto& transaction : m_activeTransactions.values())
>+    auto transactions = copyToVector(m_activeTransactions.values());
>+    for (auto& transaction : transactions)
>         transaction->connectionClosedFromServer(error);
> 
>     auto errorEvent = Event::create(m_eventNames.errorEvent, Event::CanBubble::Yes, Event::IsCancelable::No);
>diff --git a/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp b/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
>index 2e965e97d2909ec0c70f3f8741a188ff6c549b2f..d384bd3316de2540839ea6d1910a2185bde01b4a 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
>@@ -81,6 +81,13 @@ IDBTransaction::IDBTransaction(IDBDatabase& database, const IDBTransactionInfo&
> {
>     LOG(IndexedDB, "IDBTransaction::IDBTransaction - %s", m_info.loggingString().utf8().data());
>     ASSERT(&m_database->originThread() == &Thread::current());
>+    
>+    {
>+        LockHolder locker(m_countMutex);
>+        auto addResult = allIDBTransactions().add(this);
>+        ASSERT_UNUSED(addResult, addResult.isNewEntry);
>+    }
>+
> 
>     if (m_info.mode() == IDBTransactionMode::Versionchange) {
>         ASSERT(m_openDBRequest);
>@@ -105,9 +112,20 @@ IDBTransaction::IDBTransaction(IDBDatabase& database, const IDBTransactionInfo&
> 
> IDBTransaction::~IDBTransaction()
> {
>+    {
>+        LockHolder locker(m_countMutex);
>+        ASSERT(allIDBTransactions().contains(this));
>+        allIDBTransactions().remove(this);
>+    }
>     ASSERT(&m_database->originThread() == &Thread::current());
> }
> 
>+HashSet<IDBTransaction*>& IDBTransaction::allIDBTransactions()
>+{
>+    static NeverDestroyed<HashSet<IDBTransaction*>> transactions;
>+    return transactions;
>+}
>+
> IDBClient::IDBConnectionProxy& IDBTransaction::connectionProxy()
> {
>     return m_database->connectionProxy();
>@@ -1434,7 +1452,8 @@ void IDBTransaction::connectionClosedFromServer(const IDBError& error)
> {
>     LOG(IndexedDB, "IDBTransaction::connectionClosedFromServer - %s", error.message().utf8().data());
> 
>-    m_state = IndexedDB::TransactionState::Aborting;
>+    m_database->willAbortTransaction(*this);
>+    transitionedToFinishing(IndexedDB::TransactionState::Aborting);
> 
>     abortInProgressOperations(error);
> 
>@@ -1445,6 +1464,7 @@ void IDBTransaction::connectionClosedFromServer(const IDBError& error)
>         ASSERT(m_transactionOperationsInProgressQueue.first() == operation.get());
>         operation->doComplete(IDBResultData::error(operation->identifier(), error));
>     }
>+    m_currentlyCompletingRequest = nullptr;
> 
>     connectionProxy().forgetActiveOperations(operations);
> 
>@@ -1454,6 +1474,7 @@ void IDBTransaction::connectionClosedFromServer(const IDBError& error)
> 
>     m_idbError = error;
>     m_domError = error.toDOMException();
>+    m_database->didAbortTransaction(*this);
>     fireOnAbort();
> }
> 
>diff --git a/Source/WebCore/Modules/indexeddb/IDBTransaction.h b/Source/WebCore/Modules/indexeddb/IDBTransaction.h
>index 722f12c84a787dc1a32a33bc5d236f0cecdc4f04..7ac79ae599be68e6e3b09105526cd73650057c8b 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBTransaction.h
>+++ b/Source/WebCore/Modules/indexeddb/IDBTransaction.h
>@@ -152,6 +152,8 @@ public:
> 
>     void visitReferencedObjectStores(JSC::SlotVisitor&) const;
> 
>+    WEBCORE_EXPORT static HashSet<IDBTransaction*>& allIDBTransactions();
>+
> private:
>     IDBTransaction(IDBDatabase&, const IDBTransactionInfo&, IDBOpenDBRequest*);
> 
>@@ -258,6 +260,8 @@ private:
>     RefPtr<IDBRequest> m_currentlyCompletingRequest;
> 
>     bool m_contextStopped { false };
>+
>+    Lock m_countMutex;
> };
> 
> class TransactionActivator {
>diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index b06a1f83c37c84b6271d09d29a2532bd77d4cc60..c30d5e1dd29c7a93100e7ec37939715c9e1c4c4b 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -93,6 +93,8 @@
> #include "HistoryController.h"
> #include "HistoryItem.h"
> #include "HitTestResult.h"
>+#include "IDBRequest.h"
>+#include "IDBTransaction.h"
> #include "InspectorClient.h"
> #include "InspectorController.h"
> #include "InspectorFrontendClientLocal.h"
>@@ -2383,6 +2385,11 @@ ExceptionOr<unsigned> Internals::countFindMatches(const String& text, const Vect
>     return document->page()->countFindMatches(text, parsedOptions.releaseReturnValue(), 1000);
> }
> 
>+unsigned Internals::numberOfIDBTransactions() const
>+{
>+    return IDBTransaction::allIDBTransactions().size();
>+}
>+
> unsigned Internals::numberOfLiveNodes() const
> {
>     unsigned nodeCount = 0;
>diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
>index c19382f1639c5dadff3dd36bab16d71a2369ecb3..4e4d9d74a774658c8d086c6ec6c8341a07b86bd2 100644
>--- a/Source/WebCore/testing/Internals.h
>+++ b/Source/WebCore/testing/Internals.h
>@@ -378,6 +378,8 @@ public:
>     ExceptionOr<void> insertAuthorCSS(const String&) const;
>     ExceptionOr<void> insertUserCSS(const String&) const;
> 
>+    unsigned numberOfIDBTransactions() const;
>+
>     unsigned numberOfLiveNodes() const;
>     unsigned numberOfLiveDocuments() const;
>     unsigned referencingNodeCount(const Document&) const;
>diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
>index e638512de819c0c105b094b3549fa51cb0982e33..01399ec80ade258a1a19dfd3c6ff6db7697ef8ed 100644
>--- a/Source/WebCore/testing/Internals.idl
>+++ b/Source/WebCore/testing/Internals.idl
>@@ -405,6 +405,8 @@ enum CompositingPolicy {
>     void beginSimulatedMemoryPressure();
>     void endSimulatedMemoryPressure();
> 
>+    unsigned long numberOfIDBTransactions();
>+
>     unsigned long numberOfLiveNodes();
>     unsigned long numberOfLiveDocuments();
>     unsigned long referencingNodeCount(Document document);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index e26eaf6e984904ac4398e158332c2c689e209500..bc262c8e95bf7aa0114668a458f9a9e6349c5a3e 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-02-20  Sihui Liu  <sihui_liu@apple.com>
>+
>+        IndexedDB: IDBDatabase and IDBTransaction are leaked in layout tests
>+        https://bugs.webkit.org/show_bug.cgi?id=194709
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestExpectations:
>+        * platform/wk2/TestExpectations:
>+        * storage/indexeddb/IDBObject-leak-expected.txt: Added.
>+        * storage/indexeddb/IDBObject-leak.html: Added.
>+
> 2019-02-19  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r241722.
>diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
>index c2c19fb1153ea659530df86108bd5fff7d9cb26a..b7c46238f0d531d73abebcba5aae5d24e94f1d47 100644
>--- a/LayoutTests/TestExpectations
>+++ b/LayoutTests/TestExpectations
>@@ -130,6 +130,7 @@ http/tests/storageAccess/ [ Skip ]
> http/tests/navigation/process-swap-window-open.html [ Skip ]
> http/tests/navigation/useragent-reload.php [ Skip ]
> storage/indexeddb/modern/opendatabase-after-storage-crash.html [ Skip ]
>+storage/indexeddb/IDBObject-leak.html [ Skip ]
> fast/forms/call-text-did-change-in-text-field-when-typing.html [ Skip ]
> 
> # Only Mac and iOS have an implementation of UIScriptController::doAsyncTask().
>diff --git a/LayoutTests/platform/wk2/TestExpectations b/LayoutTests/platform/wk2/TestExpectations
>index 42d447e536b1573e903040a69a385f793a48400a..c40ff4402a061abaf6a840722556e7256f766e17 100644
>--- a/LayoutTests/platform/wk2/TestExpectations
>+++ b/LayoutTests/platform/wk2/TestExpectations
>@@ -746,6 +746,7 @@ http/wpt/cross-origin-resource-policy/ [ Pass ]
> 
> http/tests/navigation/useragent-reload.php [ Pass ]
> storage/indexeddb/modern/opendatabase-after-storage-crash.html [ Pass ]
>+storage/indexeddb/IDBObject-leak.html [ Pass ]
> 
> fast/forms/call-text-did-change-in-text-field-when-typing.html [ Pass ]
> 
>diff --git a/LayoutTests/storage/indexeddb/IDBObject-leak-expected.txt b/LayoutTests/storage/indexeddb/IDBObject-leak-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..b286595a5bdb0f7b0982880b1f9f3170420539c8
>--- /dev/null
>+++ b/LayoutTests/storage/indexeddb/IDBObject-leak-expected.txt
>@@ -0,0 +1,10 @@
>+This test verifies that IDBTransaction objects are freed.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS internals.numberOfIDBTransactions() is 0
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/storage/indexeddb/IDBObject-leak.html b/LayoutTests/storage/indexeddb/IDBObject-leak.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..742838da689c235545c2ce1757cefe8fe57a5506
>--- /dev/null
>+++ b/LayoutTests/storage/indexeddb/IDBObject-leak.html
>@@ -0,0 +1,39 @@
>+<!DOCTYPE html>
>+<script src="../../resources/js-test.js"></script>
>+<script src="resources/shared.js"></script>
>+<script>
>+description('This test verifies that IDBTransaction objects are freed.');
>+
>+function test() {
>+    if (!window.internals || !internals.numberOfIDBTransactions) {
>+        testFailed('This test requires access to the Internals object');
>+        finishJSTest();
>+        return;
>+    }
>+
>+    if (sessionStorage.doneFirstLoad) {
>+        gc();
>+        shouldBeEqualToNumber("internals.numberOfIDBTransactions()", 0);
>+        finishJSTest();
>+        return;
>+    }
>+
>+    var dbname = setDBNameFromPath() + Date();
>+    var request =  window.indexedDB.open(dbname);
>+    request.onupgradeneeded = function(evt) {
>+        sessionStorage.doneFirstLoad = true;
>+        if (!window.testRunner || !testRunner.terminateNetworkProcess) {
>+            testFailed('This test requires access to the TestRunner object and terminateNetworkProcess() function');
>+            finishJSTest();
>+            return;
>+        } 
>+        testRunner.waitUntilDone();
>+        testRunner.terminateNetworkProcess();
>+        setTimeout((()=> {
>+            location.reload();
>+        }), 1);
>+    }
>+}
>+
>+test();
>+</script>
>\ No newline at end of file

Actions: View | Formatted Diff | Diff

Attachments on bug 194709: 362122 | 362196 | 362300 | 362332 | 362346 | 362353 | 362356 | 362504 | 362509 | 362516 | 362517 | 362773