From 82b42cefbd2c63968ec334be12b507ef4d7fd421 Mon Sep 17 00:00:00 2001
From: Linus Groh <mail@linusgroh.de>
Date: Fri, 6 Nov 2020 12:52:44 +0000
Subject: [PATCH] LibJS: Handle circular references in
 Array.prototype.toLocaleString()

Also use ArmedScopeGuard for removing seen objects to account for early
returns.

Fixes #3963.
---
 Libraries/LibJS/Runtime/ArrayPrototype.cpp    | 21 +++++++++++++++----
 .../Array/Array.prototype.toLocaleString.js   |  8 +++++++
 .../Array/Array.prototype.toString.js         |  8 +++++++
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/Libraries/LibJS/Runtime/ArrayPrototype.cpp b/Libraries/LibJS/Runtime/ArrayPrototype.cpp
index 39117d4c50..0afdbb2436 100644
--- a/Libraries/LibJS/Runtime/ArrayPrototype.cpp
+++ b/Libraries/LibJS/Runtime/ArrayPrototype.cpp
@@ -28,6 +28,7 @@
 
 #include <AK/Function.h>
 #include <AK/HashTable.h>
+#include <AK/ScopeGuard.h>
 #include <AK/StringBuilder.h>
 #include <LibJS/Runtime/Array.h>
 #include <LibJS/Runtime/ArrayIterator.h>
@@ -288,10 +289,19 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayPrototype::to_locale_string)
     auto* this_object = vm.this_value(global_object).to_object(global_object);
     if (!this_object)
         return {};
-    String separator = ","; // NOTE: This is implementation-specific.
+
+    if (s_array_join_seen_objects.contains(this_object))
+        return js_string(vm, "");
+    s_array_join_seen_objects.set(this_object);
+    ArmedScopeGuard unsee_object_guard = [&] {
+        s_array_join_seen_objects.remove(this_object);
+    };
+
     auto length = get_length(vm, *this_object);
     if (vm.exception())
         return {};
+
+    String separator = ","; // NOTE: This is implementation-specific.
     StringBuilder builder;
     for (size_t i = 0; i < length; ++i) {
         if (i > 0)
@@ -302,7 +312,8 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayPrototype::to_locale_string)
         if (value.is_nullish())
             continue;
         auto* value_object = value.to_object(global_object);
-        ASSERT(value_object);
+        if (!value_object)
+            return {};
         auto locale_string_result = value_object->invoke("toLocaleString");
         if (vm.exception())
             return {};
@@ -322,9 +333,13 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayPrototype::join)
 
     // This is not part of the spec, but all major engines do some kind of circular reference checks.
     // FWIW: engine262, a "100% spec compliant" ECMA-262 impl, aborts with "too much recursion".
+    // Same applies to Array.prototype.toLocaleString().
     if (s_array_join_seen_objects.contains(this_object))
         return js_string(vm, "");
     s_array_join_seen_objects.set(this_object);
+    ArmedScopeGuard unsee_object_guard = [&] {
+        s_array_join_seen_objects.remove(this_object);
+    };
 
     auto length = get_length(vm, *this_object);
     if (vm.exception())
@@ -350,8 +365,6 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayPrototype::join)
         builder.append(string);
     }
 
-    s_array_join_seen_objects.remove(this_object);
-
     return js_string(vm, builder.to_string());
 }
 
diff --git a/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toLocaleString.js b/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toLocaleString.js
index c566848567..05854a1d0c 100644
--- a/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toLocaleString.js
+++ b/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toLocaleString.js
@@ -51,4 +51,12 @@ describe("normal behavior", () => {
         expect([o, undefined, o, null, o].toLocaleString()).toBe("o,,o,,o");
         expect(toStringCalled).toBe(3);
     });
+
+    test("array with circular references", () => {
+        const a = ["foo", [], [1, 2, []], ["bar"]];
+        a[1] = a;
+        a[2][2] = a;
+        // [ "foo", <circular>, [ 1, 2, <circular> ], [ "bar" ] ]
+        expect(a.toLocaleString()).toBe("foo,,1,2,,bar");
+    });
 });
diff --git a/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toString.js b/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toString.js
index d07e8af163..1c18fb54ed 100644
--- a/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toString.js
+++ b/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toString.js
@@ -55,4 +55,12 @@ describe("normal behavior", () => {
         expect([o, undefined, o, null, o].toString()).toBe("o,,o,,o");
         expect(toStringCalled).toBe(3);
     });
+
+    test("array with circular references", () => {
+        const a = ["foo", [], [1, 2, []], ["bar"]];
+        a[1] = a;
+        a[2][2] = a;
+        // [ "foo", <circular>, [ 1, 2, <circular> ], [ "bar" ] ]
+        expect(a.toString()).toBe("foo,,1,2,,bar");
+    });
 });