forked from WebKit/WebKit
-
Notifications
You must be signed in to change notification settings - Fork 52
SamplingProfiler: stop the stack walk at a null entry frame instead of dereferencing vmEntryRecord(nullptr) #395
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.
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
Open
robobun
wants to merge
2
commits into
main
Choose a base branch
from
fix/sampling-profiler-null-entry-frame
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+62
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
52 changes: 52 additions & 0 deletions
52
Tools/TestWebKitAPI/Tests/JavaScriptCore/UnsafeCallerFrame.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /* | ||
| * Copyright (C) 2026 Apple Inc. All rights reserved. | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions | ||
| * are met: | ||
| * 1. Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * 2. Redistributions in binary form must reproduce the above copyright | ||
| * notice, this list of conditions and the following disclaimer in the | ||
| * documentation and/or other materials provided with the distribution. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' | ||
| * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, | ||
| * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR | ||
| * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS | ||
| * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR | ||
| * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF | ||
| * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS | ||
| * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN | ||
| * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) | ||
| * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF | ||
| * THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
|
|
||
| #include "config.h" | ||
| #include <JavaScriptCore/CallFrame.h> | ||
| #include <JavaScriptCore/InitializeThreading.h> | ||
|
|
||
| namespace TestWebKitAPI { | ||
|
|
||
| // The sampling profiler walks stacks through CallFrame::unsafeCallerFrame | ||
| // while its EntryFrame cursor can be null: vm.topEntryFrame is null in the | ||
| // windows around vmEntryToJavaScript where vm.entryScope is already set, and | ||
| // the walk can start from a stale vm.topCallFrame or a half-built entry | ||
| // frame. A walked frame whose caller slot reads null used to match the null | ||
| // cursor and dereference vmEntryRecord(nullptr), faulting just below address | ||
| // zero. Model that exact state with a zeroed frame and a null entry frame: | ||
| // unsafeCallerFrame must report the end of the stack instead of crashing. | ||
| TEST(JavaScriptCore_CallFrame, UnsafeCallerFrameWithNullEntryFrame) | ||
| { | ||
| JSC::initialize(); | ||
|
|
||
| alignas(JSC::Register) uint64_t zeroedFrame[16] = { }; | ||
| JSC::CallFrame* callFrame = JSC::CallFrame::create(reinterpret_cast<JSC::Register*>(zeroedFrame)); | ||
|
|
||
| JSC::EntryFrame* entryFrame = nullptr; | ||
| EXPECT_EQ(callFrame->unsafeCallerFrame(entryFrame), nullptr); | ||
|
Check warning on line 48 in Tools/TestWebKitAPI/Tests/JavaScriptCore/UnsafeCallerFrame.cpp
|
||
| EXPECT_EQ(entryFrame, nullptr); | ||
| } | ||
|
|
||
| } // namespace TestWebKitAPI | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
unsafeCallerFrameis declared inCallFrame.h:227withoutJS_EXPORT_PRIVATE(unlike the adjacentcallerFrameon line 228), so on ports that build JavaScriptCore as a shared library with hidden visibility (Win, GTK, WPE, Cocoa, PlayStation, JSCOnly withoutENABLE_STATIC_JSC) this test won't link. Bun's own CI usesENABLE_STATIC_JSC=ONand doesn't build TestWebKitAPI so nothing breaks here, but addingJS_EXPORT_PRIVATEto the declaration is a one-word fix that keeps the test portable and eases upstreaming.Extended reasoning...
What the bug is
The new test at
Tools/TestWebKitAPI/Tests/JavaScriptCore/UnsafeCallerFrame.cpp:48callscallFrame->unsafeCallerFrame(entryFrame). InSource/JavaScriptCore/interpreter/CallFrame.hthat method is declared as:and defined out-of-line in
CallFrame.cpp(not inline in the header). WithoutJS_EXPORT_PRIVATE, the symbol is not exported from a shared JavaScriptCore library.The code path that triggers it
The PR adds
Tests/JavaScriptCore/UnsafeCallerFrame.cppunconditionally toTestJavaScriptCore_SOURCESunderif (ENABLE_JAVASCRIPTCORE). Several ports buildTestJavaScriptCoreand setJavaScriptCore_LIBRARY_TYPE SHAREDwith hidden default visibility:OptionsGTK.cmake:491— SHARED, plusCXX_VISIBILITY_PRESET hiddenOptionsWin.cmake:193— SHARED (DLL export table)OptionsCocoa.cmake:409— SHARED,-fvisibility=hiddenOptionsPlayStation.cmake— SHARED, hiddenOptionsJSCOnly.cmake:124— SHARED whenENABLE_STATIC_JSCis OFF, with hidden visibility (:7)On any of those, linking
TestJavaScriptCorewill fail with an undefined reference / unresolved external forJSC::CallFrame::unsafeCallerFrame(JSC::EntryFrame*&) const.Why existing code doesn't prevent it
Until this PR,
unsafeCallerFrame's only caller wasSamplingProfiler.cpp, which is compiled into the JavaScriptCore library itself, so the symbol never needed to cross the library boundary. The new test is the first out-of-library caller. The PR was verified only on a JSCOnly Linux build withENABLE_STATIC_JSC=ON, where all symbols are visible regardless of the export macro, so the missing export was not observed.Step-by-step proof
-DENABLE_STATIC_JSC=OFF): CMake setsJavaScriptCore_LIBRARY_TYPE=SHAREDandCXX_VISIBILITY_PRESET hidden.CallFrame::unsafeCallerFrameis compiled intolibJavaScriptCore.sowith default (hidden) visibility because its declaration lacksJS_EXPORT_PRIVATE(which expands to__attribute__((visibility("default")))/__declspec(dllexport)).nm -D libJavaScriptCore.so | c++filt | grep unsafeCallerFrame→ no output; the symbol is local.TestJavaScriptCorecompilesUnsafeCallerFrame.cpp, which references_ZNK3JSC9CallFrame17unsafeCallerFrameERPNS_10EntryFrameE.ldfails:undefined reference to 'JSC::CallFrame::unsafeCallerFrame(JSC::EntryFrame*&) const'.Impact
None on this fork's CI:
.github/workflows/build.ymlbuilds withENABLE_STATIC_JSC=ONand does not build theTestWebKitAPItarget. But it will break theTestJavaScriptCorebuild on every shared-library port and would block upstreaming this fix to WebKit as-is.Fix
One word — add
JS_EXPORT_PRIVATEto the declaration inCallFrame.h:227, matching the adjacentcallerFrame: