Diff
Modified: trunk/LayoutTests/ChangeLog (256413 => 256414)
--- trunk/LayoutTests/ChangeLog 2020-02-12 01:00:19 UTC (rev 256413)
+++ trunk/LayoutTests/ChangeLog 2020-02-12 01:05:47 UTC (rev 256414)
@@ -1,3 +1,17 @@
+2020-02-11 Sihui Liu <sihui_...@apple.com>
+
+ IndexedDB: iteration of cursors skip records if deleted
+ https://bugs.webkit.org/show_bug.cgi?id=207437
+
+ Reviewed by Brady Eidson.
+
+ * storage/indexeddb/cursor-update-while-iterating-expected.txt:
+ * storage/indexeddb/resources/cursor-update-while-iterating.js:
+ (populateObjectStore):
+ (onOpenSuccess.request.onsuccess):
+ (onOpenSuccess):
+ (prepareDatabase): Deleted.
+
2020-02-11 Jason Lawrence <lawrenc...@apple.com>
[ Mac wk2 ] webgpu/whlsl/do-while-loop.html is flaky failing.
Modified: trunk/LayoutTests/storage/indexeddb/cursor-update-while-iterating-expected.txt (256413 => 256414)
--- trunk/LayoutTests/storage/indexeddb/cursor-update-while-iterating-expected.txt 2020-02-12 01:00:19 UTC (rev 256413)
+++ trunk/LayoutTests/storage/indexeddb/cursor-update-while-iterating-expected.txt 2020-02-12 01:05:47 UTC (rev 256414)
@@ -11,7 +11,7 @@
prepareDatabase():
db = event.target.result
Deleted all object stores.
-objectStore = db.createObjectStore('objectStore', {autoIncrement: true})
+objectStore = db.createObjectStore('objectStore')
objectStore.createIndex('key', 'key', {unique: false})
onOpenSuccess():
@@ -40,7 +40,11 @@
Delete cursor
Cursor continues
-PASS Successfully iterated whole array with cursor updates.
+PASS JSON.stringify(cursor.value) is "{\"key\":\"key3\",\"value\":\"value5\"}"
+Delete last record
+Cursor continues
+
+PASS totalRecordCount is 6
PASS successfullyParsed is true
TEST COMPLETE
Modified: trunk/LayoutTests/storage/indexeddb/resources/cursor-update-while-iterating.js (256413 => 256414)
--- trunk/LayoutTests/storage/indexeddb/resources/cursor-update-while-iterating.js 2020-02-12 01:00:19 UTC (rev 256413)
+++ trunk/LayoutTests/storage/indexeddb/resources/cursor-update-while-iterating.js 2020-02-12 01:05:47 UTC (rev 256414)
@@ -12,12 +12,15 @@
{ key: "key1", value: "value1" },
{ key: "key1", value: "value3" },
{ key: "key2", value: "value2" },
- { key: "key2", value: "value4" }
+ { key: "key2", value: "value4" },
+ { key: "key3", value: "value5" },
+ { key: "key3", value: "value6" },
];
function populateObjectStore() {
- for (let object of objectArray)
- objectStore.add(object)._onerror_ = unexpectedErrorCallback;
+ objectArray.forEach((object, i)=>{
+ objectStore.add(object, i)._onerror_ = unexpectedErrorCallback;
+ });
}
function prepareDatabase(event)
@@ -26,7 +29,7 @@
evalAndLog("db = event.target.result");
deleteAllObjectStores(db);
- objectStore = evalAndLog("objectStore = db.createObjectStore('objectStore', {autoIncrement: true})");
+ objectStore = evalAndLog("objectStore = db.createObjectStore('objectStore')");
evalAndLog("objectStore.createIndex('key', 'key', {unique: false})");
populateObjectStore();
@@ -43,28 +46,30 @@
evalAndLog("index = objectStore.index('key')");
request = evalAndLog("index.openCursor()");
- var n = 0;
+ totalRecordCount = 0;
request._onsuccess_ = function(event) {
cursor = event.target.result;
if (cursor) {
- shouldBeEqualToString("JSON.stringify(cursor.value)", JSON.stringify(objectArray[n++]));
+ shouldBeEqualToString("JSON.stringify(cursor.value)", JSON.stringify(objectArray[totalRecordCount++]));
if (cursor.key == "key1") {
debug("Update cursor");
const {value} = cursor;
cursor.update(value);
- } else {
+ }
+ if (cursor.key == "key2") {
debug("Delete cursor");
cursor.delete();
}
+ if (cursor.key == "key3") {
+ debug("Delete last record");
+ objectStore.delete(6);
+ }
debug("Cursor continues\n");
cursor.continue();
} else {
- if (n != objectArray.length)
- testFailed("Cursor didn't go through whole array.");
- else
- testPassed("Successfully iterated whole array with cursor updates.");
+ shouldBeEqualToNumber("totalRecordCount", objectArray.length - 1);
}
}
Modified: trunk/Source/WebCore/ChangeLog (256413 => 256414)
--- trunk/Source/WebCore/ChangeLog 2020-02-12 01:00:19 UTC (rev 256413)
+++ trunk/Source/WebCore/ChangeLog 2020-02-12 01:05:47 UTC (rev 256414)
@@ -1,3 +1,36 @@
+2020-02-11 Sihui Liu <sihui_...@apple.com>
+
+ IndexedDB: iteration of cursors skip records if deleted
+ https://bugs.webkit.org/show_bug.cgi?id=207437
+
+ Reviewed by Brady Eidson.
+
+ When changes are made to records, cursors will dump cached records and set new key range for iteration.
+ The new range did not include key of current cursor record, which is wrong because two index records can have
+ the same key but different values.
+ r237590 tried fixing this issue by caching all the following records which have the same key as current record.
+ That is not quite right as there could be changes on the cached records, and the cached records were not
+ updated.
+
+ To correctly fix the issue, set the new key range to include key of current cursor record and exclude values
+ visited before. To not regress preformance, we complete this by making an extra statment and changing
+ IndexRecordsIndex index of table IndexRecords to cover value column.
+
+ Added test case in: storage/indexeddb/cursor-update-while-iterating.html
+ Index upgrade covered by existing API test: IndexedDB.IDBObjectStoreInfoUpgradeToV2
+
+ * Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:
+ (WebCore::IDBServer::SQLiteIDBBackingStore::ensureValidIndexRecordsIndex):
+ * Modules/indexeddb/server/SQLiteIDBCursor.cpp:
+ (WebCore::IDBServer::buildPreIndexStatement):
+ (WebCore::IDBServer::SQLiteIDBCursor::objectStoreRecordsChanged):
+ (WebCore::IDBServer::SQLiteIDBCursor::resetAndRebindPreIndexStatementIfNecessary):
+ (WebCore::IDBServer::SQLiteIDBCursor::fetch):
+ (WebCore::IDBServer::SQLiteIDBCursor::fetchNextRecord):
+ (WebCore::IDBServer::SQLiteIDBCursor::internalFetchNextRecord):
+ * Modules/indexeddb/server/SQLiteIDBCursor.h:
+ (WebCore::IDBServer::SQLiteIDBCursor::isDirectionNext const):
+
2020-02-11 Ryan Haddad <ryanhad...@apple.com>
Unreviewed, partial rollout of r255037.
Modified: trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp (256413 => 256414)
--- trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp 2020-02-12 01:00:19 UTC (rev 256413)
+++ trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp 2020-02-12 01:05:47 UTC (rev 256414)
@@ -64,6 +64,7 @@
constexpr auto objectStoreInfoTableNameAlternate = "\"ObjectStoreInfo\""_s;
constexpr auto v2ObjectStoreInfoSchema = "CREATE TABLE ObjectStoreInfo (id INTEGER PRIMARY KEY NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT FAIL, name TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT FAIL, keyPath BLOB NOT NULL ON CONFLICT FAIL, autoInc INTEGER NOT NULL ON CONFLICT FAIL)"_s;
constexpr auto v1IndexRecordsRecordIndexSchema = "CREATE INDEX IndexRecordsRecordIndex ON IndexRecords (objectStoreID, objectStoreRecordID)"_s;
+constexpr auto v2IndexRecordsIndexSchema = "CREATE INDEX IndexRecordsIndex ON IndexRecords (key, value)"_s;
// Current version of the metadata schema being used in the metadata database.
static const int currentMetadataVersion = 1;
@@ -514,7 +515,7 @@
// If there is no IndexRecordsIndex index at all, create it and then bail.
if (sqliteResult == SQLITE_DONE) {
- if (!m_sqliteDB->executeCommand(v1IndexRecordsIndexSchema())) {
+ if (!m_sqliteDB->executeCommand(v2IndexRecordsIndexSchema)) {
LOG_ERROR("Could not create IndexRecordsIndex index in database (%i) - %s", m_sqliteDB->lastError(), m_sqliteDB->lastErrorMsg());
return false;
}
@@ -533,11 +534,27 @@
ASSERT(!currentSchema.isEmpty());
// If the schema in the backing store is the current schema, we're done.
- if (currentSchema == v1IndexRecordsIndexSchema())
+ if (currentSchema == v2IndexRecordsIndexSchema)
return true;
- // There is currently no outdated schema for the IndexRecordsIndex, so any other existing schema means this database is invalid.
- return false;
+ RELEASE_ASSERT(currentSchema == v1IndexRecordsIndexSchema());
+
+ SQLiteTransaction transaction(*m_sqliteDB);
+ transaction.begin();
+
+ if (!m_sqliteDB->executeCommand("DROP INDEX IndexRecordsIndex")) {
+ LOG_ERROR("Could not drop index IndexRecordsIndex in database (%i) - %s", m_sqliteDB->lastError(), m_sqliteDB->lastErrorMsg());
+ return false;
+ }
+
+ if (!m_sqliteDB->executeCommand(v2IndexRecordsIndexSchema)) {
+ LOG_ERROR("Could not create IndexRecordsIndex index in database (%i) - %s", m_sqliteDB->lastError(), m_sqliteDB->lastErrorMsg());
+ return false;
+ }
+
+ transaction.commit();
+
+ return true;
}
bool SQLiteIDBBackingStore::ensureValidIndexRecordsRecordIndex()
Modified: trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp (256413 => 256414)
--- trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp 2020-02-12 01:00:19 UTC (rev 256413)
+++ trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp 2020-02-12 01:05:47 UTC (rev 256414)
@@ -116,6 +116,25 @@
result = { currentRecord.record.key, currentRecord.record.primaryKey, currentRecord.record.value ? *currentRecord.record.value : IDBValue(), keyPath};
}
+static String buildPreIndexStatement(bool isDirectionNext)
+{
+ StringBuilder builder;
+
+ builder.appendLiteral("SELECT rowid, key, value FROM IndexRecords WHERE indexID = ? AND key = CAST(? AS TEXT) AND value ");
+ if (isDirectionNext)
+ builder.append('>');
+ else
+ builder.append('<');
+
+ builder.appendLiteral(" CAST(? AS TEXT) ORDER BY value");
+ if (!isDirectionNext)
+ builder.appendLiteral(" DESC");
+
+ builder.append(';');
+
+ return builder.toString();
+}
+
static String buildIndexStatement(const IDBKeyRangeData& keyRange, IndexedDB::CursorDirection cursorDirection)
{
StringBuilder builder;
@@ -218,28 +237,14 @@
ASSERT(!m_fetchedRecords.isEmpty());
m_currentKeyForUniqueness = m_fetchedRecords.first().record.key;
+ if (m_indexID != IDBIndexInfo::InvalidId)
+ m_currentIndexRecordValue = m_fetchedRecords.first().record.primaryKey;
- if (m_cursorDirection != IndexedDB::CursorDirection::Nextunique && m_cursorDirection != IndexedDB::CursorDirection::Prevunique) {
- if (!m_fetchedRecords.last().isTerminalRecord())
- fetch(ShouldFetchForSameKey::Yes);
-
- while (m_fetchedRecords.last().record.key != m_fetchedRecords.first().record.key) {
- ASSERT(m_fetchedRecordsSize >= m_fetchedRecords.last().record.size());
- m_fetchedRecordsSize -= m_fetchedRecords.last().record.size();
- m_fetchedRecords.removeLast();
- }
- } else {
- m_fetchedRecords.clear();
- m_fetchedRecordsSize = 0;
- }
-
// If ObjectStore or Index contents changed, we need to reset the statement and bind new parameters to it.
// This is to pick up any changes that might exist.
- // We also need to throw away any fetched records as they may no longer be valid.
-
m_statementNeedsReset = true;
- if (m_cursorDirection == IndexedDB::CursorDirection::Next || m_cursorDirection == IndexedDB::CursorDirection::Nextunique) {
+ if (isDirectionNext()) {
m_currentLowerKey = m_currentKeyForUniqueness;
if (!m_keyRange.lowerOpen) {
m_keyRange.lowerOpen = true;
@@ -254,6 +259,10 @@
m_statement = nullptr;
}
}
+
+ // We also need to throw away any fetched records as they may no longer be valid.
+ m_fetchedRecords.clear();
+ m_fetchedRecordsSize = 0;
}
void SQLiteIDBCursor::resetAndRebindStatement()
@@ -304,6 +313,52 @@
return true;
}
+bool SQLiteIDBCursor::resetAndRebindPreIndexStatementIfNecessary()
+{
+ if (m_indexID == IDBIndexInfo::InvalidId)
+ return true;
+
+ if (m_currentIndexRecordValue.isNull())
+ return true;
+
+ auto& database = m_transaction->sqliteTransaction()->database();
+ if (!m_preIndexStatement) {
+ m_preIndexStatement = makeUnique<SQLiteStatement>(database, buildPreIndexStatement(isDirectionNext()));
+
+ if (m_preIndexStatement->prepare() != SQLITE_OK) {
+ LOG_ERROR("Could not prepare pre statement - '%s'", database.lastErrorMsg());
+ return false;
+ }
+ }
+
+ if (m_preIndexStatement->reset() != SQLITE_OK) {
+ LOG_ERROR("Could not reset pre statement - '%s'", database.lastErrorMsg());
+ return false;
+ }
+
+ auto key = isDirectionNext() ? m_currentLowerKey : m_currentUpperKey;
+ int currentBindArgument = 1;
+
+ if (m_preIndexStatement->bindInt64(currentBindArgument++, m_boundID) != SQLITE_OK) {
+ LOG_ERROR("Could not bind id argument to pre statement (bound ID)");
+ return false;
+ }
+
+ RefPtr<SharedBuffer> buffer = serializeIDBKeyData(key);
+ if (m_preIndexStatement->bindBlob(currentBindArgument++, buffer->data(), buffer->size()) != SQLITE_OK) {
+ LOG_ERROR("Could not bind id argument to pre statement (key)");
+ return false;
+ }
+
+ buffer = serializeIDBKeyData(m_currentIndexRecordValue);
+ if (m_preIndexStatement->bindBlob(currentBindArgument++, buffer->data(), buffer->size()) != SQLITE_OK) {
+ LOG_ERROR("Could not bind id argument to pre statement (value)");
+ return false;
+ }
+
+ return true;
+}
+
bool SQLiteIDBCursor::prefetch()
{
LOG(IndexedDB, "SQLiteIDBCursor::prefetch() - Cursor already has %zu fetched records", m_fetchedRecords.size());
@@ -374,13 +429,13 @@
return true;
}
-bool SQLiteIDBCursor::fetch(ShouldFetchForSameKey shouldFetchForSameKey)
+bool SQLiteIDBCursor::fetch()
{
ASSERT(m_fetchedRecords.isEmpty() || !m_fetchedRecords.last().isTerminalRecord());
m_fetchedRecords.append({ });
- bool isUnique = m_cursorDirection == IndexedDB::CursorDirection::Nextunique || m_cursorDirection == IndexedDB::CursorDirection::Prevunique || shouldFetchForSameKey == ShouldFetchForSameKey::Yes;
+ bool isUnique = m_cursorDirection == IndexedDB::CursorDirection::Nextunique || m_cursorDirection == IndexedDB::CursorDirection::Prevunique;
if (!isUnique) {
bool fetchSucceeded = fetchNextRecord(m_fetchedRecords.last());
if (fetchSucceeded)
@@ -397,8 +452,7 @@
if (m_fetchedRecords.last().completed)
return false;
- if (shouldFetchForSameKey == ShouldFetchForSameKey::Yes)
- m_fetchedRecords.append({ });
+ m_fetchedRecordsSize -= m_fetchedRecords.last().record.size();
}
return false;
@@ -406,8 +460,10 @@
bool SQLiteIDBCursor::fetchNextRecord(SQLiteCursorRecord& record)
{
- if (m_statementNeedsReset)
+ if (m_statementNeedsReset) {
+ resetAndRebindPreIndexStatementIfNecessary();
resetAndRebindStatement();
+ }
FetchResult result;
do {
@@ -434,26 +490,40 @@
record.record.value = nullptr;
- int result = m_statement->step();
- if (result == SQLITE_DONE) {
- // When a cursor reaches its end, that is indicated by having undefined keys/values
- record = { };
- record.completed = true;
+ auto& database = m_transaction->sqliteTransaction()->database();
+ SQLiteStatement* statement = nullptr;
- return FetchResult::Success;
- }
+ int result;
+ if (m_preIndexStatement) {
+ ASSERT(m_indexID != IDBIndexInfo::InvalidId);
- if (result != SQLITE_ROW) {
- LOG_ERROR("Error advancing cursor - (%i) %s", result, m_transaction->sqliteTransaction()->database().lastErrorMsg());
- markAsErrored(record);
- return FetchResult::Failure;
+ result = m_preIndexStatement->step();
+ if (result == SQLITE_ROW)
+ statement = m_preIndexStatement.get();
+ else if (result != SQLITE_DONE)
+ LOG_ERROR("Error advancing with pre statement - (%i) %s", result, database.lastErrorMsg());
}
+
+ if (!statement) {
+ result = m_statement->step();
+ if (result == SQLITE_DONE) {
+ record = { };
+ record.completed = true;
+ return FetchResult::Success;
+ }
+ if (result != SQLITE_ROW) {
+ LOG_ERROR("Error advancing cursor - (%i) %s", result, database.lastErrorMsg());
+ markAsErrored(record);
+ return FetchResult::Failure;
+ }
+ statement = m_statement.get();
+ }
- record.rowID = m_statement->getColumnInt64(0);
+ record.rowID = statement->getColumnInt64(0);
ASSERT(record.rowID);
Vector<uint8_t> keyData;
- m_statement->getColumnBlobAsVector(1, keyData);
+ statement->getColumnBlobAsVector(1, keyData);
if (!deserializeIDBKeyData(keyData.data(), keyData.size(), record.record.key)) {
LOG_ERROR("Unable to deserialize key data from database while advancing cursor");
@@ -461,7 +531,7 @@
return FetchResult::Failure;
}
- m_statement->getColumnBlobAsVector(2, keyData);
+ statement->getColumnBlobAsVector(2, keyData);
// The primaryKey of an ObjectStore cursor is the same as its key.
if (m_indexID == IDBIndexInfo::InvalidId) {
@@ -485,7 +555,7 @@
}
if (!m_cachedObjectStoreStatement || m_cachedObjectStoreStatement->reset() != SQLITE_OK) {
- m_cachedObjectStoreStatement = makeUnique<SQLiteStatement>(m_statement->database(), "SELECT value FROM Records WHERE key = CAST(? AS TEXT) and objectStoreID = ?;");
+ m_cachedObjectStoreStatement = makeUnique<SQLiteStatement>(database, "SELECT value FROM Records WHERE key = CAST(? AS TEXT) and objectStoreID = ?;");
if (m_cachedObjectStoreStatement->prepare() != SQLITE_OK)
m_cachedObjectStoreStatement = nullptr;
}
@@ -493,7 +563,7 @@
if (!m_cachedObjectStoreStatement
|| m_cachedObjectStoreStatement->bindBlob(1, keyData.data(), keyData.size()) != SQLITE_OK
|| m_cachedObjectStoreStatement->bindInt64(2, m_objectStoreID) != SQLITE_OK) {
- LOG_ERROR("Could not create index cursor statement into object store records (%i) '%s'", m_statement->database().lastError(), m_statement->database().lastErrorMsg());
+ LOG_ERROR("Could not create index cursor statement into object store records (%i) '%s'", database.lastError(), database.lastErrorMsg());
markAsErrored(record);
return FetchResult::Failure;
}
@@ -508,7 +578,7 @@
// Skip over it.
return FetchResult::ShouldFetchAgain;
} else {
- LOG_ERROR("Could not step index cursor statement into object store records (%i) '%s'", m_statement->database().lastError(), m_statement->database().lastErrorMsg());
+ LOG_ERROR("Could not step index cursor statement into object store records (%i) '%s'", database.lastError(), database.lastErrorMsg());
markAsErrored(record);
return FetchResult::Failure;
Modified: trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.h (256413 => 256414)
--- trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.h 2020-02-12 01:00:19 UTC (rev 256413)
+++ trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.h 2020-02-12 01:05:47 UTC (rev 256414)
@@ -44,8 +44,6 @@
namespace IDBServer {
-enum class ShouldFetchForSameKey : bool { No, Yes };
-
class SQLiteIDBTransaction;
class SQLiteIDBCursor {
@@ -86,6 +84,7 @@
bool createSQLiteStatement(const String& sql);
bool bindArguments();
+ bool resetAndRebindPreIndexStatementIfNecessary();
void resetAndRebindStatement();
enum class FetchResult {
@@ -94,7 +93,7 @@
ShouldFetchAgain
};
- bool fetch(ShouldFetchForSameKey = ShouldFetchForSameKey::No);
+ bool fetch();
struct SQLiteCursorRecord {
IDBCursorRecord record;
@@ -108,6 +107,8 @@
void markAsErrored(SQLiteCursorRecord&);
+ bool isDirectionNext() const { return m_cursorDirection == IndexedDB::CursorDirection::Next || m_cursorDirection == IndexedDB::CursorDirection::Nextunique; }
+
SQLiteIDBTransaction* m_transaction;
IDBResourceIdentifier m_cursorIdentifier;
int64_t m_objectStoreID;
@@ -118,11 +119,13 @@
IDBKeyData m_currentLowerKey;
IDBKeyData m_currentUpperKey;
+ IDBKeyData m_currentIndexRecordValue;
Deque<SQLiteCursorRecord> m_fetchedRecords;
uint64_t m_fetchedRecordsSize { 0 };
IDBKeyData m_currentKeyForUniqueness;
+ std::unique_ptr<SQLiteStatement> m_preIndexStatement;
std::unique_ptr<SQLiteStatement> m_statement;
std::unique_ptr<SQLiteStatement> m_cachedObjectStoreStatement;
