IndexedDB: Abort transactions because of leveldb errors part 4
authordgrogan@chromium.org <dgrogan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Dec 2012 22:56:26 +0000 (22:56 +0000)
committerdgrogan@chromium.org <dgrogan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Dec 2012 22:56:26 +0000 (22:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=103964

Reviewed by Tony Chang.

Source/WebCore:

deleteDatabase, open, and deleteObjectStore will now fire more aborts
and errors in case of leveldb problems

* Modules/indexeddb/IDBBackingStore.cpp:
(WebCore::getVarInt): Make return value indicate leveldb error.
(WebCore::getString): ditto.
(WebCore::IDBBackingStore::getIDBDatabaseMetaData):
Change return value to indicate leveldb error.

(WebCore::IDBBackingStore::deleteDatabase):
Already had the desired return value semantics. As a consumer of
getIDBDatabaseMetadata, will return an error (causing an abort) more
often.

(WebCore::IDBBackingStore::deleteObjectStore):
Needed return value change. Will return error to DatabaseBackend to
indicate leveldb problems.

* Modules/indexeddb/IDBBackingStore.h:
(IDBBackingStore):
* Modules/indexeddb/IDBDatabaseBackendImpl.cpp:
(WebCore::IDBDatabaseBackendImpl::openInternal):
Pass leveldb errors up to callers, who already handle internal errors.

(WebCore::IDBDatabaseBackendImpl::DeleteObjectStoreOperation::perform):
Abort transaction if there were leveldb problems deleting an object
store.

Source/WebKit/chromium:

* tests/IDBFakeBackingStore.h:
  Update one overridden method signature, delete another.

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

Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp
Source/WebCore/Modules/indexeddb/IDBBackingStore.h
Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/tests/IDBFakeBackingStore.h

index f771239..07b3b74 100644 (file)
@@ -1,5 +1,40 @@
 2012-12-06  David Grogan  <dgrogan@chromium.org>
 
+        IndexedDB: Abort transactions because of leveldb errors part 4
+        https://bugs.webkit.org/show_bug.cgi?id=103964
+
+        Reviewed by Tony Chang.
+
+        deleteDatabase, open, and deleteObjectStore will now fire more aborts
+        and errors in case of leveldb problems
+
+        * Modules/indexeddb/IDBBackingStore.cpp:
+        (WebCore::getVarInt): Make return value indicate leveldb error.
+        (WebCore::getString): ditto.
+        (WebCore::IDBBackingStore::getIDBDatabaseMetaData):
+        Change return value to indicate leveldb error.
+
+        (WebCore::IDBBackingStore::deleteDatabase):
+        Already had the desired return value semantics. As a consumer of
+        getIDBDatabaseMetadata, will return an error (causing an abort) more
+        often.
+
+        (WebCore::IDBBackingStore::deleteObjectStore):
+        Needed return value change. Will return error to DatabaseBackend to
+        indicate leveldb problems.
+
+        * Modules/indexeddb/IDBBackingStore.h:
+        (IDBBackingStore):
+        * Modules/indexeddb/IDBDatabaseBackendImpl.cpp:
+        (WebCore::IDBDatabaseBackendImpl::openInternal):
+        Pass leveldb errors up to callers, who already handle internal errors.
+
+        (WebCore::IDBDatabaseBackendImpl::DeleteObjectStoreOperation::perform):
+        Abort transaction if there were leveldb problems deleting an object
+        store.
+
+2012-12-06  David Grogan  <dgrogan@chromium.org>
+
         IndexedDB: Add webkitErrorMessage to IDBTransaction
         https://bugs.webkit.org/show_bug.cgi?id=104199
 
index b76157e..8882deb 100644 (file)
@@ -67,6 +67,7 @@ enum IDBLevelDBBackingStoreInternalErrorType {
     IDBLevelDBBackingStoreReadErrorGetPrimaryKeyViaIndex,
     IDBLevelDBBackingStoreReadErrorKeyExistsInIndex,
     IDBLevelDBBackingStoreReadErrorVersionExists,
+    IDBLevelDBBackingStoreReadErrorDeleteObjectStore,
     IDBLevelDBBackingStoreInternalErrorMax,
 };
 static inline void recordInternalError(IDBLevelDBBackingStoreInternalErrorType type)
@@ -121,17 +122,17 @@ static void putInt(LevelDBTransaction* transaction, const LevelDBSlice& key, int
 }
 
 template <typename DBOrTransaction>
-static bool getVarInt(DBOrTransaction* db, const LevelDBSlice& key, int64_t& foundInt)
+WARN_UNUSED_RETURN static bool getVarInt(DBOrTransaction* db, const LevelDBSlice& key, int64_t& foundInt, bool& found)
 {
     Vector<char> result;
-    bool found = false;
     bool ok = db->safeGet(key, result, found);
-    // FIXME: Notify the caller if !ok.
-    ASSERT_UNUSED(ok, ok);
-    if (!found)
+    if (!ok)
         return false;
+    if (!found)
+        return true;
 
-    return decodeVarInt(result.begin(), result.end(), foundInt) == result.end();
+    found = decodeVarInt(result.begin(), result.end(), foundInt) == result.end();
+    return true;
 }
 
 static void putVarInt(LevelDBTransaction* transaction, const LevelDBSlice& key, int64_t value)
@@ -140,15 +141,15 @@ static void putVarInt(LevelDBTransaction* transaction, const LevelDBSlice& key,
 }
 
 template <typename DBOrTransaction>
-static bool getString(DBOrTransaction* db, const LevelDBSlice& key, String& foundString)
+WARN_UNUSED_RETURN static bool getString(DBOrTransaction* db, const LevelDBSlice& key, String& foundString, bool& found)
 {
     Vector<char> result;
-    bool found = false;
+    found = false;
     bool ok = db->safeGet(key, result, found);
-    // FIXME: Notify the caller if !ok.
-    ASSERT_UNUSED(ok, ok);
-    if (!found)
+    if (!ok)
         return false;
+    if (!found)
+        return true;
 
     foundString = decodeString(result.begin(), result.end());
     return true;
@@ -364,25 +365,35 @@ Vector<String> IDBBackingStore::getDatabaseNames()
     return foundNames;
 }
 
-bool IDBBackingStore::getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata* metadata)
+bool IDBBackingStore::getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata* metadata, bool& found)
 {
     const Vector<char> key = DatabaseNameKey::encode(m_identifier, name);
+    found = false;
 
-    bool ok = getInt(m_db.get(), key, metadata->id);
-    if (!ok)
-        return false;
+    found = getInt(m_db.get(), key, metadata->id);
+    if (!found)
+        return true;
 
-    ok = getString(m_db.get(), DatabaseMetaDataKey::encode(metadata->id, DatabaseMetaDataKey::UserVersion), metadata->version);
+    bool ok = getString(m_db.get(), DatabaseMetaDataKey::encode(metadata->id, DatabaseMetaDataKey::UserVersion), metadata->version, found);
     if (!ok) {
         InternalError(IDBLevelDBBackingStoreReadErrorGetIDBDatabaseMetaData);
         return false;
     }
+    if (!found) {
+        InternalError(IDBLevelDBBackingStoreConsistencyError);
+        return false;
+    }
 
-    ok = getVarInt(m_db.get(), DatabaseMetaDataKey::encode(metadata->id, DatabaseMetaDataKey::UserIntVersion), metadata->intVersion);
+    ok = getVarInt(m_db.get(), DatabaseMetaDataKey::encode(metadata->id, DatabaseMetaDataKey::UserIntVersion), metadata->intVersion, found);
     if (!ok) {
         InternalError(IDBLevelDBBackingStoreReadErrorGetIDBDatabaseMetaData);
         return false;
     }
+    if (!found) {
+        InternalError(IDBLevelDBBackingStoreConsistencyError);
+        return false;
+    }
+
     if (metadata->intVersion == IDBDatabaseMetadata::DefaultIntVersion)
         metadata->intVersion = IDBDatabaseMetadata::NoIntVersion;
 
@@ -459,7 +470,11 @@ bool IDBBackingStore::deleteDatabase(const String& name)
     OwnPtr<LevelDBWriteOnlyTransaction> transaction = LevelDBWriteOnlyTransaction::create(m_db.get());
 
     IDBDatabaseMetadata metadata;
-    if (!getIDBDatabaseMetaData(name, &metadata))
+    bool success = false;
+    bool ok = getIDBDatabaseMetaData(name, &metadata, success);
+    if (!ok)
+        return false;
+    if (!success)
         return true;
 
     const Vector<char> startKey = DatabaseMetaDataKey::encode(metadata.id, DatabaseMetaDataKey::OriginName);
@@ -622,13 +637,22 @@ bool IDBBackingStore::createObjectStore(IDBBackingStore::Transaction* transactio
     return true;
 }
 
-void IDBBackingStore::deleteObjectStore(IDBBackingStore::Transaction* transaction, int64_t databaseId, int64_t objectStoreId)
+bool IDBBackingStore::deleteObjectStore(IDBBackingStore::Transaction* transaction, int64_t databaseId, int64_t objectStoreId)
 {
     IDB_TRACE("IDBBackingStore::deleteObjectStore");
     LevelDBTransaction* levelDBTransaction = IDBBackingStore::Transaction::levelDBTransactionFrom(transaction);
 
     String objectStoreName;
-    getString(levelDBTransaction, ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, ObjectStoreMetaDataKey::Name), objectStoreName);
+    bool found = false;
+    bool ok = getString(levelDBTransaction, ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, ObjectStoreMetaDataKey::Name), objectStoreName, found);
+    if (!ok) {
+        InternalError(IDBLevelDBBackingStoreReadErrorDeleteObjectStore);
+        return false;
+    }
+    if (!found) {
+        InternalError(IDBLevelDBBackingStoreConsistencyError);
+        return false;
+    }
 
     deleteRange(levelDBTransaction, ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, 0), ObjectStoreMetaDataKey::encodeMaxKey(databaseId, objectStoreId));
 
