LibWeb/IDB: Introduce an Invalid KeyType

This cleans up the code around failure/invalid/exception a bit

Libraries/LibWeb/IndexedDB/IDBFactory.cpp

@@ -108,18 +108,18 @@ WebIDL::ExceptionOr<i8> IDBFactory::cmp(JS::Value first, JS::Value second)
     auto a = TRY(convert_a_value_to_a_key(realm(), first));
 
     // 2. If a is invalid, throw a "DataError" DOMException.
-    if (a.is_error())
+    if (a->is_invalid())
         return WebIDL::DataError::create(realm(), "Failed to convert a value to a key"_string);
 
     // 3. Let b be the result of converting a value to a key with second. Rethrow any exceptions.
     auto b = TRY(convert_a_value_to_a_key(realm(), second));
 
     // 4. If b is invalid, throw a "DataError" DOMException.
-    if (b.is_error())
+    if (b->is_invalid())
         return WebIDL::DataError::create(realm(), "Failed to convert a value to a key"_string);
 
     // 5. Return the results of comparing two keys with a and b.
-    return Key::compare_two_keys(a.release_value(), b.release_value());
+    return Key::compare_two_keys(a, b);
 }
 
 // https://w3c.github.io/IndexedDB/#dom-idbfactory-deletedatabase

Libraries/LibWeb/IndexedDB/IDBKeyRange.cpp

@@ -64,14 +64,12 @@ WebIDL::ExceptionOr<GC::Ref<IDBKeyRange>> IDBKeyRange::only(JS::VM& vm, JS::Valu
     auto& realm = *vm.current_realm();
 
     // 1. Let key be the result of converting a value to a key with value. Rethrow any exceptions.
-    auto maybe_key = TRY(convert_a_value_to_a_key(realm, value));
+    auto key = TRY(convert_a_value_to_a_key(realm, value));
 
     // 2. If key is invalid, throw a "DataError" DOMException.
-    if (maybe_key.is_error())
+    if (key->is_invalid())
         return WebIDL::DataError::create(realm, "Value is invalid"_string);
 
-    auto key = maybe_key.release_value();
-
     // 3. Create and return a new key range containing only key.
     return IDBKeyRange::create(realm, key, key, false, false);
 }
@@ -82,14 +80,14 @@ WebIDL::ExceptionOr<GC::Ref<IDBKeyRange>> IDBKeyRange::lower_bound(JS::VM& vm, J
     auto& realm = *vm.current_realm();
 
     // 1. Let lowerKey be the result of converting a value to a key with lower. Rethrow any exceptions.
-    auto lower_key = TRY(convert_a_value_to_a_key(realm, lower));
+    auto key = TRY(convert_a_value_to_a_key(realm, lower));
 
     // 2. If lowerKey is invalid, throw a "DataError" DOMException.
-    if (lower_key.is_error())
+    if (key->is_invalid())
         return WebIDL::DataError::create(realm, "Value is invalid"_string);
 
     // 3. Create and return a new key range with lower bound set to lowerKey, lower open flag set to open, upper bound set to null, and upper open flag set to true.
-    return IDBKeyRange::create(realm, lower_key.release_value(), {}, open, true);
+    return IDBKeyRange::create(realm, key, {}, open, true);
 }
 
 // https://w3c.github.io/IndexedDB/#dom-idbkeyrange-upperbound
@@ -98,14 +96,14 @@ WebIDL::ExceptionOr<GC::Ref<IDBKeyRange>> IDBKeyRange::upper_bound(JS::VM& vm, J
     auto& realm = *vm.current_realm();
 
     // 1. Let upperKey be the result of converting a value to a key with upper. Rethrow any exceptions.
-    auto upper_key = TRY(convert_a_value_to_a_key(realm, upper));
+    auto key = TRY(convert_a_value_to_a_key(realm, upper));
 
     // 2. If upperKey is invalid, throw a "DataError" DOMException.
-    if (upper_key.is_error())
+    if (key->is_invalid())
         return WebIDL::DataError::create(realm, "Value is invalid"_string);
 
     // 3. Create and return a new key range with lower bound set to null, lower open flag set to true, upper bound set to upperKey, and upper open flag set to open.
-    return IDBKeyRange::create(realm, {}, upper_key.release_value(), true, open);
+    return IDBKeyRange::create(realm, {}, key, true, open);
 }
 
 // https://w3c.github.io/IndexedDB/#dom-idbkeyrange-bound
@@ -117,22 +115,22 @@ WebIDL::ExceptionOr<GC::Ref<IDBKeyRange>> IDBKeyRange::bound(JS::VM& vm, JS::Val
     auto lower_key = TRY(convert_a_value_to_a_key(realm, lower));
 
     // 2. If lowerKey is invalid, throw a "DataError" DOMException.
-    if (lower_key.is_error())
+    if (lower_key->is_invalid())
         return WebIDL::DataError::create(realm, "Value is invalid"_string);
 
     // 3. Let upperKey be the result of converting a value to a key with upper. Rethrow any exceptions.
     auto upper_key = TRY(convert_a_value_to_a_key(realm, upper));
 
     // 4. If upperKey is invalid, throw a "DataError" DOMException.
-    if (upper_key.is_error())
+    if (upper_key->is_invalid())
         return WebIDL::DataError::create(realm, "Value is invalid"_string);
 
     // 5. If lowerKey is greater than upperKey, throw a "DataError" DOMException.
-    if (Key::less_than(upper_key.release_value(), lower_key.release_value()))
+    if (Key::less_than(upper_key, lower_key))
         return WebIDL::DataError::create(realm, "Lower key is greater than upper key"_string);
 
     // 6. Create and return a new key range with lower bound set to lowerKey, lower open flag set to lowerOpen, upper bound set to upperKey and upper open flag set to upperOpen.
-    return IDBKeyRange::create(realm, lower_key.release_value(), upper_key.release_value(), lower_open, upper_open);
+    return IDBKeyRange::create(realm, lower_key, upper_key, lower_open, upper_open);
 }
 
 // https://w3c.github.io/IndexedDB/#dom-idbkeyrange-includes
@@ -144,11 +142,11 @@ WebIDL::ExceptionOr<bool> IDBKeyRange::includes(JS::Value key)
     auto k = TRY(convert_a_value_to_a_key(realm, key));
 
     // 2. If k is invalid, throw a "DataError" DOMException.
-    if (k.is_error())
+    if (k->is_invalid())
         return WebIDL::DataError::create(realm, "Value is invalid"_string);
 
     // 3. Return true if k is in this range, and false otherwise.
-    return is_in_range(k.release_value());
+    return is_in_range(k);
 }
 
 // https://w3c.github.io/IndexedDB/#dom-idbkeyrange-lower

Libraries/LibWeb/IndexedDB/IDBObjectStore.cpp

@@ -257,13 +257,14 @@ WebIDL::ExceptionOr<GC::Ref<IDBRequest>> IDBObjectStore::add_or_put(GC::Ref<IDBO
     // 8. If key was given, then:
     if (key_was_given) {
         // 1. Let r be the result of converting a value to a key with key. Rethrow any exceptions.
-        auto maybe_key = TRY(convert_a_value_to_a_key(realm, key.value()));
+        auto r = TRY(convert_a_value_to_a_key(realm, key.value()));
+
         // 2. If r is invalid, throw a "DataError" DOMException.
-        if (maybe_key.is_error())
+        if (r->is_invalid())
             return WebIDL::DataError::create(realm, "Key is invalid"_string);
 
         // 3. Let key be r.
-        key_value = maybe_key.release_value();
+        key_value = r;
     }
 
     // 9. Let targetRealm be a user-agent defined Realm.
@@ -277,13 +278,14 @@ WebIDL::ExceptionOr<GC::Ref<IDBRequest>> IDBObjectStore::add_or_put(GC::Ref<IDBO
         // 1. Let kpk be the result of extracting a key from a value using a key path with clone and store’s key path. Rethrow any exceptions.
         auto maybe_kpk = TRY(extract_a_key_from_a_value_using_a_key_path(realm, clone, store.key_path().value()));
 
-        // 2. If kpk is invalid, throw a "DataError" DOMException.
+        // NOTE: Step 2 and 3 is reversed here, since we check for failure before validity.
-        if (maybe_kpk.is_error())
-            return WebIDL::DataError::create(realm, "Key path is invalid"_string);
-
         // 3. If kpk is not failure, let key be kpk.
         if (!maybe_kpk.is_error()) {
             key_value = maybe_kpk.release_value();
+
+            // 2. If kpk is invalid, throw a "DataError" DOMException.
+            if (key_value->is_invalid())
+                return WebIDL::DataError::create(realm, "Key path is invalid"_string);
         }
 
         // 4. Otherwise (kpk is failure):
@@ -300,12 +302,9 @@ WebIDL::ExceptionOr<GC::Ref<IDBRequest>> IDBObjectStore::add_or_put(GC::Ref<IDBO
 
     // 12. Let operation be an algorithm to run store a record into an object store with store, clone, key, and no-overwrite flag.
     auto operation = GC::Function<WebIDL::ExceptionOr<JS::Value>()>::create(realm.heap(), [&realm, &store, clone, key_value, no_overwrite] -> WebIDL::ExceptionOr<JS::Value> {
-        auto maybe_key = store_a_record_into_an_object_store(realm, store, clone, key_value, no_overwrite);
+        auto optional_key = TRY(store_a_record_into_an_object_store(realm, store, clone, key_value, no_overwrite));
-        if (maybe_key.is_error())
-            return maybe_key.release_error();
 
-        auto optional_key = maybe_key.release_value();
+        if (!optional_key || optional_key->is_invalid())
-        if (optional_key == nullptr)
             return JS::js_undefined();
 
         return convert_a_key_to_a_value(realm, GC::Ref(*optional_key));

Libraries/LibWeb/IndexedDB/Internal/Algorithms.cpp

@@ -207,14 +207,14 @@ bool fire_a_version_change_event(JS::Realm& realm, FlyString const& event_name,
 }
 
 // https://w3c.github.io/IndexedDB/#convert-value-to-key
-WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_key(JS::Realm& realm, JS::Value input, Vector<JS::Value> seen)
+WebIDL::ExceptionOr<GC::Ref<Key>> convert_a_value_to_a_key(JS::Realm& realm, JS::Value input, Vector<JS::Value> seen)
 {
     // 1. If seen was not given, then let seen be a new empty set.
     // NOTE: This is handled by the caller.
 
     // 2. If seen contains input, then return invalid.
     if (seen.contains_slow(input))
-        return Error::from_string_literal("Already seen key");
+        return Key::create_invalid(realm, "Already seen key"_string);
 
     // 3. Jump to the appropriate step below:
 
@@ -223,7 +223,7 @@ WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_key(JS::Realm& r
 
         // 1. If input is NaN then return invalid.
         if (input.is_nan())
-            return Error::from_string_literal("NaN key");
+            return Key::create_invalid(realm, "NaN key"_string);
 
         // 2. Otherwise, return a new key with type number and value input.
         return Key::create_number(realm, input.as_double());
@@ -238,7 +238,7 @@ WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_key(JS::Realm& r
 
         // 2. If ms is NaN then return invalid.
         if (isnan(ms))
-            return Error::from_string_literal("NaN key");
+            return Key::create_invalid(realm, "NaN key"_string);
 
         // 3. Otherwise, return a new key with type date and value ms.
         return Key::create_date(realm, ms);
@@ -256,10 +256,10 @@ WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_key(JS::Realm& r
 
         // 1. If input is detached then return invalid.
         if (WebIDL::is_buffer_source_detached(input))
-            return Error::from_string_literal("Detached buffer is not supported as key");
+            return Key::create_invalid(realm, "Detached buffer is not supported as key"_string);
 
         // 2. Let bytes be the result of getting a copy of the bytes held by the buffer source input.
-        auto data_buffer = TRY(WebIDL::get_buffer_source_copy(input.as_object()));
+        auto data_buffer = MUST(WebIDL::get_buffer_source_copy(input.as_object()));
 
         // 3. Return a new key with type binary and value bytes.
         return Key::create_binary(realm, data_buffer);
@@ -287,15 +287,18 @@ WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_key(JS::Realm& r
 
             // 2. If hop is false, return invalid.
             if (!hop)
-                return Error::from_string_literal("Array-like object has no property");
+                return Key::create_invalid(realm, "Array-like object has no property"_string);
 
             // 3. Let entry be ? Get(input, index).
             auto entry = TRY(input.as_object().get(index));
 
             // 4. Let key be the result of converting a value to a key with arguments entry and seen.
             // 5. ReturnIfAbrupt(key).
+            auto key = TRY(convert_a_value_to_a_key(realm, entry, seen));
+
             // 6. If key is invalid abort these steps and return invalid.
-            auto key = TRY(TRY(convert_a_value_to_a_key(realm, entry, seen)));
+            if (key->is_invalid())
+                return key;
 
             // 7. Append key to keys.
             keys.append(key);
@@ -309,7 +312,8 @@ WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_key(JS::Realm& r
     }
 
     // - Otherwise
-    return Error::from_string_literal("Unknown key type");
+    // Return invalid.
+    return Key::create_invalid(realm, "Unable to convert value to key. Its not of a known type"_string);
 }
 
 // https://w3c.github.io/IndexedDB/#close-a-database-connection
@@ -682,6 +686,8 @@ JS::Value convert_a_key_to_a_value(JS::Realm& realm, GC::Ref<Key> key)
         // 6. Return array.
         return array;
     }
+    case Key::KeyType::Invalid:
+        VERIFY_NOT_REACHED();
     }
 
     VERIFY_NOT_REACHED();
@@ -812,7 +818,7 @@ WebIDL::ExceptionOr<JS::Value> clone_in_realm(JS::Realm& target_realm, JS::Value
 }
 
 // https://w3c.github.io/IndexedDB/#convert-a-value-to-a-multientry-key
-WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_multi_entry_key(JS::Realm& realm, JS::Value value)
+WebIDL::ExceptionOr<GC::Ref<Key>> convert_a_value_to_a_multi_entry_key(JS::Realm& realm, JS::Value value)
 {
     // 1. If input is an Array exotic object, then:
     if (value.is_object() && is<JS::Array>(value.as_object())) {
@@ -843,16 +849,12 @@ WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_multi_entry_key(
 
                 // 2. If key is not invalid or an abrupt completion, and there is no item in keys equal to key, then append key to keys.
                 if (!completion_key.is_error()) {
-                    auto maybe_key = completion_key.release_value();
+                    auto key = completion_key.release_value();
 
-                    if (!maybe_key.is_error()) {
+                    if (!key->is_invalid() && !keys.contains_slow(key))
-                        auto key = maybe_key.release_value();
-
-                        if (!keys.contains_slow(key))
                         keys.append(key);
                 }
             }
-            }
 
             // 3. Increase index by 1.
             index++;
@@ -1368,15 +1370,22 @@ WebIDL::ExceptionOr<GC::Ptr<Key>> store_a_record_into_an_object_store(JS::Realm&
     // 5. For each index which references store:
     for (auto const& [name, index] : store->index_set()) {
         // 1. Let index key be the result of extracting a key from a value using a key path with value, index’s key path, and index’s multiEntry flag.
-        auto index_key = TRY(extract_a_key_from_a_value_using_a_key_path(realm, value, index->key_path(), index->multi_entry()));
+        auto completion_index_key = extract_a_key_from_a_value_using_a_key_path(realm, value, index->key_path(), index->multi_entry());
 
         // 2. If index key is an exception, or invalid, or failure, take no further actions for index, and continue these steps for the next index.
-        if (index_key.is_error())
+        if (completion_index_key.is_error())
+            continue;
+
+        auto failure_index_key = completion_index_key.release_value();
+        if (failure_index_key.is_error())
+            continue;
+
+        auto index_key = failure_index_key.release_value();
+        if (index_key->is_invalid())
             continue;
 
-        auto index_key_value = index_key.value();
         auto index_multi_entry = index->multi_entry();
-        auto index_key_is_array = index_key_value->type() == Key::KeyType::Array;
+        auto index_key_is_array = index_key->type() == Key::KeyType::Array;
         auto index_is_unique = index->unique();
 
         // 3. If index’s multiEntry flag is false, or if index key is not an array key,
@@ -1384,7 +1393,7 @@ WebIDL::ExceptionOr<GC::Ptr<Key>> store_a_record_into_an_object_store(JS::Realm&
         //    and index’s unique flag is true,
         //    then this operation failed with a "ConstraintError" DOMException.
         //    Abort this algorithm without taking any further steps.
-        if ((!index_multi_entry || !index_key_is_array) && index_is_unique && index->has_record_with_key(index_key_value))
+        if ((!index_multi_entry || !index_key_is_array) && index_is_unique && index->has_record_with_key(index_key))
             return WebIDL::ConstraintError::create(realm, "Record already exists in index"_string);
 
         // 4. If index’s multiEntry flag is true and index key is an array key,
@@ -1393,7 +1402,7 @@ WebIDL::ExceptionOr<GC::Ptr<Key>> store_a_record_into_an_object_store(JS::Realm&
         //    then this operation failed with a "ConstraintError" DOMException.
         //    Abort this algorithm without taking any further steps.
         if (index_multi_entry && index_key_is_array && index_is_unique) {
-            for (auto const& subkey : index_key_value->subkeys()) {
+            for (auto const& subkey : index_key->subkeys()) {
                 if (index->has_record_with_key(*subkey))
                     return WebIDL::ConstraintError::create(realm, "Record already exists in index"_string);
             }

Libraries/LibWeb/IndexedDB/Internal/Algorithms.h

@@ -19,7 +19,7 @@ using KeyPath = Variant<String, Vector<String>>;
 
 WebIDL::ExceptionOr<GC::Ref<IDBDatabase>> open_a_database_connection(JS::Realm&, StorageAPI::StorageKey, String, Optional<u64>, GC::Ref<IDBRequest>);
 bool fire_a_version_change_event(JS::Realm&, FlyString const&, GC::Ref<DOM::EventTarget>, u64, Optional<u64>);
-WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_key(JS::Realm&, JS::Value, Vector<JS::Value> = {});
+WebIDL::ExceptionOr<GC::Ref<Key>> convert_a_value_to_a_key(JS::Realm&, JS::Value, Vector<JS::Value> = {});
 void close_a_database_connection(GC::Ref<IDBDatabase>, bool forced = false);
 GC::Ref<IDBTransaction> upgrade_a_database(JS::Realm&, GC::Ref<IDBDatabase>, u64, GC::Ref<IDBRequest>);
 WebIDL::ExceptionOr<u64> delete_a_database(JS::Realm&, StorageAPI::StorageKey, String, GC::Ref<IDBRequest>);
@@ -29,7 +29,7 @@ bool is_valid_key_path(KeyPath const&);
 GC::Ref<HTML::DOMStringList> create_a_sorted_name_list(JS::Realm&, Vector<String>);
 void commit_a_transaction(JS::Realm&, GC::Ref<IDBTransaction>);
 WebIDL::ExceptionOr<JS::Value> clone_in_realm(JS::Realm&, JS::Value, GC::Ref<IDBTransaction>);
-WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> convert_a_value_to_a_multi_entry_key(JS::Realm&, JS::Value);
+WebIDL::ExceptionOr<GC::Ref<Key>> convert_a_value_to_a_multi_entry_key(JS::Realm&, JS::Value);
 WebIDL::ExceptionOr<ErrorOr<JS::Value>> evaluate_key_path_on_a_value(JS::Realm&, JS::Value, KeyPath const&);
 WebIDL::ExceptionOr<ErrorOr<GC::Ref<Key>>> extract_a_key_from_a_value_using_a_key_path(JS::Realm&, JS::Value, KeyPath const&, bool = false);
 bool check_that_a_key_could_be_injected_into_a_value(JS::Realm&, JS::Value, KeyPath const&);

Libraries/LibWeb/IndexedDB/Internal/Key.cpp

@@ -73,6 +73,8 @@ i8 Key::compare_two_keys(GC::Ref<Key> a, GC::Ref<Key> b)
 
     // 6. Switch on ta:
     switch (ta) {
+    case KeyType::Invalid:
+        VERIFY_NOT_REACHED();
     // number
     // date
     case KeyType::Number:

Libraries/LibWeb/IndexedDB/Internal/Key.h

@@ -31,6 +31,7 @@ class Key : public JS::Cell {
 
     // A key has an associated type which is one of: number, date, string, binary, or array.
     enum KeyType {
+        Invalid,
         Number,
         Date,
         String,
@@ -45,6 +46,8 @@ public:
     [[nodiscard]] KeyType type() { return m_type; }
     [[nodiscard]] KeyValue value() { return m_value; }
 
+    [[nodiscard]] bool is_invalid() { return m_type == Invalid; }
+
     [[nodiscard]] double value_as_double() { return m_value.get<double>(); }
     [[nodiscard]] AK::String value_as_string() { return m_value.get<AK::String>(); }
     [[nodiscard]] ByteBuffer value_as_byte_buffer() { return m_value.get<ByteBuffer>(); }
@@ -60,6 +63,7 @@ public:
     [[nodiscard]] static GC::Ref<Key> create_string(JS::Realm& realm, AK::String const& value) { return create(realm, String, value); }
     [[nodiscard]] static GC::Ref<Key> create_binary(JS::Realm& realm, ByteBuffer const& value) { return create(realm, Binary, value); }
     [[nodiscard]] static GC::Ref<Key> create_array(JS::Realm& realm, Vector<GC::Root<Key>> const& value) { return create(realm, Array, value); }
+    [[nodiscard]] static GC::Ref<Key> create_invalid(JS::Realm& realm, AK::String const& value) { return create(realm, Invalid, value); }
 
     [[nodiscard]] static i8 compare_two_keys(GC::Ref<Key> a, GC::Ref<Key> b);
     [[nodiscard]] static bool equals(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) == 0; }