Skip to content

LibWeb/IDB: Handle cursor iteration more correctly #6628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
trflynn89 merged 1 commit into from
Nov 13, 2025
Merged

LibWeb/IDB: Handle cursor iteration more correctly #6628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 74 additions & 90 deletions Libraries/LibWeb/IndexedDB/Internal/Algorithms.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1599,27 +1599,28 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
// * If key is defined:
if (key) {
// * The record’s key is greater than or equal to key.
auto is_greater_than_or_equal = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[key](auto const& inner_record) {
return Key::greater_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
});

if (!is_greater_than_or_equal)
if (!record.visit([&](auto const& inner_record) {
return Key::greater_than_or_equal(inner_record.key, *key);
}))
return false;
}

// * If primaryKey is defined:
if (primary_key) {
auto const& inner_record = record.get<IndexRecord>();

// * The record’s key is equal to key and the record’s value is greater than or equal to primaryKey
if (!(Key::equals(inner_record.key, *key) && (Key::greater_than(inner_record.value, *primary_key) || Key::equals(inner_record.value, *primary_key))))
return false;

// * The record’s key is greater than key.
if (!Key::greater_than(inner_record.key, *key))
return false;
// * If the record’s key is equal to key:
if (Key::equals(inner_record.key, *key)) {

@lpas lpas Nov 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the retain_satisfaction I find this harder to read and the code doesn't short circuit there anymore

Seems like you could just keep the old style with the changes?

if (Key::equals(inner_record.key, *key)) {
    if (!(Key::greater_than(inner_record.value, *primary_key) || Key::equals(inner_record.value, *primary_key)))
        return false;
} else {
    if (!Key::greater_than(inner_record.key, *key))
        return false;
}

@stelar7 stelar7 Nov 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inversion of the conditions made matching the logic and spec comments harder to follow imo
Since it reads as "If this, do that", but the code say "If not this, do that"

@trflynn89 trflynn89 Nov 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree retain_satisfaction is quite awkward. It looks like a strange way to avoid using the &= operator. This also looks less efficient in that we no longer return early upon failure.

You've also changed the copied spec text in a way that makes it harder to compare to the spec itself, IMO.

Does the following not suffice?

diff --git a/Libraries/LibWeb/IndexedDB/Internal/Algorithms.cpp b/Libraries/LibWeb/IndexedDB/Internal/Algorithms.cpp
index 16ed5c6815..b69beca703 100644
--- a/Libraries/LibWeb/IndexedDB/Internal/Algorithms.cpp
+++ b/Libraries/LibWeb/IndexedDB/Internal/Algorithms.cpp
@@ -1602,7 +1602,7 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto is_greater_than_or_equal = record.visit(
                 [](Empty) { VERIFY_NOT_REACHED(); },
                 [key](auto const& inner_record) {
-                    return Key::greater_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
+                    return Key::greater_than_or_equal(inner_record.key, *key);
                 });

             if (!is_greater_than_or_equal)
@@ -1614,12 +1614,14 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto const& inner_record = record.get<IndexRecord>();

             // * The record’s key is equal to key and the record’s value is greater than or equal to primaryKey
-            if (!(Key::equals(inner_record.key, *key) && (Key::greater_than(inner_record.value, *primary_key) || Key::equals(inner_record.value, *primary_key))))
-                return false;
-
+            if (Key::equals(inner_record.key, *key)) {
+                if (!Key::greater_than_or_equal(inner_record.value, *primary_key))
+                    return false;
+            }
             // * The record’s key is greater than key.
-            if (!Key::greater_than(inner_record.key, *key))
+            else if (!Key::greater_than(inner_record.key, *key)) {
                 return false;
+            }
         }

         // * If position is defined and source is an object store:
@@ -1636,12 +1638,14 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto const& inner_record = record.get<IndexRecord>();

             // * The record’s key is equal to position and the record’s value is greater than object store position
-            if (!(Key::equals(inner_record.key, *position) && (Key::greater_than(inner_record.value, *object_store_position))))
-                return false;
-
+            if (Key::equals(inner_record.key, *position)) {
+                if (!Key::greater_than(inner_record.value, *object_store_position))
+                    return false;
+            }
             // * The record’s key is greater than position.
-            if (!Key::greater_than(inner_record.key, *position))
+            else if (!Key::greater_than(inner_record.key, *position)) {
                 return false;
+            }
         }

         // * The record’s key is in range.
@@ -1661,7 +1665,7 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto is_greater_than_or_equal = record.visit(
                 [](Empty) { VERIFY_NOT_REACHED(); },
                 [key](auto const& inner_record) {
-                    return Key::greater_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
+                    return Key::greater_than_or_equal(inner_record.key, *key);
                 });

             if (!is_greater_than_or_equal)
