LibJS: Ensure function declarations don't leak outside function scopes

When using VM::set_variable() to put the created ScriptFunction onto a
ScopeObject, we would previously unexpectedly reach the global object as
set_variable() checks each traversed scope for an existing Variable with
the given name - which would cause a leak of the inner function past the
outer function (we even had a test expecting that behaviour!). Now we
first declare functions (as DeclarationKind::Var) before setting them.
This will need some more work to make hoisting across non-lexical scopes
work, but it fixes this specific issue for now.

Fixes #6766.

3 files changed, 31 insertions, 10 deletions

diff --git a/Userland/Libraries/LibJS/Interpreter.cpp b/Userland/Libraries/LibJS/Interpreter.cpp
index f3bd3685e9..7044957c19 100644
--- a/Userland/Libraries/LibJS/Interpreter.cpp
+++ b/Userland/Libraries/LibJS/Interpreter.cpp
@@ -1,9 +1,11 @@
 /*
  * Copyright (c) 2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2020-2021, Linus Groh <linusg@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
+#include <AK/ScopeGuard.h>
 #include <AK/StringBuilder.h>
 #include <LibJS/AST.h>
 #include <LibJS/Interpreter.h>
@@ -80,13 +82,17 @@ const GlobalObject& Interpreter::global_object() const
 
 void Interpreter::enter_scope(const ScopeNode& scope_node, ScopeType scope_type, GlobalObject& global_object)
 {
-    for (auto& declaration : scope_node.functions()) {
-        auto* function = ScriptFunction::create(global_object, declaration.name(), declaration.body(), declaration.parameters(), declaration.function_length(), current_scope(), declaration.is_strict_mode());
-        vm().set_variable(declaration.name(), function, global_object);
-    }
+    ScopeGuard guard([&] {
+        for (auto& declaration : scope_node.functions()) {
+            auto* function = ScriptFunction::create(global_object, declaration.name(), declaration.body(), declaration.parameters(), declaration.function_length(), current_scope(), declaration.is_strict_mode());
+            vm().set_variable(declaration.name(), function, global_object);
+        }
+    });
 
     if (scope_type == ScopeType::Function) {
         push_scope({ scope_type, scope_node, false });
+        for (auto& declaration : scope_node.functions())
+            current_scope()->put_to_scope(declaration.name(), { js_undefined(), DeclarationKind::Var });
         return;
     }
 
diff --git a/Userland/Libraries/LibJS/Tests/functions/function-nesting.js b/Userland/Libraries/LibJS/Tests/functions/function-nesting.js
new file mode 100644
index 0000000000..daef3c7cd3
--- /dev/null
+++ b/Userland/Libraries/LibJS/Tests/functions/function-nesting.js
@@ -0,0 +1,15 @@
+test("issue #6766, nested functions should not leak to global object", () => {
+    function foo() {
+        function bar() {
+            function baz() {
+                return 42;
+            }
+            return baz();
+        }
+        return bar();
+    }
+    expect(foo()).toBe(42);
+    expect(globalThis.foo).toBeUndefined();
+    expect(globalThis.bar).toBeUndefined();
+    expect(globalThis.baz).toBeUndefined();
+});
diff --git a/Userland/Libraries/LibJS/Tests/if-statement-function-declaration.js b/Userland/Libraries/LibJS/Tests/if-statement-function-declaration.js
index 583585ebe5..f5fde6fbad 100644
--- a/Userland/Libraries/LibJS/Tests/if-statement-function-declaration.js
+++ b/Userland/Libraries/LibJS/Tests/if-statement-function-declaration.js
@@ -2,8 +2,8 @@ describe("function declarations in if statement clauses", () => {
     test("if clause", () => {
         if (true) function foo() {}
         if (false) function bar() {}
-        expect(typeof globalThis.foo).toBe("function");
-        expect(typeof globalThis.bar).toBe("undefined");
+        expect(typeof foo).toBe("function");
+        expect(typeof bar).toBe("undefined");
     });
 
     test("else clause", () => {
@@ -11,15 +11,15 @@ describe("function declarations in if statement clauses", () => {
         else function foo() {}
         if (true);
         else function bar() {}
-        expect(typeof globalThis.foo).toBe("function");
-        expect(typeof globalThis.bar).toBe("undefined");
+        expect(typeof foo).toBe("function");
+        expect(typeof bar).toBe("undefined");
     });
 
     test("if and else clause", () => {
         if (true) function foo() {}
         else function bar() {}
-        expect(typeof globalThis.foo).toBe("function");
-        expect(typeof globalThis.bar).toBe("undefined");
+        expect(typeof foo).toBe("function");
+        expect(typeof bar).toBe("undefined");
     });
 
     test("syntax error in strict mode", () => {