From 9bbd3103a882f405d1c728ff262ce81664d0981e Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 29 Mar 2024 11:26:10 -0400 Subject: [PATCH] LibJS: Include identifier information in nullish property read access When a GetById / GetByValue 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 Uncaught exception: [TypeError] Cannot access property "bar" on null object "foo" at > foo = { bar: undefined } > foo.bar.baz Uncaught exception: [TypeError] Cannot access property "baz" on undefined object "foo.bar" at Note we certainly don't capture all possible nullish property read 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 | 3 +- .../LibJS/Bytecode/CommonImplementations.h | 35 ++++++++++++--- .../Libraries/LibJS/Bytecode/Executable.h | 7 +++ .../Libraries/LibJS/Bytecode/Generator.cpp | 44 +++++++++++++++++-- Userland/Libraries/LibJS/Bytecode/Generator.h | 4 +- .../Libraries/LibJS/Bytecode/Interpreter.cpp | 12 +++-- Userland/Libraries/LibJS/Bytecode/Op.h | 10 ++++- Userland/Libraries/LibJS/Runtime/ErrorTypes.h | 3 ++ .../LibJS/Tests/null-or-undefined-access.js | 36 +++++++++++++++ 9 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 Userland/Libraries/LibJS/Tests/null-or-undefined-access.js diff --git a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp index beb358aaf16..fa73d46786a 100644 --- a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp +++ b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp @@ -1540,7 +1540,8 @@ static Bytecode::CodeGenerationErrorOr get_base_and_value_from_mem base, generator.intern_identifier(verify_cast(member_expression.property()).string())); } else { - generator.emit_get_by_id(value, base, generator.intern_identifier(verify_cast(member_expression.property()).string())); + auto base_identifier = generator.intern_identifier_for_expression(member_expression.object()); + generator.emit_get_by_id(value, base, generator.intern_identifier(verify_cast(member_expression.property()).string()), move(base_identifier)); } return BaseAndValue { base, value }; diff --git a/Userland/Libraries/LibJS/Bytecode/CommonImplementations.h b/Userland/Libraries/LibJS/Bytecode/CommonImplementations.h index d5a1fcc4bc1..d9e2b55e1f7 100644 --- a/Userland/Libraries/LibJS/Bytecode/CommonImplementations.h +++ b/Userland/Libraries/LibJS/Bytecode/CommonImplementations.h @@ -58,7 +58,30 @@ inline void fast_typed_array_set_element(TypedArrayBase& typed_array, u32 index, *slot = value; } -ALWAYS_INLINE ThrowCompletionOr> base_object_for_get(VM& vm, Value base_value) +template +ALWAYS_INLINE Completion throw_null_or_undefined_property_access(VM& vm, Value base_value, BaseType const& base_identifier, PropertyType const& property_identifier) +{ + VERIFY(base_value.is_nullish()); + + bool has_base_identifier = true; + bool has_property_identifier = true; + + if constexpr (requires { base_identifier.has_value(); }) + has_base_identifier = base_identifier.has_value(); + if constexpr (requires { property_identifier.has_value(); }) + has_property_identifier = property_identifier.has_value(); + + if (has_base_identifier && has_property_identifier) + return vm.throw_completion(ErrorType::ToObjectNullOrUndefinedWithPropertyAndName, property_identifier, base_value, base_identifier); + if (has_property_identifier) + return vm.throw_completion(ErrorType::ToObjectNullOrUndefinedWithProperty, property_identifier, base_value); + if (has_base_identifier) + return vm.throw_completion(ErrorType::ToObjectNullOrUndefinedWithName, base_identifier, base_value); + return vm.throw_completion(ErrorType::ToObjectNullOrUndefined); +} + +template +ALWAYS_INLINE ThrowCompletionOr> base_object_for_get(VM& vm, Value base_value, BaseType const& base_identifier, PropertyType property_identifier) { if (base_value.is_object()) [[likely]] return base_value.as_object(); @@ -77,10 +100,10 @@ ALWAYS_INLINE ThrowCompletionOr> base_object_for_get(VM& vm return realm.intrinsics().symbol_prototype(); // NOTE: At this point this is guaranteed to throw (null or undefined). - return base_value.to_object(vm); + return throw_null_or_undefined_property_access(vm, base_value, base_identifier, property_identifier); } -inline ThrowCompletionOr get_by_id(VM& vm, DeprecatedFlyString const& property, Value base_value, Value this_value, PropertyLookupCache& cache) +inline ThrowCompletionOr get_by_id(VM& vm, Optional const& base_identifier, DeprecatedFlyString const& property, Value base_value, Value this_value, PropertyLookupCache& cache) { if (base_value.is_string()) { auto string_value = TRY(base_value.as_string().get(vm, property)); @@ -88,7 +111,7 @@ inline ThrowCompletionOr get_by_id(VM& vm, DeprecatedFlyString const& pro return *string_value; } - auto base_obj = TRY(base_object_for_get(vm, base_value)); + auto base_obj = TRY(base_object_for_get(vm, base_value, base_identifier, property)); // OPTIMIZATION: Fast path for the magical "length" property on Array objects. if (base_obj->has_magical_length_property() && property == vm.names.length.as_string()) { @@ -112,7 +135,7 @@ inline ThrowCompletionOr get_by_id(VM& vm, DeprecatedFlyString const& pro return value; } -inline ThrowCompletionOr get_by_value(VM& vm, Value base_value, Value property_key_value) +inline ThrowCompletionOr get_by_value(VM& vm, Optional const& base_identifier, Value base_value, Value property_key_value) { // OPTIMIZATION: Fast path for simple Int32 indexes in array-like objects. if (base_value.is_object() && property_key_value.is_int32() && property_key_value.as_i32() >= 0) { @@ -169,7 +192,7 @@ inline ThrowCompletionOr get_by_value(VM& vm, Value base_value, Value pro } } - auto object = TRY(base_object_for_get(vm, base_value)); + auto object = TRY(base_object_for_get(vm, base_value, base_identifier, property_key_value)); auto property_key = TRY(property_key_value.to_property_key(vm)); diff --git a/Userland/Libraries/LibJS/Bytecode/Executable.h b/Userland/Libraries/LibJS/Bytecode/Executable.h index 150e9bc6f5f..e593ccd851c 100644 --- a/Userland/Libraries/LibJS/Bytecode/Executable.h +++ b/Userland/Libraries/LibJS/Bytecode/Executable.h @@ -74,6 +74,13 @@ public: ByteString const& get_string(StringTableIndex index) const { return string_table->get(index); } DeprecatedFlyString const& get_identifier(IdentifierTableIndex index) const { return identifier_table->get(index); } + Optional get_identifier(Optional const& index) const + { + if (!index.has_value()) + return {}; + return get_identifier(*index); + } + void dump() const; private: diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.cpp b/Userland/Libraries/LibJS/Bytecode/Generator.cpp index 388607084f4..122a6c9b549 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Generator.cpp @@ -253,13 +253,16 @@ CodeGenerationErrorOr Generator::emit_load_from_re super_reference.loaded_value = dst; return super_reference; } + auto base = TRY(expression.object().generate_bytecode(*this)).value(); + auto base_identifier = intern_identifier_for_expression(expression.object()); + if (expression.is_computed()) { auto property = TRY(expression.property().generate_bytecode(*this)).value(); auto saved_property = Operand(allocate_register()); emit(saved_property, property); auto dst = preferred_dst.has_value() ? preferred_dst.value() : Operand(allocate_register()); - emit(dst, base, property); + emit(dst, base, property, move(base_identifier)); return ReferenceOperands { .base = base, .referenced_name = saved_property, @@ -270,7 +273,7 @@ CodeGenerationErrorOr Generator::emit_load_from_re if (expression.property().is_identifier()) { auto identifier_table_ref = intern_identifier(verify_cast(expression.property()).string()); auto dst = preferred_dst.has_value() ? preferred_dst.value() : Operand(allocate_register()); - emit_get_by_id(dst, base, identifier_table_ref); + emit_get_by_id(dst, base, identifier_table_ref, move(base_identifier)); return ReferenceOperands { .base = base, .referenced_identifier = identifier_table_ref, @@ -443,6 +446,39 @@ void Generator::emit_set_variable(JS::Identifier const& identifier, Operand valu } } +static Optional expression_identifier(Expression const& expression) +{ + if (expression.is_identifier()) { + auto const& identifier = static_cast(expression); + return identifier.string(); + } + + if (expression.is_member_expression()) { + auto const& member_expression = static_cast(expression); + StringBuilder builder; + + if (auto identifer = expression_identifier(member_expression.object()); identifer.has_value()) + builder.append(*identifer); + + if (auto identifer = expression_identifier(member_expression.property()); identifer.has_value()) { + if (!builder.is_empty()) + builder.append('.'); + builder.append(*identifer); + } + + return builder.to_byte_string(); + } + + return {}; +} + +Optional Generator::intern_identifier_for_expression(Expression const& expression) +{ + if (auto identifer = expression_identifier(expression); identifer.has_value()) + return intern_identifier(identifer.release_value()); + return {}; +} + void Generator::generate_scoped_jump(JumpType type) { TemporaryChange temp { m_current_unwind_context, m_current_unwind_context }; @@ -596,9 +632,9 @@ CodeGenerationErrorOr> Generator::emit_named_evaluation_if_ano return expression.generate_bytecode(*this, preferred_dst); } -void Generator::emit_get_by_id(Operand dst, Operand base, IdentifierTableIndex id) +void Generator::emit_get_by_id(Operand dst, Operand base, IdentifierTableIndex property_identifier, Optional base_identifier) { - emit(dst, base, id, m_next_property_lookup_cache++); + emit(dst, base, property_identifier, move(base_identifier), m_next_property_lookup_cache++); } void Generator::emit_get_by_id_with_this(Operand dst, Operand base, IdentifierTableIndex id, Operand this_value) diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.h b/Userland/Libraries/LibJS/Bytecode/Generator.h index 79d0302b297..1b9b81ae69c 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.h +++ b/Userland/Libraries/LibJS/Bytecode/Generator.h @@ -183,6 +183,8 @@ public: return m_identifier_table->insert(move(string)); } + Optional intern_identifier_for_expression(Expression const& expression); + bool is_in_generator_or_async_function() const { return m_enclosing_function_kind == FunctionKind::Async || m_enclosing_function_kind == FunctionKind::Generator || m_enclosing_function_kind == FunctionKind::AsyncGenerator; } bool is_in_generator_function() const { return m_enclosing_function_kind == FunctionKind::Generator || m_enclosing_function_kind == FunctionKind::AsyncGenerator; } bool is_in_async_function() const { return m_enclosing_function_kind == FunctionKind::Async || m_enclosing_function_kind == FunctionKind::AsyncGenerator; } @@ -247,7 +249,7 @@ public: m_boundaries.take_last(); } - void emit_get_by_id(Operand dst, Operand base, IdentifierTableIndex); + void emit_get_by_id(Operand dst, Operand base, IdentifierTableIndex property_identifier, Optional base_identifier = {}); void emit_get_by_id_with_this(Operand dst, Operand base, IdentifierTableIndex, Operand this_value); diff --git a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp index 96b512d7433..249a826975f 100644 --- a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp @@ -1083,9 +1083,13 @@ ThrowCompletionOr SetLocal::execute_impl(Bytecode::Interpreter&) const ThrowCompletionOr GetById::execute_impl(Bytecode::Interpreter& interpreter) const { + auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier); + auto const& property_identifier = interpreter.current_executable().get_identifier(m_property); + auto base_value = interpreter.get(base()); auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index]; - interpreter.set(dst(), TRY(get_by_id(interpreter.vm(), interpreter.current_executable().get_identifier(m_property), base_value, base_value, cache))); + + interpreter.set(dst(), TRY(get_by_id(interpreter.vm(), base_identifier, property_identifier, base_value, base_value, cache))); return {}; } @@ -1094,7 +1098,7 @@ ThrowCompletionOr GetByIdWithThis::execute_impl(Bytecode::Interpreter& int auto base_value = interpreter.get(m_base); auto this_value = interpreter.get(m_this_value); auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index]; - interpreter.set(dst(), TRY(get_by_id(interpreter.vm(), interpreter.current_executable().get_identifier(m_property), base_value, this_value, cache))); + interpreter.set(dst(), TRY(get_by_id(interpreter.vm(), {}, interpreter.current_executable().get_identifier(m_property), base_value, this_value, cache))); return {}; } @@ -1499,7 +1503,9 @@ ThrowCompletionOr Await::execute_impl(Bytecode::Interpreter& interpreter) ThrowCompletionOr GetByValue::execute_impl(Bytecode::Interpreter& interpreter) const { - interpreter.set(dst(), TRY(get_by_value(interpreter.vm(), interpreter.get(m_base), interpreter.get(m_property)))); + auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier); + + interpreter.set(dst(), TRY(get_by_value(interpreter.vm(), base_identifier, interpreter.get(m_base), interpreter.get(m_property)))); return {}; } diff --git a/Userland/Libraries/LibJS/Bytecode/Op.h b/Userland/Libraries/LibJS/Bytecode/Op.h index 4f79de991f9..a38d11180b1 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.h +++ b/Userland/Libraries/LibJS/Bytecode/Op.h @@ -629,11 +629,12 @@ private: class GetById final : public Instruction { public: - GetById(Operand dst, Operand base, IdentifierTableIndex property, u32 cache_index) + GetById(Operand dst, Operand base, IdentifierTableIndex property, Optional base_identifier, u32 cache_index) : Instruction(Type::GetById, sizeof(*this)) , m_dst(dst) , m_base(base) , m_property(property) + , m_base_identifier(move(base_identifier)) , m_cache_index(cache_index) { } @@ -650,6 +651,7 @@ private: Operand m_dst; Operand m_base; IdentifierTableIndex m_property; + Optional m_base_identifier; u32 m_cache_index { 0 }; }; @@ -874,11 +876,12 @@ private: class GetByValue final : public Instruction { public: - explicit GetByValue(Operand dst, Operand base, Operand property) + GetByValue(Operand dst, Operand base, Operand property, Optional base_identifier = {}) : Instruction(Type::GetByValue, sizeof(*this)) , m_dst(dst) , m_base(base) , m_property(property) + , m_base_identifier(move(base_identifier)) { } @@ -889,10 +892,13 @@ public: Operand base() const { return m_base; } Operand property() const { return m_property; } + Optional base_identifier(Bytecode::Interpreter const&) const; + private: Operand m_dst; Operand m_base; Operand m_property; + Optional m_base_identifier; }; class GetByValueWithThis final : public Instruction { diff --git a/Userland/Libraries/LibJS/Runtime/ErrorTypes.h b/Userland/Libraries/LibJS/Runtime/ErrorTypes.h index 828233085b5..5bfc6600705 100644 --- a/Userland/Libraries/LibJS/Runtime/ErrorTypes.h +++ b/Userland/Libraries/LibJS/Runtime/ErrorTypes.h @@ -302,6 +302,9 @@ M(ThisHasNotBeenInitialized, "|this| has not been initialized") \ M(ThisIsAlreadyInitialized, "|this| is already initialized") \ M(ToObjectNullOrUndefined, "ToObject on null or undefined") \ + M(ToObjectNullOrUndefinedWithName, "\"{}\" is {}") \ + M(ToObjectNullOrUndefinedWithProperty, "Cannot access property \"{}\" on {} object") \ + M(ToObjectNullOrUndefinedWithPropertyAndName, "Cannot access property \"{}\" on {} object \"{}\"") \ M(TopLevelVariableAlreadyDeclared, "Redeclaration of top level variable '{}'") \ M(ToPrimitiveReturnedObject, "Can't convert {} to primitive with hint \"{}\", its @@toPrimitive method returned an object") \ M(TypedArrayContentTypeMismatch, "Can't create {} from {}") \ diff --git a/Userland/Libraries/LibJS/Tests/null-or-undefined-access.js b/Userland/Libraries/LibJS/Tests/null-or-undefined-access.js new file mode 100644 index 00000000000..2c167ceee89 --- /dev/null +++ b/Userland/Libraries/LibJS/Tests/null-or-undefined-access.js @@ -0,0 +1,36 @@ +test("null/undefined object", () => { + [null, undefined].forEach(value => { + let foo = value; + + expect(() => { + foo.bar; + }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object "foo"`); + + expect(() => { + foo[0]; + }).toThrowWithMessage(TypeError, `Cannot access property "0" on ${value} object "foo"`); + }); +}); + +test("null/undefined object key", () => { + [null, undefined].forEach(value => { + let foo = { bar: value }; + + expect(() => { + foo.bar.baz; + }).toThrowWithMessage( + TypeError, + `Cannot access property "baz" on ${value} object "foo.bar"` + ); + }); +}); + +test("null/undefined array index", () => { + [null, undefined].forEach(value => { + let foo = [value]; + + expect(() => { + foo[0].bar; + }).toThrowWithMessage(TypeError, `Cannot access property "bar" on ${value} object`); + }); +});