Modern IDB: Simplify the relationship between IDBObjectStore and IDBIndex.
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Feb 2016 23:01:20 +0000 (23:01 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Feb 2016 23:01:20 +0000 (23:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154187

Reviewed by Alex Christensen.

Source/WebCore:

Tests: storage/indexeddb/modern/deleteindex-3-private.html
       storage/indexeddb/modern/deleteindex-3.html

Instead of allowing IDBIndex to have two different lifecycle modes, it is now always
owned by an IDBObjectStore.

To support the case where an IDBIndex is deleted from its IDBObjectStore, the object
store simply hangs on to deleted indexes until it is destroyed itself.

* Modules/indexeddb/client/IDBIndexImpl.cpp:
(WebCore::IDBClient::IDBIndex::markAsDeleted):
(WebCore::IDBClient::IDBIndex::ref):
(WebCore::IDBClient::IDBIndex::deref):
* Modules/indexeddb/client/IDBIndexImpl.h:

* Modules/indexeddb/client/IDBObjectStoreImpl.cpp:
(WebCore::IDBClient::IDBObjectStore::deleteIndex):
* Modules/indexeddb/client/IDBObjectStoreImpl.h:

LayoutTests:

* storage/indexeddb/modern/deleteindex-3-expected.txt: Added.
* storage/indexeddb/modern/deleteindex-3-private-expected.txt: Added.
* storage/indexeddb/modern/deleteindex-3-private.html: Added.
* storage/indexeddb/modern/deleteindex-3.html: Added.
* storage/indexeddb/modern/resources/deleteindex-3.js: Added.

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

LayoutTests/ChangeLog
LayoutTests/storage/indexeddb/modern/deleteindex-3-expected.txt [new file with mode: 0644]
LayoutTests/storage/indexeddb/modern/deleteindex-3-private-expected.txt [new file with mode: 0644]
LayoutTests/storage/indexeddb/modern/deleteindex-3-private.html [new file with mode: 0644]
LayoutTests/storage/indexeddb/modern/deleteindex-3.html [new file with mode: 0644]
LayoutTests/storage/indexeddb/modern/resources/deleteindex-3.js [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp
Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.h
Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp
Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.h

index ae30385..31b07bf 100644 (file)
@@ -1,3 +1,16 @@
+2016-02-12  Brady Eidson  <beidson@apple.com>
+
+        Modern IDB: Simplify the relationship between IDBObjectStore and IDBIndex.
+        https://bugs.webkit.org/show_bug.cgi?id=154187
+
+        Reviewed by Alex Christensen.
+
+        * storage/indexeddb/modern/deleteindex-3-expected.txt: Added.
+        * storage/indexeddb/modern/deleteindex-3-private-expected.txt: Added.
+        * storage/indexeddb/modern/deleteindex-3-private.html: Added.
+        * storage/indexeddb/modern/deleteindex-3.html: Added.
+        * storage/indexeddb/modern/resources/deleteindex-3.js: Added.
+
 2016-02-12  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [ES6] Implement @@search
diff --git a/LayoutTests/storage/indexeddb/modern/deleteindex-3-expected.txt b/LayoutTests/storage/indexeddb/modern/deleteindex-3-expected.txt
new file mode 100644 (file)
index 0000000..7305d58
--- /dev/null
@@ -0,0 +1,23 @@
+This tests creating an index, deleting it, creating a new one with the same name, and making sure those two indexes aren't equal.
+
+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;
+
+indexedDB.deleteDatabase(dbname)
+indexedDB.open(dbname)
+First and Second indexes are the same: false
+First index keyPath: foo
+Second index keyPath: bar
+First index unique: false
+Second index unique: true
+First index references back to objectStore: true
+Second index references back to objectStore: true
+objectStore's index for 'FooIndex' is equal to first index: false
+objectStore's index for 'FooIndex' is equal to second index: true
+Initial upgrade versionchange transaction complete
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/storage/indexeddb/modern/deleteindex-3-private-expected.txt b/LayoutTests/storage/indexeddb/modern/deleteindex-3-private-expected.txt
new file mode 100644 (file)
index 0000000..7305d58
--- /dev/null
@@ -0,0 +1,23 @@
+This tests creating an index, deleting it, creating a new one with the same name, and making sure those two indexes aren't equal.
+
+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;
+
+indexedDB.deleteDatabase(dbname)
+indexedDB.open(dbname)
+First and Second indexes are the same: false
+First index keyPath: foo
+Second index keyPath: bar
+First index unique: false
+Second index unique: true
+First index references back to objectStore: true
+Second index references back to objectStore: true
+objectStore's index for 'FooIndex' is equal to first index: false
+objectStore's index for 'FooIndex' is equal to second index: true
+Initial upgrade versionchange transaction complete
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/storage/indexeddb/modern/deleteindex-3-private.html b/LayoutTests/storage/indexeddb/modern/deleteindex-3-private.html
new file mode 100644 (file)
index 0000000..105b867
--- /dev/null
@@ -0,0 +1,12 @@
+<html>
+<head>
+<script>
+enablePrivateBrowsing = true;
+</script>
+<script src="../../../resources/js-test.js"></script>
+<script src="../resources/shared.js"></script>
+</head>
+<body>
+<script src="resources/deleteindex-3.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/storage/indexeddb/modern/deleteindex-3.html b/LayoutTests/storage/indexeddb/modern/deleteindex-3.html
new file mode 100644 (file)
index 0000000..515ef8b
--- /dev/null
@@ -0,0 +1,9 @@
+<html>
+<head>
+<script src="../../../resources/js-test.js"></script>
+<script src="../resources/shared.js"></script>
+</head>
+<body>
+<script src="resources/deleteindex-3.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/storage/indexeddb/modern/resources/deleteindex-3.js b/LayoutTests/storage/indexeddb/modern/resources/deleteindex-3.js
new file mode 100644 (file)
index 0000000..0e68599
--- /dev/null
@@ -0,0 +1,41 @@
+description("This tests creating an index, deleting it, creating a new one with the same name, and making sure those two indexes aren't equal.");
+
+indexedDBTest(prepareDatabase);
+
+function prepareDatabase(event)
+{
+    var versionTransaction = event.target.transaction;
+    var database = event.target.result;
+    var objectStore = database.createObjectStore("TestObjectStore");
+    objectStore.put({ foo: "a", bar: "A" }, 1);
+    objectStore.put({ foo: "b", bar: "B" }, 2);
+
+    var index1 = objectStore.createIndex("FooIndex", "foo");
+    objectStore.deleteIndex("FooIndex");
+    var index2 = objectStore.createIndex("FooIndex", "bar", { unique: true });
+
+    debug("First and Second indexes are the same: " + (index1 == index2));
+    debug("First index keyPath: " + index1.keyPath);
+    debug("Second index keyPath: " + index2.keyPath);
+    debug("First index unique: " + index1.unique);
+    debug("Second index unique: " + index2.unique);
+    debug("First index references back to objectStore: " + (index1.objectStore == objectStore));
+    debug("Second index references back to objectStore: " + (index2.objectStore == objectStore));
+    debug("objectStore's index for 'FooIndex' is equal to first index: " + (objectStore.index("FooIndex") == index1));
+    debug("objectStore's index for 'FooIndex' is equal to second index: " + (objectStore.index("FooIndex") == index2));
+    
+    versionTransaction.onabort = function(event) {
+        debug("Initial upgrade versionchange transaction unexpected abort");
+        finishJSTest();
+    }
+
+    versionTransaction.oncomplete = function(event) {
+        debug("Initial upgrade versionchange transaction complete");
+        finishJSTest();
+    }
+
+    versionTransaction.onerror = function(event) {
+        debug("Initial upgrade versionchange transaction unexpected error" + event);
+        finishJSTest();
+    }
+}
index 83f4643..e5ff7a0 100644 (file)
@@ -1,3 +1,29 @@
+2016-02-12  Brady Eidson  <beidson@apple.com>
+
+        Modern IDB: Simplify the relationship between IDBObjectStore and IDBIndex.
+        https://bugs.webkit.org/show_bug.cgi?id=154187
+
+        Reviewed by Alex Christensen.
+
+        Tests: storage/indexeddb/modern/deleteindex-3-private.html
+               storage/indexeddb/modern/deleteindex-3.html
+
+        Instead of allowing IDBIndex to have two different lifecycle modes, it is now always
+        owned by an IDBObjectStore.
+        
+        To support the case where an IDBIndex is deleted from its IDBObjectStore, the object
+        store simply hangs on to deleted indexes until it is destroyed itself.
+        
+        * Modules/indexeddb/client/IDBIndexImpl.cpp:
+        (WebCore::IDBClient::IDBIndex::markAsDeleted):
+        (WebCore::IDBClient::IDBIndex::ref):
+        (WebCore::IDBClient::IDBIndex::deref):
+        * Modules/indexeddb/client/IDBIndexImpl.h:
+        
+        * Modules/indexeddb/client/IDBObjectStoreImpl.cpp:
+        (WebCore::IDBClient::IDBObjectStore::deleteIndex):
+        * Modules/indexeddb/client/IDBObjectStoreImpl.h:
+
 2016-02-12  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [CSS Font Loading] Implement CSSFontFace Boilerplate
index cf2e6e2..aa1057c 100644 (file)
@@ -339,49 +339,20 @@ RefPtr<WebCore::IDBRequest> IDBIndex::doGetKey(ScriptExecutionContext& context,
     return transaction.requestGetKey(context, *this, range);
 }
 
-void IDBIndex::markAsDeleted(std::unique_ptr<IDBIndex>&& indexOwner)
+void IDBIndex::markAsDeleted()
 {
     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();
+    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;
-    }
+    m_objectStore.deref();
 }
 
 } // namespace IDBClient
index b082f67..4c73a7c 100644 (file)
@@ -79,7 +79,7 @@ public:
 
     IDBObjectStore& modernObjectStore() { return m_objectStore; }
 
-    void markAsDeleted(std::unique_ptr<IDBIndex>&&);
+    void markAsDeleted();
     bool isDeleted() const { return m_deleted; }
 
     virtual bool isModern() const override { return true; }
@@ -96,19 +96,9 @@ private:
 
     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.
+    // IDBIndex objects are always owned by their referencing IDBObjectStore.
+    // Indexes will never outlive ObjectStores so its okay to keep a raw C++ reference here.
     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
index 2a2d390..fd17d7a 100644 (file)
@@ -567,8 +567,11 @@ void IDBObjectStore::deleteIndex(const String& name, ExceptionCodeWithMessage& e
 
     {
         Locker<Lock> locker(m_referencedIndexLock);
-        if (auto index = m_referencedIndexes.take(name))
-            index->markAsDeleted(WTFMove(index));
+        if (auto index = m_referencedIndexes.take(name)) {
+            index->markAsDeleted();
+            m_deletedIndexes.add(WTFMove(index));
+        }
+
     }
 
     m_transaction->deleteIndex(m_info.identifier(), name);
index 6096afe..21b283d 100644 (file)
@@ -117,6 +117,7 @@ private:
 
     mutable Lock m_referencedIndexLock;
     HashMap<String, std::unique_ptr<IDBIndex>> m_referencedIndexes;
+    HashSet<std::unique_ptr<IDBIndex>> m_deletedIndexes;
 };
 
 } // namespace IDBClient