Title: [141316] trunk
Revision
141316
Author
alecfl...@chromium.org
Date
2013-01-30 14:39:41 -0800 (Wed, 30 Jan 2013)

Log Message

IndexedDB: Avoid crashing when deleting indexes
https://bugs.webkit.org/show_bug.cgi?id=108356

Reviewed by Tony Chang.

Source/WebCore:

It is reasonable that the backend aborts a transaction before
the frontend is aware, depending on the timing of events. This
allows the transactionId to be invalid rather than asserting.

Test: storage/indexeddb/createIndex-after-failure.html

* Modules/indexeddb/IDBDatabaseBackendImpl.cpp:
(WebCore::IDBDatabaseBackendImpl::createObjectStore):
(WebCore::IDBDatabaseBackendImpl::deleteObjectStore):
(WebCore::IDBDatabaseBackendImpl::createIndex):
(WebCore::IDBDatabaseBackendImpl::deleteIndex):
(WebCore::IDBDatabaseBackendImpl::get):
(WebCore::IDBDatabaseBackendImpl::put):
(WebCore::IDBDatabaseBackendImpl::setIndexKeys):
(WebCore::IDBDatabaseBackendImpl::setIndexesReady):
(WebCore::IDBDatabaseBackendImpl::openCursor):
(WebCore::IDBDatabaseBackendImpl::count):
(WebCore::IDBDatabaseBackendImpl::deleteRange):
(WebCore::IDBDatabaseBackendImpl::clear):

LayoutTests:

This test works on an edge case around the asynchronous
creation/deletion of indexes that used to crash. It doesn't fail/crash even
without this patch but was the test condition that uncovered the overall problem
before https://bugs.webkit.org/show_bug.cgi?id=107311 changed the timing of
some of the events.

* storage/indexeddb/createIndex-after-failure.html: Added.
* storage/indexeddb/resources/createIndex-after-failure.js: Added.
(sleep):
(prepareDatabase):
(deleteIndexAfterGet):
* storage/indexeddb/resources/shared.js:
(.requests.forEach):
(waitForRequests):

* storage/indexeddb/createIndex-after-failure.html: Added.
* storage/indexeddb/resources/createIndex-after-failure.js: Added.
(prepareDatabase):
(deleteIndexAfterGet):
* storage/indexeddb/resources/shared.js:
(.requests.forEach):
(waitForRequests):

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (141315 => 141316)


--- trunk/LayoutTests/ChangeLog	2013-01-30 22:36:24 UTC (rev 141315)
+++ trunk/LayoutTests/ChangeLog	2013-01-30 22:39:41 UTC (rev 141316)
@@ -1,3 +1,33 @@
+2013-01-30  Alec Flett  <alecfl...@chromium.org>
+
+        IndexedDB: Avoid crashing when deleting indexes
+        https://bugs.webkit.org/show_bug.cgi?id=108356
+
+        Reviewed by Tony Chang.
+
+        This test works on an edge case around the asynchronous
+        creation/deletion of indexes that used to crash. It doesn't fail/crash even
+        without this patch but was the test condition that uncovered the overall problem
+        before https://bugs.webkit.org/show_bug.cgi?id=107311 changed the timing of
+        some of the events.
+
+        * storage/indexeddb/createIndex-after-failure.html: Added.
+        * storage/indexeddb/resources/createIndex-after-failure.js: Added.
+        (sleep):
+        (prepareDatabase):
+        (deleteIndexAfterGet):
+        * storage/indexeddb/resources/shared.js:
+        (.requests.forEach):
+        (waitForRequests):
+
+        * storage/indexeddb/createIndex-after-failure.html: Added.
+        * storage/indexeddb/resources/createIndex-after-failure.js: Added.
+        (prepareDatabase):
+        (deleteIndexAfterGet):
+        * storage/indexeddb/resources/shared.js:
+        (.requests.forEach):
+        (waitForRequests):
+
 2013-01-30  Philip Rogers  <p...@google.com>
 
          Update svg/zoom test expectations after r141303

Added: trunk/LayoutTests/storage/indexeddb/createIndex-after-failure-expected.txt (0 => 141316)


--- trunk/LayoutTests/storage/indexeddb/createIndex-after-failure-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/storage/indexeddb/createIndex-after-failure-expected.txt	2013-01-30 22:39:41 UTC (rev 141316)
@@ -0,0 +1,31 @@
+Test IndexedDB's basics.
+
+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 = "createIndex-after-failure.html"
+indexedDB.deleteDatabase(dbname)
+indexedDB.open(dbname)
+objectStore.createIndex('index', 'key', {unique: true})
+objectStore.deleteIndex('index')
+Expecting exception from objectStore.deleteIndex('index')
+PASS Exception was thrown.
+PASS code is DOMException.NOT_FOUND_ERR
+PASS ename is 'NotFoundError'
+Now requesting object2
+now we wait.
+deleteIndexAfterGet()
+Expecting exception from objectStore.deleteIndex('index')
+PASS Exception was thrown.
+PASS code is DOMException.NOT_FOUND_ERR
+PASS ename is 'NotFoundError'
+Expecting exception from objectStore.deleteIndex('index')
+PASS Exception was thrown.
+PASS code is DOMException.NOT_FOUND_ERR
+PASS ename is 'NotFoundError'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/storage/indexeddb/createIndex-after-failure.html (0 => 141316)


--- trunk/LayoutTests/storage/indexeddb/createIndex-after-failure.html	                        (rev 0)
+++ trunk/LayoutTests/storage/indexeddb/createIndex-after-failure.html	2013-01-30 22:39:41 UTC (rev 141316)
@@ -0,0 +1,10 @@
+<html>
+<head>
+<script src=""
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>

Added: trunk/LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js (0 => 141316)


--- trunk/LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js	                        (rev 0)
+++ trunk/LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js	2013-01-30 22:39:41 UTC (rev 141316)
@@ -0,0 +1,46 @@
+if (this.importScripts) {
+    importScripts('../../../fast/js/resources/js-test-pre.js');
+    importScripts('shared.js');
+}
+
+description("Test IndexedDB's basics.");
+
+indexedDBTest(prepareDatabase);
+function prepareDatabase(event)
+{
+    trans = event.target.transaction;
+    db = event.target.result;
+
+    db.createObjectStore("objectStore");
+    objectStore = trans.objectStore("objectStore");
+    var req1 = objectStore.put({"key": "value"}, "object1");
+    var req2 = objectStore.put({"key": "value"}, "object2");
+
+    waitForRequests([req1, req2], createIndex);
+}
+
+function createIndex() {
+    // This will asynchronously abort in the backend because of constraint failures.
+    evalAndLog("objectStore.createIndex('index', 'key', {unique: true})");
+    // Now immediately delete it.
+    evalAndLog("objectStore.deleteIndex('index')");
+    // Delete it again: backend may have asynchronously aborted the
+    // index creation, but this makes sure the frontend doesn't get
+    // confused and crash, or think the index still exists.
+    evalAndExpectException("objectStore.deleteIndex('index')", "DOMException.NOT_FOUND_ERR", "'NotFoundError'");
+    debug("Now requesting object2");
+    var req3 = objectStore.get("object2");
+    req3._onsuccess_ = deleteIndexAfterGet;
+    req3._onerror_ = unexpectedErrorCallback;
+    debug("now we wait.");
+}
+
+function deleteIndexAfterGet() {
+    // so we will delete it next, but it should already be gone... right?
+    debug("deleteIndexAfterGet()");
+    // the index should still be gone, and this should not crash.
+    evalAndExpectException("objectStore.deleteIndex('index')", "DOMException.NOT_FOUND_ERR", "'NotFoundError'");
+    evalAndExpectException("objectStore.deleteIndex('index')", "DOMException.NOT_FOUND_ERR", "'NotFoundError'");
+
+    finishJSTest();
+}
\ No newline at end of file

Modified: trunk/LayoutTests/storage/indexeddb/resources/shared.js (141315 => 141316)


--- trunk/LayoutTests/storage/indexeddb/resources/shared.js	2013-01-30 22:36:24 UTC (rev 141315)
+++ trunk/LayoutTests/storage/indexeddb/resources/shared.js	2013-01-30 22:39:41 UTC (rev 141316)
@@ -204,3 +204,21 @@
             (optionalParameters['runAfterOpen'])();
     };
 }
+
+function waitForRequests(requests, callback) {
+    var count = requests.length;
+
+    if (!count) {
+        callback(requests);
+        return;
+    }
+
+    requests.forEach(function(req) {
+        req._onsuccess_ = function() {
+            --count;
+            if (!count)
+                callback(requests);
+        };
+        req._onerror_ = unexpectedErrorCallback;
+    });
+}

Modified: trunk/Source/WebCore/ChangeLog (141315 => 141316)


--- trunk/Source/WebCore/ChangeLog	2013-01-30 22:36:24 UTC (rev 141315)
+++ trunk/Source/WebCore/ChangeLog	2013-01-30 22:39:41 UTC (rev 141316)
@@ -1,3 +1,30 @@
+2013-01-30  Alec Flett  <alecfl...@chromium.org>
+
+        IndexedDB: Avoid crashing when deleting indexes
+        https://bugs.webkit.org/show_bug.cgi?id=108356
+
+        Reviewed by Tony Chang.
+
+        It is reasonable that the backend aborts a transaction before
+        the frontend is aware, depending on the timing of events. This
+        allows the transactionId to be invalid rather than asserting.
+
+        Test: storage/indexeddb/createIndex-after-failure.html
+
+        * Modules/indexeddb/IDBDatabaseBackendImpl.cpp:
+        (WebCore::IDBDatabaseBackendImpl::createObjectStore):
+        (WebCore::IDBDatabaseBackendImpl::deleteObjectStore):
+        (WebCore::IDBDatabaseBackendImpl::createIndex):
+        (WebCore::IDBDatabaseBackendImpl::deleteIndex):
+        (WebCore::IDBDatabaseBackendImpl::get):
+        (WebCore::IDBDatabaseBackendImpl::put):
+        (WebCore::IDBDatabaseBackendImpl::setIndexKeys):
+        (WebCore::IDBDatabaseBackendImpl::setIndexesReady):
+        (WebCore::IDBDatabaseBackendImpl::openCursor):
+        (WebCore::IDBDatabaseBackendImpl::count):
+        (WebCore::IDBDatabaseBackendImpl::deleteRange):
+        (WebCore::IDBDatabaseBackendImpl::clear):
+
 2013-01-30  Kentaro Hara  <hara...@chromium.org>
 
         isSameAsCurrentState() should take SerializedScriptValue* instead of PassRefPtr

Modified: trunk/Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp (141315 => 141316)


--- trunk/Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp	2013-01-30 22:36:24 UTC (rev 141315)
+++ trunk/Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp	2013-01-30 22:39:41 UTC (rev 141316)
@@ -556,11 +556,12 @@
 void IDBDatabaseBackendImpl::createObjectStore(int64_t transactionId, int64_t objectStoreId, const String& name, const IDBKeyPath& keyPath, bool autoIncrement)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::createObjectStore");
-    ASSERT(!m_metadata.objectStores.contains(objectStoreId));
-
-    ASSERT(m_transactions.contains(transactionId));
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
+    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);
 
+    ASSERT(!m_metadata.objectStores.contains(objectStoreId));
     IDBObjectStoreMetadata objectStoreMetadata(name, objectStoreId, keyPath, autoIncrement, IDBDatabaseBackendInterface::MinimumIndexId);
 
     transaction->scheduleTask(CreateObjectStoreOperation::create(m_backingStore, objectStoreMetadata), CreateObjectStoreAbortOperation::create(this, objectStoreId));
