IDB: indexeddb/mozilla/add-twice-failure.html fails
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Feb 2014 00:26:59 +0000 (00:26 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Feb 2014 00:26:59 +0000 (00:26 +0000)
<rdar://problem/15982569> and https://bugs.webkit.org/show_bug.cgi?id=128208

Reviewed by Tim Horton.

Source/WebCore:

Covered specifically by indexeddb/mozilla/add-twice-failure.html and a handful of others.

* Modules/indexeddb/IDBRequest.cpp:
(WebCore::IDBRequest::onError): Improve logging.

* Modules/indexeddb/IDBTransactionBackend.cpp:
(WebCore::IDBTransactionBackend::abort): Improve logging.

* Modules/indexeddb/IDBTransactionBackendOperations.cpp:
(WebCore::PutOperation::perform): Don’t abort the transaction when an error occurred.

* WebCore.exp.in:

Source/WebKit2:

Note that besides making indexeddb/mozilla/add-twice-failure.html pass this also makes some other tests
pass and also improves the failure modes of others.

A full accounting of which tests pass is coming soon.

* DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:
(WebKit::UniqueIDBDatabase::putRecordInBackingStore): After checking for existence of the key and before
  adding the record, remove any previous record. (Defined by the spec, found exploring this test).

* DatabaseProcess/IndexedDB/UniqueIDBDatabaseBackingStore.h:
* DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:
(WebKit::UniqueIDBDatabaseBackingStoreSQLite::keyExistsInObjectStore): Implement this.
(WebKit::UniqueIDBDatabaseBackingStoreSQLite::deleteRecord): Add a form to be used as mentioned above in
  UniqueIDBDatabase::putRecordInBackingStore.
(WebKit::UniqueIDBDatabaseBackingStoreSQLite::getKeyRecordFromObjectStore): CAST the key argument properly.
(WebKit::UniqueIDBDatabaseBackingStoreSQLite::getKeyRangeRecordFromObjectStore): Ditto.
* DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h:

LayoutTests:

* platform/mac-wk2/TestExpectations: Reenable this test for WK2

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/mac-wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/IDBRequest.cpp
Source/WebCore/Modules/indexeddb/IDBTransactionBackend.cpp
Source/WebCore/Modules/indexeddb/IDBTransactionBackendOperations.cpp
Source/WebCore/WebCore.exp.in
Source/WebKit2/ChangeLog
Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp
Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseBackingStore.h
Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp
Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h

index 59d91f9..101f88d 100644 (file)
@@ -1,3 +1,12 @@
+2014-02-04  Brady Eidson  <beidson@apple.com>
+
+        IDB: indexeddb/mozilla/add-twice-failure.html fails
+        <rdar://problem/15982569> and https://bugs.webkit.org/show_bug.cgi?id=128208
+
+        Reviewed by Tim Horton.
+
+        * platform/mac-wk2/TestExpectations: Reenable this test for WK2
+
 2014-02-04  Yoav Weiss  <yoav@yoav.ws>
 
         Use srcset's pixel density to determine intrinsic size
index e6c3b68..9c7948d 100644 (file)
@@ -464,6 +464,10 @@ fast/forms/color/input-color-onchange-event.html [ Pass ]
 # This test times out in WebKit1 only.
 fullscreen/anonymous-block-merge-crash.html [ Pass ]
 
+# All IndexedDB tests are skipped in WK1.
+# Reenable individual tests here that are known to pass, with the eventual goal of re-enabling the entire directory.
+storage/indexeddb/mozilla/add-twice-failure.html [ Pass ]
+
 ### END OF (5) Features that are not supported in WebKit1, so skipped in mac/TestExpectations then re-enabled here
 ########################################
 
index aea654e..77e5180 100644 (file)
@@ -1,3 +1,23 @@
+2014-02-04  Brady Eidson  <beidson@apple.com>
+
+        IDB: indexeddb/mozilla/add-twice-failure.html fails
+        <rdar://problem/15982569> and https://bugs.webkit.org/show_bug.cgi?id=128208
+
+        Reviewed by Tim Horton.
+
+        Covered specifically by indexeddb/mozilla/add-twice-failure.html and a handful of others.
+
+        * Modules/indexeddb/IDBRequest.cpp:
+        (WebCore::IDBRequest::onError): Improve logging.
+
+        * Modules/indexeddb/IDBTransactionBackend.cpp:
+        (WebCore::IDBTransactionBackend::abort): Improve logging.
+
+        * Modules/indexeddb/IDBTransactionBackendOperations.cpp:
+        (WebCore::PutOperation::perform): Don’t abort the transaction when an error occurred.
+
+        * WebCore.exp.in:
+
 2014-02-04  Yoav Weiss  <yoav@yoav.ws>
 
         Use srcset's pixel density to determine intrinsic size
index 8da84cb..5b2ead5 100644 (file)
@@ -249,7 +249,7 @@ bool IDBRequest::shouldEnqueueEvent() const
 
 void IDBRequest::onError(PassRefPtr<IDBDatabaseError> error)
 {
-    LOG(StorageAPI, "IDBRequest::onError()");
+    LOG(StorageAPI, "IDBRequest::onError() (%s) '%s'", error->name().utf8().data(), error->message().utf8().data());
     if (!shouldEnqueueEvent())
         return;
 
index bfb8088..0fd8d73 100644 (file)
@@ -116,7 +116,13 @@ void IDBTransactionBackend::abort()
 
 void IDBTransactionBackend::abort(PassRefPtr<IDBDatabaseError> error)
 {
-    LOG(StorageAPI, "IDBTransactionBackend::abort");
+#ifndef NDEBUG
+    if (error)
+        LOG(StorageAPI, "IDBTransactionBackend::abort - (%s) %s", error->name().utf8().data(), error->message().utf8().data());
+    else
+        LOG(StorageAPI, "IDBTransactionBackend::abort (no error)");
+#endif
+
     if (m_state == Finished)
         return;
 
index 8d92097..86b7c2b 100644 (file)
@@ -123,10 +123,7 @@ void PutOperation::perform(std::function<void()> completionCallback)
     ASSERT(m_transaction->mode() != IndexedDB::TransactionMode::ReadOnly);
     ASSERT(m_indexIDs.size() == m_indexKeys.size());
 
-    RefPtr<PutOperation> operation(this);
-    STANDARD_DATABASE_ERROR_CALLBACK;
-
-    m_transaction->database().serverConnection().put(*m_transaction, *this, [this, operation, operationCallback](PassRefPtr<IDBKey> key, PassRefPtr<IDBDatabaseError> prpError) {
+    m_transaction->database().serverConnection().put(*m_transaction, *this, [this, completionCallback](PassRefPtr<IDBKey> key, PassRefPtr<IDBDatabaseError> prpError) {
         RefPtr<IDBDatabaseError> error = prpError;
         if (key) {
             ASSERT(!error);
@@ -135,7 +132,7 @@ void PutOperation::perform(std::function<void()> completionCallback)
             ASSERT(error);
             m_callbacks->onError(error);
         }
-        operationCallback(error.release());
+        completionCallback();
     });
 }
 
index 3f61973..cecaffa 100644 (file)
@@ -774,6 +774,7 @@ __ZN7WebCore19ResourceRequestBase6setURLERKNS_3URLE
 __ZN7WebCore19PlatformCAAnimation6createEP19CAPropertyAnimation
 __ZN7WebCore19PlatformCAAnimation10setToValueERKNS_20TransformationMatrixE
 __ZN7WebCore19PlatformCAAnimationD1Ev
+__ZN7WebCore19SQLResultConstraintE
 __ZN7WebCore19TextResourceDecoder5flushEv
 __ZN7WebCore19TextResourceDecoder6decodeEPKcm
 __ZN7WebCore19TextResourceDecoderC1ERKN3WTF6StringERKNS_12TextEncodingEb
index fb37579..7854ff4 100644 (file)
@@ -1,3 +1,28 @@
+2014-02-04  Brady Eidson  <beidson@apple.com>
+
+        IDB: indexeddb/mozilla/add-twice-failure.html fails
+        <rdar://problem/15982569> and https://bugs.webkit.org/show_bug.cgi?id=128208
+
+        Reviewed by Tim Horton.
+
+        Note that besides making indexeddb/mozilla/add-twice-failure.html pass this also makes some other tests
+        pass and also improves the failure modes of others.
+
+        A full accounting of which tests pass is coming soon.
+
+        * DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:
+        (WebKit::UniqueIDBDatabase::putRecordInBackingStore): After checking for existence of the key and before
+          adding the record, remove any previous record. (Defined by the spec, found exploring this test).
+
+        * DatabaseProcess/IndexedDB/UniqueIDBDatabaseBackingStore.h:
+        * DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:
+        (WebKit::UniqueIDBDatabaseBackingStoreSQLite::keyExistsInObjectStore): Implement this.
+        (WebKit::UniqueIDBDatabaseBackingStoreSQLite::deleteRecord): Add a form to be used as mentioned above in
+          UniqueIDBDatabase::putRecordInBackingStore.
+        (WebKit::UniqueIDBDatabaseBackingStoreSQLite::getKeyRecordFromObjectStore): CAST the key argument properly.
+        (WebKit::UniqueIDBDatabaseBackingStoreSQLite::getKeyRangeRecordFromObjectStore): Ditto.
+        * DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h:
+
 2014-02-04  Dan Bernstein  <mitz@apple.com>
 
         [Cocoa] Expose more WKWebProcessPlugInNodeHandle properties
index ff68583..cadfb92 100644 (file)
@@ -837,7 +837,7 @@ void UniqueIDBDatabase::putRecordInBackingStore(uint64_t requestID, const IDBIde
 
     if (putMode == IDBDatabaseBackend::AddOnly) {
         bool keyExists;
-        if (!m_backingStore->keyExistsInObjectStore(transaction, objectStoreMetadata.id, *key, keyExists)) {
+        if (!m_backingStore->keyExistsInObjectStore(transaction, objectStoreMetadata.id, keyData, keyExists)) {
             postMainThreadTask(createAsyncTask(*this, &UniqueIDBDatabase::didPutRecordInBackingStore, requestID, IDBKeyData(), IDBDatabaseException::UnknownError, ASCIILiteral("Internal backing store error checking for key existence")));
             return;
         }
@@ -847,6 +847,13 @@ void UniqueIDBDatabase::putRecordInBackingStore(uint64_t requestID, const IDBIde
         }
     }
 
+    // The spec says that even if we're about to overwrite the record, perform the steps to delete it first.
+    // This is important because formally deleting it from from the object store also removes it from the appropriate indexes.
+    if (!m_backingStore->deleteRecord(transaction, objectStoreMetadata.id, keyData)) {
+        postMainThreadTask(createAsyncTask(*this, &UniqueIDBDatabase::didPutRecordInBackingStore, requestID, IDBKeyData(), IDBDatabaseException::UnknownError, ASCIILiteral("Replacing an existing key in backing store, unable to delete previous record.")));
+        return;
+    }
+
     if (!m_backingStore->putRecord(transaction, objectStoreMetadata.id, *key, value.data(), value.size())) {
         postMainThreadTask(createAsyncTask(*this, &UniqueIDBDatabase::didPutRecordInBackingStore, requestID, IDBKeyData(), IDBDatabaseException::UnknownError, ASCIILiteral("Internal backing store error putting a record")));
         return;
index d1a93b8..4b759dc 100644 (file)
@@ -37,6 +37,7 @@ class IDBKeyRange;
 class SharedBuffer;
 
 struct IDBDatabaseMetadata;
+struct IDBKeyData;
 struct IDBObjectStoreMetadata;
 }
 
@@ -66,11 +67,12 @@ public:
     virtual bool generateKeyNumber(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, int64_t& generatedKey) = 0;
     virtual bool updateKeyGeneratorNumber(const IDBIdentifier& transactionIdentifier, int64_t objectStoreId, int64_t keyNumber, bool checkCurrent) = 0;
 
-    virtual bool keyExistsInObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKey&, bool& keyExists) = 0;
+    virtual bool keyExistsInObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyData&, bool& keyExists) = 0;
     virtual bool putRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKey&, const uint8_t* valueBuffer, size_t valueSize) = 0;
     virtual bool putIndexRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, int64_t indexID, const WebCore::IDBKeyData&, const WebCore::IDBKeyData& indexKey) = 0;
     virtual bool getIndexRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, int64_t indexID, const WebCore::IDBKeyRangeData&, RefPtr<WebCore::SharedBuffer>& result) = 0;
     virtual bool deleteRange(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyRangeData&) = 0;
+    virtual bool deleteRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyData&) = 0;
 
     virtual bool getKeyRecordFromObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKey&, RefPtr<WebCore::SharedBuffer>& result) = 0;
     virtual bool getKeyRangeRecordFromObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyRange&, RefPtr<WebCore::SharedBuffer>& result, RefPtr<WebCore::IDBKey>& resultKey) = 0;
index fc03932..6362809 100644 (file)
@@ -728,10 +728,48 @@ bool UniqueIDBDatabaseBackingStoreSQLite::updateKeyGeneratorNumber(const IDBIden
     return true;
 }
 
-bool UniqueIDBDatabaseBackingStoreSQLite::keyExistsInObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const IDBKey&, bool& keyExists)
+bool UniqueIDBDatabaseBackingStoreSQLite::keyExistsInObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const IDBKeyData& keyData, bool& keyExists)
 {
-    // FIXME: When Get support is implemented, we need to implement this also (<rdar://problem/15779644>)
-    return false;
+    ASSERT(!isMainThread());
+    ASSERT(m_sqliteDB);
+    ASSERT(m_sqliteDB->isOpen());
+
+    keyExists = false;
+
+    SQLiteIDBTransaction* transaction = m_transactions.get(transactionIdentifier);
+    if (!transaction || !transaction->inProgress()) {
+        LOG_ERROR("Attempt to see if key exists in objectstore without established, in-progress transaction");
+        return false;
+    }
+
+    RefPtr<SharedBuffer> keyBuffer = serializeIDBKeyData(keyData);
+    if (!keyBuffer) {
+        LOG_ERROR("Unable to serialize IDBKey to check for existence");
+        return false;
+    }
+
+    SQLiteStatement sql(*m_sqliteDB, ASCIILiteral("SELECT key FROM Records WHERE objectStoreID = ? AND key = CAST(? AS TEXT) LIMIT 1;"));
+    if (sql.prepare() != SQLResultOk
+        || sql.bindInt64(1, objectStoreID) != SQLResultOk
+        || sql.bindBlob(2, keyBuffer->data(), keyBuffer->size()) != SQLResultOk) {
+        LOG_ERROR("Could not get record from object store %lli from Records table (%i) - %s", objectStoreID, m_sqliteDB->lastError(), m_sqliteDB->lastErrorMsg());
+        return false;
+    }
+
+    int sqlResult = sql.step();
+    if (sqlResult == SQLResultOk || sqlResult == SQLResultDone) {
+        keyExists = false;
+        return true;
+    }
+
+    if (sqlResult != SQLResultRow) {
+        // There was an error fetching the record from the database.
+        LOG_ERROR("Could not check if key exists in object store (%i) - %s", m_sqliteDB->lastError(), m_sqliteDB->lastErrorMsg());
+        return false;
+    }
+
+    keyExists = true;
+    return true;
 }
 
 bool UniqueIDBDatabaseBackingStoreSQLite::putRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const IDBKey& key, const uint8_t* valueBuffer, size_t valueSize)
