From 5674f8bbe00a55e2450ccddde29cbde1073ac32d Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 26 Jan 2026 20:40:05 +0100 Subject: [PATCH] LibJS: Limit eval() deoptimization to the containing function scope Previously, when direct eval() was called, we would mark the entire environment chain as "permanently screwed by eval", disabling variable access caching all the way up to the global scope. This was overly conservative. According to the ECMAScript specification, a sloppy direct eval() can only inject var declarations into its containing function's variable environment - it cannot inject variables into parent function scopes. This patch makes two changes: 1. Stop propagating the "screwed by eval" flag at function boundaries. When set_permanently_screwed_by_eval() hits a FunctionEnvironment or GlobalEnvironment, it no longer continues to outer environments. 2. Check each environment during cache lookup traversal. If any environment in the path is marked as screwed, we bail to the slow path. This catches the case where we're inside a function with eval and have a cached coordinate pointing to an outer scope. The second change is necessary because eval can create local variables that shadow outer bindings. When looking up a variable from inside a function that called eval, we can't trust cached coordinates that point to outer scopes, since eval may have created a closer binding. This improves performance for code with nested functions where an inner function uses eval but parent functions perform many variable accesses. The parent functions can now use cached environment coordinates. All 29 new tests verify behavior matches V8. --- Libraries/LibJS/Bytecode/Interpreter.cpp | 25 +- Libraries/LibJS/Runtime/Environment.cpp | 7 + Libraries/LibJS/Runtime/Environment.h | 6 +- Tests/LibJS/Runtime/scope-local-eval-deopt.js | 384 ++++++++++++++++++ 4 files changed, 416 insertions(+), 6 deletions(-) create mode 100644 Tests/LibJS/Runtime/scope-local-eval-deopt.js diff --git a/Libraries/LibJS/Bytecode/Interpreter.cpp b/Libraries/LibJS/Bytecode/Interpreter.cpp index 6fd0a57c079..dacf419b7ed 100644 --- a/Libraries/LibJS/Bytecode/Interpreter.cpp +++ b/Libraries/LibJS/Bytecode/Interpreter.cpp @@ -1204,8 +1204,11 @@ inline ThrowCompletionOr get_callee_and_this_from_environment(Int if (cache.is_valid()) [[likely]] { auto const* environment = interpreter.running_execution_context().lexical_environment.ptr(); - for (size_t i = 0; i < cache.hops; ++i) + for (size_t i = 0; i < cache.hops; ++i) { + if (environment->is_permanently_screwed_by_eval()) [[unlikely]] + goto slow_path; environment = environment->outer_environment(); + } if (!environment->is_permanently_screwed_by_eval()) [[likely]] { callee = TRY(static_cast(*environment).get_binding_value_direct(vm, cache.index)); auto this_value = js_undefined(); @@ -1216,6 +1219,7 @@ inline ThrowCompletionOr get_callee_and_this_from_environment(Int .this_value = this_value, }; } + slow_path: cache = {}; } @@ -2083,8 +2087,11 @@ static ThrowCompletionOr get_binding(Interpreter& interpreter, Operand dst if (cache.is_valid()) [[likely]] { auto const* environment = interpreter.running_execution_context().lexical_environment.ptr(); - for (size_t i = 0; i < cache.hops; ++i) + for (size_t i = 0; i < cache.hops; ++i) { + if (environment->is_permanently_screwed_by_eval()) [[unlikely]] + goto slow_path; environment = environment->outer_environment(); + } if (!environment->is_permanently_screwed_by_eval()) [[likely]] { Value value; if constexpr (binding_is_known_to_be_initialized == BindingIsKnownToBeInitialized::No) { @@ -2095,6 +2102,7 @@ static ThrowCompletionOr get_binding(Interpreter& interpreter, Operand dst interpreter.set(dst, value); return {}; } + slow_path: cache = {}; } @@ -2102,6 +2110,7 @@ static ThrowCompletionOr get_binding(Interpreter& interpreter, Operand dst auto reference = TRY(vm.resolve_binding(executable.get_identifier(identifier), strict)); if (reference.environment_coordinate().has_value()) cache = reference.environment_coordinate().value(); + interpreter.set(dst, TRY(reference.get_value(vm))); return {}; } @@ -2345,8 +2354,11 @@ static ThrowCompletionOr initialize_or_set_binding(Interpreter& interprete : interpreter.running_execution_context().variable_environment.ptr(); if (cache.is_valid()) [[likely]] { - for (size_t i = 0; i < cache.hops; ++i) + for (size_t i = 0; i < cache.hops; ++i) { + if (environment->is_permanently_screwed_by_eval()) [[unlikely]] + goto slow_path; environment = environment->outer_environment(); + } if (!environment->is_permanently_screwed_by_eval()) [[likely]] { if constexpr (initialization_mode == BindingInitializationMode::Initialize) { TRY(static_cast(*environment).initialize_binding_direct(vm, cache.index, value, Environment::InitializeBindingHint::Normal)); @@ -2355,6 +2367,7 @@ static ThrowCompletionOr initialize_or_set_binding(Interpreter& interprete } return {}; } + slow_path: cache = {}; } @@ -3179,13 +3192,17 @@ ThrowCompletionOr TypeofBinding::execute_impl(Bytecode::Interpreter& inter if (m_cache.is_valid()) [[likely]] { auto const* environment = interpreter.running_execution_context().lexical_environment.ptr(); - for (size_t i = 0; i < m_cache.hops; ++i) + for (size_t i = 0; i < m_cache.hops; ++i) { + if (environment->is_permanently_screwed_by_eval()) [[unlikely]] + goto slow_path; environment = environment->outer_environment(); + } if (!environment->is_permanently_screwed_by_eval()) [[likely]] { auto value = TRY(static_cast(*environment).get_binding_value_direct(vm, m_cache.index)); interpreter.set(dst(), value.typeof_(vm)); return {}; } + slow_path: m_cache = {}; } diff --git a/Libraries/LibJS/Runtime/Environment.cpp b/Libraries/LibJS/Runtime/Environment.cpp index b598815028a..45b1ee9b7b6 100644 --- a/Libraries/LibJS/Runtime/Environment.cpp +++ b/Libraries/LibJS/Runtime/Environment.cpp @@ -26,6 +26,13 @@ void Environment::set_permanently_screwed_by_eval() if (m_permanently_screwed_by_eval) return; m_permanently_screwed_by_eval = true; + + // Stop propagation at function or global boundaries. + // Eval can only inject vars into its containing function's variable environment, + // not into parent function scopes. + if (is_function_environment() || is_global_environment()) + return; + if (outer_environment()) outer_environment()->set_permanently_screwed_by_eval(); } diff --git a/Libraries/LibJS/Runtime/Environment.h b/Libraries/LibJS/Runtime/Environment.h index a267aeee871..1081503a1a1 100644 --- a/Libraries/LibJS/Runtime/Environment.h +++ b/Libraries/LibJS/Runtime/Environment.h @@ -54,8 +54,10 @@ public: template bool fast_is() const = delete; - // This flag is set on the entire variable environment chain when direct eval() is performed. - // It is used to disable non-local variable access caching. + // This flag is set on environments within a function when direct eval() is performed in that function. + // It propagates up to the function boundary (not beyond) and is used to disable variable access caching. + // Code in parent functions is not affected because eval can only inject vars into its containing + // function's variable environment, not into parent function scopes. bool is_permanently_screwed_by_eval() const { return m_permanently_screwed_by_eval; } void set_permanently_screwed_by_eval(); diff --git a/Tests/LibJS/Runtime/scope-local-eval-deopt.js b/Tests/LibJS/Runtime/scope-local-eval-deopt.js new file mode 100644 index 00000000000..27552903232 --- /dev/null +++ b/Tests/LibJS/Runtime/scope-local-eval-deopt.js @@ -0,0 +1,384 @@ +/* + * Copyright (c) 2026, Andreas Kling + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +describe("scope-local eval deoptimization", () => { + test("eval in child function does not affect parent lookups", () => { + function outer() { + let x = 100; + function inner() { + eval("var y = 200"); + return y; + } + let innerResult = inner(); + return x + innerResult; + } + expect(outer()).toBe(300); + }); + + test("eval shadows variable from outer function within same function", () => { + function outer() { + var c = 1; + function inner() { + let before = c; + eval("var c = 2"); + let after = c; + return before * 10 + after; + } + return inner(); + } + expect(outer()).toBe(12); + }); + + test("conditional eval - false", () => { + function foo(doEval) { + var c = 1; + function bar(doEval) { + if (doEval) eval("var c = 2;"); + return c; + } + return bar(doEval); + } + expect(foo(false)).toBe(1); + }); + + test("conditional eval - true", () => { + function foo(doEval) { + var c = 1; + function bar(doEval) { + if (doEval) eval("var c = 2;"); + return c; + } + return bar(doEval); + } + expect(foo(true)).toBe(2); + }); + + test("block scope within function containing eval", () => { + function foo() { + let outerLet = 10; + let result = 0; + { + let blockLet = 20; + eval("var evalVar = 30"); + result = outerLet + blockLet + evalVar; + } + return result + outerLet; + } + expect(foo()).toBe(70); + }); + + test("multiple levels of nesting", () => { + function a() { + let aVar = 1; + function b() { + let bVar = 2; + function c() { + let cVar = 3; + eval("var evalVar = 4"); + return cVar + evalVar; + } + return bVar + c(); + } + return aVar + b(); + } + expect(a()).toBe(10); + }); + + test("eval in multiple nested functions at different levels", () => { + function a() { + let aVar = 1; + eval("var aEval = 10"); + function b() { + let bVar = 2; + eval("var bEval = 20"); + function c() { + let cVar = 3; + eval("var cEval = 30"); + return cVar + cEval; + } + return bVar + bEval + c(); + } + return aVar + aEval + b(); + } + expect(a()).toBe(66); + }); + + test("arrow function containing eval", () => { + function outer() { + let x = 5; + const arrow = () => { + eval("var y = 10"); + return x + y; + }; + let result = arrow(); + return result + x; + } + expect(outer()).toBe(20); + }); + + test("eval in for loop", () => { + function foo() { + let sum = 0; + for (let i = 0; i < 3; i++) { + eval("var loopVar = i * 10"); + sum += loopVar; + } + return sum; + } + expect(foo()).toBe(30); + }); + + test("eval creating variable that shadows outer scope", () => { + var globalVar = 100; + function foo() { + let before = globalVar; + eval("var globalVar = 200"); + let after = globalVar; + return before * 1000 + after; + } + let result = foo(); + expect(result + globalVar).toBe(100300); + }); + + test("deep nesting where only innermost has eval", () => { + function level1() { + let v1 = 1; + function level2() { + let v2 = 2; + function level3() { + let v3 = 3; + function level4() { + eval("var v4 = 4"); + return v1 + v2 + v3 + v4; + } + return level4(); + } + return level3(); + } + return level2(); + } + expect(level1()).toBe(10); + }); + + test("eval with same variable name at multiple levels", () => { + function outer() { + var x = 1; + function middle() { + var x = 2; + function inner() { + eval("var x = 3"); + return x; + } + let innerX = inner(); + return x * 10 + innerX; + } + let middleResult = middle(); + return x * 100 + middleResult; + } + expect(outer()).toBe(123); + }); + + test("strict mode eval does not affect outer scope", () => { + function foo() { + var x = 1; + function bar() { + "use strict"; + eval("var x = 2"); + return x; + } + let barResult = bar(); + return x * 10 + barResult; + } + expect(foo()).toBe(11); + }); + + test("indirect eval uses global scope", () => { + var globalForTest = 100; + function foo() { + var localVar = 200; + var globalForTest = 300; + let directResult = eval("globalForTest"); + let indirectEval = eval; + let indirectResult = indirectEval("typeof localVar"); + return directResult + "_" + indirectResult; + } + expect(foo()).toBe("300_undefined"); + }); + + test("eval in catch block", () => { + function foo() { + var x = 1; + try { + throw new Error("test"); + } catch (e) { + eval("var x = 2"); + } + return x; + } + expect(foo()).toBe(2); + }); + + test("eval with function declaration", () => { + function outer() { + var x = 1; + function inner() { + eval("function f() { return 42; }"); + return f(); + } + let result = inner(); + let fExists = typeof f !== "undefined"; + return result + "_" + fExists; + } + expect(outer()).toBe("42_false"); + }); + + test("multiple variables created by single eval", () => { + function foo() { + var a = 1; + function bar() { + eval("var a = 10; var b = 20; var c = 30;"); + return a + b + c; + } + let barResult = bar(); + return a * 1000 + barResult; + } + expect(foo()).toBe(1060); + }); + + test("eval accessing closure variables", () => { + function outer() { + let closureVar = 100; + function inner() { + return eval("closureVar + 50"); + } + return inner(); + } + expect(outer()).toBe(150); + }); + + test("repeated calls to function with eval", () => { + function outer() { + var counter = 0; + function inner() { + eval("var localCounter = " + counter); + counter++; + return localCounter; + } + let r1 = inner(); + let r2 = inner(); + let r3 = inner(); + return r1 + "_" + r2 + "_" + r3 + "_" + counter; + } + expect(outer()).toBe("0_1_2_3"); + }); + + test("eval in IIFE inside function", () => { + function outer() { + var x = 1; + (function () { + eval("var x = 2"); + })(); + return x; + } + expect(outer()).toBe(1); + }); + + test("eval does not affect function parameters", () => { + function outer(a, b) { + function inner() { + eval("var a = 100"); + return a; + } + let innerResult = inner(); + return a + "_" + innerResult; + } + expect(outer(1, 2)).toBe("1_100"); + }); + + test("eval var vs closure let", () => { + function outer() { + let x = 1; + function inner() { + let y = 2; + eval("var x = 10"); + return x + y; + } + let innerResult = inner(); + return x * 100 + innerResult; + } + expect(outer()).toBe(112); + }); + + test("typeof on variable that might be created by eval - false", () => { + function foo(doEval) { + if (doEval) { + eval("var dynamicVar = 42"); + } + return typeof dynamicVar; + } + expect(foo(false)).toBe("undefined"); + }); + + test("typeof on variable that might be created by eval - true", () => { + function foo(doEval) { + if (doEval) { + eval("var dynamicVar = 42"); + } + return typeof dynamicVar; + } + expect(foo(true)).toBe("number"); + }); + + test("global-level eval creates global variable", () => { + // Use indirect eval to execute in global scope + const globalEval = eval; + globalEval("var globalTestVar123 = 999"); + expect(globalThis.globalTestVar123).toBe(999); + delete globalThis.globalTestVar123; + }); + + test("global-level eval can modify existing global", () => { + globalThis.existingGlobal456 = 100; + const globalEval = eval; + globalEval("existingGlobal456 = 200"); + expect(globalThis.existingGlobal456).toBe(200); + delete globalThis.existingGlobal456; + }); + + test("direct eval in function does not create true global", () => { + function foo() { + eval("var localToFoo = 42"); + return localToFoo; + } + expect(foo()).toBe(42); + expect(typeof localToFoo).toBe("undefined"); + }); + + test("eval shadowing global does not modify actual global", () => { + globalThis.testGlobal789 = "original"; + function foo() { + eval("var testGlobal789 = 'shadowed'"); + return testGlobal789; + } + expect(foo()).toBe("shadowed"); + expect(globalThis.testGlobal789).toBe("original"); + delete globalThis.testGlobal789; + }); + + test("nested function accesses eval-created var in parent, not global", () => { + globalThis.sharedName111 = "global"; + function outer() { + eval("var sharedName111 = 'local'"); + function inner() { + return sharedName111; + } + return inner(); + } + expect(outer()).toBe("local"); + expect(globalThis.sharedName111).toBe("global"); + delete globalThis.sharedName111; + }); +});