IndexedDB: re-enable some leak tests
authorsihui_liu@apple.com <sihui_liu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Mar 2019 02:24:55 +0000 (02:24 +0000)
committersihui_liu@apple.com <sihui_liu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Mar 2019 02:24:55 +0000 (02:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194806

Reviewed by Geoffrey Garen.

Source/WebCore:

Protected JSIDBCursor object when advance/continue request on IDBCursor is not finished, because after the
advance operation completes on success, we need to return the same JSIDBCursor object as before the advance,
and during the wait for advance operation to complete, we need to return error as the result.

Covered by existing tests.

* Modules/indexeddb/IDBCursor.cpp:
(WebCore::IDBCursor::setGetResult):
(WebCore::IDBCursor::clearWrappers):
* Modules/indexeddb/IDBCursor.h:
* Modules/indexeddb/IDBRequest.cpp:
(WebCore::IDBRequest::stop):
(WebCore::IDBRequest::setResult):
(WebCore::IDBRequest::setResultToStructuredClone):
(WebCore::IDBRequest::setResultToUndefined):
(WebCore::IDBRequest::willIterateCursor):
(WebCore::IDBRequest::didOpenOrIterateCursor):
(WebCore::IDBRequest::clearWrappers):
* Modules/indexeddb/IDBRequest.h:
(WebCore::IDBRequest::cursorWrapper):
* bindings/js/JSIDBRequestCustom.cpp:
(WebCore::JSIDBRequest::visitAdditionalChildren):
* bindings/js/JSValueInWrappedObject.h:
(WebCore::JSValueInWrappedObject::JSValueInWrappedObject):
(WebCore::JSValueInWrappedObject::operator=):
(WebCore::JSValueInWrappedObject::clear):

LayoutTests:

* TestExpectations:
* platform/win/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:

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

19 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/platform/win/TestExpectations
LayoutTests/storage/indexeddb/connection-leak-expected.txt
LayoutTests/storage/indexeddb/connection-leak-private-expected.txt
LayoutTests/storage/indexeddb/cursor-leak-expected.txt
LayoutTests/storage/indexeddb/cursor-leak-private-expected.txt
LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt
LayoutTests/storage/indexeddb/cursor-request-cycle-private-expected.txt
LayoutTests/storage/indexeddb/request-leak-expected.txt
LayoutTests/storage/indexeddb/request-leak-private-expected.txt
LayoutTests/storage/indexeddb/resources/cursor-request-cycle.js
Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/IDBCursor.cpp
Source/WebCore/Modules/indexeddb/IDBCursor.h
Source/WebCore/Modules/indexeddb/IDBRequest.cpp
Source/WebCore/Modules/indexeddb/IDBRequest.h
Source/WebCore/bindings/js/JSIDBRequestCustom.cpp
Source/WebCore/bindings/js/JSValueInWrappedObject.h

index 4ba8378..7b8dd06 100644 (file)
@@ -1,3 +1,22 @@
+2019-03-14  Sihui Liu  <sihui_liu@apple.com>
+
+        IndexedDB: re-enable some leak tests
+        https://bugs.webkit.org/show_bug.cgi?id=194806
+
+        Reviewed by Geoffrey Garen.
+
+        * TestExpectations:
+        * platform/win/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-03-14  Simon Fraser  <simon.fraser@apple.com>
 
         Make it possible to test scrolling tree layer manipulation more easily
index 9035429..6644083 100644 (file)
@@ -1531,18 +1531,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
index b7ddb67..8875c96 100644 (file)
@@ -4278,6 +4278,16 @@ webkit.org/b/194451 accessibility/set-value-not-work-for-disabled-sliders.html [
 
 storage/indexeddb/result-request-cycle.html [ Skip ]
 storage/indexeddb/value-cursor-cycle.html [ Skip ]
+storage/indexeddb/connection-leak-private.html [ Skip ]
+storage/indexeddb/connection-leak.html [ Skip ]
+storage/indexeddb/cursor-leak-private.html [ Skip ]
+storage/indexeddb/cursor-leak.html [ Skip ]
+storage/indexeddb/cursor-request-cycle-private.html [ Skip ]
+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 [ Skip ]
+storage/indexeddb/request-leak.html [ Skip ]
 
 webkit.org/b/194711 fast/replaced/encrypted-pdf-as-object-and-embed.html [ Failure ]
 
index 9dd0cfa..94c9186 100644 (file)
@@ -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)
index 9dd0cfa..94c9186 100644 (file)
@@ -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)
index 0790c52..aeea70d 100644 (file)
@@ -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
index 0790c52..aeea70d 100644 (file)
@@ -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
index 99c7205..6184653 100644 (file)
@@ -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():
index 99c7205..6184653 100644 (file)
@@ -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():
index e1f164f..b0386e1 100644 (file)
@@ -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)
 
