Skip to content

Implement ToggleEvent.source and the request-close command #5002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 7, 2025

Conversation

Gingeh
Copy link
Contributor

@Gingeh Gingeh commented Jun 4, 2025

See commit descriptions for spec PRs

@Gingeh Gingeh force-pushed the toggle-source branch 4 times, most recently from 6113087 to fe2aa5c Compare June 5, 2025 06:10
Copy link
Contributor

@trflynn89 trflynn89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is very unhappy.

@Gingeh
Copy link
Contributor Author

Gingeh commented Jun 5, 2025

CI is very unhappy.

yeah no clue what's going wrong, the test passes perfectly on my machine

@Gingeh Gingeh force-pushed the toggle-source branch 2 times, most recently from d796998 to e582666 Compare June 5, 2025 12:25
@tcl3
Copy link
Member

tcl3 commented Jun 5, 2025

I get this crash when running headless-browser -f Text/input/wpt-import/html/semantics/popovers/popover-toggle-source.tentative.html with sanitizers enabled:

/home/tim/repos/ladybird/Libraries/LibGC/Ptr.h:176:17: runtime error: reference binding to null pointer of type 'struct Element'
    #0 0x77630b22934d in GC::Ptr<Web::DOM::Element>::operator*() const /home/tim/repos/ladybird/Libraries/LibGC/Ptr.h:176
    #1 0x77630b22934d in Web::Bindings::ToggleEventPrototype::source_getter(JS::VM&) /home/tim/repos/ladybird/Build/sanitizers/Lagom/Libraries/LibWeb/Bindings/ToggleEventPrototype.cpp:192
    #2 0x7762d963e764 in AK::Function<JS::ThrowCompletionOr<JS::Value> (JS::VM&)>::operator()(JS::VM&) const /home/tim/repos/ladybird/AK/Function.h:148
    #3 0x7762d963e764 in JS::NativeFunction::call() /home/tim/repos/ladybird/Libraries/LibJS/Runtime/NativeFunction.cpp:234
    #4 0x7762d96513df in JS::NativeFunction::internal_call(JS::ExecutionContext&, JS::Value) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/NativeFunction.cpp:163
    #5 0x7762d84f9d1b in JS::call_impl(JS::VM&, JS::FunctionObject&, JS::Value, AK::Span<JS::Value const>) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/AbstractOperations.cpp:96
    #6 0x7762d96b93e1 in JS::ThrowCompletionOr<JS::Value> JS::call<>(JS::VM&, JS::FunctionObject&, JS::Value) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/AbstractOperations.h:131
    #7 0x7762d96b93e1 in JS::Object::internal_get(JS::PropertyKey const&, JS::Value, JS::CacheablePropertyMetadata*, JS::Object::PropertyLookupPhase) const /home/tim/repos/ladybird/Libraries/LibJS/Runtime/Object.cpp:954
    #8 0x7762d96b902a in JS::Object::internal_get(JS::PropertyKey const&, JS::Value, JS::CacheablePropertyMetadata*, JS::Object::PropertyLookupPhase) const /home/tim/repos/ladybird/Libraries/LibJS/Runtime/Object.cpp:911
    #9 0x7762d7e8edd4 in JS::ThrowCompletionOr<JS::Value> JS::Bytecode::get_by_id<(JS::Bytecode::GetByIdMode)0>(JS::VM&, AK::Optional<JS::Bytecode::IdentifierTableIndex>, JS::Bytecode::IdentifierTableIndex, JS::Value, JS::Value, JS::Bytecode::PropertyLookupCache&, JS::Bytecode::Executable const&) /home/tim/repos/ladybird/Libraries/LibJS/Bytecode/Interpreter.cpp:1024
    #10 0x7762d7d43f5d in JS::Bytecode::Op::GetById::execute_impl(JS::Bytecode::Interpreter&) const /home/tim/repos/ladybird/Libraries/LibJS/Bytecode/Interpreter.cpp:2632
    #11 0x7762d7d7c76c in JS::Bytecode::Interpreter::run_bytecode(unsigned long) /home/tim/repos/ladybird/Libraries/LibJS/Bytecode/Interpreter.cpp:611
    #12 0x7762d7d9de4b in JS::Bytecode::Interpreter::run_executable(JS::Bytecode::Executable&, AK::Optional<unsigned long>, JS::Value) /home/tim/repos/ladybird/Libraries/LibJS/Bytecode/Interpreter.cpp:753
    #13 0x7762d8d85ea5 in JS::GeneratorObject::execute(JS::VM&, JS::Completion const&) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/GeneratorObject.cpp:110
    #14 0x7762d8d90293 in JS::GeneratorObject::resume(JS::VM&, JS::Value, AK::Optional<AK::StringView> const&) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/GeneratorObject.cpp:158
    #15 0x7762d87b915b in JS::AsyncFunctionDriverWrapper::continue_async_execution(JS::VM&, JS::Value, bool) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp:127
    #16 0x7762d87c2e70 in operator() /home/tim/repos/ladybird/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp:66
    #17 0x7762d87c2e70 in call /home/tim/repos/ladybird/AK/Function.h:225
    #18 0x7762d963e764 in AK::Function<JS::ThrowCompletionOr<JS::Value> (JS::VM&)>::operator()(JS::VM&) const /home/tim/repos/ladybird/AK/Function.h:148
    #19 0x7762d963e764 in JS::NativeFunction::call() /home/tim/repos/ladybird/Libraries/LibJS/Runtime/NativeFunction.cpp:234
    #20 0x7762d96513df in JS::NativeFunction::internal_call(JS::ExecutionContext&, JS::Value) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/NativeFunction.cpp:163
    #21 0x7762d84f9d1b in JS::call_impl(JS::VM&, JS::FunctionObject&, JS::Value, AK::Span<JS::Value const>) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/AbstractOperations.cpp:96
    #22 0x7762fea1ca38 in JS::call(JS::VM&, JS::FunctionObject&, JS::Value, AK::Span<JS::Value const>) /home/tim/repos/ladybird/Libraries/LibJS/Runtime/AbstractOperations.h:114
    #23 0x7762fea1ca38 in operator() /home/tim/repos/ladybird/Libraries/LibWeb/Bindings/MainThreadVM.cpp:215
    #24 0x7762fea1ca38 in call /home/tim/repos/ladybird/AK/Function.h:225
    #25 0x7762d9890f8f in AK::Function<JS::ThrowCompletionOr<JS::Value> (JS::JobCallback&, JS::Value, AK::Span<JS::Value const>)>::operator()(JS::JobCallback&, JS::Value, AK::Span<JS::Value const>) const /home/tim/repos/ladybird/AK/Function.h:148