@@ -843,6 +881,26 @@ bool UniqueIDBDatabaseBackingStoreSQLite::getIndexRecord(const IDBIdentifier& tr
     return true;
 }
 
+bool UniqueIDBDatabaseBackingStoreSQLite::deleteRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyData& keyData)
+{
+    ASSERT(!isMainThread());
+    ASSERT(m_sqliteDB);
+    ASSERT(m_sqliteDB->isOpen());
+
+    SQLiteIDBTransaction* transaction = m_transactions.get(transactionIdentifier);
+    if (!transaction || !transaction->inProgress()) {
+        LOG_ERROR("Attempt to get count from database without an established, in-progress transaction");
+        return false;
+    }
+
+    if (transaction->mode() == IndexedDB::TransactionMode::ReadOnly) {
+        LOG_ERROR("Attempt to delete range from a read-only transaction");
+        return false;
+    }
+
+    return deleteRecord(*transaction, objectStoreID, keyData);
+}
+
 bool UniqueIDBDatabaseBackingStoreSQLite::deleteRange(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const IDBKeyRangeData& keyRangeData)
 {
     ASSERT(!isMainThread());
@@ -964,7 +1022,7 @@ bool UniqueIDBDatabaseBackingStoreSQLite::getKeyRecordFromObjectStore(const IDBI
     }
 
     {
-        SQLiteStatement sql(*m_sqliteDB, ASCIILiteral("SELECT value FROM Records WHERE objectStoreID = ? AND key = ?;"));
+        SQLiteStatement sql(*m_sqliteDB, ASCIILiteral("SELECT value FROM Records WHERE objectStoreID = ? AND key = CAST(? AS TEXT);"));
         if (sql.prepare() != SQLResultOk
             || sql.bindInt64(1, objectStoreID) != SQLResultOk
             || sql.bindBlob(2, keyBuffer->data(), keyBuffer->size()) != SQLResultOk) {
@@ -1016,7 +1074,7 @@ bool UniqueIDBDatabaseBackingStoreSQLite::getKeyRangeRecordFromObjectStore(const
     }
 
     {
-        SQLiteStatement sql(*m_sqliteDB, ASCIILiteral("SELECT value FROM Records WHERE objectStoreID = ? AND key >= ? AND key <= ? ORDER BY key;"));
+        SQLiteStatement sql(*m_sqliteDB, ASCIILiteral("SELECT value FROM Records WHERE objectStoreID = ? AND key >= CAST(? AS TEXT) AND key <= CAST(? AS TEXT) ORDER BY key;"));
         if (sql.prepare() != SQLResultOk
             || sql.bindInt64(1, objectStoreID) != SQLResultOk
             || sql.bindBlob(2, lowerBuffer->data(), lowerBuffer->size()) != SQLResultOk
index 67d790e..eb14bf3 100644 (file)
@@ -71,11 +71,12 @@ public:
     virtual bool generateKeyNumber(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, int64_t& generatedKey) override;
     virtual bool updateKeyGeneratorNumber(const IDBIdentifier& transactionIdentifier, int64_t objectStoreId, int64_t keyNumber, bool checkCurrent) override;
 
-    virtual bool keyExistsInObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKey&, bool& keyExists) override;
+    virtual bool keyExistsInObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyData&, bool& keyExists) override;
     virtual bool putRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKey&, const uint8_t* valueBuffer, size_t valueSize) override;
     virtual bool putIndexRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, int64_t indexID, const WebCore::IDBKeyData& keyValue, const WebCore::IDBKeyData& indexKey) override;
     virtual bool getIndexRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, int64_t indexID, const WebCore::IDBKeyRangeData&, RefPtr<WebCore::SharedBuffer>& result) override;
     virtual bool deleteRange(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyRangeData&) override;
+    virtual bool deleteRecord(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyData&) override;
 
     virtual bool getKeyRecordFromObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKey&, RefPtr<WebCore::SharedBuffer>& result) override;
     virtual bool getKeyRangeRecordFromObjectStore(const IDBIdentifier& transactionIdentifier, int64_t objectStoreID, const WebCore::IDBKeyRange&, RefPtr<WebCore::SharedBuffer>& result, RefPtr<WebCore::IDBKey>& resultKey) override;