Modern IDB: Ref cycle between IDBObjectStore and IDBIndex.

https://bugs.webkit.org/show_bug.cgi?id=154110 Reviewed by Darin Adler. No new tests (Currently untestable). The lifetime of IDBObjectStore and IDBIndex are closely intertwined, but we have to break the ref cycle. This patch does a few semi-gnarly things: 1 - Makes both IDBIndex and IDBObjectStore have a custom marking function so they can add each other as opaque roots. 2 - Adds a lock to protect IDBObjectStore's collection of referenced indexes to support #1, as GC marking can happen on any thread. 3 - Makes IDBIndex not be traditionally RefCounted; Instead, IDBIndex::ref()/deref() simply ref()/deref() the owning IDBObjectStore. 4 - ...Except when somebody deletes an IDBIndex from its IDBObjectStore. Once that happens, the object store no longer has a reference back to the index, but the index still needs a reference back to the object store. To support this, the IDBIndex becomes "traditionally RefCounted" while holding a ref to its IDBObjectStore. * CMakeLists.txt: * WebCore.xcodeproj/project.pbxproj: * Modules/indexeddb/IDBIndex.h: (WebCore::IDBIndex::isModern): * Modules/indexeddb/IDBIndex.idl: * Modules/indexeddb/IDBObjectStore.h: (WebCore::IDBObjectStore::isModern): * Modules/indexeddb/IDBObjectStore.idl: * Modules/indexeddb/client/IDBIndexImpl.cpp: (WebCore::IDBClient::IDBIndex::objectStore): (WebCore::IDBClient::IDBIndex::openCursor): (WebCore::IDBClient::IDBIndex::doCount): (WebCore::IDBClient::IDBIndex::openKeyCursor): (WebCore::IDBClient::IDBIndex::doGet): (WebCore::IDBClient::IDBIndex::doGetKey): (WebCore::IDBClient::IDBIndex::markAsDeleted): (WebCore::IDBClient::IDBIndex::ref): (WebCore::IDBClient::IDBIndex::deref): (WebCore::IDBClient::IDBIndex::create): Deleted. * Modules/indexeddb/client/IDBIndexImpl.h: (WebCore::IDBClient::IDBIndex::modernObjectStore): * Modules/indexeddb/client/IDBObjectStoreImpl.cpp: (WebCore::IDBClient::IDBObjectStore::createIndex): (WebCore::IDBClient::IDBObjectStore::index): (WebCore::IDBClient::IDBObjectStore::deleteIndex): (WebCore::IDBClient::IDBObjectStore::visitReferencedIndexes): * Modules/indexeddb/client/IDBObjectStoreImpl.h: * Modules/indexeddb/client/IDBTransactionImpl.cpp: (WebCore::IDBClient::IDBTransaction::createIndex): * Modules/indexeddb/client/IDBTransactionImpl.h: * Modules/indexeddb/legacy/LegacyIndex.cpp: (WebCore::LegacyIndex::ref): (WebCore::LegacyIndex::deref): * Modules/indexeddb/legacy/LegacyIndex.h: * bindings/js/JSIDBIndexCustom.cpp: Added. (WebCore::JSIDBIndex::visitAdditionalChildren): * bindings/js/JSIDBObjectStoreCustom.cpp: (WebCore::JSIDBObjectStore::visitAdditionalChildren): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@196482 268f45cc-cd09-0410-ab3c-d52691b4dbfc
haiku · Feb 12, 2016 · a578b0e · a578b0e
1 parent e6eb0ea
commit a578b0e
Show file tree

Hide file tree