I was able to "fix" the issue by making ToggleEvent.source nullable in the IDL file.

@Gingeh
Copy link
Contributor Author

Gingeh commented Jun 6, 2025

that sounds like a correct fix to me, because the field is nullable (spec bug?)

@tcl3
Copy link
Member

tcl3 commented Jun 6, 2025

that sounds like a correct fix to me, because the field is nullable

Ah, I'd only did the quickest thing I could think of to resolve the problem. My initial quick reading of the spec led me to believe that source shouldn't ever be null. Looking closer I think that it definitely can be, so I agree that changing this field to be nullable would be correct.

This would also match the definition of CommandEvent.source, which is currently nullable.

@Gingeh
Copy link
Contributor Author

Gingeh commented Jun 6, 2025

Should I open a spec issue for the ToggleEvent.source idl definition not being nullable?

@tcl3
Copy link
Member

tcl3 commented Jun 6, 2025

Should I open a spec issue for the ToggleEvent.source idl definition not being nullable?

I think that would be a good idea - then you could add a comment that references the issue to make it clear that we are diverging from the current spec.

@Gingeh
Copy link
Contributor Author

Gingeh commented Jun 6, 2025

Good news: The popover source test doesn't crash anymore!
Bad news: Crashed: Text/input/navigation/run-script-before-iframe-initial-navigation.html flake?

@tcl3
Copy link
Member

tcl3 commented Jun 6, 2025

I assume it's a flake? I can't reproduce it locally and the crash only occurs on one of the CI jobs. I've restarted the CI job to see if it happens again.

@Gingeh
Copy link
Contributor Author

Gingeh commented Jun 6, 2025

Made a spec issue and added a link

@Gingeh
Copy link
Contributor Author

Gingeh commented Jun 6, 2025

yay everything passed!

@Gingeh Gingeh requested a review from trflynn89 June 6, 2025 05:25
@Gingeh
Copy link
Contributor Author

Gingeh commented Jun 7, 2025

whatwg/html#11360 has already been resolved, so I've removed the corresponding AD-HOC comment

@tcl3 tcl3 merged commit 9ef9f8d into LadybirdBrowser:master Jun 7, 2025
7 checks passed
@Gingeh Gingeh deleted the toggle-source branch June 7, 2025 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants