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.

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

Make sure ImageData objects are shared when doing structured cloning #14605

Merged
merged 1 commit into from Jun 2, 2023
Merged

Make sure ImageData objects are shared when doing structured cloning #14605

merged 1 commit into from Jun 2, 2023

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Jun 1, 2023 •
edited by webkit-early-warning-system

dd1484f

Make sure ImageData objects are shared when doing structured cloning
https://bugs.webkit.org/show_bug.cgi?id=257626

Reviewed by Darin Adler.

Make sure ImageData objects are shared when doing structured cloning so that
if you send the same ImageData object is present multiple times in the
object being serialized, the object identity is preserved after deserializing.

* LayoutTests/imported/w3c/web-platform-tests/webmessaging/without-ports/028-expected.txt:
* Source/WebCore/bindings/js/SerializedScriptValue.cpp:
(WebCore::CloneSerializer::dumpIfTerminal):
(WebCore::CloneSerializer::writeImageDataIndex):
(WebCore::CloneDeserializer::readImageDataIndex):
(WebCore::CloneDeserializer::readTerminal):

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

7bf6bc1

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
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@cdumez cdumez self-assigned this Jun 1, 2023
@cdumez cdumez added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Jun 1, 2023
@webkit-early-warning-system
Copy link
Collaborator

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

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
✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2
⏳ 🛠 tv-sim
⏳ 🛠 watch
✅ 🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

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

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

@webkit-early-warning-system
Copy link
Collaborator

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

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
⏳ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@cdumez cdumez marked this pull request as ready for review June 2, 2023 01:11
darinadler
darinadler approved these changes Jun 2, 2023
View reviewed changes
Source/WebCore/bindings/js/SerializedScriptValue.cpp Outdated
@@ -437,6 +437,7 @@ static const unsigned CurrentVersion = 13;
static const unsigned TerminatorTag = 0xFFFFFFFF;
static const unsigned StringPoolTag = 0xFFFFFFFE;
static const unsigned NonIndexPropertiesTag = 0xFFFFFFFD;
static const uint32_t ImageDataPoolTag = 0xFFFFFFFE;
Copy link
Member

Choose a reason for hiding this comment

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

In new code we want to use constexpr.

Source/WebCore/bindings/js/SerializedScriptValue.cpp Outdated
@@ -2178,6 +2190,8 @@ class CloneSerializer : CloneBase {
#endif
typedef HashMap<RefPtr<UniquedStringImpl>, uint32_t, IdentifierRepHash> StringConstantPool;
StringConstantPool m_constantPool;
using ImageDataPool = HashMap<RefPtr<ImageData>, uint32_t>;
Copy link
Member

Choose a reason for hiding this comment

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

Ref?

Source/WebCore/bindings/js/SerializedScriptValue.cpp Outdated
@@ -4072,6 +4091,18 @@ class CloneDeserializer : CloneBase {
uint32_t width;
if (!read(width))
return JSValue();
if (width == ImageDataPoolTag) {
unsigned index = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this would be better using std::optional instead of an out argument.

@webkit-early-warning-system
Copy link
Collaborator

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

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

@webkit-early-warning-system
Copy link
Collaborator

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

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
  🧪 api-ios   🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 7bf6bc1)

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
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Jun 2, 2023
@cdumez
Make sure ImageData objects are shared when doing structured cloning
dd1484f 
https://bugs.webkit.org/show_bug.cgi?id=257626

Reviewed by Darin Adler.

Make sure ImageData objects are shared when doing structured cloning so that
if you send the same ImageData object is present multiple times in the
object being serialized, the object identity is preserved after deserializing.

* LayoutTests/imported/w3c/web-platform-tests/webmessaging/without-ports/028-expected.txt:
* Source/WebCore/bindings/js/SerializedScriptValue.cpp:
(WebCore::CloneSerializer::dumpIfTerminal):
(WebCore::CloneSerializer::writeImageDataIndex):
(WebCore::CloneDeserializer::readImageDataIndex):
(WebCore::CloneDeserializer::readTerminal):

Canonical link: https://commits.webkit.org/264824@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264824@main (dd1484f): https://commits.webkit.org/264824@main

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

@webkit-commit-queue webkit-commit-queue merged commit dd1484f into WebKit:main Jun 2, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darinadler darinadler darinadler approved these changes

@achristensen07 achristensen07 Awaiting requested review from achristensen07

@youennf youennf Awaiting requested review from youennf

@Constellation Constellation Awaiting requested review from Constellation

Assignees

@cdumez cdumez

Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
Milestone
No milestone
4 participants
@cdumez @webkit-early-warning-system @webkit-commit-queue @darinadler