Skip to content

[JSC] add support for Uint8Array.fromBase64 and Uint8Array.prototype.setFromBase64 #30821

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

Jump to bottom
Merged
merged 1 commit into from
Jul 16, 2024
Merged

[JSC] add support for Uint8Array.fromBase64 and Uint8Array.prototype.setFromBase64 #30821

merged 1 commit into from
Jul 16, 2024
+1,665 −53

Conversation

dcrousso
Copy link
Member

@dcrousso dcrousso commented Jul 15, 2024 •
edited by webkit-early-warning-system
Loading

ca33dbd

[JSC] add support for `Uint8Array.fromBase64` and `Uint8Array.prototype.setFromBase64`
https://bugs.webkit.org/show_bug.cgi?id=276215

Reviewed by Yusuke Suzuki.

These methods will make it easier for developers to decode typed character/byte data.

Note that there are other methods in the related proposal, but for ease of reviewing they will be implemented separately.

Spec: <https://tc39.es/proposal-arraybuffer-base64/spec/>
Proposal: <https://github.com/tc39/proposal-arraybuffer-base64>

* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.h:
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:
(JSC::JSGenericTypedArrayViewConstructor<ViewClass>::finishCreation):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.cpp:
(JSC::uint8ArrayConstructorFromBase64): Added.

* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.h:
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeInlines.h:
(JSC::JSGenericTypedArrayViewPrototype<ViewClass>::finishCreation):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.cpp:
(JSC::uint8ArrayPrototypeSetFromBase64): Added.

* Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:
* Source/JavaScriptCore/runtime/JSGenericTypedArrayView.cpp: Added.
(JSC::fromBase64):
Create shared utility file for special base64 parsing logic.

* Source/JavaScriptCore/runtime/CommonIdentifiers.h:
Create builtin identifier for `lastChunkHandling`.

* Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:
* Source/JavaScriptCore/Sources.txt:

* JSTests/stress/uint8array-fromBase64.js: Added.
* JSTests/stress/uint8array-setFromBase64.js: Added.

* JSTests/stress/uint8array-fromHex.js:
* JSTests/stress/uint8array-setFromHex.js:
* JSTests/stress/uint8array-toBase64.js:
* JSTests/stress/uint8array-toHex.js:

Drive-by: fix a few invalid tests and check that things that should throw actually do.
Canonical link: https://commits.webkit.org/281014@main

aba268e

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ❌ 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@dcrousso dcrousso requested a review from a team as a code owner July 15, 2024 15:31
@dcrousso dcrousso self-assigned this Jul 15, 2024
@dcrousso dcrousso added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Jul 15, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 15, 2024 •
edited
Loading

EWS run on previous version of this PR (hash cb3ed88)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ❌ 🛠 wpe ❌ 🛠 wincairo
❌ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ❌ 🧪 wincairo-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ❌ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
❌ 🛠 🧪 jsc ❌ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🛠 gtk
❌ 🛠 🧪 jsc-arm64 ❌ 🛠 vision   🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
  🛠 vision-sim ❌ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
⏳ 🧪 vision-wk2 ❌ 🛠 jsc-armv7
❌ 🛠 tv ❌ 🧪 jsc-armv7-tests
❌ 🛠 tv-sim
❌ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 15, 2024
@dcrousso dcrousso removed the merging-blocked Applied to prevent a change from being merged label Jul 15, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 15, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 754f1bb)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv ✅ 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

Constellation
Constellation approved these changes Jul 15, 2024
View reviewed changes
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

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

r=me

Source/JavaScriptCore/runtime/JSGenericTypedArrayView.cpp
size_t read = 0;
size_t length = static_cast<size_t>(string.length());

Vector<uint8_t> bytes;
Copy link
Member

Choose a reason for hiding this comment

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

It is worth having a inlineCapacity.

Copy link
Member Author

Choose a reason for hiding this comment

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

what size would you recommend?

base64DecodeInternal seems to use a Vector with a minCapacity of 16, but that seems to be entirely different from inlineCapacity

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 16, 2024 •
edited
Loading

EWS run on current version of this PR (hash aba268e)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ❌ 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jul 16, 2024
@dcrousso @Constellation
[JSC] add support for Uint8Array.fromBase64 and `Uint8Array.prototy…
ca33dbd 
…pe.setFromBase64`

https://bugs.webkit.org/show_bug.cgi?id=276215

Reviewed by Yusuke Suzuki.

These methods will make it easier for developers to decode typed character/byte data.

Note that there are other methods in the related proposal, but for ease of reviewing they will be implemented separately.

Spec: <https://tc39.es/proposal-arraybuffer-base64/spec/>
Proposal: <https://github.com/tc39/proposal-arraybuffer-base64>

* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.h:
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:
(JSC::JSGenericTypedArrayViewConstructor<ViewClass>::finishCreation):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.cpp:
(JSC::uint8ArrayConstructorFromBase64): Added.

* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.h:
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeInlines.h:
(JSC::JSGenericTypedArrayViewPrototype<ViewClass>::finishCreation):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.cpp:
(JSC::uint8ArrayPrototypeSetFromBase64): Added.

* Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:
* Source/JavaScriptCore/runtime/JSGenericTypedArrayView.cpp: Added.
(JSC::fromBase64):
Create shared utility file for special base64 parsing logic.

* Source/JavaScriptCore/runtime/CommonIdentifiers.h:
Create builtin identifier for `lastChunkHandling`.

* Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:
* Source/JavaScriptCore/Sources.txt:

* JSTests/stress/uint8array-fromBase64.js: Added.
* JSTests/stress/uint8array-setFromBase64.js: Added.

* JSTests/stress/uint8array-fromHex.js:
* JSTests/stress/uint8array-setFromHex.js:
* JSTests/stress/uint8array-toBase64.js:
* JSTests/stress/uint8array-toHex.js:

Drive-by: fix a few invalid tests and check that things that should throw actually do.
Canonical link: https://commits.webkit.org/281014@main
@webkit-commit-queue
Copy link
Collaborator

Committed 281014@main (ca33dbd): https://commits.webkit.org/281014@main

Reviewed commits have been landed. Closing PR #30821 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit ca33dbd into WebKit:main Jul 16, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jul 16, 2024
@dcrousso dcrousso deleted the eng/276215 branch July 16, 2024 19:31
@bakkot bakkot mentioned this pull request Aug 1, 2024
Open
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Constellation Constellation Constellation approved these changes

Assignees

@dcrousso dcrousso

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dcrousso @webkit-early-warning-system @webkit-commit-queue @Constellation @webkit-ews-buildbot