Skip to content

Conversation

stelar7
Copy link
Contributor

@stelar7 stelar7 commented Oct 15, 2025

Since getAllKeys takes an any parameter, we cant be sure that the object we get is of the type IDBGetAllOptions. As such, there is no default values applied to the object passed in.

If the object passed then doesnt contain direction, we will try to call retrieve_multiple_items_from_an_XXX with an undefined value for direction. This in turn will result in 0 records being returned. I assume this is not the intention, and as such this PR aims to set the default value of "next" in those cases.

The same would apply to query and count, but those are explicitly handled to return an unbounded key, and infinite results in other places.

Ideally there would be some wording like "Set direction to queryOrOptions['direction'] if it exists.", but I'm not sure of the correct spec syntax in these cases

The following tasks have been completed:

  • Confirmed there are no ReSpec/BikeShed errors or warnings.
  • Modified Web platform tests (link to pull request)

Implementation commitment:


Preview | Diff

stelar7 added a commit to stelar7/ladybird that referenced this pull request Oct 17, 2025
IDBGetAllOptions is supposed to have a default value for direction.
When the value passed is not a potentially valid key range, we
need to default the direction argument, and not assume its set

Spec issue: w3c/IndexedDB#478
gmta pushed a commit to LadybirdBrowser/ladybird that referenced this pull request Oct 17, 2025
IDBGetAllOptions is supposed to have a default value for direction.
When the value passed is not a potentially valid key range, we
need to default the direction argument, and not assume its set

Spec issue: w3c/IndexedDB#478
@SteveBeckerMSFT
Copy link
Collaborator

Thanks for identifying this issue and proposing a fix. #475 also reported this issue. Maybe we should update "creating a request to retrieve multiple items" to explicitly handle an undefined queryOrOptions?

Prior to getAllRecords(), the steps for "convert a value to a key range " handled undefined values. Specifically, step 2, which returns an unbounded key range for an undefined value.

If value is undefined or is null, then throw a "DataError" DOMException if null disallowed flag is true, or return an unbounded key range otherwise.

We could update step 8 in "creating a request to retrieve multiple items" to say something like this:

If queryOrOptions is undefined or running is a potentially valid key range with queryOrOptions is true, then:

Thoughts?

@stelar7
Copy link
Contributor Author

stelar7 commented Oct 17, 2025

This issue is slightly different than the one in #475.
In this case, queryOrOptions is just missing the direction parameter, and as such is not a potentially valid key range.

{
    "query": "a",
    "count": 1
}

This takes us to step 9.3, where we encounter an undefined value, and its never set to its default

@SteveBeckerMSFT
Copy link
Collaborator

Thanks for clarifying. For the dictionary without a direction case, I think the default value in the IDBGetAllOptions dictionary is applied:

dictionary IDBGetAllOptions {
...
  [IDBCursorDirection](https://w3c.github.io/IndexedDB/#enumdef-idbcursordirection) direction = "next";
};

https://w3c.github.io/IndexedDB/#dictdef-idbgetalloptions

See also the WebIDL spec: https://webidl.spec.whatwg.org/#dfn-dictionary-member-default-value

@stelar7
Copy link
Contributor Author

stelar7 commented Oct 17, 2025

The IDBGetAllOptions dictionary has a default yes, but there is afaik nothing that makes the object we pass be of the type of that dictionary. (assuming the potentially valid key range check fails)

@SteveBeckerMSFT
Copy link
Collaborator

Any JavaScript object may become an IDBGetAllOptions dictionary when passed into a function like getAllRecords(). Dictionary types do not exist in JavaScript. This looks like the relevant section from the webidl spec:

https://webidl.spec.whatwg.org/#js-dictionary

You can also see this in action in Chromium's IDBGetAllOptions implementation, which is automatically generated by the V8 bindings code:

https://source.chromium.org/chromium/chromium/src/+/main:out/chromeos-Debug/gen/third_party/blink/renderer/bindings/modules/v8/v8_idb_get_all_options.cc;drc=240825a2c96b5bd0ae6d124919de323054ac6080;bpv=1;bpt=1;l=32?q=IDBGetAllOptions%20-file:out&ss=chromium%2Fchromium%2Fsrc

@stelar7
Copy link
Contributor Author

stelar7 commented Oct 17, 2025

Any JavaScript object may become an IDBGetAllOptions dictionary when passed into a function like getAllRecords().

Since the parameter type for getAllKeys is any, queryOrOptions doesnt know its supposed to be IDBGetAllOptions. (Atleast thats my understanding of any)

There are no steps that say i.e Convert 'queryOrOptions' to the 'IDBGetAllOptions' type.
What about adding a step at the start of step 9. Else: for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants