[JSC] The check for an untouched StringObject is for the realm of the conversion

[dylan-conway]

Compiled code can go on converting a StringObject to its original string after valueOf, toString or Symbol.toPrimitive has been replaced on its String.prototype, when the object comes from another realm.

let other = $vm.createGlobalObject();
let make = other.Function("return new String('abc');");   // the other realm's
function f() { return make() + "!"; }                     // this realm's; make() is inlined

for (let i = 0; i < 100000; ++i)
    f();

other.String.prototype.valueOf = other.Function("return 'replaced';");
f();   // LLInt, Baseline: "replaced!"   DFG, FTL: "abc!"

Cause

The DFG and the FTL turn a StringObject into its string without a call (ToString with StringObjectUse or StringOrStringObjectUse) on two conditions:

• a CheckStructure for a realm's stringObjectStructure(), which says the object has no toString, valueOf or Symbol.toPrimitive of its own and still has String.prototype;
• watchpoints on what String.prototype has for those three, which Graph::canOptimizeStringObjectAccess() sets up.

The two have to be about one realm. FixupPhase::addCheckStructureForOriginalStringObjectUse() took the realm from the origin of the node that produces the object, and its callers ask canOptimizeStringObjectAccess() about the origin of the conversion. They differ when the object comes from an inlined function of another realm: the check accepted that realm's objects while nobody watched its String.prototype.

Fix

The structure comes from the origin of the conversion (node->origin.semantic becomes origin.semantic). An object of another realm fails the check and is converted by the call. All nine callers ask canOptimizeStringObjectAccess() about node->origin.semantic and pass node->origin, so the two now agree everywhere.

The helper's parameter is renamed from node to child: in every caller node is the conversion.

The other places in the compilers that take a realm from a child's origin (the spread of an array and of a Set: Graph::canDoFastSpread(), the abstract interpreter, and the two lowerings) take their watchpoints from the child as well, and say so, so they do not have this.

Cost

For an object of the site's own realm nothing changes: the DFG's graph and machine code for object + suffix are the same as on main, line for line (499 lines), and the FTL's differ by less than two runs of main differ from each other.

An object of another realm loses a fast path it should not have had. A site that sees one is reoptimized once and then stays as it is (no further compile in a million calls). Bun built with this engine, two builds that differ by this change alone, the other realm a node:vm context, median ns per call over 7 processes:

	same realm: before	after	other realm: before	after
make() + s	5.91	5.89	24.17	50.64
`${make()}${s}`	3.10	3.09	22.97	47.95
String(make())	0.33	0.33	18.86	46.46

The fast path could be kept for such an object by watching the child's realm, as the spread does. That is more code, and is not done here.

Test

JSTests/stress/string-object-from-inlined-function-of-another-realm.js: seven conversions (+ on either side, + with three operands, a template literal, String(), concat(), a computed class member), two ways of producing the object in the other realm's code (an allocation, a load from an object), and three replacements (valueOf, toString, Symbol.toPrimitive), each replacement in a realm of its own, made while every site is hot.

	main, LLInt and Baseline	main, DFG and FTL	this branch
the 28 cases in which the conversion asks for what was replaced	28 right	28 wrong	28 right
the 14 in which it does not	14 right	14 right	14 right

The conversion is made from five places. With the realm taken from the operand again at one of them at a time:

put back at	caught by
attemptToMakeFastStringAdd()	add, addLeft, concat
fixupToStringOrCallStringConstructor()	template, stringCall
fixupToPrimitive()	addThree
ToPropertyKey	propertyKey
attemptToForceStringArrayModeByToStringConversion(), for length	nothing: no method is asked for, so nothing can be observed

It also checks that an object of the site's own realm is still converted without a reoptimization, and that a change to this realm's String.prototype still reaches it.

Through run-jsc-stress-tests: 25 runs of its 17 modes on a release build and 25 on a build with assertions, no failure. It takes 44 ms in the default mode and 144 ms in the slowest measured. All of JSTests/stress (5,931 files) ends as it does on main, apart from this test, one test the suite skips that fails now and then on main as well, and two tests that need more memory than the run allowed.

#765 has this change as one of its commits, because what it adds takes its realm from the same place.

[github-actions]

Preview build of a49dfb6: autobuild-preview-pr-766-a49dfb69

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Essentials
• Run ID: b09d8264-80c8-4f58-a7b2-919895be91c1

📥 Commits

Reviewing files that changed from the base of the PR and between 52bda5e and df443ff.

📒 Files selected for processing (2)

• JSTests/stress/string-object-from-inlined-function-of-another-realm.js
• Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

Included review availability: This review used your included allowance. Your plan provides up to 5 included reviews per hour; 0 remain after this review.

---

Walkthrough

The DFG fixup now selects the StringObject structure from the conversion’s semantic origin. A new stress test checks String-object conversions when methods on another realm’s String.prototype change, and includes a same-realm case.

Changes

String Object Conversion

Layer / File(s)	Summary
Select the conversion realm Source/JavaScriptCore/dfg/DFGFixupPhase.cpp	The fixup uses the conversion’s semantic origin to select the realm’s StringObject structure.
Exercise conversion across realms JSTests/stress/string-object-from-inlined-function-of-another-realm.js	The test covers five conversion expressions and two object producers. It checks results before and after replacing conversion methods on another realm’s String.prototype, and includes a same-realm case.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to df443

The change targets cross-realm String-object conversion and adds stress coverage for the described cases. No merge-blocking issue is identified in the supplied context.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the change: the StringObject structure check now uses the conversion’s realm.
Description check	✅ Passed	The description explains the bug, cause, fix, performance impact, and test results. It does not include the Bugzilla bug title or link, reviewer line, or a complete changed-file list required by the t…

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Code review found no issues

No high-confidence issues detected in this change.