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

[UNIX] Web process crash in websites using service workers while doing garbage collection #6360

Merged
merged 1 commit into from Nov 11, 2022
Merged

[UNIX] Web process crash in websites using service workers while doing garbage collection #6360

merged 1 commit into from Nov 11, 2022

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Nov 10, 2022 •
edited by webkit-early-warning-system

292a165

[UNIX] Web process crash in websites using service workers while doing garbage collection
https://bugs.webkit.org/show_bug.cgi?id=247727
rdar://102209090

Reviewed by Mark Lam.

MarkedBlock::Handle::start() is not returning page aligned address, so this is not appropriate for mincore.

* Source/JavaScriptCore/heap/BlockDirectory.cpp:
(JSC::BlockDirectory::updatePercentageOfPagedOutPages):

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

3720a87

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

@Constellation Constellation requested a review from a team as a code owner November 10, 2022 21:30
@Constellation Constellation self-assigned this Nov 10, 2022
@Constellation Constellation added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Nov 10, 2022
@webkit-early-warning-system
Copy link
Collaborator

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

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

MenloDorian
MenloDorian approved these changes Nov 10, 2022
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.

Thanks for fixing this.

r=me with suggestions for improvements.

Source/JavaScriptCore/heap/BlockDirectory.cpp Outdated

for (auto* handle : m_blocks) {
if (!handle)
continue;

auto markedBlockSizeInBytes = static_cast<size_t>(reinterpret_cast<char*>(handle->end()) - reinterpret_cast<char*>(handle->start()));
char* pageStart = reinterpret_cast<char*>(handle->atomAt(0));
Copy link

Choose a reason for hiding this comment

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

Instead of using atomAt(0), can you add a MarkedBlock::Handle::pageStart() method that returns the value of reinterpret_cast<char*>(atomAt(0))? I think that would be clearer and would have avoided this fall out when we changed the position of the header/footer in the MarkedBlock.

Have you also grepped the code for uses of handle->start() to make sure that there're no other instances of this bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

MenloDorian
MenloDorian reviewed Nov 10, 2022
View reviewed changes
Source/JavaScriptCore/heap/BlockDirectory.cpp Outdated

for (auto* handle : m_blocks) {
if (!handle)
continue;

auto markedBlockSizeInBytes = static_cast<size_t>(reinterpret_cast<char*>(handle->end()) - reinterpret_cast<char*>(handle->start()));
char* pageStart = reinterpret_cast<char*>(handle->atomAt(0));
auto markedBlockSizeInBytes = static_cast<size_t>(reinterpret_cast<char*>(handle->end()) - pageStart);
Copy link

Choose a reason for hiding this comment

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

Let's also introduce and use a MarkedBlock::Handle::pageSize() for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually MarkedBlock can be multiple pageSize, or so, so I think we should just call it backingStorageSize.

@Constellation Constellation force-pushed the eng/UNIX-Web-process-crash-in-websites-using-service-workers-while-doing-garbage-collection branch from c212bd0 to 3720a87 Compare November 11, 2022 00:06
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 3720a87)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios   🛠 mac   🛠 wpe   🛠 🧪 win
✅ 🛠 ios-sim   🛠 mac-debug ✅ 🛠 gtk   🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2   🛠 mac-AS-debug   🧪 gtk-wk2
  🧪 api-ios   🧪 api-mac   🧪 api-gtk
✅ 🛠 🧪 jsc ✅ 🛠 tv   🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim   🧪 mac-wk2   🧪 jsc-armv7-tests
✅ 🛠 watch   🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim   🧪 mac-wk2-stress ✅ 🧪 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 Nov 11, 2022
@Constellation
[UNIX] Web process crash in websites using service workers while doin…
292a165 
…g garbage collection

https://bugs.webkit.org/show_bug.cgi?id=247727
rdar://102209090

Reviewed by Mark Lam.

MarkedBlock::Handle::start() is not returning page aligned address, so this is not appropriate for mincore.

* Source/JavaScriptCore/heap/BlockDirectory.cpp:
(JSC::BlockDirectory::updatePercentageOfPagedOutPages):

Canonical link: https://commits.webkit.org/256554@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/UNIX-Web-process-crash-in-websites-using-service-workers-while-doing-garbage-collection branch from 3720a87 to 292a165 Compare November 11, 2022 00:28
@webkit-early-warning-system webkit-early-warning-system merged commit 292a165 into WebKit:main Nov 11, 2022
@webkit-commit-queue
Copy link
Collaborator

Committed 256554@main (292a165): https://commits.webkit.org/256554@main

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

@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 Nov 11, 2022
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