Skip to content

Seek target range #31

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

Closed
jesup opened this issue May 27, 2022 · 7 comments · Fixed by #84
Closed

Seek target range #31

jesup opened this issue May 27, 2022 · 7 comments · Fixed by #84

Comments

@jesup
Copy link
Contributor

jesup commented May 27, 2022

the webidl has
[[EnforceRange](https://webidl.spec.whatwg.org/#EnforceRange)] required [unsigned long long](https://webidl.spec.whatwg.org/#idl-unsigned-long-long) at;

However the WPT test of negative seek targets for SyncAccessHandles looks for dom NotSupportedError; the webidl spec says it should throw a TypeError. Likely this is simply a bug in the WPT test

@a-sully
Copy link
Collaborator

a-sully commented May 27, 2022

Yup, that's an error. Looks like the Chrome implementation was missing the [EnforceRange] in our idl file, which caused this to slip through the cracks. I have patch up to fix this and update the WPT

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 27, 2022
Passing an invalid read or write offset should throw a TypeError

This was pointed out in whatwg/fs#31

Bug: 1327741
Change-Id: Icd22cfefa49900a3f2e50c9baaeb766a2f1134fb
@a-sully
Copy link
Collaborator

a-sully commented May 28, 2022

After thinking about this a bit more, we might want to consider what the other end of the target range looks like, as well. Using an unsigned long long makes sense to guarantee the value is non-negative, but most operating systems do not support offsets of more than the max long long (off_t is signed, for example). And 32-bit OSes may not even support more than the max long.

Should we have language in the spec about what to do if the value is not valid for file operations on that OS? (InvalidModificationError? TypeError?)

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 31, 2022
Passing an invalid read or write offset should throw a TypeError

This was pointed out in whatwg/fs#31

Bug: 1327741
Change-Id: Icd22cfefa49900a3f2e50c9baaeb766a2f1134fb
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 2, 2022
Passing an invalid read or write offset should throw a TypeError

This was pointed out in whatwg/fs#31

Bug: 1327741
Change-Id: Icd22cfefa49900a3f2e50c9baaeb766a2f1134fb
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 3, 2022
Passing an invalid read or write offset should throw a TypeError

This was pointed out in whatwg/fs#31

Bug: 1327741
Change-Id: Icd22cfefa49900a3f2e50c9baaeb766a2f1134fb
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 3, 2022
Passing an invalid read or write offset should throw a TypeError

This was pointed out in whatwg/fs#31

Bug: 1327741
Change-Id: Icd22cfefa49900a3f2e50c9baaeb766a2f1134fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3671807
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Austin Sullivan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1010629}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 3, 2022
Passing an invalid read or write offset should throw a TypeError

This was pointed out in whatwg/fs#31

Bug: 1327741
Change-Id: Icd22cfefa49900a3f2e50c9baaeb766a2f1134fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3671807
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Austin Sullivan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1010629}
@a-sully
Copy link
Collaborator

a-sully commented Jun 3, 2022

The WPT has been updated to throw a TypeError if a negative value is passed, but I'll leave this issue open for the discussion of what we should do when an offset is passed which is too large for the underlying OS to handle

@annevk
Copy link
Member

annevk commented Jun 7, 2022

Using a TypeError as well seems reasonable.

@a-sully
Copy link
Collaborator

a-sully commented Jun 7, 2022

Thoughts on NotSupportedError, since whether the value is supported as an offset may depend on the OS? (sorry, I'm not sure whether this is consistent with how the error code is used elsewhere.) Otherwise TypeError seems reasonable to me :)

@annevk
Copy link
Member

annevk commented Jun 7, 2022

That could work as well, to some extent this depends on whether we expect web developers to branch on the exception type. If we don't, using the generic exception is preferred (that's TypeError to be clear).

I don't think we'd expect web developers to branch here. At least not based on this, but I could be mistaken. It seems to me that if the OS limit needs to be exposed there are better ways to do that, e.g., through a static getter.

@a-sully
Copy link
Collaborator

a-sully commented Jun 7, 2022

Good point! I'll update Chrome's implementation to make this a TypeError. We probably can't write a WPT asserting this (since some OSes/browsers could support values above the max signed long long) but we should add a note in the spec (#21).

aarongable pushed a commit to chromium/chromium that referenced this issue Jun 7, 2022
Follow-up to https://crrev.com/c/3671807. The idl enforces that an
offset is an unsigned long long, but Chrome's underlying file
implementation does not support anything more than a signed long long.
In this case, we now throw a TypeError.

See discussion on whatwg/fs#31

Also updates the error code in a case where we don't have enough quota to
write to return an out-of-quota error.

Bug: 1327741
Change-Id: I70ff8cea3810c3e61abdba7803f2ed117dd691d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3692211
Reviewed-by: Ayu Ishii <[email protected]>
Commit-Queue: Ayu Ishii <[email protected]>
Auto-Submit: Austin Sullivan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1011435}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 10, 2022
…Options.at, a=testonly

Automatic update from web-platform-tests
FSA: EnforceRange on FileSystemReadWriteOptions.at

Passing an invalid read or write offset should throw a TypeError

This was pointed out in whatwg/fs#31

Bug: 1327741
Change-Id: Icd22cfefa49900a3f2e50c9baaeb766a2f1134fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3671807
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Austin Sullivan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1010629}

--

wpt-commits: 1e43eadaa121e0577b8a78f97d608687ed160351
wpt-pr: 34240
a-sully added a commit to a-sully/fs that referenced this issue Jan 6, 2023
a-sully added a commit that referenced this issue Jan 11, 2023
#84)

* throw TypeError if passed an unsupported offset

Fixes #31

* properly [=throw=]

Co-authored-by: Anne van Kesteren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants