IndexedDB: Implement IndexedDB abort rollback for object store metadata and key generators - #43950
Merged
Merged
Conversation
|
🔨 Triggering try run (#24004841875) for Linux (WPT) |
|
|
|
🔨 Triggering try run (#24005188571) for Linux (WPT) |
|
Test results for linux-wpt from try job (#24005188571): Flaky unexpected result (28)
Stable unexpected results that are known to be intermittent (19)
|
|
✨ Try run (#24005188571) succeeded. |
|
🔨 Triggering try run (#24006594206) for Linux (WPT) |
|
Test results for linux-wpt from try job (#24006594206): Flaky unexpected result (33)
Stable unexpected results that are known to be intermittent (20)
|
|
✨ Try run (#24006594206) succeeded. |
Contributor
|
I wonder if this could be done with SQLite transactions. |
Member
Author
I think only with a larger refactor, right now ourr IndexedDB transaction is split into engine batches, and the SQLite backend opens a fresh connection per batch rather than holding one SQLite transaction for the whole IDB transaction. Upgrade schema changes are also applied eagerly outside process_transaction(). |
janvarga
self-requested a review
April 5, 2026 23:24
Member
|
Let me look at this tomorrow or so. |
Member
|
Sorry, I can't get to this today, so I'm ok if anyone else is willing to take a look. |
Member
Author
no rush. |
gterzian
approved these changes
May 21, 2026
gterzian
left a comment
Member
There was a problem hiding this comment.
LGTM with some small suggestions
| #[derive(MallocSizeOf)] | ||
| struct KeyGeneratorSnapshot { | ||
| store_name: String, | ||
| current_number: i32, |
Member
There was a problem hiding this comment.
| newly_created_during_transaction: bool, | ||
| rollback_name_on_abort: DomRefCell<Option<DOMString>>, | ||
| #[no_trace] | ||
| rollback_indexes_on_abort: DomRefCell<Vec<indexeddb::IndexedDBIndex>>, |
Member
There was a problem hiding this comment.
why not store an optional IDBObjectStoreAbortState? (and add the rollback name to it) It just seems cleaner to do everything with one struct.
also for example newly_created_during_transaction I think just could mean "does not have an abort state"
| return Err(Error::QueryReturnedNoRows); | ||
| } | ||
| Ok(()) | ||
| } |
Member
There was a problem hiding this comment.
is this only used in a case where the number reset to a previous value?
Because if the rows are not affected because they were unchanged then the error would teh wrong one, so in other words the existence check is collapsed into a check for an update, and the two might actually be different unless this is only used to update an existing row, and then it could be documented as such.
.to_string() is that necessary?
| struct TxnInfo { | ||
| created_seq: u64, | ||
| mode: IndexedDBTxnMode, | ||
| scope: HashSet<String>, |
Member
There was a problem hiding this comment.
here I think scope can be updated to be a datastructure including the KeyGeneratorSnapshot, because the spec always refers to "he key generator for each object store in the transaction’s scope"
| } | ||
| } | ||
|
|
||
| for store in object_stores { |
Member
There was a problem hiding this comment.
i find the logic of this method quite hard to follow, could you document the method with an overview of what it does?
Essentially it resets the db in a previous state right? deleting stores and indices that would have been created during the transaction and resetting the state of pre-existing ones?
…ta and key generators Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
Taym95
force-pushed
the
indexeddb-abort-rollback
branch
from
May 22, 2026 10:38
fa398d5 to
e0a3c73
Compare
|
🔨 Triggering try run (#26283001798) for Linux (WPT) |
|
Test results for linux-wpt from try job (#26283001798): Flaky unexpected result (67)
Stable unexpected results that are known to be intermittent (20)
Stable unexpected results (3)
|
|
|
Taym95
force-pushed
the
indexeddb-abort-rollback
branch
from
May 22, 2026 11:29
e0a3c73 to
08cd9f9
Compare
|
🔨 Triggering try run (#26285265815) for Linux (WPT) |
|
Test results for linux-wpt from try job (#26285265815): Flaky unexpected result (53)
Stable unexpected results that are known to be intermittent (26)
|
|
✨ Try run (#26285265815) succeeded. |
Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
Taym95
force-pushed
the
indexeddb-abort-rollback
branch
from
May 22, 2026 13:00
08cd9f9 to
304a31b
Compare
Taym95
enabled auto-merge
May 22, 2026 13:00
Wzy312
added a commit
to Wzy312/servo-ohos
that referenced
this pull request
Jul 27, 2026
delete_database now removes the directories rows of the target database explicitly, inside the same transaction and ahead of the parent row. The foreign_keys pragma applies per connection, and the dedicated transaction connections open with it off, so the explicit delete replaces the cascade on this path. A device check after create and delete shows the registry clean of that database and free of orphan directories rows. A write whose SQL execution fails now ends the batch. The native transaction rolls back and every queued callback reports the error. Validation failures that leave the store untouched, such as a missing key, an exhausted key generator, or an unknown store, keep reporting through their own request while the batch continues. Widening the abort to those cases pushed upgrade transactions into the abort path that servo#43950 deferred, and turned seven WPT keypath subtests into device timeouts. The engine comment now matches this behavior. The quota bucket covers RDB_E_SQLITE_IOERR alongside RDB_E_SQLITE_FULL. The Api error carries a bare status code, so the mapping reads the code directly, while the ENOSPC source-chain check keeps serving the Io variant. This is slightly broader than the sqlite twin, where an IOERR also needs an ENOSPC in its chain. A device-tier unit test pins the mapping end to end against a max_page_count-limited store, with the pragma set on the transaction connection where it takes effect. The three on-device unit tests expect the rdb/ subdirectory that RDB creates under the database dir. The integration tier now builds for the device target, with the crate-level gate lifted and the inner webstorage gate removed, while tests/client_storage.rs keeps its gate for rusqlite. Four engine-agnostic IndexedDB cases join the suite, covering cross-type key sort order, key-range open and closed bounds, auto-increment, and a batch write failure surfacing an error. The two path-included test modules carry allow(dead_code), which restores a warning-free host test build. The webstorage load-failure warning now says the in-memory map starts empty and the final save is skipped. This round was validated with cargo test -p servo-storage on a Linux host, 52 tests passing with a warning-free build, and with cargo test --no-run for the aarch64-unknown-linux-ohos target with the ohos-rdb-backend feature, which also builds warning-free. On the device, the ten WPT storage tests match desktop expectations on 1218 of 1225 subtests, and the previous device-tested build reproduces the same seven-timeout keypath pattern on the same day, which places the drift in the environment. The six storage cases pass, an interior-NUL value survives a process kill and restart, and the registry row checks above come from the pulled reg.rdb. AI-assissted: Yes (agent session; implementation and testing on device)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IndexedDB: Implement IndexedDB abort rollback for object store metadata and key generators
Testing: abort rollback WPT test passed.
part of #40983