atob() clones the output more than necessary #15781

kmiller68 · 2023-07-12T16:53:54Z

`985e100`

atob() clones the output more than necessary
https://bugs.webkit.org/show_bug.cgi?id=258789
rdar://111993365

Reviewed by David Kilzer.

Right now several users of `base64Decode` take the `Vector` returned and immediately convert it to a `String`. This
conversion requires a copy as the `Vector` returned by `base64Decode` uses `VectorBufferMalloc` but `String` requires
that the Vector backing store is malloced with `StringImplMalloc`. This patch simply adds a `base64DecodeToString` that
calls `base64DecodeInternal` with a new template parameter telling `base64DecodeInternal` to use `StringImplMalloc`. Thus,
avoiding the extra string copy.

* Source/JavaScriptCore/jsc.cpp:
(JSC_DEFINE_HOST_FUNCTION):
* Source/WTF/wtf/text/Base64.cpp:
(WTF::base64DecodeInternal):
(WTF::base64DecodeToString):
* Source/WTF/wtf/text/Base64.h:
* Source/WTF/wtf/text/WTFString.h:
* Source/WebCore/page/Base64Utilities.cpp:
(WebCore::Base64Utilities::atob):
* Source/WebCore/page/Page.cpp:
(WebCore::Page::userStyleSheetLocationChanged):

Canonical link: https://commits.webkit.org/266016@main

bd645f8

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

webkit-early-warning-system · 2023-07-12T16:54:04Z

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

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

kmiller68 · 2023-07-12T16:59:17Z

Methinks this patch works but maybe I misunderstood how base64 works. AFAICT, it should only use the following characters ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/= so it's already Latin1?

mdubet · 2023-07-12T17:31:26Z

Source/WTF/wtf/text/Base64.cpp

@@ -267,4 +268,18 @@ std::optional<Vector<uint8_t>> base64Decode(StringView input, Base64DecodeMode m
    return base64DecodeInternal(std::span(input.characters16(), length), mode);
 }

+std::optional<String> base64DecodeToString(StringView input, Base64DecodeMode mode)\
+{
+    auto toString = [&] (auto optionalBuffer) -> std::optional<String> {


I think this PR really shows that we need to introduce a NonNullableString in the codebase at some point or at least a Markable<String>.

In the meantime, I guess we should use the isNull() function instead of using an std::optional<String>.

Ah, you're right. I was thinking for some reason that String::adopt would return the nullString if given an empty vector but that's not the case. Will change the API.

webkit-early-warning-system · 2023-07-12T19:00:03Z

EWS run on previous version of this PR (hash 65fba56)

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

webkit-early-warning-system · 2023-07-12T19:12:46Z

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

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

webkit-early-warning-system · 2023-07-12T19:46:48Z

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

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

ddkilzer · 2023-07-12T20:36:56Z

Methinks this patch works but maybe I misunderstood how base64 works. AFAICT, it should only use the following characters ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/= so it's already Latin1?

Base64 was created so that binary data could be sent as plain text (ASCII/Latin1).

https://en.wikipedia.org/wiki/Base64

ddkilzer

r=me

https://bugs.webkit.org/show_bug.cgi?id=258789 rdar://111993365 Reviewed by David Kilzer. Right now several users of `base64Decode` take the `Vector` returned and immediately convert it to a `String`. This conversion requires a copy as the `Vector` returned by `base64Decode` uses `VectorBufferMalloc` but `String` requires that the Vector backing store is malloced with `StringImplMalloc`. This patch simply adds a `base64DecodeToString` that calls `base64DecodeInternal` with a new template parameter telling `base64DecodeInternal` to use `StringImplMalloc`. Thus, avoiding the extra string copy. * Source/JavaScriptCore/jsc.cpp: (JSC_DEFINE_HOST_FUNCTION): * Source/WTF/wtf/text/Base64.cpp: (WTF::base64DecodeInternal): (WTF::base64DecodeToString): * Source/WTF/wtf/text/Base64.h: * Source/WTF/wtf/text/WTFString.h: * Source/WebCore/page/Base64Utilities.cpp: (WebCore::Base64Utilities::atob): * Source/WebCore/page/Page.cpp: (WebCore::Page::userStyleSheetLocationChanged): Canonical link: https://commits.webkit.org/266016@main

webkit-commit-queue · 2023-07-13T00:17:45Z

Committed 266016@main (985e100): https://commits.webkit.org/266016@main

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

kmiller68 requested review from cdumez and a team as code owners July 12, 2023 16:53

kmiller68 self-assigned this Jul 12, 2023

kmiller68 added the WebCore JavaScript Bugs in the JavaScript support that WebCore layers on top of JavaScriptCore. label Jul 12, 2023

cdumez requested review from weinig and darinadler July 12, 2023 17:03

mdubet reviewed Jul 12, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 12, 2023

kmiller68 removed the merging-blocked Applied to prevent a change from being merged label Jul 12, 2023

kmiller68 force-pushed the eng/atob-clones-the-output-more-than-necessary branch from e9e97e8 to 65fba56 Compare July 12, 2023 18:59

kmiller68 force-pushed the eng/atob-clones-the-output-more-than-necessary branch from 65fba56 to f64e896 Compare July 12, 2023 19:12

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 12, 2023

kmiller68 removed the merging-blocked Applied to prevent a change from being merged label Jul 12, 2023

kmiller68 force-pushed the eng/atob-clones-the-output-more-than-necessary branch from f64e896 to bd645f8 Compare July 12, 2023 19:46

ddkilzer approved these changes Jul 12, 2023

View reviewed changes

ddkilzer requested a review from mdubet July 12, 2023 20:41

kmiller68 added the merge-queue Applied to send a pull request to merge-queue label Jul 12, 2023

webkit-commit-queue force-pushed the eng/atob-clones-the-output-more-than-necessary branch from bd645f8 to 985e100 Compare July 13, 2023 00:17

webkit-commit-queue merged commit 985e100 into WebKit:main Jul 13, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

atob() clones the output more than necessary #15781

atob() clones the output more than necessary #15781

kmiller68 commented Jul 12, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jul 12, 2023 •

edited

Loading

kmiller68 commented Jul 12, 2023

mdubet Jul 12, 2023

kmiller68 Jul 12, 2023

webkit-early-warning-system commented Jul 12, 2023 •

edited

Loading

webkit-early-warning-system commented Jul 12, 2023 •

edited

Loading

webkit-early-warning-system commented Jul 12, 2023 •

edited

Loading

ddkilzer commented Jul 12, 2023

ddkilzer left a comment

webkit-commit-queue commented Jul 13, 2023

atob() clones the output more than necessary #15781

atob() clones the output more than necessary #15781

Conversation

kmiller68 commented Jul 12, 2023 • edited by webkit-early-warning-system Loading

985e100

webkit-early-warning-system commented Jul 12, 2023 • edited Loading

kmiller68 commented Jul 12, 2023

mdubet Jul 12, 2023

Choose a reason for hiding this comment

kmiller68 Jul 12, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented Jul 12, 2023 • edited Loading

webkit-early-warning-system commented Jul 12, 2023 • edited Loading

webkit-early-warning-system commented Jul 12, 2023 • edited Loading

ddkilzer commented Jul 12, 2023

ddkilzer left a comment

Choose a reason for hiding this comment

webkit-commit-queue commented Jul 13, 2023

kmiller68 commented Jul 12, 2023 •

edited by webkit-early-warning-system

Loading

`985e100`

webkit-early-warning-system commented Jul 12, 2023 •

edited

Loading

webkit-early-warning-system commented Jul 12, 2023 •

edited

Loading

webkit-early-warning-system commented Jul 12, 2023 •

edited

Loading

webkit-early-warning-system commented Jul 12, 2023 •

edited

Loading