@@ -581,13 +582,14 @@
 void IDBDatabaseBackendImpl::deleteObjectStore(int64_t transactionId, int64_t objectStoreId)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::deleteObjectStore");
+    IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
+    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);
+
     ASSERT(m_metadata.objectStores.contains(objectStoreId));
     const IDBObjectStoreMetadata& objectStoreMetadata = m_metadata.objectStores.get(objectStoreId);
 
-    ASSERT(m_transactions.contains(transactionId));
-    IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
-    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);
-
     transaction->scheduleTask(DeleteObjectStoreOperation::create(m_backingStore, objectStoreMetadata),  DeleteObjectStoreAbortOperation::create(this, objectStoreMetadata));
     removeObjectStore(objectStoreId);
 }
@@ -595,22 +597,22 @@
 void IDBDatabaseBackendImpl::createIndex(int64_t transactionId, int64_t objectStoreId, int64_t indexId, const String& name, const IDBKeyPath& keyPath, bool unique, bool multiEntry)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::createIndex");
+    IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
+    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);
+
     ASSERT(m_metadata.objectStores.contains(objectStoreId));
     const IDBObjectStoreMetadata objectStore = m_metadata.objectStores.get(objectStoreId);
 
     ASSERT(!objectStore.indexes.contains(indexId));
     const IDBIndexMetadata indexMetadata(name, indexId, keyPath, unique, multiEntry);
 
-    ASSERT(m_transactions.contains(transactionId));
-    IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
-    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);
-
     transaction->scheduleTask(CreateIndexOperation::create(m_backingStore, objectStoreId, indexMetadata), CreateIndexAbortOperation::create(this, objectStoreId, indexId));
 
     addIndex(objectStoreId, indexMetadata, indexId);
 }
 
-
 void CreateIndexOperation::perform(IDBTransactionBackendImpl* transaction)
 {
     IDB_TRACE("CreateIndexOperation");
@@ -630,16 +632,17 @@
 void IDBDatabaseBackendImpl::deleteIndex(int64_t transactionId, int64_t objectStoreId, int64_t indexId)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::deleteIndex");
+    IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
+    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);
+
     ASSERT(m_metadata.objectStores.contains(objectStoreId));
     const IDBObjectStoreMetadata objectStore = m_metadata.objectStores.get(objectStoreId);
 
     ASSERT(objectStore.indexes.contains(indexId));
     const IDBIndexMetadata& indexMetadata = objectStore.indexes.get(indexId);
 
-    ASSERT(m_transactions.contains(transactionId));
-    IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
-    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);
-
     transaction->scheduleTask(DeleteIndexOperation::create(m_backingStore, objectStoreId, indexId), DeleteIndexAbortOperation::create(this, objectStoreId, indexMetadata));
 
     removeIndex(objectStoreId, indexId);
@@ -682,8 +685,9 @@
 void IDBDatabaseBackendImpl::get(int64_t transactionId, int64_t objectStoreId, int64_t indexId, PassRefPtr<IDBKeyRange> keyRange, bool keyOnly, PassRefPtr<IDBCallbacks> callbacks)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::get");
-    ASSERT(m_transactions.contains(transactionId));
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
 
     transaction->scheduleTask(GetOperation::create(m_backingStore, m_metadata, objectStoreId, indexId, keyRange, keyOnly ? IDBCursorBackendInterface::KeyOnly : IDBCursorBackendInterface::KeyAndValue, callbacks));
 }
@@ -779,9 +783,9 @@
 void IDBDatabaseBackendImpl::put(int64_t transactionId, int64_t objectStoreId, Vector<uint8_t>* value, PassRefPtr<IDBKey> key, PutMode putMode, PassRefPtr<IDBCallbacks> callbacks, const Vector<int64_t>& indexIds, const Vector<IndexKeys>& indexKeys)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::put");
