From 22fdcfbc5051e24daffc839408e3271ebc964d03 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 29 Mar 2024 12:10:52 -0400 Subject: [PATCH] LibJS: Include identifier information in nullish property write access When a PutById / PutByValue bytecode operation results in accessing a nullish object, we now include the name of the property and the object being accessed in the exception message (if available). This should make it easier to debug live websites. For example, the following errors would all previously produce a generic error message of "ToObject on null or undefined": > foo = null > foo.bar = 1 Uncaught exception: [TypeError] Cannot access property "bar" on null object "foo" at > foo = { bar: undefined } > foo.bar.baz = 1 Uncaught exception: [TypeError] Cannot access property "baz" on undefined object "foo.bar" at Note we certainly don't capture all possible nullish property write accesses here. This just covers cases I've seen most on live websites; we can cover more cases as they arise. --- .../Libraries/LibJS/Bytecode/ASTCodegen.cpp | 5 +++-- .../LibJS/Bytecode/CommonImplementations.h | 12 ++++++++---- .../Libraries/LibJS/Bytecode/Interpreter.cpp | 10 ++++++---- Userland/Libraries/LibJS/Bytecode/Op.h | 8 ++++++-- .../LibJS/Tests/null-or-undefined-access.js | 19 +++++++++++++++++++ 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp index fa73d46786a..eb565696680 100644 --- a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp +++ b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp @@ -489,16 +489,17 @@ Bytecode::CodeGenerationErrorOr> AssignmentExpressio generator.emit_set_variable(identifier, rval); } else if (is(*lhs)) { auto& expression = static_cast(*lhs); + auto base_identifier = generator.intern_identifier_for_expression(expression.object()); if (expression.is_computed()) { if (!lhs_is_super_expression) - generator.emit(*base, *computed_property, rval); + generator.emit(*base, *computed_property, rval, Bytecode::Op::PropertyKind::KeyValue, move(base_identifier)); else generator.emit(*base, *computed_property, *this_value, rval); } else if (expression.property().is_identifier()) { auto identifier_table_ref = generator.intern_identifier(verify_cast(expression.property()).string()); if (!lhs_is_super_expression) - generator.emit(*base, identifier_table_ref, rval, Bytecode::Op::PropertyKind::KeyValue, generator.next_property_lookup_cache()); + generator.emit(*base, identifier_table_ref, rval, Bytecode::Op::PropertyKind::KeyValue, generator.next_property_lookup_cache(), move(base_identifier)); else generator.emit(*base, *this_value, identifier_table_ref, rval, Bytecode::Op::PropertyKind::KeyValue, generator.next_property_lookup_cache()); } else if (expression.property().is_private_identifier()) { diff --git a/Userland/Libraries/LibJS/Bytecode/CommonImplementations.h b/Userland/Libraries/LibJS/Bytecode/CommonImplementations.h index d9e2b55e1f7..7ab35fd0b1d 100644 --- a/Userland/Libraries/LibJS/Bytecode/CommonImplementations.h +++ b/Userland/Libraries/LibJS/Bytecode/CommonImplementations.h @@ -248,14 +248,18 @@ inline ThrowCompletionOr get_global(Bytecode::Interpreter& interpreter, D return vm.throw_completion(ErrorType::UnknownIdentifier, identifier); } -inline ThrowCompletionOr put_by_property_key(VM& vm, Value base, Value this_value, Value value, PropertyKey name, Op::PropertyKind kind, PropertyLookupCache* cache = nullptr) +inline ThrowCompletionOr put_by_property_key(VM& vm, Value base, Value this_value, Value value, Optional const& base_identifier, PropertyKey name, Op::PropertyKind kind, PropertyLookupCache* cache = nullptr) { // Better error message than to_object would give if (vm.in_strict_mode() && base.is_nullish()) return vm.throw_completion(ErrorType::ReferenceNullishSetProperty, name, base.to_string_without_side_effects()); // a. Let baseObj be ? ToObject(V.[[Base]]). - auto object = TRY(base.to_object(vm)); + auto maybe_object = base.to_object(vm); + if (maybe_object.is_error()) + return throw_null_or_undefined_property_access(vm, base, base_identifier, name); + auto object = maybe_object.release_value(); + if (kind == Op::PropertyKind::Getter || kind == Op::PropertyKind::Setter) { // The generator should only pass us functions for getters and setters. VERIFY(value.is_function()); @@ -411,7 +415,7 @@ inline Value new_function(VM& vm, FunctionExpression const& function_node, Optio return value; } -inline ThrowCompletionOr put_by_value(VM& vm, Value base, Value property_key_value, Value value, Op::PropertyKind kind) +inline ThrowCompletionOr put_by_value(VM& vm, Value base, Optional const& base_identifier, Value property_key_value, Value value, Op::PropertyKind kind) { // OPTIMIZATION: Fast path for simple Int32 indexes in array-like objects. if ((kind == Op::PropertyKind::KeyValue || kind == Op::PropertyKind::DirectKeyValue) @@ -489,7 +493,7 @@ inline ThrowCompletionOr put_by_value(VM& vm, Value base, Value property_k } auto property_key = kind != Op::PropertyKind::Spread ? TRY(property_key_value.to_property_key(vm)) : PropertyKey {}; - TRY(put_by_property_key(vm, base, base, value, property_key, kind)); + TRY(put_by_property_key(vm, base, base, value, base_identifier, property_key, kind)); return {}; } diff --git a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp index 249a826975f..b768174b9d6 100644 --- a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp @@ -1132,9 +1132,10 @@ ThrowCompletionOr PutById::execute_impl(Bytecode::Interpreter& interpreter auto& vm = interpreter.vm(); auto value = interpreter.get(m_src); auto base = interpreter.get(m_base); + auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier); PropertyKey name = interpreter.current_executable().get_identifier(m_property); auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index]; - TRY(put_by_property_key(vm, base, base, value, name, m_kind, &cache)); + TRY(put_by_property_key(vm, base, base, value, base_identifier, name, m_kind, &cache)); return {}; } @@ -1145,7 +1146,7 @@ ThrowCompletionOr PutByIdWithThis::execute_impl(Bytecode::Interpreter& int auto base = interpreter.get(m_base); PropertyKey name = interpreter.current_executable().get_identifier(m_property); auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index]; - TRY(put_by_property_key(vm, base, interpreter.get(m_this_value), value, name, m_kind, &cache)); + TRY(put_by_property_key(vm, base, interpreter.get(m_this_value), value, {}, name, m_kind, &cache)); return {}; } @@ -1523,7 +1524,8 @@ ThrowCompletionOr PutByValue::execute_impl(Bytecode::Interpreter& interpre { auto& vm = interpreter.vm(); auto value = interpreter.get(m_src); - TRY(put_by_value(vm, interpreter.get(m_base), interpreter.get(m_property), value, m_kind)); + auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier); + TRY(put_by_value(vm, interpreter.get(m_base), base_identifier, interpreter.get(m_property), value, m_kind)); return {}; } @@ -1533,7 +1535,7 @@ ThrowCompletionOr PutByValueWithThis::execute_impl(Bytecode::Interpreter& auto value = interpreter.get(m_src); auto base = interpreter.get(m_base); auto property_key = m_kind != PropertyKind::Spread ? TRY(interpreter.get(m_property).to_property_key(vm)) : PropertyKey {}; - TRY(put_by_property_key(vm, base, interpreter.get(m_this_value), value, property_key, m_kind)); + TRY(put_by_property_key(vm, base, interpreter.get(m_this_value), value, {}, property_key, m_kind)); return {}; } diff --git a/Userland/Libraries/LibJS/Bytecode/Op.h b/Userland/Libraries/LibJS/Bytecode/Op.h index a38d11180b1..7c91b2f095c 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.h +++ b/Userland/Libraries/LibJS/Bytecode/Op.h @@ -741,13 +741,14 @@ enum class PropertyKind { class PutById final : public Instruction { public: - explicit PutById(Operand base, IdentifierTableIndex property, Operand src, PropertyKind kind, u32 cache_index) + explicit PutById(Operand base, IdentifierTableIndex property, Operand src, PropertyKind kind, u32 cache_index, Optional base_identifier = {}) : Instruction(Type::PutById, sizeof(*this)) , m_base(base) , m_property(property) , m_src(src) , m_kind(kind) , m_cache_index(cache_index) + , m_base_identifier(move(base_identifier)) { } @@ -766,6 +767,7 @@ private: Operand m_src; PropertyKind m_kind; u32 m_cache_index { 0 }; + Optional m_base_identifier {}; }; class PutByIdWithThis final : public Instruction { @@ -929,12 +931,13 @@ private: class PutByValue final : public Instruction { public: - PutByValue(Operand base, Operand property, Operand src, PropertyKind kind = PropertyKind::KeyValue) + PutByValue(Operand base, Operand property, Operand src, PropertyKind kind = PropertyKind::KeyValue, Optional base_identifier = {}) : Instruction(Type::PutByValue, sizeof(*this)) , m_base(base) , m_property(property) , m_src(src) , m_kind(kind) + , m_base_identifier(move(base_identifier)) { } @@ -951,6 +954,7 @@ private: Operand m_property; Operand m_src; PropertyKind m_kind; + Optional m_base_identifier; }; class PutByValueWithThis final : public Instruction { diff --git a/Userland/Libraries/LibJS/Tests/null-or-undefined-access.js b/Userland/Libraries/LibJS/Tests/null-or-undefined-access.js index 2c167ceee89..c5a1a754cdc 100644 --- a/Userland/Libraries/LibJS/Tests/null-or-undefined-access.js +++ b/Userland/Libraries/LibJS/Tests/null-or-undefined-access.js @@ -6,9 +6,17 @@ test("null/undefined object", () => { foo.bar; }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object "foo"`); + expect(() => { + foo.bar = 1; + }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object "foo"`); + expect(() => { foo[0]; }).toThrowWithMessage(TypeError, `Cannot access property "0" on ${value} object "foo"`); + + expect(() => { + foo[0] = 1; + }).toThrowWithMessage(TypeError, `Cannot access property "0" on ${value} object "foo"`); }); }); @@ -22,6 +30,13 @@ test("null/undefined object key", () => { TypeError, `Cannot access property "baz" on ${value} object "foo.bar"` ); + + expect(() => { + foo.bar.baz = 1; + }).toThrowWithMessage( + TypeError, + `Cannot access property "baz" on ${value} object "foo.bar"` + ); }); }); @@ -32,5 +47,9 @@ test("null/undefined array index", () => { expect(() => { foo[0].bar; }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object`); + + expect(() => { + foo[0].bar = 1; + }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object`); }); });