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

[JSC] Fix Object.assign's temporary structure transition #12626

Merged
merged 1 commit into from Apr 11, 2023
Merged

[JSC] Fix Object.assign's temporary structure transition #12626

merged 1 commit into from Apr 11, 2023

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Apr 11, 2023 •
edited by webkit-early-warning-system

b7ecdfa

[JSC] Fix Object.assign's temporary structure transition
https://bugs.webkit.org/show_bug.cgi?id=255304
rdar://107903084

Reviewed by Mark Lam.

Fixes Object.assign's temporary structure transition. We should store oldStructure's StructureID, but we were storing oldStructure->structure()'s ID.
Interestingly this does not affect on release build because,

1. After setting this and before setStructure, we have no GC invocation operations.
2. Inline property offset is static. It is not depending on Structure. So, every store just works as expected.

So, it is just assertion hit ultimately. But it is not correct. This patch fixes it.
Also, we need to visit BrandedStructures' finalizer too. This patch also fixes it.

* Source/JavaScriptCore/heap/Heap.cpp:
(JSC::Heap::finalizeUnconditionalFinalizers):
* Source/JavaScriptCore/runtime/JSObject.cpp:
(JSC::JSObject::putOwnDataPropertyBatching):

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

52d3a9d

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 wincairo
✅ 🛠 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 ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@Constellation Constellation requested a review from a team as a code owner April 11, 2023 21:37
@Constellation Constellation self-assigned this Apr 11, 2023
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Apr 11, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 4c82b93)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ⏳ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 wincairo
✅ 🛠 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   🧪 mac-wk2-stress ✅ 🧪 jsc-armv7-tests
⏳ 🛠 watch ✅ 🛠 jsc-mips
⏳ 🛠 watch-sim   🧪 jsc-mips-tests

MenloDorian
MenloDorian approved these changes Apr 11, 2023
View reviewed changes
Copy link

@MenloDorian MenloDorian left a comment

Choose a reason for hiding this comment

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

typo in commit msg: /oldStructure->strucutre/oldStructure->structure/

r=me

@Constellation Constellation force-pushed the eng/JSC-Fix-Object-assigns-temporary-structure-transition branch from 4c82b93 to 52d3a9d Compare April 11, 2023 21:46
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 52d3a9d)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 wincairo
✅ 🛠 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 ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Apr 11, 2023
@Constellation
[JSC] Fix Object.assign's temporary structure transition
b7ecdfa 
https://bugs.webkit.org/show_bug.cgi?id=255304
rdar://107903084

Reviewed by Mark Lam.

Fixes Object.assign's temporary structure transition. We should store oldStructure's StructureID, but we were storing oldStructure->structure()'s ID.
Interestingly this does not affect on release build because,

1. After setting this and before setStructure, we have no GC invocation operations.
2. Inline property offset is static. It is not depending on Structure. So, every store just works as expected.

So, it is just assertion hit ultimately. But it is not correct. This patch fixes it.
Also, we need to visit BrandedStructures' finalizer too. This patch also fixes it.

* Source/JavaScriptCore/heap/Heap.cpp:
(JSC::Heap::finalizeUnconditionalFinalizers):
* Source/JavaScriptCore/runtime/JSObject.cpp:
(JSC::JSObject::putOwnDataPropertyBatching):

Canonical link: https://commits.webkit.org/262843@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-Fix-Object-assigns-temporary-structure-transition branch from 52d3a9d to b7ecdfa Compare April 11, 2023 22:26
@webkit-commit-queue
Copy link
Collaborator

Committed 262843@main (b7ecdfa): https://commits.webkit.org/262843@main

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

@webkit-commit-queue webkit-commit-queue merged commit b7ecdfa into WebKit:main Apr 11, 2023
@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 Apr 11, 2023
@Constellation Constellation deleted the eng/JSC-Fix-Object-assigns-temporary-structure-transition branch April 11, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MenloDorian MenloDorian MenloDorian approved these changes

Assignees

@Constellation Constellation

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
4 participants
@Constellation @webkit-early-warning-system @webkit-commit-queue @MenloDorian