-
-    ASSERT(m_transactions.contains(transactionId));
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
     ASSERT(transaction->mode() != IDBTransaction::READ_ONLY);
 
     const IDBObjectStoreMetadata objectStoreMetadata = m_metadata.objectStores.get(objectStoreId);
@@ -862,16 +866,13 @@
     m_callbacks->onSuccess(key.release());
 }
 
-
 void IDBDatabaseBackendImpl::setIndexKeys(int64_t transactionId, int64_t objectStoreId, PassRefPtr<IDBKey> prpPrimaryKey, const Vector<int64_t>& indexIds, const Vector<IndexKeys>& indexKeys)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::setIndexKeys");
-    if (!m_transactions.contains(transactionId))
-        return;
-
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
-    if (transaction->isFinished())
+    if (!transaction || transaction->isFinished())
         return;
+    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);
 
     RefPtr<IDBKey> primaryKey = prpPrimaryKey;
     RefPtr<IDBBackingStore> store = backingStore();
@@ -914,11 +915,8 @@
 {
     IDB_TRACE("IDBObjectStoreBackendImpl::setIndexesReady");
 
-    if (!m_transactions.contains(transactionId))
-        return;
-
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
-    if (transaction->isFinished())
+    if (!transaction || transaction->isFinished())
         return;
 
     if (!transaction->scheduleTask(IDBDatabaseBackendInterface::PreemptiveTask, SetIndexesReadyOperation::create(indexIds.size())))
@@ -935,8 +933,9 @@
 void IDBDatabaseBackendImpl::openCursor(int64_t transactionId, int64_t objectStoreId, int64_t indexId, PassRefPtr<IDBKeyRange> keyRange, unsigned short direction, bool keyOnly, TaskType taskType, PassRefPtr<IDBCallbacks> callbacks)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::openCursor");
-    ASSERT(m_transactions.contains(transactionId));
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
 
     transaction->scheduleTask(OpenCursorOperation::create(m_backingStore, id(), objectStoreId, indexId, keyRange, direction, keyOnly ? IDBCursorBackendInterface::KeyOnly : IDBCursorBackendInterface::KeyAndValue, taskType, callbacks));
 }
@@ -978,8 +977,10 @@
 void IDBDatabaseBackendImpl::count(int64_t transactionId, int64_t objectStoreId, int64_t indexId, PassRefPtr<IDBKeyRange> keyRange, PassRefPtr<IDBCallbacks> callbacks)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::count");
-    ASSERT(m_transactions.contains(transactionId));
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
+
     ASSERT(m_metadata.objectStores.contains(objectStoreId));
     transaction->scheduleTask(CountOperation::create(m_backingStore, id(), objectStoreId, indexId, keyRange, callbacks));
 }
@@ -1009,9 +1010,9 @@
 void IDBDatabaseBackendImpl::deleteRange(int64_t transactionId, int64_t objectStoreId, PassRefPtr<IDBKeyRange> keyRange, PassRefPtr<IDBCallbacks> callbacks)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::deleteRange");
-
-    ASSERT(m_transactions.contains(transactionId));
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
     ASSERT(transaction->mode() != IDBTransaction::READ_ONLY);
 
     transaction->scheduleTask(DeleteRangeOperation::create(m_backingStore, id(), objectStoreId, keyRange, callbacks));
@@ -1033,9 +1034,9 @@
 void IDBDatabaseBackendImpl::clear(int64_t transactionId, int64_t objectStoreId, PassRefPtr<IDBCallbacks> callbacks)
 {
     IDB_TRACE("IDBDatabaseBackendImpl::clear");
-
-    ASSERT(m_transactions.contains(transactionId));
     IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
+    if (!transaction)
+        return;
     ASSERT(transaction->mode() != IDBTransaction::READ_ONLY);
 
     transaction->scheduleTask(ClearOperation::create(m_backingStore, id(), objectStoreId, callbacks));
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to