Skip to content
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.

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

Inconsistency between spec and tests for detached arraybuffers; spec for keys says don't throw, test for keys says throw, spec for values says throw #417

Open
asutherland opened this issue Mar 26, 2024 · 4 comments
Assignees
Labels

Comments

@asutherland
Copy link
Collaborator

While addressing a Firefox test failure in idb-binary-key-detached.htm in https://phabricator.services.mozilla.com/D202947 we became aware of a chromium issue https://issues.chromium.org/issues/40282817 which correctly points out that the IDB invocation of getting a copy of the bytes held by the buffer source in convert a value to a key means we shouldn't throw because webidl says to "return the empty byte sequence".

However, StructuredSerializeInternal step 13.2.1 says:

If IsDetachedBuffer(value) is true, then throw a "DataCloneError" DOMException.

Arguably the current test behavior (which Blink implements) is the most consistent and developer-friendly behavior. It would be weird for a detached arraybuffer to fail as a value but succeed with a potentially incorrect result as a key.

I would propose we:

  • Modify the IDB spec to explicitly return invalid if "IsDetachedBuffer".
    • Note this will throw a DataError rather than the TypeError expected by the test. I'm not opposed to retaining TypeError, but it would be good to have some rationale for choosing that given that we otherwise just seem to return DataError and structured serialization returns DataCloneError.
  • Update the existing test to expect DataError
  • Add a case for a key that is a JS array that contains a detached arraybuffer.
@inexorabletash
Copy link
Member

SGTM. @evanstade - any opinions?

Anyone up for a PR for the spec?

@evanstade
Copy link

sgtm

@inexorabletash
Copy link
Member

cc: @SteveBeckerMSFT

@SteveBeckerMSFT
Copy link

cc: @SteveBeckerMSFT

@inexorabletash , sorry, missed this until now. Yes, we can work on a spec PR for this inconsistency.

aarongable pushed a commit to chromium/chromium that referenced this issue Aug 7, 2024
Addressed an issue where detached ArrayBuffers were not handled
correctly during IndexedDB key conversion. According to the consensus
arrived in w3c/IndexedDB#417, detached buffers
should result in a DataError. This change ensures that a DataError is
thrown, aligning with the test expectation.

Bug: 40282817
Change-Id: If38cc2effe003f7f4340144a6704e20fae78c543
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5759605
Reviewed-by: Kentaro Hara <[email protected]>
Auto-Submit: Garima Chadha <[email protected]>
Reviewed-by: Rahul Singh <[email protected]>
Reviewed-by: Evan Stade <[email protected]>
Commit-Queue: Kentaro Hara <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1338786}
@SteveBeckerMSFT SteveBeckerMSFT self-assigned this Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants