Add source URL remapping to SamplingProfiler sourcemap callback#166
Add source URL remapping to SamplingProfiler sourcemap callback#166Jarred-Sumner merged 2 commits intomainfrom
Conversation
Extend the computeLineColumnWithSourcemap callback signature to accept an optional String* parameter for returning the remapped source URL. When a sourcemap resolves a bundled location to an original source file, the SamplingProfiler's stackTracesAsJSON() now emits a "sourceURL" field on each frame with the original file path. This is needed for compiled binaries (bun build --compile --sourcemap) where the SourceProvider URL is an internal path like /$bunfs/root/chunk-xyz.js. Co-Authored-By: Claude <noreply@anthropic.com>
…emapping Update the callback signature to match the WebKit-side change (oven-sh/WebKit#166) that adds an optional String* parameter for returning the remapped source URL through the sourcemap callback. Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds an extra output parameter for remapped source URLs to sourcemap lookup calls and the VM's computeLineColumnWithSourcemap function signature; SamplingProfiler now records a remapped "sourceURL" into frame JSON when provided. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/JavaScriptCore/runtime/SamplingProfiler.cpp`:
- Around line 1112-1116: The inliner path currently passes nullptr for the
out-URL parameter to the callback which is inconsistent with the semantic
location path that uses a local String; update the inliner path in
SamplingProfiler.cpp so it allocates a local String (e.g., remappedURL) and
passes its address to fn (same pattern as fn(m_vm, provider,
sourceMappedLineColumn, &remappedURL)), ensuring callbacks always receive a
valid String* even if its value is ignored.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Source/JavaScriptCore/runtime/SamplingProfiler.cppSource/JavaScriptCore/runtime/VM.h
| String remappedURL; | ||
| fn(m_vm, provider, sourceMappedLineColumn, &remappedURL); | ||
| if (!remappedURL.isEmpty()) | ||
| result->setString("sourceURL"_s, remappedURL); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure
fd SamplingProfiler.cppRepository: oven-sh/WebKit
Length of output: 108
🏁 Script executed:
# Get the context around lines 1112-1116 and 1138
wc -l Source/JavaScriptCore/runtime/SamplingProfiler.cppRepository: oven-sh/WebKit
Length of output: 113
🏁 Script executed:
# Look at the code context around the mentioned lines
sed -n '1100,1150p' Source/JavaScriptCore/runtime/SamplingProfiler.cpp | cat -nRepository: oven-sh/WebKit
Length of output: 2955
🏁 Script executed:
# Search for callback-related code patterns
rg -n "setComputeLineColumnWithSourcemap|computeLineColumnWithSourcemap" --max-count=30Repository: oven-sh/WebKit
Length of output: 1044
🏁 Script executed:
# Search for where nullptr is passed vs where a String pointer is passed
rg -n "fn\(m_vm|computeLineColumn" -B2 -A2Repository: oven-sh/WebKit
Length of output: 3370
🏁 Script executed:
# Look for the function type definition and callback setup
rg -n "ComputeLineColumnWithSourcemap|m_computeLineColumnWithSourcemap" --max-count=20Repository: oven-sh/WebKit
Length of output: 804
🏁 Script executed:
# Get more detailed context around lines 1110-1145
sed -n '1105,1145p' Source/JavaScriptCore/runtime/SamplingProfiler.cpp | cat -nRepository: oven-sh/WebKit
Length of output: 2612
🏁 Script executed:
# Search for all places where the callback is invoked to understand the nullptr vs String* pattern
rg -n 'fn\(' Source/JavaScriptCore/runtime/SamplingProfiler.cpp -B1 -A1Repository: oven-sh/WebKit
Length of output: 361
🏁 Script executed:
# Let's look at the SamplingProfiler header to understand the callback type
find . -name "SamplingProfiler.h" | head -1 | xargs cat | head -200Repository: oven-sh/WebKit
Length of output: 6971
🏁 Script executed:
# Search for the callback setup and its type definition more specifically
rg -n "m_computeLineColumnWithSourcemap|ComputeLineColumnWithSourcemapFunctor" Source/JavaScriptCore/runtime/ -A3Repository: oven-sh/WebKit
Length of output: 1491
🏁 Script executed:
# Let's search for where this callback is registered/set
rg -n "setComputeLineColumnWithSourcemap" Source/JavaScriptCore/ -B5 -A5Repository: oven-sh/WebKit
Length of output: 1292
🏁 Script executed:
# Check if there are any implementations that actually use the callback parameter
rg -n "remappedURL" Source/JavaScriptCore/runtime/SamplingProfiler.cpp -B2 -A2Repository: oven-sh/WebKit
Length of output: 423
Pass a local String instead of nullptr to the callback in the inliner path.
At line 1138, the inliner path passes nullptr for the out-URL parameter to the callback, while line 1113 in the semantic location path passes a valid String*. If any callback implementation assumes a valid pointer without null-checking, this inconsistency will cause a crash. Always pass a local String and ignore it when not needed to ensure consistent callback behavior.
✅ Safer local out-parameter (avoids nullptr)
if (fn) {
- fn(m_vm, provider, sourceMappedLineColumn, nullptr);
+ String ignoredRemappedURL;
+ fn(m_vm, provider, sourceMappedLineColumn, &ignoredRemappedURL);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/JavaScriptCore/runtime/SamplingProfiler.cpp` around lines 1112 - 1116,
The inliner path currently passes nullptr for the out-URL parameter to the
callback which is inconsistent with the semantic location path that uses a local
String; update the inliner path in SamplingProfiler.cpp so it allocates a local
String (e.g., remappedURL) and passes its address to fn (same pattern as
fn(m_vm, provider, sourceMappedLineColumn, &remappedURL)), ensuring callbacks
always receive a valid String* even if its value is ignored.
Preview Builds
|
Use a reference instead of a pointer for the remapped URL parameter. This is safer and more idiomatic C++ - callers always provide a valid String to write into, eliminating the need for null checks. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/JavaScriptCore/runtime/SamplingProfiler.cpp (1)
1133-1146:⚠️ Potential issue | 🟠 MajorInliner remapped URL computed but discarded; also uses the wrong
SourceProviderTwo issues in this block:
1.
inlinerRemappedURLis never used (lines 1138–1140):
The callback is invoked and its URL output captured, butinlinerRemappedURLis never checked or written into theinlinerJSON object. If the goal is to emit"sourceURL"on all frames (including inliner frames), the result needs to be applied—mirroring lines 1114–1115. If the inliner URL is intentionally ignored, the variable and thefn()call in this block can be removed entirely.2. Wrong
SourceProviderpassed tofn()(line 1138):
sourceMappedLineColumnat line 1133 comes frommachineLocation->first.lineColumn—this is the machine frame's source location. Butprovider(from the outerstackFrame.sourceProviderAndID()) is the inlined function's provider. For a correct sourcemap lookup of the machine location, the machine frame's provider should be used.🔧 Proposed fix — emit sourceURL for inliner and use the correct provider
LineColumn sourceMappedLineColumn = machineLocation->first.lineColumn; - if (provider) { + auto* machineProvider = machineLocation->second->ownerExecutable()->source().provider(); + if (machineProvider) { `#if` USE(BUN_JSC_ADDITIONS) auto& fn = m_vm.computeLineColumnWithSourcemap(); if (fn) { String inlinerRemappedURL; - fn(m_vm, provider, sourceMappedLineColumn, inlinerRemappedURL); + fn(m_vm, machineProvider, sourceMappedLineColumn, inlinerRemappedURL); + if (!inlinerRemappedURL.isEmpty()) + inliner->setString("sourceURL"_s, inlinerRemappedURL); } `#endif` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/JavaScriptCore/runtime/SamplingProfiler.cpp` around lines 1133 - 1146, The inliner block currently calls fn(m_vm, provider, sourceMappedLineColumn, inlinerRemappedURL) but uses the wrong provider and then discards inlinerRemappedURL; fix by invoking m_vm.computeLineColumnWithSourcemap() with the machine frame's SourceProvider (the provider associated with machineLocation) instead of the inlined-frame provider, and if fn yields a non-empty inlinerRemappedURL, write it into the inliner JSON (use inliner->setString("sourceURL"_s, inlinerRemappedURL) analogous to the earlier frame handling); alternatively remove the call and variable if you intentionally do not emit an inliner sourceURL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/JavaScriptCore/runtime/VM.h`:
- Around line 1142-1143: The data member m_computeLineColumnWithSourcemap has
the wrong function-type instantiation (uses String*) causing type mismatch with
the accessors computeLineColumnWithSourcemap() which use String&; change the
member's type to WTF::Function<void(VM&, SourceProvider*, LineColumn&, String&)>
so it matches the getter/setter signatures and call sites (e.g.,
SamplingProfiler.cpp passing a String lvalue), and update any other
declarations/uses of m_computeLineColumnWithSourcemap to the String& variant.
---
Outside diff comments:
In `@Source/JavaScriptCore/runtime/SamplingProfiler.cpp`:
- Around line 1133-1146: The inliner block currently calls fn(m_vm, provider,
sourceMappedLineColumn, inlinerRemappedURL) but uses the wrong provider and then
discards inlinerRemappedURL; fix by invoking
m_vm.computeLineColumnWithSourcemap() with the machine frame's SourceProvider
(the provider associated with machineLocation) instead of the inlined-frame
provider, and if fn yields a non-empty inlinerRemappedURL, write it into the
inliner JSON (use inliner->setString("sourceURL"_s, inlinerRemappedURL)
analogous to the earlier frame handling); alternatively remove the call and
variable if you intentionally do not emit an inliner sourceURL.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Source/JavaScriptCore/runtime/SamplingProfiler.cppSource/JavaScriptCore/runtime/VM.h
| const WTF::Function<void(VM&, SourceProvider*, LineColumn&, String&)>& computeLineColumnWithSourcemap() const { return m_computeLineColumnWithSourcemap; } | ||
| WTF::Function<void(VM&, SourceProvider*, LineColumn&, String&)>& computeLineColumnWithSourcemap() { return m_computeLineColumnWithSourcemap; } |
There was a problem hiding this comment.
String* / String& type mismatch between data member and accessors — compile error
The accessors (lines 1142–1143) declare their return type as WTF::Function<void(VM&, SourceProvider*, LineColumn&, String&)>&, but the underlying member (line 1367) is typed as WTF::Function<void(VM&, SourceProvider*, LineColumn&, String*)>.
WTF::Function<..., String&> and WTF::Function<..., String*> are distinct template instantiations. Returning a reference to the member from a function that declares a different function-type reference, and move-assigning a String&-typed callable into a String*-typed member in the setter (line 1148), will both fail to compile. The call site in SamplingProfiler.cpp (line 1113) also passes a plain String lvalue, which is only valid for String&, not String*.
The data member declaration at line 1367 was not updated from String* to String& to match the rest of the API.
🔧 Fix the member declaration
- WTF::Function<void(VM&, SourceProvider*, LineColumn&, String*)> m_computeLineColumnWithSourcemap;
+ WTF::Function<void(VM&, SourceProvider*, LineColumn&, String&)> m_computeLineColumnWithSourcemap;Also applies to: 1367-1367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/JavaScriptCore/runtime/VM.h` around lines 1142 - 1143, The data member
m_computeLineColumnWithSourcemap has the wrong function-type instantiation (uses
String*) causing type mismatch with the accessors
computeLineColumnWithSourcemap() which use String&; change the member's type to
WTF::Function<void(VM&, SourceProvider*, LineColumn&, String&)> so it matches
the getter/setter signatures and call sites (e.g., SamplingProfiler.cpp passing
a String lvalue), and update any other declarations/uses of
m_computeLineColumnWithSourcemap to the String& variant.
2 participants
Summary
computeLineColumnWithSourcemapVM callback to accept an optionalString*parameter for returning the remapped source URLstackTracesAsJSON()method builds Inspector protocol output, it now emits a"sourceURL"field on each frame when sourcemap remapping resolves to a different filebun build --compile --sourcemap) show internal/$bunfs/root/chunk-xyz.jspaths instead of original source file paths in profiler outputTest plan
FormatStackTraceForJS.cppto match the new callback signaturetest/cli/run/cpu-prof.test.tsverifies sourcemapped paths appear in CPU profiles for compiled binaries🤖 Generated with Claude Code