From bfbc3352b52bfc2e509624c8cfd426e9690ca4f7 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Thu, 23 Apr 2026 18:45:03 +0200 Subject: [PATCH] LibJS: Extend Array.prototype.shift() fast path to holey arrays indexed_take_first() already memmoves elements down for both Packed and Holey storage, but the caller at ArrayPrototype::shift() only entered the fast path for Packed arrays. Holey arrays fell through to the spec-literal per-element loop (has_property / get / set / delete_property_or_throw), which is substantially slower. Add a separate Holey predicate with the additional safety checks the spec semantics require: default_prototype_chain_intact() (so HasProperty on a hole doesn't escape to a poisoned prototype) and extensible() (so set() on a hole slot doesn't create a new own property on a non-extensible object). The existing Packed predicate is left unchanged -- packed arrays don't need these checks because every index in [0, size) is already an own data property. Allows us to fail at Cloudflare Turnstile way much faster! --- Libraries/LibJS/Runtime/ArrayPrototype.cpp | 23 +++++- .../builtins/Array/Array.prototype.shift.js | 75 +++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) diff --git a/Libraries/LibJS/Runtime/ArrayPrototype.cpp b/Libraries/LibJS/Runtime/ArrayPrototype.cpp index adea0e30aa4..6a13848e7a4 100644 --- a/Libraries/LibJS/Runtime/ArrayPrototype.cpp +++ b/Libraries/LibJS/Runtime/ArrayPrototype.cpp @@ -152,6 +152,26 @@ static bool can_use_packed_array_fast_path(Array const& array) return array.is_simple_packed_array() && array.default_prototype_chain_intact(); } +static bool can_use_packed_shift_fast_path(Array const& array) +{ + // Packed: every index in [0, size) is an own property, so memmove semantics match the spec even if the prototype + // chain has indexed properties (HasProperty never escapes to the proto) and even if the array is non-extensible + // (no new own properties are created). + return array.is_simple_packed_array() && array.length_is_writable(); +} + +static bool can_use_holey_shift_fast_path(Array const& array) +{ + // Holey: the spec path uses HasProperty + Get on every index, so a poisoned prototype changes the outcome on holes. + // A set() on a hole slot also creates a new own property, which a non-extensible array rejects with TypeError. + return !array.is_proxy_target() + && !array.may_interfere_with_indexed_property_access() + && array.indexed_storage_kind() == IndexedStorageKind::Holey + && array.default_prototype_chain_intact() + && array.extensible() + && array.length_is_writable(); +} + static Array* fast_array_species_result(Object& object) { auto* array = as_if(object); @@ -1306,7 +1326,8 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayPrototype::shift) // - has intact prototype chain, which means we don't have to worry about getters/setters potentially defined for holes. // - has simple storage type, which means all values have default attributes (if some elements have configurable=false, we cannot use fast path, because delete operation will fail). // then we could take a fast path by directly taking first element from indexed storage. - if (auto* array = as_if(*this_object); array && array->is_simple_packed_array() && array->length_is_writable()) { + if (auto* array = as_if(*this_object); + array && (can_use_packed_shift_fast_path(*array) || can_use_holey_shift_fast_path(*array))) { auto first = array->indexed_take_first().value; if (first.is_special_empty_value()) return js_undefined(); diff --git a/Tests/LibJS/Runtime/builtins/Array/Array.prototype.shift.js b/Tests/LibJS/Runtime/builtins/Array/Array.prototype.shift.js index 95d5412ed3b..6d803450242 100644 --- a/Tests/LibJS/Runtime/builtins/Array/Array.prototype.shift.js +++ b/Tests/LibJS/Runtime/builtins/Array/Array.prototype.shift.js @@ -47,3 +47,78 @@ test("throws if the array length is not writable", () => { expect(1 in a).toBeFalse(); expect(a.length).toBe(2); }); + +describe("holey arrays", () => { + test("shift on clean holey array with interior hole preserves hole", () => { + const a = [0, , 2]; + const result = a.shift(); + expect(result).toBe(0); + expect(a.length).toBe(2); + expect(0 in a).toBeFalse(); + expect(a[0]).toBeUndefined(); + expect(1 in a).toBeTrue(); + expect(a[1]).toBe(2); + }); + + test("shift on clean holey array with leading hole returns undefined", () => { + const a = [, 1, 2]; + const result = a.shift(); + expect(result).toBeUndefined(); + expect(a.length).toBe(2); + expect(a[0]).toBe(1); + expect(a[1]).toBe(2); + }); + + test("shift on clean holey array with trailing hole propagates it", () => { + const a = [0, 1, ,]; + expect(a.length).toBe(3); + const result = a.shift(); + expect(result).toBe(0); + expect(a.length).toBe(2); + expect(a[0]).toBe(1); + expect(1 in a).toBeFalse(); + }); + + test("holey shift with prototype pollution follows spec", () => { + Array.prototype[1] = "polluted"; + try { + const a = [0, , 2]; + const result = a.shift(); + expect(result).toBe(0); + // Spec: HasProperty(a, 1) is true via proto, Get returns "polluted", + // which is set as an own property at index 0. + expect(a[0]).toBe("polluted"); + expect(0 in a).toBeTrue(); + expect(a[1]).toBe(2); + expect(a.length).toBe(2); + } finally { + delete Array.prototype[1]; + } + }); + + test("holey shift on non-extensible array throws TypeError", () => { + const a = [, 1]; + Object.preventExtensions(a); + expect(() => a.shift()).toThrow(TypeError); + }); + + test("packed shift with prototype pollution still fast-paths correctly", () => { + Array.prototype[0] = "polluted"; + try { + const a = [1, 2, 3]; + expect(a.shift()).toBe(1); + expect(a).toEqual([2, 3]); + } finally { + delete Array.prototype[0]; + } + }); + + test("packed shift on non-extensible array works (no new properties)", () => { + const a = [1, 2, 3]; + Object.preventExtensions(a); + expect(a.shift()).toBe(1); + expect(a.length).toBe(2); + expect(a[0]).toBe(2); + expect(a[1]).toBe(3); + }); +});