index e1f164f..b0386e1 100644 (file)
@@ -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)
 
index bbbec8c..646ce1e 100644 (file)
@@ -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) {
index d8c7ace..f810793 100644 (file)
@@ -1,3 +1,37 @@
+2019-03-14  Sihui Liu  <sihui_liu@apple.com>
+
+        IndexedDB: re-enable some leak tests
+        https://bugs.webkit.org/show_bug.cgi?id=194806
+
+        Reviewed by Geoffrey Garen.
+
+        Protected JSIDBCursor object when advance/continue request on IDBCursor is not finished, because after the 
+        advance operation completes on success, we need to return the same JSIDBCursor object as before the advance, 
+        and during the wait for advance operation to complete, we need to return error as the result. 
+        Covered by existing tests.
+
+        * Modules/indexeddb/IDBCursor.cpp:
+        (WebCore::IDBCursor::setGetResult):
+        (WebCore::IDBCursor::clearWrappers):
+        * Modules/indexeddb/IDBCursor.h:
+        * Modules/indexeddb/IDBRequest.cpp:
+        (WebCore::IDBRequest::stop):
+        (WebCore::IDBRequest::setResult):
+        (WebCore::IDBRequest::setResultToStructuredClone):
+        (WebCore::IDBRequest::setResultToUndefined):
+        (WebCore::IDBRequest::willIterateCursor):
+        (WebCore::IDBRequest::didOpenOrIterateCursor):
+        (WebCore::IDBRequest::clearWrappers):
+        * Modules/indexeddb/IDBRequest.h:
+        (WebCore::IDBRequest::cursorWrapper):
+        * bindings/js/JSIDBRequestCustom.cpp:
+        (WebCore::JSIDBRequest::visitAdditionalChildren):
+        * bindings/js/JSValueInWrappedObject.h:
+        (WebCore::JSValueInWrappedObject::JSValueInWrappedObject):
+        (WebCore::JSValueInWrappedObject::operator=):
+        (WebCore::JSValueInWrappedObject::clear):
+
 2019-03-14  Shawn Roberts  <sroberts@apple.com>
 
         Unreviewed, rolling out r242981.
index 253a3b1..4fd8456 100644 (file)
@@ -309,11 +309,18 @@ ExceptionOr<Ref<WebCore::IDBRequest>> IDBCursor::deleteFunction(ExecState& state
     return WTFMove(request);
 }
 
-void IDBCursor::setGetResult(IDBRequest&, const IDBGetResult& getResult)
+bool IDBCursor::setGetResult(IDBRequest& request, 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());
 
+    auto* context = request.scriptExecutionContext();
+    if (!context)
+        return false;
+
+    VM& vm = context->vm();
+    JSLockHolder lock(vm);
+
     m_keyWrapper = { };
     m_primaryKeyWrapper = { };
     m_valueWrapper = { };
@@ -326,7 +333,7 @@ void IDBCursor::setGetResult(IDBRequest&, const IDBGetResult& getResult)
         m_value = { };
 
         m_gotValue = false;
-        return;
+        return false;
     }
 
     m_keyData = getResult.keyData();
@@ -338,6 +345,14 @@ void IDBCursor::setGetResult(IDBRequest&, const IDBGetResult& getResult)
         m_value = getResult.value();
 
     m_gotValue = true;
+    return true;
+}
+
+void IDBCursor::clearWrappers()
+{
+    m_keyWrapper.clear();
+    m_primaryKeyWrapper.clear();
+    m_valueWrapper.clear();
 }
 
 } // namespace WebCore
index 03e6b1e..7b7b171 100644 (file)
@@ -74,9 +74,10 @@ public:
 
     void setRequest(IDBRequest& request) { m_request = makeWeakPtr(&request); }
     void clearRequest() { m_request.clear(); }
+    void clearWrappers();
     IDBRequest* request() { return m_request.get(); }
 
-    void setGetResult(IDBRequest&, const IDBGetResult&);
+    bool setGetResult(IDBRequest&, const IDBGetResult&);
 
     virtual bool isKeyCursorWithValue() const { return false; }
 
index 650c1a9..46be5e1 100644 (file)
@@ -277,6 +277,8 @@ void IDBRequest::stop()
 
     removeAllEventListeners();
 
+    clearWrappers();
+
     m_contextStopped = true;
 }
 
@@ -368,10 +370,6 @@ void IDBRequest::setResult(const IDBKeyData& keyData)
     if (!context)
         return;
 
-    auto* state = context->execState();
-    if (!state)
-        return;
-
     VM& vm = context->vm();
     JSLockHolder lock(vm);
     m_result = keyData;
@@ -386,10 +384,6 @@ void IDBRequest::setResult(const Vector<IDBKeyData>& keyDatas)
     if (!context)
         return;
 
-    auto* state = context->execState();
-    if (!state)
-        return;
-
     VM& vm = context->vm();
     JSLockHolder lock(vm);
     m_result = keyDatas;
@@ -404,10 +398,6 @@ void IDBRequest::setResult(const Vector<IDBValue>& values)
     if (!context)
         return;
 
-    auto* state = context->execState();
-    if (!state)
-        return;
-
     VM& vm = context->vm();
     JSLockHolder lock(vm);
     m_result = values;
@@ -422,6 +412,8 @@ void IDBRequest::setResult(uint64_t number)
     if (!context)
         return;
 
+    VM& vm = context->vm();
+    JSLockHolder lock(vm);
     m_result = number;
     m_resultWrapper = { };
 }
@@ -436,10 +428,6 @@ void IDBRequest::setResultToStructuredClone(const IDBValue& value)
     if (!context)
         return;
 
-    auto* state = context->execState();
-    if (!state)
-        return;
-
     VM& vm = context->vm();
     JSLockHolder lock(vm);
     m_result = value;
@@ -450,6 +438,12 @@ void IDBRequest::setResultToUndefined()
 {
     ASSERT(&originThread() == &Thread::current());
 
+    auto* context = scriptExecutionContext();
+    if (!context)
+        return;
+    
+    VM& vm = context->vm();
+    JSLockHolder lock(vm);
     m_result = NullResultType::Undefined;
     m_resultWrapper = { };
 }
@@ -476,6 +470,16 @@ void IDBRequest::willIterateCursor(IDBCursor& cursor)
     m_pendingCursor = &cursor;
     m_hasPendingActivity = true;
     m_result = NullResultType::Empty;
+
+    auto* context = scriptExecutionContext();
+    if (!context)
+        return;
+
+    VM& vm = context->vm();
+    JSLockHolder lock(vm);
+
+    if (m_resultWrapper)
+        m_cursorWrapper = m_resultWrapper;
     m_resultWrapper = { };
     m_readyState = ReadyState::Pending;
     m_domError = nullptr;
@@ -487,11 +491,19 @@ void IDBRequest::didOpenOrIterateCursor(const IDBResultData& resultData)
     ASSERT(&originThread() == &Thread::current());
     ASSERT(m_pendingCursor);
 
+    auto* context = scriptExecutionContext();
+    if (!context)
+        return;
+
+    VM& vm = context->vm();
+    JSLockHolder lock(vm);
+
     m_result = NullResultType::Empty;
     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)
+            m_resultWrapper = m_cursorWrapper;
         if (resultData.getResult().isDefined())
             m_result = m_pendingCursor;
     }