@@ -1674,7 +1678,7 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto is_greater_than_position = record.visit(
                 [](Empty) { VERIFY_NOT_REACHED(); },
                 [position](auto const& inner_record) {
-                    return Key::greater_than(inner_record.key, *position) || Key::equals(inner_record.key, *position);
+                    return Key::greater_than(inner_record.key, *position);
                 });

             if (!is_greater_than_position)
@@ -1698,7 +1702,7 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto is_less_than_or_equal = record.visit(
                 [](Empty) { VERIFY_NOT_REACHED(); },
                 [key](auto const& inner_record) {
-                    return Key::less_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
+                    return Key::less_than_or_equal(inner_record.key, *key);
                 });

             if (!is_less_than_or_equal)
@@ -1710,12 +1714,14 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto const& inner_record = record.get<IndexRecord>();

             // * The record’s key is equal to key and the record’s value is less than or equal to primaryKey
-            if (!(Key::equals(inner_record.key, *key) && (Key::less_than(inner_record.value, *primary_key) || Key::equals(inner_record.value, *primary_key))))
-                return false;
-
+            if (Key::equals(inner_record.key, *key)) {
+                if (!Key::less_than_or_equal(inner_record.value, *primary_key))
+                    return false;
+            }
             // * The record’s key is less than key.
-            if (!Key::less_than(inner_record.key, *key))
+            else if (!Key::less_than(inner_record.key, *key)) {
                 return false;
+            }
         }

         // * If position is defined and source is an object store:
@@ -1732,12 +1738,14 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto const& inner_record = record.get<IndexRecord>();

             // * The record’s key is equal to position and the record’s value is less than object store position
-            if (!(Key::equals(inner_record.key, *position) && Key::less_than(inner_record.value, *object_store_position)))
-                return false;
-
+            if (Key::equals(inner_record.key, *position)) {
+                if (!Key::less_than(inner_record.value, *object_store_position))
+                    return false;
+            }
             // * The record’s key is less than position.
-            if (!Key::less_than(inner_record.key, *position))
+            else if (!Key::less_than(inner_record.key, *position)) {
                 return false;
+            }
         }

         // * The record’s key is in range.
@@ -1757,7 +1765,7 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto is_less_than_or_equal = record.visit(
                 [](Empty) { VERIFY_NOT_REACHED(); },
                 [key](auto const& inner_record) {
-                    return Key::less_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
+                    return Key::less_than_or_equal(inner_record.key, *key);
                 });

             if (!is_less_than_or_equal)
@@ -1770,7 +1778,7 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
             auto is_less_than_position = record.visit(
                 [](Empty) { VERIFY_NOT_REACHED(); },
                 [position](auto const& inner_record) {
-                    return Key::less_than(inner_record.key, *position) || Key::equals(inner_record.key, *position);
+                    return Key::less_than(inner_record.key, *position);
                 });

             if (!is_less_than_position)

@stelar7 stelar7 Nov 6, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree retain_satisfaction is quite awkward. It looks like a strange way to avoid using the &= operator.

I forgot that &= existed tbh 😅

You've also changed the copied spec text in a way that makes it harder to compare to the spec itself, IMO.

It depends on the outcome of the linked spec PR. For now its matching the wording in that PR.

This also looks less efficient in that we no longer return early upon failure.

Ive restored the early return (and imo. improved readability) with a macro now

@trflynn89 trflynn89 Nov 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive restored the early return (and imo. improved readability) with a macro now

This does not feel like an improvement to me, a macro just adds a level of indirection here when reading this code. Let's not be unnecessarily fancy, I would much prefer just a bog standard:

if (!condition)
    return false;

@stelar7 stelar7 Nov 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Macro removed 👍

// * The record’s value is greater than or equal to primaryKey
if (!Key::greater_than_or_equal(inner_record.value, *primary_key))
return false;
}
// * Else:
else {
// * The record’s key is greater than key.
if (!Key::greater_than(inner_record.key, *key))
return false;
}
}

// * If position is defined and source is an object store:
Expand All @@ -1635,87 +1636,79 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
if (position && source.has<GC::Ref<Index>>()) {
auto const& inner_record = record.get<IndexRecord>();

// * The record’s key is equal to position and the record’s value is greater than object store position
if (!(Key::equals(inner_record.key, *position) && (Key::greater_than(inner_record.value, *object_store_position))))
return false;

// * The record’s key is greater than position.
if (!Key::greater_than(inner_record.key, *position))
return false;
// * If the record’s key is equal to position:
if (Key::equals(inner_record.key, *position)) {
// * The record’s value is greater than object store position
if (!Key::greater_than(inner_record.value, *object_store_position))
return false;
}
// * Else:
else {
// * The record’s key is greater than position.
if (!Key::greater_than(inner_record.key, *position))
return false;
}
}

// * The record’s key is in range.
auto is_in_range = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[range](auto const& inner_record) {
return record.visit(
[&](auto const& inner_record) {
return range->is_in_range(inner_record.key);
});

return is_in_range;
};

auto next_unique_requirements = [&](Variant<ObjectStoreRecord, IndexRecord> const& record) -> bool {
// * If key is defined:
if (key) {
// * The record’s key is greater than or equal to key.
auto is_greater_than_or_equal = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[key](auto const& inner_record) {
return Key::greater_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
});

if (!is_greater_than_or_equal)
if (!record.visit([&](auto const& inner_record) {
return Key::greater_than_or_equal(inner_record.key, *key);
}))
return false;
}

// * If position is defined:
if (position) {
// * The record’s key is greater than position.
auto is_greater_than_position = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[position](auto const& inner_record) {
return Key::greater_than(inner_record.key, *position) || Key::equals(inner_record.key, *position);
});

if (!is_greater_than_position)
if (!record.visit([&](auto const& inner_record) {
return Key::greater_than(inner_record.key, *position);
}))
return false;
}

// * The record’s key is in range.
auto is_in_range = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[range](auto const& inner_record) {
return record.visit(
[&](auto const& inner_record) {
return range->is_in_range(inner_record.key);
});

return is_in_range;
};

auto prev_requirements = [&](Variant<ObjectStoreRecord, IndexRecord> const& record) -> bool {
// * If key is defined:
if (key) {
// * The record’s key is less than or equal to key.
auto is_less_than_or_equal = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[key](auto const& inner_record) {
return Key::less_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
});

if (!is_less_than_or_equal)
if (!record.visit([&](auto const& inner_record) {
return Key::less_than_or_equal(inner_record.key, *key);
}))
return false;
}

// * If primaryKey is defined:
if (primary_key) {
auto const& inner_record = record.get<IndexRecord>();

// * The record’s key is equal to key and the record’s value is less than or equal to primaryKey
if (!(Key::equals(inner_record.key, *key) && (Key::less_than(inner_record.value, *primary_key) || Key::equals(inner_record.value, *primary_key))))
return false;

// * The record’s key is less than key.
if (!Key::less_than(inner_record.key, *key))
return false;
// * If the record’s key is equal to key:
if (Key::equals(inner_record.key, *key)) {
// * The record’s value is less than or equal to primaryKey
if (!Key::less_than_or_equal(inner_record.value, *primary_key))
return false;
}
// * Else:
else {
// * The record’s key is less than key.
if (!Key::less_than(inner_record.key, *key))
return false;
}
}

// * If position is defined and source is an object store:
Expand All @@ -1731,60 +1724,51 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
if (position && source.has<GC::Ref<Index>>()) {
auto const& inner_record = record.get<IndexRecord>();

// * The record’s key is equal to position and the record’s value is less than object store position
if (!(Key::equals(inner_record.key, *position) && Key::less_than(inner_record.value, *object_store_position)))
return false;

// * The record’s key is less than position.
if (!Key::less_than(inner_record.key, *position))
return false;
// * If the record’s key is equal to position:
if (Key::equals(inner_record.key, *position)) {
// * The record’s value is less than object store position
if (!Key::less_than(inner_record.value, *object_store_position))
return false;
}
// Else:
else {
// * The record’s key is less than position.
if (!Key::less_than(inner_record.key, *position))
return false;
}
}

// * The record’s key is in range.
auto is_in_range = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[range](auto const& inner_record) {
return record.visit(
[&](auto const& inner_record) {
return range->is_in_range(inner_record.key);
});

return is_in_range;
};

auto prev_unique_requirements = [&](Variant<ObjectStoreRecord, IndexRecord> const& record) -> bool {
// * If key is defined:
if (key) {
// * The record’s key is less than or equal to key.
auto is_less_than_or_equal = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[key](auto const& inner_record) {
return Key::less_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
});

if (!is_less_than_or_equal)
if (!record.visit([&](auto const& inner_record) {
return Key::less_than_or_equal(inner_record.key, *key);
}))
return false;
}

//* If position is defined:
if (position) {
// * The record’s key is less than position.
auto is_less_than_position = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[position](auto const& inner_record) {
return Key::less_than(inner_record.key, *position) || Key::equals(inner_record.key, *position);
});

if (!is_less_than_position)
if (!record.visit([&](auto const& inner_record) {
return Key::less_than(inner_record.key, *position);
}))
return false;
}

// * The record’s key is in range.
auto is_in_range = record.visit(
[](Empty) { VERIFY_NOT_REACHED(); },
[range](auto const& inner_record) {
return record.visit(
[&](auto const& inner_record) {
return range->is_in_range(inner_record.key);
});

return is_in_range;
};

// 9. While count is greater than 0:
Expand Down
2 changes: 2 additions & 0 deletions Libraries/LibWeb/IndexedDB/Internal/Key.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ class Key : public JS::Cell {
[[nodiscard]] static bool equals(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) == 0; }
[[nodiscard]] static bool less_than(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) < 0; }
[[nodiscard]] static bool greater_than(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) > 0; }
[[nodiscard]] static bool less_than_or_equal(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) <= 0; }
[[nodiscard]] static bool greater_than_or_equal(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) >= 0; }

AK::String dump() const;

Expand Down
7 changes: 3 additions & 4 deletions Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-advance-invalid.any.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@ Harness status: OK

Found 6 tests

4 Pass
2 Fail
Fail IDBCursor.advance() - invalid - attempt to call advance twice
6 Pass
Pass IDBCursor.advance() - invalid - attempt to call advance twice
Pass IDBCursor.advance() - invalid - pass something other than number
Pass IDBCursor.advance() - invalid - pass null/undefined
Pass IDBCursor.advance() - invalid - missing argument
Pass IDBCursor.advance() - invalid - pass negative numbers
Fail IDBCursor.advance() - invalid - got value not set on exception
Pass IDBCursor.advance() - invalid - got value not set on exception
11 changes: 5 additions & 6 deletions Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-advance.any.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@ Harness status: OK

Found 6 tests

2 Pass
4 Fail
Fail IDBCursor.advance() - advances
Fail IDBCursor.advance() - advances backwards
6 Pass
Pass IDBCursor.advance() - advances
Pass IDBCursor.advance() - advances backwards
Pass IDBCursor.advance() - skip far forward
Fail IDBCursor.advance() - within range
Pass IDBCursor.advance() - within range
Pass IDBCursor.advance() - within single key range
Fail IDBCursor.advance() - within single key range, with several results
Pass IDBCursor.advance() - within single key range, with several results
11 changes: 5 additions & 6 deletions Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-continue.any.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@ Harness status: OK

Found 6 tests

2 Pass
4 Fail
Fail IDBCursor.continue() - continues
Fail IDBCursor.continue() - with given key
6 Pass
Pass IDBCursor.continue() - continues
Pass IDBCursor.continue() - with given key
Pass IDBCursor.continue() - skip far forward
Fail IDBCursor.continue() - within range
Pass IDBCursor.continue() - within range
Pass IDBCursor.continue() - within single key range
Fail IDBCursor.continue() - within single key range, with several results
Pass IDBCursor.continue() - within single key range, with several results
9 changes: 9 additions & 0 deletions Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-direction-index-keyrange.any.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Harness status: OK

Found 4 tests

4 Pass
Pass IDBCursor direction - index with keyrange - next
Pass IDBCursor direction - index with keyrange - prev
Pass IDBCursor direction - index with keyrange - nextunique
Pass IDBCursor direction - index with keyrange - prevunique
7 changes: 3 additions & 4 deletions Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-request-source.any.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@ Harness status: OK

Found 8 tests

6 Pass
2 Fail
8 Pass
Pass IDBObjectStore::openCursor's request source must be the IDBObjectStore instance that opened the cursor
Pass IDBObjectStore::openKeyCursor's request source must be the IDBObjectStore instance that opened the cursor
Fail IDBIndex::openCursor's request source must be the IDBIndex instance that opened the cursor
Fail IDBIndex::openKeyCursor's request source must be the IDBIndex instance that opened the cursor
Pass IDBIndex::openCursor's request source must be the IDBIndex instance that opened the cursor
Pass IDBIndex::openKeyCursor's request source must be the IDBIndex instance that opened the cursor
Pass The source of the request from IDBObjectStore::update() is the cursor itself
Pass The source of the request from IDBObjectStore::delete() is the cursor itself
Pass The source of the request from IDBIndex::update() is the cursor itself
Expand Down
15 changes: 15 additions & 0 deletions Tests/LibWeb/Text/input/wpt-import/IndexedDB/idbcursor-direction-index-keyrange.any.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!doctype html>
<meta charset=utf-8>
<title>IDBCursor direction - index with keyrange</title>
<script>
self.GLOBAL = {
isWindow: function() { return true; },
isWorker: function() { return false; },
isShadowRealm: function() { return false; },
};
</script>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<script src="resources/support.js"></script>
<div id=log></div>
<script src="../IndexedDB/idbcursor-direction-index-keyrange.any.js"></script>
Loading
Loading