Showing 17 changed files with 276 additions and 46 deletions.
diff --git a/Source/WebCore/CMakeLists.txt b/Source/WebCore/CMakeLists.txt
@@ -1180,6 +1180,7 @@ set(WebCore_SOURCES
     bindings/js/JSIDBCursorCustom.cpp
     bindings/js/JSIDBCursorWithValueCustom.cpp
     bindings/js/JSIDBDatabaseCustom.cpp
+    bindings/js/JSIDBIndexCustom.cpp
     bindings/js/JSIDBObjectStoreCustom.cpp
     bindings/js/JSImageConstructor.cpp
     bindings/js/JSImageDataCustom.cpp

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,73 @@
+2016-02-12  Brady Eidson  <beidson@apple.com>
+
+        Modern IDB: Ref cycle between IDBObjectStore and IDBIndex.
+        https://bugs.webkit.org/show_bug.cgi?id=154110
+
+        Reviewed by Darin Adler.
+
+        No new tests (Currently untestable).
+
+        The lifetime of IDBObjectStore and IDBIndex are closely intertwined, but we have to break the ref cycle.
+
+        This patch does a few semi-gnarly things:
+        1 - Makes both IDBIndex and IDBObjectStore have a custom marking function so they can add each other as 
+            opaque roots.
+        2 - Adds a lock to protect IDBObjectStore's collection of referenced indexes to support #1, as GC marking
+            can happen on any thread.
+        3 - Makes IDBIndex not be traditionally RefCounted; Instead, IDBIndex::ref()/deref() simply ref()/deref()
+            the owning IDBObjectStore.
+        4 - ...Except when somebody deletes an IDBIndex from its IDBObjectStore. Once that happens, the object
+            store no longer has a reference back to the index, but the index still needs a reference back to the
+            object store. To support this, the IDBIndex becomes "traditionally RefCounted" while holding a ref to
+            its IDBObjectStore.
+
+        * CMakeLists.txt:
+        * WebCore.xcodeproj/project.pbxproj:
+
+        * Modules/indexeddb/IDBIndex.h:
+        (WebCore::IDBIndex::isModern):
+        * Modules/indexeddb/IDBIndex.idl:
+
+        * Modules/indexeddb/IDBObjectStore.h:
+        (WebCore::IDBObjectStore::isModern):
+        * Modules/indexeddb/IDBObjectStore.idl:
+
+        * Modules/indexeddb/client/IDBIndexImpl.cpp:
+        (WebCore::IDBClient::IDBIndex::objectStore):
+        (WebCore::IDBClient::IDBIndex::openCursor):
+        (WebCore::IDBClient::IDBIndex::doCount):
+        (WebCore::IDBClient::IDBIndex::openKeyCursor):
+        (WebCore::IDBClient::IDBIndex::doGet):
+        (WebCore::IDBClient::IDBIndex::doGetKey):
+        (WebCore::IDBClient::IDBIndex::markAsDeleted):
+        (WebCore::IDBClient::IDBIndex::ref):
+        (WebCore::IDBClient::IDBIndex::deref):
+        (WebCore::IDBClient::IDBIndex::create): Deleted.
+        * Modules/indexeddb/client/IDBIndexImpl.h:
+        (WebCore::IDBClient::IDBIndex::modernObjectStore):
+
+        * Modules/indexeddb/client/IDBObjectStoreImpl.cpp:
+        (WebCore::IDBClient::IDBObjectStore::createIndex):
+        (WebCore::IDBClient::IDBObjectStore::index):
+        (WebCore::IDBClient::IDBObjectStore::deleteIndex):
+        (WebCore::IDBClient::IDBObjectStore::visitReferencedIndexes):
+        * Modules/indexeddb/client/IDBObjectStoreImpl.h:
+
+        * Modules/indexeddb/client/IDBTransactionImpl.cpp:
+        (WebCore::IDBClient::IDBTransaction::createIndex):
+        * Modules/indexeddb/client/IDBTransactionImpl.h:
+
+        * Modules/indexeddb/legacy/LegacyIndex.cpp:
+        (WebCore::LegacyIndex::ref):
+        (WebCore::LegacyIndex::deref):
+        * Modules/indexeddb/legacy/LegacyIndex.h:
+
+        * bindings/js/JSIDBIndexCustom.cpp: Added.
+        (WebCore::JSIDBIndex::visitAdditionalChildren):
+
+        * bindings/js/JSIDBObjectStoreCustom.cpp:
+        (WebCore::JSIDBObjectStore::visitAdditionalChildren):
+
 2016-02-12  Csaba Osztrogonác  <ossy@webkit.org>
 
         [EFL][GTK] Fix ENABLE(SVG_OTF_CONVERTER) build

diff --git a/Source/WebCore/Modules/indexeddb/IDBIndex.h b/Source/WebCore/Modules/indexeddb/IDBIndex.h
@@ -43,7 +43,7 @@ namespace WebCore {
 
 class IDBObjectStore;
 
-class IDBIndex : public RefCounted<IDBIndex> {
+class IDBIndex {
 public:
     virtual ~IDBIndex() { }
 
@@ -77,6 +77,14 @@ class IDBIndex : public RefCounted<IDBIndex> {
     virtual RefPtr<IDBRequest> getKey(ScriptExecutionContext*, IDBKeyRange*, ExceptionCodeWithMessage&) = 0;
     virtual RefPtr<IDBRequest> getKey(ScriptExecutionContext*, const Deprecated::ScriptValue& key, ExceptionCodeWithMessage&) = 0;
 
+    virtual bool isModern() const { return false; }
+
+    // We use our own ref/deref function because Legacy IDB and Modern IDB have very different
+    // lifetime management of their indexes.
+    // This will go away once Legacy IDB is dropped.
+    virtual void ref() = 0;
+    virtual void deref() = 0;
+
 protected:
     IDBIndex();
 };

diff --git a/Source/WebCore/Modules/indexeddb/IDBIndex.idl b/Source/WebCore/Modules/indexeddb/IDBIndex.idl
@@ -27,6 +27,8 @@
     Conditional=INDEXED_DATABASE,
     EnabledAtRuntime=IndexedDB,
     SkipVTableValidation,
+    JSCustomMarkFunction,
+    GenerateIsReachable=Impl,
 ] interface IDBIndex {
     readonly attribute DOMString name;
     readonly attribute IDBObjectStore objectStore;

diff --git a/Source/WebCore/Modules/indexeddb/IDBObjectStore.h b/Source/WebCore/Modules/indexeddb/IDBObjectStore.h
@@ -92,6 +92,8 @@ class IDBObjectStore : public RefCounted<IDBObjectStore> {
     virtual RefPtr<IDBRequest> count(ScriptExecutionContext*, IDBKeyRange*, ExceptionCodeWithMessage&) = 0;
     virtual RefPtr<IDBRequest> count(ScriptExecutionContext*, const Deprecated::ScriptValue& key, ExceptionCodeWithMessage&) = 0;
 
+    virtual bool isModern() const { return false; }
+
 protected:
     IDBObjectStore();
 };

diff --git a/Source/WebCore/Modules/indexeddb/IDBObjectStore.idl b/Source/WebCore/Modules/indexeddb/IDBObjectStore.idl
@@ -27,6 +27,8 @@
     Conditional=INDEXED_DATABASE,
     EnabledAtRuntime=IndexedDB,
     SkipVTableValidation,
+    JSCustomMarkFunction,
+    GenerateIsReachable=Impl,
 ] interface IDBObjectStore {
     [TreatReturnedNullStringAs=Null] readonly attribute DOMString name;
     [ImplementedAs=keyPathAny] readonly attribute IDBAny keyPath;

diff --git a/Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp b/Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp
@@ -41,11 +41,6 @@
 namespace WebCore {
 namespace IDBClient {
 
-Ref<IDBIndex> IDBIndex::create(const IDBIndexInfo& info, IDBObjectStore& objectStore)
-{
-    return adoptRef(*new IDBIndex(info, objectStore));
-}
-
 IDBIndex::IDBIndex(const IDBIndexInfo& info, IDBObjectStore& objectStore)
     : m_info(info)
     , m_objectStore(objectStore)
@@ -63,7 +58,7 @@ const String& IDBIndex::name() const
 
 RefPtr<WebCore::IDBObjectStore> IDBIndex::objectStore()
 {
-    return &m_objectStore.get();
+    return &m_objectStore;
 }
 
 RefPtr<WebCore::IDBAny> IDBIndex::keyPathAny() const
@@ -90,13 +85,13 @@ RefPtr<WebCore::IDBRequest> IDBIndex::openCursor(ScriptExecutionContext* context
 {
     LOG(IndexedDB, "IDBIndex::openCursor");
 
-    if (m_deleted || m_objectStore->isDeleted()) {
+    if (m_deleted || m_objectStore.isDeleted()) {
         ec.code = IDBDatabaseException::InvalidStateError;
         ec.message = ASCIILiteral("Failed to execute 'openCursor' on 'IDBIndex': The index or its object store has been deleted.");
         return nullptr;
     }
 
-    if (!m_objectStore->modernTransaction().isActive()) {
+    if (!m_objectStore.modernTransaction().isActive()) {
         ec.code = IDBDatabaseException::TransactionInactiveError;
         ec.message = ASCIILiteral("Failed to execute 'openCursor' on 'IDBIndex': The transaction is inactive or finished.");
         return nullptr;
@@ -114,9 +109,8 @@ RefPtr<WebCore::IDBRequest> IDBIndex::openCursor(ScriptExecutionContext* context
     if (rangeData.upperKey.isNull())
         rangeData.upperKey = IDBKeyData::maximum();
 
-    auto info = IDBCursorInfo::indexCursor(m_objectStore->modernTransaction(), m_objectStore->info().identifier(), m_info.identifier(), rangeData, direction, IndexedDB::CursorType::KeyAndValue);
-    Ref<IDBRequest> request = m_objectStore->modernTransaction().requestOpenCursor(*context, *this, info);
-    return WTFMove(request);
+    auto info = IDBCursorInfo::indexCursor(m_objectStore.modernTransaction(), m_objectStore.info().identifier(), m_info.identifier(), rangeData, direction, IndexedDB::CursorType::KeyAndValue);
+    return m_objectStore.modernTransaction().requestOpenCursor(*context, *this, info);
 }
 
 RefPtr<WebCore::IDBRequest> IDBIndex::openCursor(ScriptExecutionContext* context, const Deprecated::ScriptValue& key, const String& direction, ExceptionCodeWithMessage& ec)
@@ -177,7 +171,7 @@ RefPtr<WebCore::IDBRequest> IDBIndex::count(ScriptExecutionContext* context, con
 
 RefPtr<WebCore::IDBRequest> IDBIndex::doCount(ScriptExecutionContext& context, const IDBKeyRangeData& range, ExceptionCodeWithMessage& ec)
 {
-    if (m_deleted || m_objectStore->isDeleted()) {
+    if (m_deleted || m_objectStore.isDeleted()) {
         ec.code = IDBDatabaseException::InvalidStateError;
         ec.message = ASCIILiteral("Failed to execute 'count' on 'IDBIndex': The index or its object store has been deleted.");
         return nullptr;
@@ -188,7 +182,7 @@ RefPtr<WebCore::IDBRequest> IDBIndex::doCount(ScriptExecutionContext& context, c
         return nullptr;
     }
 
-    auto& transaction = m_objectStore->modernTransaction();
+    auto& transaction = m_objectStore.modernTransaction();
     if (!transaction.isActive()) {
         ec.code = IDBDatabaseException::TransactionInactiveError;
         ec.message = ASCIILiteral("Failed to execute 'count' on 'IDBIndex': The transaction is inactive or finished.");
@@ -202,13 +196,13 @@ RefPtr<WebCore::IDBRequest> IDBIndex::openKeyCursor(ScriptExecutionContext* cont
 {
     LOG(IndexedDB, "IDBIndex::openKeyCursor");
 
-    if (m_deleted || m_objectStore->isDeleted()) {
+    if (m_deleted || m_objectStore.isDeleted()) {
         ec.code = IDBDatabaseException::InvalidStateError;
         ec.message = ASCIILiteral("Failed to execute 'openKeyCursor' on 'IDBIndex': The index or its object store has been deleted.");
         return nullptr;
     }
 
-    if (!m_objectStore->modernTransaction().isActive()) {
+    if (!m_objectStore.modernTransaction().isActive()) {
         ec.code = IDBDatabaseException::TransactionInactiveError;
         ec.message = ASCIILiteral("Failed to execute 'openKeyCursor' on 'IDBIndex': The transaction is inactive or finished.");
         return nullptr;
@@ -220,9 +214,8 @@ RefPtr<WebCore::IDBRequest> IDBIndex::openKeyCursor(ScriptExecutionContext* cont
         return nullptr;
     }
 
-    auto info = IDBCursorInfo::indexCursor(m_objectStore->modernTransaction(), m_objectStore->info().identifier(), m_info.identifier(), range, direction, IndexedDB::CursorType::KeyOnly);
-    Ref<IDBRequest> request = m_objectStore->modernTransaction().requestOpenCursor(*context, *this, info);
-    return WTFMove(request);
+    auto info = IDBCursorInfo::indexCursor(m_objectStore.modernTransaction(), m_objectStore.info().identifier(), m_info.identifier(), range, direction, IndexedDB::CursorType::KeyOnly);
+    return m_objectStore.modernTransaction().requestOpenCursor(*context, *this, info);
 }
 
 RefPtr<WebCore::IDBRequest> IDBIndex::openKeyCursor(ScriptExecutionContext* context, const Deprecated::ScriptValue& key, const String& direction, ExceptionCodeWithMessage& ec)
@@ -270,7 +263,7 @@ RefPtr<WebCore::IDBRequest> IDBIndex::get(ScriptExecutionContext* context, const
 
 RefPtr<WebCore::IDBRequest> IDBIndex::doGet(ScriptExecutionContext& context, const IDBKeyRangeData& range, ExceptionCodeWithMessage& ec)
 {
-    if (m_deleted || m_objectStore->isDeleted()) {
+    if (m_deleted || m_objectStore.isDeleted()) {
         ec.code = IDBDatabaseException::InvalidStateError;
         ec.message = ASCIILiteral("Failed to execute 'get' on 'IDBIndex': The index or its object store has been deleted.");
         return nullptr;
@@ -281,7 +274,7 @@ RefPtr<WebCore::IDBRequest> IDBIndex::doGet(ScriptExecutionContext& context, con
         return nullptr;
     }
 
-    auto& transaction = m_objectStore->modernTransaction();
+    auto& transaction = m_objectStore.modernTransaction();
     if (!transaction.isActive()) {
         ec.code = IDBDatabaseException::TransactionInactiveError;
         ec.message = ASCIILiteral("Failed to execute 'get' on 'IDBIndex': The transaction is inactive or finished.");
@@ -325,7 +318,7 @@ RefPtr<WebCore::IDBRequest> IDBIndex::getKey(ScriptExecutionContext* context, co
 
 RefPtr<WebCore::IDBRequest> IDBIndex::doGetKey(ScriptExecutionContext& context, const IDBKeyRangeData& range, ExceptionCodeWithMessage& ec)
 {
-    if (m_deleted || m_objectStore->isDeleted()) {
+    if (m_deleted || m_objectStore.isDeleted()) {
         ec.code = IDBDatabaseException::InvalidStateError;
         ec.message = ASCIILiteral("Failed to execute 'getKey' on 'IDBIndex': The index or its object store has been deleted.");
         return nullptr;
@@ -336,7 +329,7 @@ RefPtr<WebCore::IDBRequest> IDBIndex::doGetKey(ScriptExecutionContext& context,
         return nullptr;
     }
 
-    auto& transaction = m_objectStore->modernTransaction();
+    auto& transaction = m_objectStore.modernTransaction();
     if (!transaction.isActive()) {
         ec.code = IDBDatabaseException::TransactionInactiveError;
         ec.message = ASCIILiteral("Failed to execute 'getKey' on 'IDBIndex': The transaction is inactive or finished.");
@@ -346,11 +339,51 @@ RefPtr<WebCore::IDBRequest> IDBIndex::doGetKey(ScriptExecutionContext& context,
     return transaction.requestGetKey(context, *this, range);
 }
 
-void IDBIndex::markAsDeleted()
+void IDBIndex::markAsDeleted(std::unique_ptr<IDBIndex>&& indexOwner)
 {
+    ASSERT(!m_deleted);
+    ASSERT(!m_selfOwner);
+    ASSERT(indexOwner.get() == this);
+
+    // If nobody was keeping a ref to this IDBIndex while under IDBObjectStore ownership,
+    // it can be deleted now by letting indexOwner go out of scope.
+    if (!m_refCount)
+        return;
+
+    m_selfOwner = WTFMove(indexOwner);
+
+    // Now that the IDBIndex is managing its own lifetime, it must ref() its IDBObjectStore to keep it alive.
+    m_objectStoreRef = &m_objectStore;
+
+    // It must undo all of the refs it had previously given its IDBObjectStore when the lifetimes were intertwined.
+    for (unsigned i = m_refCount; i > 0; --i)
+        m_objectStore.deref();
+
     m_deleted = true;
 }
 
+void IDBIndex::ref()
+{
+    ++m_refCount;
+
+    if (!m_deleted)
+        m_objectStore.ref();
+}
+
+void IDBIndex::deref()
+{
+    --m_refCount;
+
+    if (!m_deleted)
+        m_objectStore.deref();
+    else {
+        // This IDBIndex has been detached from its IDBObjectStore so if its RefCount
+        // just went to 0 it should be destroyed.
+        if (!m_refCount)
+            m_selfOwner = nullptr;
+    }
+}
+
 } // namespace IDBClient
 } // namespace WebCore
 

diff --git a/Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.h b/Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.h
@@ -42,7 +42,7 @@ class IDBObjectStore;
 
 class IDBIndex : public WebCore::IDBIndex {
 public:
-    static Ref<IDBIndex> create(const IDBIndexInfo&, IDBObjectStore&);
+    IDBIndex(const IDBIndexInfo&, IDBObjectStore&);
 
     virtual ~IDBIndex();
 
@@ -77,22 +77,38 @@ class IDBIndex : public WebCore::IDBIndex {
 
     const IDBIndexInfo& info() const { return m_info; }
 
-    IDBObjectStore& modernObjectStore() { return m_objectStore.get(); }
+    IDBObjectStore& modernObjectStore() { return m_objectStore; }
 
-    void markAsDeleted();
+    void markAsDeleted(std::unique_ptr<IDBIndex>&&);
     bool isDeleted() const { return m_deleted; }
 
-private:
-    IDBIndex(const IDBIndexInfo&, IDBObjectStore&);
+    virtual bool isModern() const override { return true; }
 
+    void ref() override;
+    void deref() override;
+
+private:
     RefPtr<WebCore::IDBRequest> doCount(ScriptExecutionContext&, const IDBKeyRangeData&, ExceptionCodeWithMessage&);
     RefPtr<WebCore::IDBRequest> doGet(ScriptExecutionContext&, const IDBKeyRangeData&, ExceptionCodeWithMessage&);
     RefPtr<WebCore::IDBRequest> doGetKey(ScriptExecutionContext&, const IDBKeyRangeData&, ExceptionCodeWithMessage&);
 
     IDBIndexInfo m_info;
-    Ref<IDBObjectStore> m_objectStore;
 
     bool m_deleted { false };
+
+    // Most of the time, an IDBObjectStore owns an IDBIndex through a std::unique_ptr.
+    // In that scenario, attempts to ref() the IDBIndex directly ref the IDBObjectStore, so it is okay to
+    // keep a raw reference to the IDBObjectStore because it will always outlive the IDBIndex.
+    IDBObjectStore& m_objectStore;
+
+    // But when an IDBIndex is deleted from its IDBObjectStore that lifetime is no longer guaranteed.
+    // The IDBObjectStore no longer owns the IDBIndex, so the following needs to change:
+    // 1 - The IDBIndex must directly ref its IDBObjectStore to keep it alive.
+    // 2 - The IDBIndex becomes traditionally RefCounted.
+    // 2 - The IDBIndex holds its own std::unique_ptr, which it will clear out when its RefCount reaches 0.
+    RefPtr<IDBObjectStore> m_objectStoreRef;
+    unsigned m_refCount { 0 };
+    std::unique_ptr<IDBIndex> m_selfOwner;
 };
 
 } // namespace IDBClient