@@ -638,6 +662,7 @@ void IDBBackingStore::deleteObjectStore(IDBBackingStore::Transaction* transactio
     deleteRange(levelDBTransaction, IndexMetaDataKey::encode(databaseId, objectStoreId, 0, 0), IndexMetaDataKey::encodeMaxKey(databaseId, objectStoreId));
 
     clearObjectStore(transaction, databaseId, objectStoreId);
+    return true;
 }
 
 bool IDBBackingStore::getRecord(IDBBackingStore::Transaction* transaction, int64_t databaseId, int64_t objectStoreId, const IDBKey& key, String& record)
index d3a3cf9..e6ed4d8 100644 (file)
@@ -52,7 +52,7 @@ public:
     static PassRefPtr<IDBBackingStore> open(SecurityOrigin*, const String& pathBase, const String& fileIdentifier, IDBFactoryBackendImpl*);
 
     virtual Vector<String> getDatabaseNames();
-    virtual bool getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata*);
+    virtual bool getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata*, bool& success) WARN_UNUSED_RETURN;
     virtual bool createIDBDatabaseMetaData(const String& name, const String& version, int64_t intVersion, int64_t& rowId);
     virtual bool updateIDBDatabaseMetaData(IDBBackingStore::Transaction*, int64_t rowId, const String& version);
     virtual bool updateIDBDatabaseIntVersion(IDBBackingStore::Transaction*, int64_t rowId, int64_t intVersion);
