`JSOp::ArgumentsLength` specialization returns wrong value for block-hoisted `function arguments(){}`
Categories
(Core :: JavaScript Engine, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox152 | --- | fixed |
People
(Reporter: parkjuny, Assigned: arai)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Summary
SpiderMonkey specializes arguments.length into a dedicated bytecode op (JSOp::ArgumentsLength) that reads numActualArgs() directly from the stack frame, bypassing the variable-environment binding for arguments. The specialization is gated on a parser flag (isEligibleForArgumentsLength()) that is intended to hold only when arguments will resolve to the Arguments object at runtime. A sloppy-mode block-scoped function arguments(){} overwrites that binding with the function value via Annex B.3.2, but the parser does not clear the eligibility flag for this case, so the specialization fires anyway.
For the input
function f() { { function arguments() {} } return arguments.length; }
- Per spec:
argumentsresolves to the function object after the block executes;arguments.lengthis the function's declared-parameter count, i.e.0. - V8/Node returns
0. - SpiderMonkey returns
numActualArgs()(e.g.3forf(1,2,3)).
I reopened this with correct type - please ignore 2032775
Bug
Summary
SpiderMonkey's parser decides whether to emit JSOp::ArgumentsLength based on FunctionBox::isEligibleForArgumentsLength(). The flag is cleared whenever Parser.cpp observes a usage pattern that makes the arguments binding observable in ways the intrinsic cannot satisfy (Parser.cpp:2502, 10433, 10495, 10705, 11138, 11690, 12322, …).
Annex B.3.2's SetMutableBinding("arguments", fObj, false) is another such pattern — it overwrites the binding the intrinsic is "optimizing" with an unrelated function value. But it is not in the parser's disqualification list. The bytecode emitter correctly emits the Annex-B sync (GetLocal <block>; SetLocal <var>) that populates the var-scope binding for "arguments" with the function value, and then immediately emits JSOp::ArgumentsLength, which ignores that binding and reads numActualArgs() from the frame. The result disagrees with the spec.
Detail
The Annex-B var binding (ParseContext.cpp:272-288).
When the parser finishes a scope and propagates pending Annex-B function boxes up to the enclosing var scope, it creates a var binding for the function name with DeclarationKind::VarForAnnexBLexicalFunction:
// js/src/frontend/ParseContext.cpp:272-288
for (FunctionBox* funbox : *possibleAnnexBFunctionBoxes_) {
bool annexBApplies;
if (!pc->computeAnnexBAppliesToLexicalFunctionInInnermostScope(
funbox, parser, &annexBApplies)) {
return false;
}
if (annexBApplies) {
if (!pc->tryDeclareVar(funbox->explicitName(), parser,
DeclarationKind::VarForAnnexBLexicalFunction,
DeclaredNameInfo::npos, &redeclaredKind,
&unused)) {
return false;
}
MOZ_ASSERT(!redeclaredKind);
funbox->isAnnexB = true;
}
}
For the input function f(){ { function arguments(){} } ... }, this produces a var binding for "arguments" of kind VarForAnnexBLexicalFunction in f's var scope. The block's lexical binding for arguments is initialized with the function value when the block is entered, and the engine emits a sync that copies that value into the enclosing var-scope binding when the function declaration is evaluated (implementing the Annex-B replacement Evaluation's SetMutableBinding). This sync is visible in the disassembly as GetLocal 1; SetLocal 0.
The intrinsic-eligibility decision (ParseContext.cpp:678-701).
For non-lazy compilation, declareFunctionArgumentsObject decides whether to use the intrinsic based purely on per-FunctionBox flags:
// js/src/frontend/ParseContext.cpp:678-701
} else {
bool bindingClosedOver =
hasClosedOverFunctionSpecialName(usedNames, argumentsName);
bool bindingUsedOnlyHere =
hasUsedFunctionSpecialName(usedNames, argumentsName) &&
!bindingClosedOver;
tryDeclareArguments =
!funbox->isEligibleForArgumentsLength() || bindingClosedOver;
if (bindingUsedOnlyHere && funbox->isEligibleForArgumentsLength()) {
MOZ_ASSERT(!tryDeclareArguments);
funbox->setUsesArgumentsIntrinsics();
} else if (tryDeclareArguments) {
needsArgsObject = true;
}
}
isEligibleForArgumentsLength() remains true because none of the disqualifying patterns in Parser.cpp cover Annex-B-hoisted function arguments(){}. setUsesArgumentsIntrinsics() is therefore called and needsArgsObject stays false.
The "is there an existing var binding for arguments?" probe (ParseContext.cpp:710-730).
Immediately after, declareFunctionArgumentsObject looks for a pre-existing var-scope declaration of arguments:
// js/src/frontend/ParseContext.cpp:710-730
DeclaredNamePtr p = _varScope.lookupDeclaredName(argumentsName);
if (p && p->value()->kind() == DeclarationKind::Var) {
if (hasExtraBodyVarScope) {
tryDeclareArguments = true;
} else {
if (needsArgsObject) {
funbox->setNeedsArgsObj();
}
return true;
}
}
This probe matches DeclarationKind::Var only. The Annex-B binding installed at ParseContext.cpp:272-288 is DeclarationKind::VarForAnnexBLexicalFunction, which the probe does not recognise. The intrinsic stays armed.
Specialized lowering — JSOp::ArgumentsLength (BytecodeEmitter.cpp:12822-12846).
Once the function box is UsesArgumentsIntrinsics and not needsArgsObj, the emitter substitutes the intrinsic for the property access. Otherwise it falls through to a regular NameOpEmitter("arguments") + GetProp "length" — the ordinary property-access lowering every other arguments.length reference uses:
// js/src/frontend/BytecodeEmitter.cpp:12822-12846
case ParseNodeKind::ArgumentsLength: {
if (sc->isFunctionBox() &&
sc->asFunctionBox()->isEligibleForArgumentsLength() &&
!sc->asFunctionBox()->needsArgsObj()) {
if (!emit1(JSOp::ArgumentsLength)) {
return false;
}
} else {
PropOpEmitter poe(this, PropOpEmitter::Kind::Get,
PropOpEmitter::ObjKind::Other);
if (!poe.prepareForObj()) {
return false;
}
NameOpEmitter noe(this, TaggedParserAtomIndex::WellKnown::arguments(),
NameOpEmitter::Kind::Get);
if (!noe.emitGet()) {
return false;
}
if (!poe.emitGet(TaggedParserAtomIndex::WellKnown::length())) {
return false;
}
}
break;
}
At runtime, the intrinsic reads numActualArgs() from the frame and ignores the env binding entirely:
// js/src/vm/Interpreter.cpp:3708-3712
CASE(ArgumentsLength) {
MOZ_ASSERT(!script->needsArgsObj());
PUSH_INT32(REGS.fp()->numActualArgs());
}
END_CASE(ArgumentsLength)
ECMA-262 reference
Under Annex B.3.2, FunctionDeclarationInstantiation installs a replacement Evaluation for eligible block-scoped function declarations; when evaluated, that replacement performs fEnv.SetMutableBinding(F, fObj, false) where fEnv is the running execution context's VariableEnvironment. The F is not "arguments" guard applies only to the sibling CreateMutableBinding substep, not to this one. For function arguments(){} in a sloppy-mode function, the VariableEnvironment already holds the Arguments-object binding for "arguments"; the replacement Evaluation overwrites it with the function value, and arguments.length reads the function's declared-parameter count (0).
Trigger Conditions
- The enclosing function is in sloppy mode.
- A block-scoped
function arguments() {}declaration appears inside the function body in a position where Annex B.3.2 applies (so the parser inserts theVarForAnnexBLexicalFunctionbinding and the bytecode emitter emits theSetMutableBindingsync). arguments.lengthis the only access toargumentsin the function (so the parser does not callsetIneligibleForArgumentsLength()and the specialized lowering is taken).- The function is called with at least one argument (so
numActualArgs() > 0produces a value distinguishable from the spec-mandated0).
Adding any other use of arguments (e.g. var a = arguments;) disqualifies the intrinsic and the ordinary property-access lowering is selected — the pivot the PoC uses to exhibit the divergence.
Version
Reproduced Version
mainbranch tip at the time of testing (2026-04-17):5e465cbe324a291bb69aa85c5c4231e052cabe9a.- Reproduces in both
obj-x64.release/dist/bin/jsandobj-x64.debug/dist/bin/js. No assertions fire in the debug build — the conformance violation is silent at every layer.
Bisect
The intrinsic and the surrounding eligibility logic were added by:
commit ded1e584ad3de4a16d38fb9a5fecd7c8ef146880
Author: Matthew Gaudet <mgaudet@mozilla.com>
Date: 2024-03-04 16:25:47 +0000
Bug 1825722 - Where possible avoid allocating arguments, and use
JSOp::ArgumentsLength instead r=arai
Differential Revision: https://phabricator.services.mozilla.com/D203144
This commit introduced both JSOp::ArgumentsLength and the if (p && p->value()->kind() == DeclarationKind::Var) probe in declareFunctionArgumentsObject. The Annex-B hoisting code that produces DeclarationKind::VarForAnnexBLexicalFunction predates this commit; the conformance regression is therefore attributable to Bug 1825722 having omitted the Annex-B kind from the eligibility logic and having failed to add the Annex-B pattern to the disqualification list.
Reproduction Case
The PoC exhibits the spec violation and controls for it by toggling the intrinsic's eligibility via a neighbouring use of arguments:
viaIntrinsic()— onlyarguments.lengthis read, soisEligibleForArgumentsLength()staystrueand the bytecode emitter takes the specializedJSOp::ArgumentsLengthpath. ReturnsnumActualArgs().viaBinding()— an extra bareargumentsreference disqualifies the intrinsic (Parser.cpp:2502callssetIneligibleForArgumentsLength), forcing the ordinaryNameOpEmitter+GetProp "length"path. Returns0.
Per spec, both functions must return 0. V8 returns 0 in both. SpiderMonkey returns numActualArgs() from viaIntrinsic and 0 from viaBinding.
Release Build
obj-x64.release/dist/bin/js poc.js
Result:
n=0 intrinsic=0 binding=0 (spec: 0)
n=1 intrinsic=1 binding=0 (spec: 0) MISMATCH
n=2 intrinsic=2 binding=0 (spec: 0) MISMATCH
n=3 intrinsic=3 binding=0 (spec: 0) MISMATCH
n=4 intrinsic=4 binding=0 (spec: 0) MISMATCH
n=5 intrinsic=5 binding=0 (spec: 0) MISMATCH
Debug Build
obj-x64.debug/dist/bin/js poc.js
Result:
n=0 intrinsic=0 binding=0 (spec: 0)
n=1 intrinsic=1 binding=0 (spec: 0) MISMATCH
n=2 intrinsic=2 binding=0 (spec: 0) MISMATCH
n=3 intrinsic=3 binding=0 (spec: 0) MISMATCH
n=4 intrinsic=4 binding=0 (spec: 0) MISMATCH
n=5 intrinsic=5 binding=0 (spec: 0) MISMATCH
No internal SpiderMonkey assertions fire in the debug build — the conformance violation is silent at every layer.
V8 (Chrome)
n=0 intrinsic=0 binding=0 (spec: 0)
n=1 intrinsic=0 binding=0 (spec: 0)
n=2 intrinsic=0 binding=0 (spec: 0)
n=3 intrinsic=0 binding=0 (spec: 0)
n=4 intrinsic=0 binding=0 (spec: 0)
n=5 intrinsic=0 binding=0 (spec: 0)
V8 returns 0 for every arity, matching the spec. Both viaIntrinsic and viaBinding agree.
PoC Code
function viaIntrinsic() {
{ function arguments() {} }
return arguments.length;
}
function viaBinding() {
{ function arguments() {} }
var a = arguments;
return a.length;
}
for (let n = 0; n <= 5; n++) {
const args = Array(n).fill(0);
const i = viaIntrinsic.apply(null, args);
const b = viaBinding.apply(null, args);
console.log(`n=${n} intrinsic=${i} binding=${b} (spec: 0)${i !== b ? " MISMATCH" : ""}`);
}
Suggested Patch
Disable the JSOp::ArgumentsLength specialization for any function box whose arguments binding will be overwritten by an Annex-B function-declaration hoist. The most surgical place to do this is Scope::propagateAndMarkAnnexBFunctionBoxes, which is the single point at which the engine knows a function arguments(){} Annex-B hoist applies; it precedes declareFunctionArgumentsObject for the same scope. Clearing isEligibleForArgumentsLength() there forces the bytecode emitter onto the regular binding-lookup path that the engine already uses (and gets right, per V8/Node cross-check) for every other arguments-using function.
--- a/js/src/frontend/ParseContext.cpp
+++ b/js/src/frontend/ParseContext.cpp
@@ -270,18 +270,28 @@ bool ParseContext::Scope::propagateAndMarkAnnexBFunctionBoxes(
if (this == &pc->varScope()) {
// Base case: actually declare the Annex B vars and mark applicable
// function boxes as Annex B.
Maybe<DeclarationKind> redeclaredKind;
uint32_t unused;
for (FunctionBox* funbox : *possibleAnnexBFunctionBoxes_) {
bool annexBApplies;
if (!pc->computeAnnexBAppliesToLexicalFunctionInInnermostScope(
funbox, parser, &annexBApplies)) {
return false;
}
if (annexBApplies) {
if (!pc->tryDeclareVar(funbox->explicitName(), parser,
DeclarationKind::VarForAnnexBLexicalFunction,
DeclaredNameInfo::npos, &redeclaredKind,
&unused)) {
return false;
}
MOZ_ASSERT(!redeclaredKind);
funbox->isAnnexB = true;
+
+ // Annex B.3.2 step (b) overwrites the var-env "arguments" binding
+ // with the function value; disable the numActualArgs()-based
+ // specialization in that case.
+ if (funbox->explicitName() ==
+ TaggedParserAtomIndex::WellKnown::arguments()) {
+ pc->sc()->setIneligibleForArgumentsLength();
+ }
}
}
} else {
After the patch, declareFunctionArgumentsObject sees isEligibleForArgumentsLength() return false, falls through to tryDeclareArguments = true, and BytecodeEmitter.cpp:12822-12846 emits a NameOpEmitter("arguments") lookup followed by GetProp "length" — the exact lowering that produces the correct 0 in viaBinding() today, which both matches the spec and matches V8/Node.
Why not extend the DeclarationKind::Var check at ParseContext.cpp:713-714 to also match VarForAnnexBLexicalFunction? That else branch only calls setNeedsArgsObj() when needsArgsObject was already true, but in the divergent case needsArgsObject was set to false earlier because setUsesArgumentsIntrinsics() short-circuited the assignment. Simply teaching that probe about VarForAnnexBLexicalFunction is therefore not sufficient to disable the specialization; the eligibility flag must be cleared explicitly, which is what setIneligibleForArgumentsLength() does.
Credit Information
Reporter credit: Junyoung Park (@candymate) of KAIST Hacking Lab.
|
||
Comment 1•4 months ago
|
||
Hello Arai,
Would you be able to take a look at this comprehensive report?
|
Assignee | |
Comment 2•4 months ago
|
||
Thank you for reporting.
Actually this is not really specific to Annex B.
This happens with the following code as well.
function f() {
function arguments() {}
console.log(arguments.length);
}
f(1, 2, 3);
but not with the following:
function f() {
var arguments = [];
console.log(arguments.length);
}
f(1, 2, 3);
The difference comes from the following code:
GeneralParser<ParseHandler, Unit>::functionBody(InHandling inHandling,
YieldHandling yieldHandling,
FunctionSyntaxKind kind,
FunctionBodyType type) {
...
if (pc_->numberOfArgumentsNames > 0 || kind == FunctionSyntaxKind::Arrow) {
MOZ_ASSERT(pc_->isFunctionBox());
pc_->sc()->setIneligibleForArgumentsLength();
The numberOfArgumentsNames counts the number of arguments name appeared inside the function body.
Here, the var declaration's declared name calls newName, but the function declaration's name doesn't call it, which results in this bug.
PerHandlerParser<ParseHandler>::newName(TaggedParserAtomIndex name,
TokenPos pos) {
if (name == TaggedParserAtomIndex::WellKnown::arguments()) {
this->pc_->numberOfArgumentsNames++;
So, we should perform the equivalent for the function name as well.
|
|
Assignee | |
Updated•4 months ago
|
|
|
Assignee | |
Comment 3•4 months ago
|
||
|
||
Comment 5•4 months ago
|
||
| bugherder | ||
|
||
Updated•3 months ago
|
Description
•