IndexedDB: Have IDBCursor and IDBRequest explicitly break ref cycles

Until (1) a transaction ends or (2) a cursor hits the end of its range, an IDBRequest holds on to an IDBCursor as its result. Per spec, calling continue() or advance() on the cursor re-uses the same IDBRequest, requiring a reference cycle. Previously, the cycle was broken explicitly on either of those two conditions, but until that time a cursor-request pair would "leak", holding on to potentially large script value results. This patch makes both classes RefCountedBase::deref() and check to see if they have a partner object and both refcounts are 1. If so, the cycle is broken. Special case cruft for condition #1 is removed to simplify the code - just rely on GC to reclaim the objects if necessary. R=alecflett@chromium.org,dgrogan@chromium.org BUG=225860 Review URL: https://chromiumcodereview.appspot.com/23653024 git-svn-id: svn://svn.chromium.org/blink/trunk@157382 bbb929c8-8fbe-4397-9dbb-9b2b20218538
nwjs · Sep 6, 2013 · 4b8ab16 · 4b8ab16
1 parent 0e53aff
commit 4b8ab16
Show file tree

Hide file tree

Showing 11 changed files with 187 additions and 103 deletions.
diff --git a/LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt b/LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt
@@ -0,0 +1,40 @@
+Verify that that cursors weakly hold request, and work if request is GC'd
+
+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)
+
+prepareDatabase():
+db = event.target.result
+store = db.createObjectStore('store')
+
+onOpen():
+db = event.target.result
+tx = db.transaction('store')
+store = tx.objectStore('store')
+cursorRequest = store.openCursor()
+otherRequest = store.get(0)
+
+openCursorRequest():
+cursor = cursorRequest.result
+PASS cursor is non-null.
+PASS cursor.key is "key1"
+PASS cursor.value is "value1"
+
+otherRequestSuccess():
+PASS afterCount is beforeCount
+cursor.continue()
+finalRequest = store.get(0)
+
+finalRequestSuccess():
+PASS cursor.key is "key2"
+PASS cursor.value is "value2"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/storage/indexeddb/cursor-request-cycle.html b/LayoutTests/storage/indexeddb/cursor-request-cycle.html
@@ -0,0 +1,68 @@
+<!DOCTYPE html>
+<script src="../../fast/js/resources/js-test-pre.js"></script>
+<script src="resources/shared.js"></script>
+<script>
+
+description("Verify that that cursors weakly hold request, and work if request is GC'd");
+
+indexedDBTest(prepareDatabase, onOpen);
+
+function prepareDatabase(evt)
+{
+    preamble(evt);
+    evalAndLog("db = event.target.result");
+    evalAndLog("store = db.createObjectStore('store')");
+    store.put("value1", "key1");
+    store.put("value2", "key2");
+}
+
+function onOpen(evt)
+{
+    preamble(evt);
+    evalAndLog("db = event.target.result");
+    evalAndLog("tx = db.transaction('store')");
+    evalAndLog("store = tx.objectStore('store')");
+
+    evalAndLog("cursorRequest = store.openCursor()");
+    cursorRequest.onsuccess = function openCursorRequest(evt) {
+        preamble(evt);
+        evalAndLog("cursor = cursorRequest.result");
+        shouldBeNonNull("cursor");
+        shouldBeEqualToString("cursor.key", "key1");
+        shouldBeEqualToString("cursor.value", "value1");
+    };
+
+    evalAndLog("otherRequest = store.get(0)");
+
+    otherRequest.onsuccess = function otherRequestSuccess(evt) {
+        preamble(evt);
+
+        gc();
+        gc(); // FIXME: Why is this second call necessary?
+        beforeCount = window.internals.numberOfLiveNodes();
+
+        cursorRequest.canary = document.createElement('canary');
+        cursorRequest = null;
+
+        gc();
+        gc(); // FIXME: Why is this second call necessary?
+        afterCount = window.internals.numberOfLiveNodes();
+        shouldBe("afterCount", "beforeCount");
+
+        // The following call should generate a scratch request, invisible to script:
+        evalAndLog("cursor.continue()");
+
+        evalAndLog("finalRequest = store.get(0)");
+        finalRequest.onsuccess = function finalRequestSuccess(evt) {
+            preamble(evt);
+            shouldBeEqualToString("cursor.key", "key2");
+            shouldBeEqualToString("cursor.value", "value2");
+        };
+    };
+
+    tx.oncomplete = finishJSTest;
+}
+
+
+</script>
+<script src="../../fast/js/resources/js-test-post.js"></script>
diff --git a/Source/modules/indexeddb/IDBAny.cpp b/Source/modules/indexeddb/IDBAny.cpp
@@ -62,52 +62,52 @@ IDBAny::~IDBAny()
 {
 }
 
-PassRefPtr<DOMStringList> IDBAny::domStringList()
+DOMStringList* IDBAny::domStringList()
 {
     ASSERT(m_type == DOMStringListType);
-    return m_domStringList;
+    return m_domStringList.get();
 }
 