@@ -536,10 +548,35 @@ void IDBRequest::setResult(Ref<IDBDatabase>&& database)
 {
     ASSERT(&originThread() == &Thread::current());
 
+    auto* context = scriptExecutionContext();
+    if (!context)
+        return;
+
+    VM& vm = context->vm();
+    JSLockHolder lock(vm);
+
     m_result = RefPtr<IDBDatabase> { WTFMove(database) };
     m_resultWrapper = { };
 }
 
+void IDBRequest::clearWrappers()
+{
+    auto* context = scriptExecutionContext();
+    if (!context)
+        return;
+    VM& vm = context->vm();
+    JSLockHolder lock(vm);
+    
+    m_resultWrapper.clear();
+    m_cursorWrapper.clear();
+    
+    WTF::switchOn(m_result,
+        [] (RefPtr<IDBCursor>& cursor) { cursor->clearWrappers(); },
+        [] (const auto&) { }
+    );
+}
+
+
 } // namespace WebCore
 
 #endif // ENABLE(INDEXED_DATABASE)
index 635105d..d5be99f 100644 (file)
@@ -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; }
@@ -165,12 +166,15 @@ private:
     void onError();
     void onSuccess();
 
+    void clearWrappers();
+
     IDBCursor* resultCursor();
 
     IDBError m_idbError;
     IDBResourceIdentifier m_resourceIdentifier;
 
     JSValueInWrappedObject m_resultWrapper;
+    JSValueInWrappedObject m_cursorWrapper;
     Result m_result;
     Optional<Source> m_source;
 
index 2e710a1..5ebb7cc 100644 (file)
@@ -75,6 +75,7 @@ void JSIDBRequest::visitAdditionalChildren(SlotVisitor& visitor)
 {
     auto& request = wrapped();
     request.resultWrapper().visit(visitor);
+    request.cursorWrapper().visit(visitor);
 }
 
 }
index 07aefb7..9f5dda2 100644 (file)
@@ -36,9 +36,12 @@ namespace WebCore {
 class JSValueInWrappedObject {
 public:
     JSValueInWrappedObject(JSC::JSValue = { });
+    JSValueInWrappedObject(const JSValueInWrappedObject&);
     operator JSC::JSValue() const;
     explicit operator bool() const;
+    JSValueInWrappedObject& operator=(const JSValueInWrappedObject& other);
     void visit(JSC::SlotVisitor&) const;
+    void clear();
 
 private:
     // Use a weak pointer here so that if this code or client code has a visiting mistake,
@@ -66,6 +69,11 @@ inline auto JSValueInWrappedObject::makeValue(JSC::JSValue value) -> Value
 }
 
 inline JSValueInWrappedObject::JSValueInWrappedObject(JSC::JSValue value)
+    : m_value(makeValue(JSC::JSValue(value)))
+{
+}
+
+inline JSValueInWrappedObject::JSValueInWrappedObject(const JSValueInWrappedObject& value)
     : m_value(makeValue(value))
 {
 }
@@ -84,6 +92,12 @@ inline JSValueInWrappedObject::operator bool() const
     return JSC::JSValue { *this }.operator bool();
 }
 
+inline JSValueInWrappedObject& JSValueInWrappedObject::operator=(const JSValueInWrappedObject& other)
+{
+    m_value = makeValue(JSC::JSValue(other));
+    return *this;
+}
+
 inline void JSValueInWrappedObject::visit(JSC::SlotVisitor& visitor) const
 {
     return WTF::switchOn(m_value, [] (JSC::JSValue) {
@@ -93,6 +107,13 @@ inline void JSValueInWrappedObject::visit(JSC::SlotVisitor& visitor) const
     });
 }
 
+inline void JSValueInWrappedObject::clear()
+{
+    WTF::switchOn(m_value, [] (Weak& value) {
+        value.clear();
+    }, [] (auto&) { });
+}
+
 inline JSC::JSValue cachedPropertyValue(JSC::ExecState& state, const JSDOMObject& owner, JSValueInWrappedObject& cachedValue, const WTF::Function<JSC::JSValue()>& function)
 {
     if (cachedValue && isWorldCompatible(state, cachedValue))