@@ -60,7 +60,7 @@ public:
 
     virtual Vector<IDBObjectStoreMetadata> getObjectStores(int64_t databaseId);
     virtual bool createObjectStore(IDBBackingStore::Transaction*, int64_t databaseId, int64_t objectStoreId, const String& name, const IDBKeyPath&, bool autoIncrement);
-    virtual void deleteObjectStore(IDBBackingStore::Transaction*, int64_t databaseId, int64_t objectStoreId);
+    virtual bool deleteObjectStore(IDBBackingStore::Transaction*, int64_t databaseId, int64_t objectStoreId) WARN_UNUSED_RETURN;
 
     class RecordIdentifier {
         WTF_MAKE_NONCOPYABLE(RecordIdentifier);
index ae7af0e..ecb61ee 100644 (file)
@@ -236,8 +236,11 @@ IDBDatabaseBackendImpl::IDBDatabaseBackendImpl(const String& name, IDBBackingSto
 
 bool IDBDatabaseBackendImpl::openInternal()
 {
-    bool success = m_backingStore->getIDBDatabaseMetaData(m_metadata.name, &m_metadata);
+    bool success = false;
+    bool ok = m_backingStore->getIDBDatabaseMetaData(m_metadata.name, &m_metadata, success);
     ASSERT_WITH_MESSAGE(success == (m_metadata.id != InvalidId), "success = %s, m_id = %lld", success ? "true" : "false", static_cast<long long>(m_metadata.id));
+    if (!ok)
+        return false;
     if (success) {
         loadObjectStores();
         return true;
@@ -318,7 +321,11 @@ void IDBDatabaseBackendImpl::deleteObjectStore(int64_t id, IDBTransactionBackend
 void IDBDatabaseBackendImpl::DeleteObjectStoreOperation::perform(IDBTransactionBackendImpl* transaction)
 {
     IDB_TRACE("DeleteObjectStoreOperation");
-    m_database->m_backingStore->deleteObjectStore(transaction->backingStoreTransaction(), m_database->id(), m_objectStore->id());
+    bool ok = m_database->m_backingStore->deleteObjectStore(transaction->backingStoreTransaction(), m_database->id(), m_objectStore->id());
+    if (!ok) {
+        RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error deleting object store.");
+        transaction->abort(error);
+    }
 }
 
 void IDBDatabaseBackendImpl::VersionChangeOperation::perform(IDBTransactionBackendImpl* transaction)
index bae2f8e..ae44b9e 100644 (file)
@@ -1,3 +1,13 @@
+2012-12-06  David Grogan  <dgrogan@chromium.org>
+
+        IndexedDB: Abort transactions because of leveldb errors part 4
+        https://bugs.webkit.org/show_bug.cgi?id=103964
+
+        Reviewed by Tony Chang.
+
+        * tests/IDBFakeBackingStore.h:
+          Update one overridden method signature, delete another.
+
 2012-12-06  Tony Chang  <tony@chromium.org>
 
         REGRESSION(r135082): Restore the ability to insert author level style sheets from script
 
         * DEPS:
 
-2012-11-30  David Grogan  <dgrogan@chromium.org>
+2012-12-04  David Grogan  <dgrogan@chromium.org>
 
         IndexedDB: Propagate more leveldb errors to IDBIndex and IDBObjectStore
         https://bugs.webkit.org/show_bug.cgi?id=103782
index 55ff5b8..9b17add 100644 (file)
@@ -33,7 +33,7 @@ namespace WebCore {
 class IDBFakeBackingStore : public IDBBackingStore {
 public:
     virtual Vector<String> getDatabaseNames() OVERRIDE { return Vector<String>(); }
-    virtual bool getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata*) OVERRIDE { return false; }
+    virtual bool getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata*, bool& found) OVERRIDE { return true; }
     virtual bool createIDBDatabaseMetaData(const String& name, const String& version, int64_t intVersion, int64_t& rowId) OVERRIDE { return true; }
     virtual bool updateIDBDatabaseMetaData(Transaction*, int64_t rowId, const String& version) OVERRIDE { return false; }
     virtual bool updateIDBDatabaseIntVersion(Transaction*, int64_t rowId, int64_t version) OVERRIDE { return false; }
@@ -41,7 +41,6 @@ public:
 
     virtual Vector<IDBObjectStoreMetadata> getObjectStores(int64_t databaseId) OVERRIDE { return Vector<IDBObjectStoreMetadata>(); }
     virtual bool createObjectStore(Transaction*, int64_t databaseId, int64_t objectStoreId, const String& name, const IDBKeyPath&, bool autoIncrement) OVERRIDE { return false; };
-    virtual void deleteObjectStore(Transaction*, int64_t databaseId, int64_t objectStoreId) OVERRIDE { }
 
     virtual void putRecord(Transaction*, int64_t databaseId, int64_t objectStoreId, const IDBKey&, const String& value, RecordIdentifier*) OVERRIDE { }
     virtual void clearObjectStore(Transaction*, int64_t databaseId, int64_t objectStoreId) OVERRIDE { }