-PassRefPtr<IDBCursor> IDBAny::idbCursor()
+IDBCursor* IDBAny::idbCursor()
 {
     ASSERT(m_type == IDBCursorType);
-    return m_idbCursor;
+    return m_idbCursor.get();
 }
 
-PassRefPtr<IDBCursorWithValue> IDBAny::idbCursorWithValue()
+IDBCursorWithValue* IDBAny::idbCursorWithValue()
 {
     ASSERT(m_type == IDBCursorWithValueType);
-    return m_idbCursorWithValue;
+    return m_idbCursorWithValue.get();
 }
 
-PassRefPtr<IDBDatabase> IDBAny::idbDatabase()
+IDBDatabase* IDBAny::idbDatabase()
 {
     ASSERT(m_type == IDBDatabaseType);
-    return m_idbDatabase;
+    return m_idbDatabase.get();
 }
 
-PassRefPtr<IDBFactory> IDBAny::idbFactory()
+IDBFactory* IDBAny::idbFactory()
 {
     ASSERT(m_type == IDBFactoryType);
-    return m_idbFactory;
+    return m_idbFactory.get();
 }
 
-PassRefPtr<IDBIndex> IDBAny::idbIndex()
+IDBIndex* IDBAny::idbIndex()
 {
     ASSERT(m_type == IDBIndexType);
-    return m_idbIndex;
+    return m_idbIndex.get();
 }
 
-PassRefPtr<IDBObjectStore> IDBAny::idbObjectStore()
+IDBObjectStore* IDBAny::idbObjectStore()
 {
     ASSERT(m_type == IDBObjectStoreType);
-    return m_idbObjectStore;
+    return m_idbObjectStore.get();
 }
 
-PassRefPtr<IDBTransaction> IDBAny::idbTransaction()
+IDBTransaction* IDBAny::idbTransaction()
 {
     ASSERT(m_type == IDBTransactionType);
-    return m_idbTransaction;
+    return m_idbTransaction.get();
 }
 
 const ScriptValue& IDBAny::scriptValue()

