Attachment 362367 Details for Bug 194806 – Patch

[patch] Patch

bug-194806-20190218210249.patch (text/plain), 16.66 KB, created by Sihui Liu on 2019-02-18 21:02:50 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Sihui Liu

Created: 2019-02-18 21:02:50 PST

Size: 16.66 KB

patch

obsolete

>Subversion Revision: 241722
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 686a28f59246d67bd5917179efb4a3f25d24ff10..231fef79e39d5beeb98b94ac0765f061c6e127fb 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2019-02-18  Sihui Liu  <sihui_liu@apple.com>
>+
>+        IndexedDB: re-enable some leak tests
>+        https://bugs.webkit.org/show_bug.cgi?id=194806
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Cleaned up some code and added a cache for JSIDBCursor to ensure we get the same object if we advance cursor successfully.
>+
>+        Covered by the re-enabled tests.
>+
>+        * Modules/indexeddb/IDBCursor.cpp:
>+        (WebCore::IDBCursor::setGetResult):
>+        * Modules/indexeddb/IDBCursor.h:
>+        * Modules/indexeddb/IDBRequest.cpp:
>+        (WebCore::IDBRequest::setResult):
>+        (WebCore::IDBRequest::setResultToStructuredClone):
>+        (WebCore::IDBRequest::willIterateCursor):
>+        (WebCore::IDBRequest::didOpenOrIterateCursor):
>+        * Modules/indexeddb/IDBRequest.h:
>+        (WebCore::IDBRequest::cursorWrapper):
>+        * bindings/js/JSIDBRequestCustom.cpp:
>+        (WebCore::JSIDBRequest::visitAdditionalChildren):
>+
> 2019-02-18  Sihui Liu  <sihui_liu@apple.com>
> 
>         IndexedDB: leak IDBDatabase and IDBTransacstion in layout tests
>diff --git a/Source/WebCore/Modules/indexeddb/IDBCursor.cpp b/Source/WebCore/Modules/indexeddb/IDBCursor.cpp
>index 253a3b158bf5041af1aeb865011ecfbcf206f3cc..fe2479f5e3043fe9871eb40f1e8dd3564044044e 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBCursor.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBCursor.cpp
>@@ -309,7 +309,7 @@ ExceptionOr<Ref<WebCore::IDBRequest>> IDBCursor::deleteFunction(ExecState& state
>     return WTFMove(request);
> }
> 
>-void IDBCursor::setGetResult(IDBRequest&, const IDBGetResult& getResult)
>+bool IDBCursor::setGetResult(IDBRequest&, const IDBGetResult& getResult)
> {
>     LOG(IndexedDB, "IDBCursor::setGetResult - current key %s", getResult.keyData().loggingString().substring(0, 100).utf8().data());
>     ASSERT(&effectiveObjectStore().transaction().database().originThread() == &Thread::current());
>@@ -326,7 +326,7 @@ void IDBCursor::setGetResult(IDBRequest&, const IDBGetResult& getResult)
>         m_value = { };
> 
>         m_gotValue = false;
>-        return;
>+        return false;
>     }
> 
>     m_keyData = getResult.keyData();
>@@ -338,6 +338,7 @@ void IDBCursor::setGetResult(IDBRequest&, const IDBGetResult& getResult)
>         m_value = getResult.value();
> 
>     m_gotValue = true;
>+    return true;
> }
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/Modules/indexeddb/IDBCursor.h b/Source/WebCore/Modules/indexeddb/IDBCursor.h
>index 03e6b1eb24fe414df7a7af602055a3c44e753a93..0a378bde940b4514ac9b2a980007bd7ca9ddecc1 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBCursor.h
>+++ b/Source/WebCore/Modules/indexeddb/IDBCursor.h
>@@ -76,7 +76,7 @@ public:
>     void clearRequest() { m_request.clear(); }
>     IDBRequest* request() { return m_request.get(); }
> 
>-    void setGetResult(IDBRequest&, const IDBGetResult&);
>+    bool setGetResult(IDBRequest&, const IDBGetResult&);
> 
>     virtual bool isKeyCursorWithValue() const { return false; }
> 
>diff --git a/Source/WebCore/Modules/indexeddb/IDBRequest.cpp b/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
>index cec7c9a4f898628eff168bc7fa54b0799c18c013..c5559ce2dcb3dfebd29dc93a0b87a71a48f7fb6c 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
>@@ -354,16 +354,6 @@ void IDBRequest::setResult(const IDBKeyData& keyData)
> {
>     ASSERT(&originThread() == &Thread::current());
> 
>-    auto* context = scriptExecutionContext();
>-    if (!context)
>-        return;
>-
>-    auto* state = context->execState();
>-    if (!state)
>-        return;
>-
>-    VM& vm = context->vm();
>-    JSLockHolder lock(vm);
>     m_result = keyData;
>     m_resultWrapper = { };
> }
>@@ -372,16 +362,6 @@ void IDBRequest::setResult(const Vector<IDBKeyData>& keyDatas)
> {
>     ASSERT(&originThread() == &Thread::current());
> 
>-    auto* context = scriptExecutionContext();
>-    if (!context)
>-        return;
>-
>-    auto* state = context->execState();
>-    if (!state)
>-        return;
>-
>-    VM& vm = context->vm();
>-    JSLockHolder lock(vm);
>     m_result = keyDatas;
>     m_resultWrapper = { };
> }
>@@ -390,16 +370,6 @@ void IDBRequest::setResult(const Vector<IDBValue>& values)
> {
>     ASSERT(&originThread() == &Thread::current());
> 
>-    auto* context = scriptExecutionContext();
>-    if (!context)
>-        return;
>-
>-    auto* state = context->execState();
>-    if (!state)
>-        return;
>-
>-    VM& vm = context->vm();
>-    JSLockHolder lock(vm);
>     m_result = values;
>     m_resultWrapper = { };
> }
>@@ -408,10 +378,6 @@ void IDBRequest::setResult(uint64_t number)
> {
>     ASSERT(&originThread() == &Thread::current());
> 
>-    auto* context = scriptExecutionContext();
>-    if (!context)
>-        return;
>-
>     m_result = number;
>     m_resultWrapper = { };
> }
>@@ -422,16 +388,6 @@ void IDBRequest::setResultToStructuredClone(const IDBValue& value)
> 
>     LOG(IndexedDB, "IDBRequest::setResultToStructuredClone");
> 
>-    auto* context = scriptExecutionContext();
>-    if (!context)
>-        return;
>-
>-    auto* state = context->execState();
>-    if (!state)
>-        return;
>-
>-    VM& vm = context->vm();
>-    JSLockHolder lock(vm);
>     m_result = value;
>     m_resultWrapper = { };
> }
>@@ -466,6 +422,10 @@ void IDBRequest::willIterateCursor(IDBCursor& cursor)
>     m_pendingCursor = &cursor;
>     m_hasPendingActivity = true;
>     m_result = NullResultType::Empty;
>+    if (m_resultWrapper) {
>+        JSC::JSValue value = m_resultWrapper;
>+        m_cursorWrapper = value;
>+    }
>     m_resultWrapper = { };
>     m_readyState = ReadyState::Pending;
>     m_domError = nullptr;
>@@ -481,7 +441,10 @@ void IDBRequest::didOpenOrIterateCursor(const IDBResultData& resultData)
>     m_resultWrapper = { };
> 
>     if (resultData.type() == IDBResultType::IterateCursorSuccess || resultData.type() == IDBResultType::OpenCursorSuccess) {
>-        m_pendingCursor->setGetResult(*this, resultData.getResult());
>+        if (m_pendingCursor->setGetResult(*this, resultData.getResult()) && m_cursorWrapper) {
>+            JSC::JSValue value = m_cursorWrapper;
>+            m_resultWrapper = value;
>+        }
>         if (resultData.getResult().isDefined())
>             m_result = m_pendingCursor;
>     }
>diff --git a/Source/WebCore/Modules/indexeddb/IDBRequest.h b/Source/WebCore/Modules/indexeddb/IDBRequest.h
>index 613512b67f756cc1d708e0bdebcb26cef744f630..b37a249e0ee48e5ddf6f143962a0dde45b0db144 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBRequest.h
>+++ b/Source/WebCore/Modules/indexeddb/IDBRequest.h
>@@ -78,6 +78,7 @@ public:
>     using Result = Variant<RefPtr<IDBCursor>, RefPtr<IDBDatabase>, IDBKeyData, Vector<IDBKeyData>, IDBValue, Vector<IDBValue>, uint64_t, NullResultType>;
>     ExceptionOr<Result> result() const;
>     JSValueInWrappedObject& resultWrapper() { return m_resultWrapper; }
>+    JSValueInWrappedObject& cursorWrapper() { return m_cursorWrapper; }
> 
>     using Source = Variant<RefPtr<IDBObjectStore>, RefPtr<IDBIndex>, RefPtr<IDBCursor>>;
>     const Optional<Source>& source() const { return m_source; }
>@@ -171,6 +172,7 @@ private:
>     IDBResourceIdentifier m_resourceIdentifier;
> 
>     JSValueInWrappedObject m_resultWrapper;
>+    JSValueInWrappedObject m_cursorWrapper;
>     Result m_result;
>     Optional<Source> m_source;
> 
>diff --git a/Source/WebCore/bindings/js/JSIDBRequestCustom.cpp b/Source/WebCore/bindings/js/JSIDBRequestCustom.cpp
>index 2e710a1609ad5d0d5353888cc0a9c575df2108a5..5ebb7ccc8e437ed36d9c320f17000317f2ce4eeb 100644
>--- a/Source/WebCore/bindings/js/JSIDBRequestCustom.cpp
>+++ b/Source/WebCore/bindings/js/JSIDBRequestCustom.cpp
>@@ -75,6 +75,7 @@ void JSIDBRequest::visitAdditionalChildren(SlotVisitor& visitor)
> {
>     auto& request = wrapped();
>     request.resultWrapper().visit(visitor);
>+    request.cursorWrapper().visit(visitor);
> }
> 
> }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 2c4da96c8459cd584cbfe94f62dc767b02e61bb6..da46a87bcf7b03ad01192db053072f8374746c47 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,23 @@
>+2019-02-18  Sihui Liu  <sihui_liu@apple.com>
>+
>+        IndexedDB: re-enable some leak tests
>+        https://bugs.webkit.org/show_bug.cgi?id=194806
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We have support for internals.observeGC now.
>+
>+        * TestExpectations:
>+        * storage/indexeddb/connection-leak-expected.txt:
>+        * storage/indexeddb/connection-leak-private-expected.txt:
>+        * storage/indexeddb/cursor-leak-expected.txt:
>+        * storage/indexeddb/cursor-leak-private-expected.txt:
>+        * storage/indexeddb/cursor-request-cycle-expected.txt:
>+        * storage/indexeddb/cursor-request-cycle-private-expected.txt:
>+        * storage/indexeddb/request-leak-expected.txt:
>+        * storage/indexeddb/request-leak-private-expected.txt:
>+        * storage/indexeddb/resources/cursor-request-cycle.js:
>+
> 2019-02-18  Sihui Liu  <sihui_liu@apple.com>
> 
>         IndexedDB: leak IDBDatabase and IDBTransacstion in layout tests
>diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
>index c2c19fb1153ea659530df86108bd5fff7d9cb26a..bb77d16f32646199d04d94790486809edee84e99 100644
>--- a/LayoutTests/TestExpectations
>+++ b/LayoutTests/TestExpectations
>@@ -1429,18 +1429,6 @@ fast/history/page-cache-indexed-closed-db.html [ Failure ]
> # With Modern IDB and the in-memory backing store, that should change.
> storage/indexeddb/open-db-private-browsing.html [ Failure ]
> 
>-# Relies on internals.observeGC
>-storage/indexeddb/connection-leak-private.html [ Skip ]
>-storage/indexeddb/connection-leak.html [ Skip ]
>-storage/indexeddb/cursor-leak-private.html [ Failure ]
>-storage/indexeddb/cursor-leak.html [ Skip ]
>-storage/indexeddb/cursor-request-cycle-private.html [ Failure ]
>-storage/indexeddb/cursor-request-cycle.html [ Skip ]
>-storage/indexeddb/delete-closed-database-object-private.html [ Skip ]
>-storage/indexeddb/delete-closed-database-object.html [ Skip ]
>-storage/indexeddb/request-leak-private.html [ Failure ]
>-storage/indexeddb/request-leak.html [ Failure ]
>-
> webkit.org/b/154619 storage/indexeddb/odd-strings.html [ Skip ]
> 
> # IDB workers test fails - The worker's attempt to open the database creates a new UniqueIDBDatabase
>diff --git a/LayoutTests/storage/indexeddb/connection-leak-expected.txt b/LayoutTests/storage/indexeddb/connection-leak-expected.txt
>index 9dd0cfa1d67cb2f886e67c9ebad074da162a429f..94c9186614b3ca388b8e2dfc4b2f0c5dcf9823e7 100644
>--- a/LayoutTests/storage/indexeddb/connection-leak-expected.txt
>+++ b/LayoutTests/storage/indexeddb/connection-leak-expected.txt
>@@ -3,7 +3,6 @@ Regression test to ensure that IndexedDB connections don't leak
> On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> 
>-dbname = "connection-leak.html"
> 
> doFirstOpen():
> request = indexedDB.open(dbname, 1)
>diff --git a/LayoutTests/storage/indexeddb/connection-leak-private-expected.txt b/LayoutTests/storage/indexeddb/connection-leak-private-expected.txt
>index 9dd0cfa1d67cb2f886e67c9ebad074da162a429f..94c9186614b3ca388b8e2dfc4b2f0c5dcf9823e7 100644
>--- a/LayoutTests/storage/indexeddb/connection-leak-private-expected.txt
>+++ b/LayoutTests/storage/indexeddb/connection-leak-private-expected.txt
>@@ -3,7 +3,6 @@ Regression test to ensure that IndexedDB connections don't leak
> On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> 
>-dbname = "connection-leak.html"
> 
> doFirstOpen():
> request = indexedDB.open(dbname, 1)
>diff --git a/LayoutTests/storage/indexeddb/cursor-leak-expected.txt b/LayoutTests/storage/indexeddb/cursor-leak-expected.txt
>index 0790c522e7d3dbff8ede8b4ae8b7f431020ac313..aeea70dc943d6497354555e96e80099c17493596 100644
>--- a/LayoutTests/storage/indexeddb/cursor-leak-expected.txt
>+++ b/LayoutTests/storage/indexeddb/cursor-leak-expected.txt
>@@ -5,7 +5,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 
> indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> 
>-dbname = "cursor-leak.html"
> indexedDB.deleteDatabase(dbname)
> indexedDB.open(dbname)
> PASS cursorObserver.wasCollected is true
>diff --git a/LayoutTests/storage/indexeddb/cursor-leak-private-expected.txt b/LayoutTests/storage/indexeddb/cursor-leak-private-expected.txt
>index 0790c522e7d3dbff8ede8b4ae8b7f431020ac313..aeea70dc943d6497354555e96e80099c17493596 100644
>--- a/LayoutTests/storage/indexeddb/cursor-leak-private-expected.txt
>+++ b/LayoutTests/storage/indexeddb/cursor-leak-private-expected.txt
>@@ -5,7 +5,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 
> indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> 
>-dbname = "cursor-leak.html"
> indexedDB.deleteDatabase(dbname)
> indexedDB.open(dbname)
> PASS cursorObserver.wasCollected is true
>diff --git a/LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt b/LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt
>index 99c720509d78e7d4d2f3d2dd34b723426086bebe..6184653dba8c8956dd8ce1c88648aee6cc11382d 100644
>--- a/LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt
>+++ b/LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt
>@@ -5,7 +5,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 
> indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> 
>-dbname = "cursor-request-cycle.html"
> indexedDB.deleteDatabase(dbname)
> indexedDB.open(dbname)
> 
>@@ -40,6 +39,7 @@ cursor.continue()
> cursor = null
> gc()
> PASS cursorObservation.wasCollected is false
>+PASS cursorRequestObservation.wasCollected is false
> finalRequest = store.get(0)
> 
> cursorContinueSuccess():
>diff --git a/LayoutTests/storage/indexeddb/cursor-request-cycle-private-expected.txt b/LayoutTests/storage/indexeddb/cursor-request-cycle-private-expected.txt
>index 99c720509d78e7d4d2f3d2dd34b723426086bebe..6184653dba8c8956dd8ce1c88648aee6cc11382d 100644
>--- a/LayoutTests/storage/indexeddb/cursor-request-cycle-private-expected.txt
>+++ b/LayoutTests/storage/indexeddb/cursor-request-cycle-private-expected.txt
>@@ -5,7 +5,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 
> indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> 
>-dbname = "cursor-request-cycle.html"
> indexedDB.deleteDatabase(dbname)
> indexedDB.open(dbname)
> 
>@@ -40,6 +39,7 @@ cursor.continue()
> cursor = null
> gc()
> PASS cursorObservation.wasCollected is false
>+PASS cursorRequestObservation.wasCollected is false
> finalRequest = store.get(0)
> 
> cursorContinueSuccess():
>diff --git a/LayoutTests/storage/indexeddb/request-leak-expected.txt b/LayoutTests/storage/indexeddb/request-leak-expected.txt
>index e1f164f4a95df03735ddc52e5a03fa91c4b4a9a8..b0386e160adc88a812d74e96b708538e1495f02e 100644
>--- a/LayoutTests/storage/indexeddb/request-leak-expected.txt
>+++ b/LayoutTests/storage/indexeddb/request-leak-expected.txt
>@@ -5,7 +5,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 
> indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> 
>-dbname = "request-leak.html"
> indexedDB.deleteDatabase(dbname)
> indexedDB.open(dbname)
> 
>diff --git a/LayoutTests/storage/indexeddb/request-leak-private-expected.txt b/LayoutTests/storage/indexeddb/request-leak-private-expected.txt
>index e1f164f4a95df03735ddc52e5a03fa91c4b4a9a8..b0386e160adc88a812d74e96b708538e1495f02e 100644
>--- a/LayoutTests/storage/indexeddb/request-leak-private-expected.txt
>+++ b/LayoutTests/storage/indexeddb/request-leak-private-expected.txt
>@@ -5,7 +5,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 
> indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> 
>-dbname = "request-leak.html"
> indexedDB.deleteDatabase(dbname)
> indexedDB.open(dbname)
> 
>diff --git a/LayoutTests/storage/indexeddb/resources/cursor-request-cycle.js b/LayoutTests/storage/indexeddb/resources/cursor-request-cycle.js
>index bbbec8c773efac2db13844b75f510646cbc1d7c6..646ce1e7b0a0d074d897b23a3a112e7d0cfa8357 100644
>--- a/LayoutTests/storage/indexeddb/resources/cursor-request-cycle.js
>+++ b/LayoutTests/storage/indexeddb/resources/cursor-request-cycle.js
>@@ -66,6 +66,7 @@ function onOpen(evt)
>         evalAndLog("cursor = null");
>         evalAndLog("gc()");
>         shouldBeFalse("cursorObservation.wasCollected");
>+        shouldBeFalse("cursorRequestObservation.wasCollected");
> 
>         evalAndLog("finalRequest = store.get(0)");
>         finalRequest.onsuccess = function finalRequestSuccess(evt) {

Actions: View | Formatted Diff | Diff

Attachments on bug 194806: 362367 | 362368 | 362402 | 363946 | 363955 | 363959 | 363964 | 363992 | 363996 | 363999 | 364000 | 364002 | 364018 | 364294 | 364667 | 364749