Changeset 256414 in webkit


Ignore:
Timestamp:
Feb 11, 2020 5:05:47 PM (4 years ago)
Author:
sihui_liu@apple.com
Message:

IndexedDB: iteration of cursors skip records if deleted
​https://bugs.webkit.org/show_bug.cgi?id=207437

Reviewed by Brady Eidson.

Source/WebCore:

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):

LayoutTests:

  • storage/indexeddb/cursor-update-while-iterating-expected.txt:
  • storage/indexeddb/resources/cursor-update-while-iterating.js:

(populateObjectStore):
(onOpenSuccess.request.onsuccess):
(onOpenSuccess):
(prepareDatabase): Deleted.

Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r256398 r256414  
     12020-02-11  Sihui Liu  <sihui_liu@apple.com>
     2
     3        IndexedDB: iteration of cursors skip records if deleted
     4        https://bugs.webkit.org/show_bug.cgi?id=207437
     5
     6        Reviewed by Brady Eidson.
     7
     8        * storage/indexeddb/cursor-update-while-iterating-expected.txt:
     9        * storage/indexeddb/resources/cursor-update-while-iterating.js:
     10        (populateObjectStore):
     11        (onOpenSuccess.request.onsuccess):
     12        (onOpenSuccess):
     13        (prepareDatabase): Deleted.
     14
    1152020-02-11  Jason Lawrence  <lawrence.j@apple.com>
    216

  • trunk/LayoutTests/storage/indexeddb/cursor-update-while-iterating-expected.txt

    r237590 r256414  
    1212db = event.target.result
    1313Deleted all object stores.
    14 objectStore = db.createObjectStore('objectStore', {autoIncrement: true})
     14objectStore = db.createObjectStore('objectStore')
    1515objectStore.createIndex('key', 'key', {unique: false})
    1616
    … …  
    4141Cursor continues
    4242
    43 PASS Successfully iterated whole array with cursor updates.
     43PASS JSON.stringify(cursor.value) is "{\"key\":\"key3\",\"value\":\"value5\"}"
     44Delete last record
     45Cursor continues
     46
     47PASS totalRecordCount is 6
    4448PASS successfullyParsed is true
    4549

  • trunk/LayoutTests/storage/indexeddb/resources/cursor-update-while-iterating.js

    r237590 r256414  
    1313    { key: "key1", value: "value3" },
    1414    { key: "key2", value: "value2" },
    15     { key: "key2", value: "value4" }
     15    { key: "key2", value: "value4" },
     16        { key: "key3", value: "value5" },
     17        { key: "key3", value: "value6" },
    1618];
    1719
    1820function populateObjectStore() {
    19     for (let object of objectArray)
    20         objectStore.add(object).onerror = unexpectedErrorCallback;
     21    objectArray.forEach((object, i)=>{
     22        objectStore.add(object, i).onerror = unexpectedErrorCallback;
     23        });
    2124}
    2225
    … …  
    2730    deleteAllObjectStores(db);
    2831
    29     objectStore = evalAndLog("objectStore = db.createObjectStore('objectStore', {autoIncrement: true})");
     32    objectStore = evalAndLog("objectStore = db.createObjectStore('objectStore')");
    3033    evalAndLog("objectStore.createIndex('key', 'key', {unique: false})");
    3134
    … …  
    4447    request = evalAndLog("index.openCursor()");
    4548
    46     var n = 0;
     49    totalRecordCount = 0;
    4750    request.onsuccess = function(event) {
    4851        cursor = event.target.result;
    4952        if (cursor) {
    50             shouldBeEqualToString("JSON.stringify(cursor.value)", JSON.stringify(objectArray[n++]));
     53            shouldBeEqualToString("JSON.stringify(cursor.value)", JSON.stringify(objectArray[totalRecordCount++]));
    5154
    5255            if (cursor.key == "key1") {
    … …  
    5457                const {value} = cursor;
    5558                cursor.update(value);
    56             } else {
     59            }
     60            if (cursor.key == "key2") {
    5761                debug("Delete cursor");
    5862                cursor.delete();
     63            }
     64            if (cursor.key == "key3") {
     65                debug("Delete last record");
     66                objectStore.delete(6);
    5967            }
    6068
    … …  
    6270            cursor.continue();
    6371        } else {
    64             if (n != objectArray.length)
    65                 testFailed("Cursor didn't go through whole array.");
    66             else
    67                 testPassed("Successfully iterated whole array with cursor updates.");
     72            shouldBeEqualToNumber("totalRecordCount", objectArray.length - 1);
    6873        }
    6974    }

  • trunk/Source/WebCore/ChangeLog

    r256400 r256414  
     12020-02-11  Sihui Liu  <sihui_liu@apple.com>
     2
     3        IndexedDB: iteration of cursors skip records if deleted
     4        https://bugs.webkit.org/show_bug.cgi?id=207437
     5
     6        Reviewed by Brady Eidson.
     7
     8        When changes are made to records, cursors will dump cached records and set new key range for iteration.
     9        The new range did not include key of current cursor record, which is wrong because two index records can have
     10        the same key but different values.
     11        r237590 tried fixing this issue by caching all the following records which have the same key as current record.
     12        That is not quite right as there could be changes on the cached records, and the cached records were not
     13        updated.
     14
     15        To correctly fix the issue, set the new key range to include key of current cursor record and exclude values
     16        visited before. To not regress preformance, we complete this by making an extra statment and changing
     17        IndexRecordsIndex index of table IndexRecords to cover value column.
     18
     19        Added test case in: storage/indexeddb/cursor-update-while-iterating.html
     20        Index upgrade covered by existing API test: IndexedDB.IDBObjectStoreInfoUpgradeToV2
     21
     22        * Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:
     23        (WebCore::IDBServer::SQLiteIDBBackingStore::ensureValidIndexRecordsIndex):
     24        * Modules/indexeddb/server/SQLiteIDBCursor.cpp:
     25        (WebCore::IDBServer::buildPreIndexStatement):
     26        (WebCore::IDBServer::SQLiteIDBCursor::objectStoreRecordsChanged):
     27        (WebCore::IDBServer::SQLiteIDBCursor::resetAndRebindPreIndexStatementIfNecessary):
     28        (WebCore::IDBServer::SQLiteIDBCursor::fetch):
     29        (WebCore::IDBServer::SQLiteIDBCursor::fetchNextRecord):
     30        (WebCore::IDBServer::SQLiteIDBCursor::internalFetchNextRecord):
     31        * Modules/indexeddb/server/SQLiteIDBCursor.h:
     32        (WebCore::IDBServer::SQLiteIDBCursor::isDirectionNext const):
     33
    1342020-02-11  Ryan Haddad  <ryanhaddad@apple.com>
    235

  • trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp

    r255318 r256414  
    6565constexpr 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;
    6666constexpr auto v1IndexRecordsRecordIndexSchema = "CREATE INDEX IndexRecordsRecordIndex ON IndexRecords (objectStoreID, objectStoreRecordID)"_s;
     67constexpr auto v2IndexRecordsIndexSchema = "CREATE INDEX IndexRecordsIndex ON IndexRecords (key, value)"_s;
    6768
    6869// Current version of the metadata schema being used in the metadata database.
    … …  
    515516        // If there is no IndexRecordsIndex index at all, create it and then bail.
    516517        if (sqliteResult == SQLITE_DONE) {
    517             if (!m_sqliteDB->executeCommand(v1IndexRecordsIndexSchema())) {
     518            if (!m_sqliteDB->executeCommand(v2IndexRecordsIndexSchema)) {
    518519                LOG_ERROR("Could not create IndexRecordsIndex index in database (%i) - %s", m_sqliteDB->lastError(), m_sqliteDB->lastErrorMsg());
    519520                return false;
    … …  
    534535
    535536    // If the schema in the backing store is the current schema, we're done.
    536     if (currentSchema == v1IndexRecordsIndexSchema())
     537    if (currentSchema == v2IndexRecordsIndexSchema)
    537538        return true;
    538539
    539     // There is currently no outdated schema for the IndexRecordsIndex, so any other existing schema means this database is invalid.
    540     return false;
     540    RELEASE_ASSERT(currentSchema == v1IndexRecordsIndexSchema());
     541
     542    SQLiteTransaction transaction(*m_sqliteDB);
     543    transaction.begin();
     544
     545    if (!m_sqliteDB->executeCommand("DROP INDEX IndexRecordsIndex")) {
     546        LOG_ERROR("Could not drop index IndexRecordsIndex in database (%i) - %s", m_sqliteDB->lastError(), m_sqliteDB->lastErrorMsg());
     547        return false;
     548    }
     549
     550    if (!m_sqliteDB->executeCommand(v2IndexRecordsIndexSchema)) {
     551        LOG_ERROR("Could not create IndexRecordsIndex index in database (%i) - %s", m_sqliteDB->lastError(), m_sqliteDB->lastErrorMsg());
     552        return false;
     553    }
     554
     555    transaction.commit();
     556
     557    return true;
    541558}
    542559

  • trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp

    r255318 r256414  
    117117}
    118118
     119static String buildPreIndexStatement(bool isDirectionNext)
     120{
     121    StringBuilder builder;
     122
     123    builder.appendLiteral("SELECT rowid, key, value FROM IndexRecords WHERE indexID = ? AND key = CAST(? AS TEXT) AND value ");
     124    if (isDirectionNext)
     125        builder.append('>');
     126    else
     127        builder.append('<');
     128
     129    builder.appendLiteral(" CAST(? AS TEXT) ORDER BY value");
     130    if (!isDirectionNext)
     131        builder.appendLiteral(" DESC");
     132
     133    builder.append(';');
     134
     135    return builder.toString();
     136}
     137
    119138static String buildIndexStatement(const IDBKeyRangeData& keyRange, IndexedDB::CursorDirection cursorDirection)
    120139{
    … …  
    219238
    220239    m_currentKeyForUniqueness = m_fetchedRecords.first().record.key;
    221 
    222     if (m_cursorDirection != IndexedDB::CursorDirection::Nextunique && m_cursorDirection != IndexedDB::CursorDirection::Prevunique) {
    223         if (!m_fetchedRecords.last().isTerminalRecord())
    224             fetch(ShouldFetchForSameKey::Yes);
    225 
    226         while (m_fetchedRecords.last().record.key != m_fetchedRecords.first().record.key) {
    227             ASSERT(m_fetchedRecordsSize >= m_fetchedRecords.last().record.size());
    228             m_fetchedRecordsSize -= m_fetchedRecords.last().record.size();
    229             m_fetchedRecords.removeLast();
    230         }
    231     } else {
    232         m_fetchedRecords.clear();
    233         m_fetchedRecordsSize = 0;
    234     }
     240    if (m_indexID != IDBIndexInfo::InvalidId)
     241        m_currentIndexRecordValue = m_fetchedRecords.first().record.primaryKey;
    235242
    236243    // If ObjectStore or Index contents changed, we need to reset the statement and bind new parameters to it.
    237244    // This is to pick up any changes that might exist.
    238     // We also need to throw away any fetched records as they may no longer be valid.
    239 
    240245    m_statementNeedsReset = true;
    241246
    242     if (m_cursorDirection == IndexedDB::CursorDirection::Next || m_cursorDirection == IndexedDB::CursorDirection::Nextunique) {
     247    if (isDirectionNext()) {
    243248        m_currentLowerKey = m_currentKeyForUniqueness;
    244249        if (!m_keyRange.lowerOpen) {
    … …  
    255260        }
    256261    }
     262
     263    // We also need to throw away any fetched records as they may no longer be valid.
     264    m_fetchedRecords.clear();
     265    m_fetchedRecordsSize = 0;
    257266}
    258267
    … …  
    299308    if (m_statement->bindBlob(currentBindArgument++, buffer->data(), buffer->size()) != SQLITE_OK) {
    300309        LOG_ERROR("Could not create cursor statement (upper key)");
     310        return false;
     311    }
     312
     313    return true;
     314}
     315
     316bool SQLiteIDBCursor::resetAndRebindPreIndexStatementIfNecessary()
     317{
     318    if (m_indexID == IDBIndexInfo::InvalidId)
     319        return true;
     320
     321    if (m_currentIndexRecordValue.isNull())
     322        return true;
     323
     324    auto& database = m_transaction->sqliteTransaction()->database();
     325    if (!m_preIndexStatement) {
     326        m_preIndexStatement = makeUnique<SQLiteStatement>(database, buildPreIndexStatement(isDirectionNext()));
     327
     328        if (m_preIndexStatement->prepare() != SQLITE_OK) {
     329            LOG_ERROR("Could not prepare pre statement - '%s'", database.lastErrorMsg());
     330            return false;
     331        }
     332    }
     333
     334    if (m_preIndexStatement->reset() != SQLITE_OK) {
     335        LOG_ERROR("Could not reset pre statement - '%s'", database.lastErrorMsg());
     336        return false;
     337    }
     338
     339    auto key = isDirectionNext() ? m_currentLowerKey : m_currentUpperKey;
     340    int currentBindArgument = 1;
     341
     342    if (m_preIndexStatement->bindInt64(currentBindArgument++, m_boundID) != SQLITE_OK) {
     343        LOG_ERROR("Could not bind id argument to pre statement (bound ID)");
     344        return false;
     345    }
     346
     347    RefPtr<SharedBuffer> buffer = serializeIDBKeyData(key);
     348    if (m_preIndexStatement->bindBlob(currentBindArgument++, buffer->data(), buffer->size()) != SQLITE_OK) {
     349        LOG_ERROR("Could not bind id argument to pre statement (key)");
     350        return false;
     351    }
     352
     353    buffer = serializeIDBKeyData(m_currentIndexRecordValue);
     354    if (m_preIndexStatement->bindBlob(currentBindArgument++, buffer->data(), buffer->size()) != SQLITE_OK) {
     355        LOG_ERROR("Could not bind id argument to pre statement (value)");
    301356        return false;
    302357    }
    … …  
    375430}
    376431
    377 bool SQLiteIDBCursor::fetch(ShouldFetchForSameKey shouldFetchForSameKey)
     432bool SQLiteIDBCursor::fetch()
    378433{
    379434    ASSERT(m_fetchedRecords.isEmpty() || !m_fetchedRecords.last().isTerminalRecord());
    … …  
    381436    m_fetchedRecords.append({ });
    382437
    383     bool isUnique = m_cursorDirection == IndexedDB::CursorDirection::Nextunique || m_cursorDirection == IndexedDB::CursorDirection::Prevunique || shouldFetchForSameKey == ShouldFetchForSameKey::Yes;
     438    bool isUnique = m_cursorDirection == IndexedDB::CursorDirection::Nextunique || m_cursorDirection == IndexedDB::CursorDirection::Prevunique;
    384439    if (!isUnique) {
    385440        bool fetchSucceeded = fetchNextRecord(m_fetchedRecords.last());
    … …  
    398453            return false;
    399454
    400         if (shouldFetchForSameKey == ShouldFetchForSameKey::Yes)
    401             m_fetchedRecords.append({ });
     455        m_fetchedRecordsSize -= m_fetchedRecords.last().record.size();
    402456    }
    403457
    … …  
    407461bool SQLiteIDBCursor::fetchNextRecord(SQLiteCursorRecord& record)
    408462{
    409     if (m_statementNeedsReset)
     463    if (m_statementNeedsReset) {
     464        resetAndRebindPreIndexStatementIfNecessary();
    410465        resetAndRebindStatement();
     466    }
    411467
    412468    FetchResult result;
    … …  
    435491    record.record.value = nullptr;
    436492
    437     int result = m_statement->step();
    438     if (result == SQLITE_DONE) {
    439         // When a cursor reaches its end, that is indicated by having undefined keys/values
    440         record = { };
    441         record.completed = true;
    442 
    443         return FetchResult::Success;
    444     }
    445 
    446     if (result != SQLITE_ROW) {
    447         LOG_ERROR("Error advancing cursor - (%i) %s", result, m_transaction->sqliteTransaction()->database().lastErrorMsg());
    448         markAsErrored(record);
    449         return FetchResult::Failure;
    450     }
    451 
    452     record.rowID = m_statement->getColumnInt64(0);
     493    auto& database = m_transaction->sqliteTransaction()->database();
     494    SQLiteStatement* statement = nullptr;
     495
     496    int result;
     497    if (m_preIndexStatement) {
     498        ASSERT(m_indexID != IDBIndexInfo::InvalidId);
     499
     500        result = m_preIndexStatement->step();
     501        if (result == SQLITE_ROW)
     502            statement = m_preIndexStatement.get();
     503        else if (result != SQLITE_DONE)
     504            LOG_ERROR("Error advancing with pre statement - (%i) %s", result, database.lastErrorMsg());
     505    }
     506   
     507    if (!statement) {
     508        result = m_statement->step();
     509        if (result == SQLITE_DONE) {
     510            record = { };
     511            record.completed = true;
     512            return FetchResult::Success;
     513        }
     514        if (result != SQLITE_ROW) {
     515            LOG_ERROR("Error advancing cursor - (%i) %s", result, database.lastErrorMsg());
     516            markAsErrored(record);
     517            return FetchResult::Failure;
     518        }
     519        statement = m_statement.get();
     520    }
     521
     522    record.rowID = statement->getColumnInt64(0);
    453523    ASSERT(record.rowID);
    454524
    455525    Vector<uint8_t> keyData;
    456     m_statement->getColumnBlobAsVector(1, keyData);
     526    statement->getColumnBlobAsVector(1, keyData);
    457527
    458528    if (!deserializeIDBKeyData(keyData.data(), keyData.size(), record.record.key)) {
    … …  
    462532    }
    463533
    464     m_statement->getColumnBlobAsVector(2, keyData);
     534    statement->getColumnBlobAsVector(2, keyData);
    465535
    466536    // The primaryKey of an ObjectStore cursor is the same as its key.
    … …  
    486556
    487557        if (!m_cachedObjectStoreStatement || m_cachedObjectStoreStatement->reset() != SQLITE_OK) {
    488             m_cachedObjectStoreStatement = makeUnique<SQLiteStatement>(m_statement->database(), "SELECT value FROM Records WHERE key = CAST(? AS TEXT) and objectStoreID = ?;");
     558            m_cachedObjectStoreStatement = makeUnique<SQLiteStatement>(database, "SELECT value FROM Records WHERE key = CAST(? AS TEXT) and objectStoreID = ?;");
    489559            if (m_cachedObjectStoreStatement->prepare() != SQLITE_OK)
    490560                m_cachedObjectStoreStatement = nullptr;
    … …  
    494564            || m_cachedObjectStoreStatement->bindBlob(1, keyData.data(), keyData.size()) != SQLITE_OK
    495565            || m_cachedObjectStoreStatement->bindInt64(2, m_objectStoreID) != SQLITE_OK) {
    496             LOG_ERROR("Could not create index cursor statement into object store records (%i) '%s'", m_statement->database().lastError(), m_statement->database().lastErrorMsg());
     566            LOG_ERROR("Could not create index cursor statement into object store records (%i) '%s'", database.lastError(), database.lastErrorMsg());
    497567            markAsErrored(record);
    498568            return FetchResult::Failure;
    … …  
    509579            return FetchResult::ShouldFetchAgain;
    510580        } else {
    511             LOG_ERROR("Could not step index cursor statement into object store records (%i) '%s'", m_statement->database().lastError(), m_statement->database().lastErrorMsg());
     581            LOG_ERROR("Could not step index cursor statement into object store records (%i) '%s'", database.lastError(), database.lastErrorMsg());
    512582            markAsErrored(record);
    513583            return FetchResult::Failure;

  • trunk/Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.h

    r254264 r256414  
    4545namespace IDBServer {
    4646
    47 enum class ShouldFetchForSameKey : bool { No, Yes };
    48 
    4947class SQLiteIDBTransaction;
    5048
    … …  
    8785    bool bindArguments();
    8886
     87    bool resetAndRebindPreIndexStatementIfNecessary();
    8988    void resetAndRebindStatement();
    9089
    … …  
    9594    };
    9695
    97     bool fetch(ShouldFetchForSameKey = ShouldFetchForSameKey::No);
     96    bool fetch();
    9897
    9998    struct SQLiteCursorRecord {
    … …  
    109108    void markAsErrored(SQLiteCursorRecord&);
    110109
     110    bool isDirectionNext() const { return m_cursorDirection == IndexedDB::CursorDirection::Next || m_cursorDirection == IndexedDB::CursorDirection::Nextunique; }
     111
    111112    SQLiteIDBTransaction* m_transaction;
    112113    IDBResourceIdentifier m_cursorIdentifier;
    … …  
    119120    IDBKeyData m_currentLowerKey;
    120121    IDBKeyData m_currentUpperKey;
     122    IDBKeyData m_currentIndexRecordValue;
    121123
    122124    Deque<SQLiteCursorRecord> m_fetchedRecords;
    … …  
    124126    IDBKeyData m_currentKeyForUniqueness;
    125127
     128    std::unique_ptr<SQLiteStatement> m_preIndexStatement;
    126129    std::unique_ptr<SQLiteStatement> m_statement;
    127130    std::unique_ptr<SQLiteStatement> m_cachedObjectStoreStatement;
Note: See TracChangeset for help on using the changeset viewer.