diff --git a/Source/modules/indexeddb/IDBAny.h b/Source/modules/indexeddb/IDBAny.h
@@ -91,14 +91,14 @@ class IDBAny : public RefCounted<IDBAny> {
 
     Type type() const { return m_type; }
     // Use type() to figure out which one of these you're allowed to call.
-    PassRefPtr<DOMStringList> domStringList();
-    PassRefPtr<IDBCursor> idbCursor();
-    PassRefPtr<IDBCursorWithValue> idbCursorWithValue();
-    PassRefPtr<IDBDatabase> idbDatabase();
-    PassRefPtr<IDBFactory> idbFactory();
-    PassRefPtr<IDBIndex> idbIndex();
-    PassRefPtr<IDBObjectStore> idbObjectStore();
-    PassRefPtr<IDBTransaction> idbTransaction();
+    DOMStringList* domStringList();
+    IDBCursor* idbCursor();
+    IDBCursorWithValue* idbCursorWithValue();
+    IDBDatabase* idbDatabase();
+    IDBFactory* idbFactory();
+    IDBIndex* idbIndex();
+    IDBObjectStore* idbObjectStore();
+    IDBTransaction* idbTransaction();
     const ScriptValue& scriptValue();
     int64_t integer();
     const String& string();

diff --git a/Source/modules/indexeddb/IDBCallbacks.h b/Source/modules/indexeddb/IDBCallbacks.h
@@ -41,9 +41,11 @@ namespace WebCore {
 class DOMError;
 class IDBCursorBackendInterface;
 
-class IDBCallbacks : public RefCounted<IDBCallbacks> {
+class IDBCallbacks : public WTF::RefCountedBase {
 public:
     virtual ~IDBCallbacks() { }
+    virtual void deref() = 0;
+
 
     virtual void onError(PassRefPtr<DOMError>) = 0;
     // From IDBFactory.webkitGetDatabaseNames()

diff --git a/Source/modules/indexeddb/IDBCursor.cpp b/Source/modules/indexeddb/IDBCursor.cpp
@@ -80,7 +80,6 @@ IDBCursor::IDBCursor(PassRefPtr<IDBCursorBackendInterface> backend, IndexedDB::C
     , m_direction(direction)
     , m_source(source)
     , m_transaction(transaction)
-    , m_transactionNotifier(transaction, this)
     , m_gotValue(false)
     , m_keyDirty(true)
     , m_primaryKeyDirty(true)
@@ -274,14 +273,23 @@ void IDBCursor::close()
 {
     // The notifier may be the last reference to this cursor.
     RefPtr<IDBCursor> protect(this);
-    m_transactionNotifier.cursorFinished();
-    if (m_request) {
-        m_request->finishCursor();
-        m_request.clear();
-    }
+    m_request.clear();
     m_backend.clear();
 }
 
+void IDBCursor::checkForReferenceCycle()
+{
+    // If this cursor and its request have the only references
+    // to each other, then explicitly break the cycle.
+    if (!m_request || m_request->getResultCursor() != this)
+        return;
+
+    if (!hasOneRef() || !m_request->hasOneRef())
+        return;
+
+    m_request.clear();
+}
+
 ScriptValue IDBCursor::key(ScriptExecutionContext* context)
 {
     m_keyDirty = false;

diff --git a/Source/modules/indexeddb/IDBCursor.h b/Source/modules/indexeddb/IDBCursor.h
@@ -46,7 +46,7 @@ class IDBRequest;
 class ScriptExecutionContext;
 class SharedBuffer;
 
-class IDBCursor : public ScriptWrappable, public RefCounted<IDBCursor> {
+class IDBCursor : public ScriptWrappable, public WTF::RefCountedBase {
 public:
     static const AtomicString& directionNext();
     static const AtomicString& directionNextUnique();
@@ -80,6 +80,15 @@ class IDBCursor : public ScriptWrappable, public RefCounted<IDBCursor> {
     void close();
     void setValueReady(PassRefPtr<IDBKey>, PassRefPtr<IDBKey> primaryKey, PassRefPtr<SharedBuffer> value);
     PassRefPtr<IDBKey> idbPrimaryKey() { return m_primaryKey; }
+    IDBRequest* request() { return m_request.get(); }
+
+    void deref()
+    {
+        if (derefBase())
+            delete this;
+        else if (hasOneRef())
+            checkForReferenceCycle();
+    }
 
 protected:
     IDBCursor(PassRefPtr<IDBCursorBackendInterface>, IndexedDB::CursorDirection, IDBRequest*, IDBAny* source, IDBTransaction*);
@@ -88,14 +97,14 @@ class IDBCursor : public ScriptWrappable, public RefCounted<IDBCursor> {
 private:
     PassRefPtr<IDBObjectStore> effectiveObjectStore();
 
+    void checkForReferenceCycle();
     bool isDeleted() const;
 
     RefPtr<IDBCursorBackendInterface> m_backend;
     RefPtr<IDBRequest> m_request;
     const IndexedDB::CursorDirection m_direction;
     RefPtr<IDBAny> m_source;
     RefPtr<IDBTransaction> m_transaction;
-    IDBTransaction::OpenCursorNotifier m_transactionNotifier;
     bool m_gotValue;
     bool m_keyDirty;
     bool m_primaryKeyDirty;

diff --git a/Source/modules/indexeddb/IDBRequest.cpp b/Source/modules/indexeddb/IDBRequest.cpp
@@ -79,7 +79,6 @@ IDBRequest::IDBRequest(ScriptExecutionContext* context, PassRefPtr<IDBAny> sourc
     , m_hasPendingActivity(true)
     , m_cursorType(IndexedDB::CursorKeyAndValue)
     , m_cursorDirection(IndexedDB::CursorNext)
-    , m_cursorFinished(false)
     , m_pendingCursor(0)
     , m_didFireUpgradeNeededEvent(false)
     , m_preventPropagation(false)
@@ -184,14 +183,15 @@ void IDBRequest::setPendingCursor(PassRefPtr<IDBCursor> cursor)
     ASSERT(!m_pendingCursor);
     ASSERT(cursor == getResultCursor());
 
+    m_hasPendingActivity = true;
     m_pendingCursor = cursor;
     m_result.clear();
     m_readyState = PENDING;
     m_error.clear();
     m_transaction->registerRequest(this);
 }
 
-PassRefPtr<IDBCursor> IDBRequest::getResultCursor()
+IDBCursor* IDBRequest::getResultCursor()
 {
     if (!m_result)
         return 0;
@@ -217,11 +217,18 @@ void IDBRequest::setResultCursor(PassRefPtr<IDBCursor> cursor, PassRefPtr<IDBKey
     m_result = IDBAny::create(IDBCursorWithValue::fromCursor(cursor));
 }
 
-void IDBRequest::finishCursor()
+void IDBRequest::checkForReferenceCycle()
 {
-    m_cursorFinished = true;
-    if (m_readyState != PENDING)
-        m_hasPendingActivity = false;
+    // If this request and its cursor have the only references
+    // to each other, then explicitly break the cycle.
+    IDBCursor* cursor = getResultCursor();
+    if (!cursor || cursor->request() != this)
+        return;
+
+    if (!hasOneRef() || !cursor->hasOneRef())
+        return;
+
+    m_result.clear();
 }
 
 bool IDBRequest::shouldEnqueueEvent() const
@@ -496,7 +503,7 @@ bool IDBRequest::dispatchEvent(PassRefPtr<Event> event)
     if (cursorToNotify)
         cursorToNotify->postSuccessHandlerCallback();
 
-    if (m_readyState == DONE && (!cursorToNotify || m_cursorFinished) && event->type() != eventNames().upgradeneededEvent)
+    if (m_readyState == DONE && event->type() != eventNames().upgradeneededEvent)
         m_hasPendingActivity = false;
 
     return dontPreventDefault;