Comments (9)
jdm
commented on June 26, 2024
I'm going to need to re-learn what the safety considerations are for
servo/components/script/dom/document.rs
Line 2658 in bd14541
from servo.
jdm
commented on June 26, 2024
Ok, so I think the issue we run into is #19307 (comment) - running arbitrary JS while the parser is active means that document.write can trigger a borrow error.
from servo.
jdm
commented on June 26, 2024
One possibility here might be to rewrite html5ever to use internal mutability (ie. Cell/RefCell) instead of requiring &mut self in the interfaces. That's the reason that we run into double-borrow errors, since the parser is re-entrant.
from servo.
jdm
commented on June 26, 2024
However, I think it's worth starting by looking at the callers of https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions and verifying if there is any case where we should be invoking that algorithm synchronously in Servo at the moment. If not, that would be an implementation error in Servo that we should address.
from servo.
jdm
commented on June 26, 2024
https://html.spec.whatwg.org/multipage/parsing.html#creating-and-inserting-nodes:insert-an-element-at-the-adjusted-insertion-location is the one that concerns me, since it invokes https://html.spec.whatwg.org/multipage/parsing.html#insert-an-element-at-the-adjusted-insertion-location which invokes https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions.
from servo.
jdm
commented on June 26, 2024
I can reproduce the panic with the following testcase:
<script>
class PopupInfo extends HTMLElement {
connectedCallback() {
dispatchEvent(new CustomEvent("custom"));
}
}
customElements.define("popup-info", PopupInfo);
</script>
<popup-info></popup-info>When I amend the testcase as follows, I have verified that we observe sync parsing/reaction behaviour in both Firefox and Chrome:
<script>
class PopupInfo extends HTMLElement {
connectedCallback() {
dispatchEvent(new CustomEvent("custom"));
}
}
customElements.define("popup-info", PopupInfo);
sync = true;
addEventListener("custom", () => {
console.log("handled event from connected callback with " + (sync ? "sync" : "async") + " parsing behaviour");
});
</script>
<popup-info></popup-info>
<script>
sync = false;
</script>from servo.
jdm
commented on June 26, 2024
Given that document.write can be called from the connectedCallback without triggering any exceptions, this looks like we need to support reentrant parsing. I think the easiest path forward is replacing all the &mut self from html5ever's Tokenizer interface with &self.
from servo.
jdm
commented on June 26, 2024
Additional data point: when I use the following testcase in Servo, I receive the following panic (which is what the assertion is meant to highlight).
<script>
class PopupInfo extends HTMLElement {
connectedCallback() {
document.write("hello");
}
}
customElements.define("popup-info", PopupInfo);
</script>
<popup-info></popup-info>already borrowed: BorrowMutError (thread Script(1,1), at components/script/dom/bindings/cell.rs:94)
0: backtrace::backtrace::libunwind::trace
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/backtrace-0.3.72/src/backtrace/libunwind.rs:116:5
backtrace::backtrace::trace_unsynchronized
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/backtrace-0.3.72/src/backtrace/mod.rs:66:5
1: <servoshell::backtrace::Print as core::fmt::Debug>::fmt
at /Users/jdm/src/servo/ports/servoshell/backtrace.rs:54:13
2: core::fmt::rt::Argument::fmt
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/fmt/rt.rs:142:9
core::fmt::write
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/fmt/mod.rs:1153:17
3: std::io::Write::write_fmt
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/io/mod.rs:1843:15
4: servoshell::backtrace::print
at /Users/jdm/src/servo/ports/servoshell/backtrace.rs:18:5
5: servoshell::main::{{closure}}
at /Users/jdm/src/servo/ports/servoshell/lib.rs:134:21
6: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/alloc/src/boxed.rs:2034:9
std::panicking::rust_panic_with_hook
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:783:13
7: std::panicking::begin_panic_handler::{{closure}}
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:657:13
8: std::sys_common::backtrace::__rust_end_short_backtrace
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/sys_common/backtrace.rs:171:18
9: rust_begin_unwind
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:645:5
10: core::panicking::panic_fmt
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:72:14
11: core::cell::panic_already_borrowed
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/cell.rs:770:5
12: core::cell::RefCell<T>::borrow_mut
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/cell.rs:1061:25
13: script::dom::bindings::cell::DomRefCell<T>::borrow_mut
at /Users/jdm/src/servo/components/script/dom/bindings/cell.rs:94:9
14: script::dom::servoparser::ServoParser::abort
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:403:10
15: script::dom::document::Document::abort
at /Users/jdm/src/servo/components/script/dom/document.rs:2638:13
16: <script::dom::document::Document as script::dom::bindings::codegen::Bindings::DocumentBinding::Document_Binding::DocumentMethods>::Open
at /Users/jdm/src/servo/components/script/dom/document.rs:5096:13
17: <script::dom::document::Document as script::dom::bindings::codegen::Bindings::DocumentBinding::Document_Binding::DocumentMethods>::Write
at /Users/jdm/src/servo/components/script/dom/document.rs:5199:17
18: script::dom::bindings::codegen::Bindings::DocumentBinding::Document_Binding::write::{{closure}}::{{closure}}
at /Users/jdm/src/servo/target/debug/build/script-f44eb85e52fb34de/out/Bindings/DocumentBinding.rs:3062:41
19: script::dom::bindings::codegen::Bindings::DocumentBinding::Document_Binding::write::{{closure}}
at /Users/jdm/src/servo/target/debug/build/script-f44eb85e52fb34de/out/Bindings/DocumentBinding.rs:3038:33
20: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &mut F>::call_once
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/ops/function.rs:305:13
21: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panic/unwind_safe.rs:272:9
22: std::panicking::try::do_call
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:552:40
23: ___rust_try
24: std::panicking::try
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:516:19
25: std::panic::catch_unwind
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panic.rs:146:14
26: mozjs::panic::wrap_panic
at /Users/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/8603cbf/mozjs/src/panic.rs:22:11
27: script::dom::bindings::codegen::Bindings::DocumentBinding::Document_Binding::write
at /Users/jdm/src/servo/target/debug/build/script-f44eb85e52fb34de/out/Bindings/DocumentBinding.rs:3038:5
28: CallJitMethodOp
at /Users/runner/work/mozjs/mozjs/mozjs-sys/src/jsglue.cpp:616:10
29: script::dom::bindings::utils::generic_call
at /Users/jdm/src/servo/components/script/dom/bindings/utils.rs:520:5
30: script::dom::bindings::utils::generic_method
at /Users/jdm/src/servo/components/script/dom/bindings/utils.rs:536:5
31: __ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructENS_10CallReasonE
32: __ZN2js9InterpretEP9JSContextRNS_8RunStateE
33: __ZN2js9RunScriptEP9JSContextRNS_8RunStateE
34: __ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructENS_10CallReasonE
35: __ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EENS_10CallReasonE
36: __Z20JS_CallFunctionValueP9JSContextN2JS6HandleIP8JSObjectEENS2_INS1_5ValueEEERKNS1_16HandleValueArrayENS1_13MutableHandleIS6_EE
37: mozjs::rust::wrappers::JS_CallFunctionValue
at /Users/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/8603cbf/mozjs/src/rust.rs:1080:51
38: script::dom::bindings::codegen::Bindings::FunctionBinding::Function::Call
at /Users/jdm/src/servo/target/debug/build/script-f44eb85e52fb34de/out/Bindings/FunctionBinding.rs:61:18
39: script::dom::bindings::codegen::Bindings::FunctionBinding::Function::Call_
at /Users/jdm/src/servo/target/debug/build/script-f44eb85e52fb34de/out/Bindings/FunctionBinding.rs:33:18
40: script::dom::customelementregistry::CustomElementReaction::invoke
at /Users/jdm/src/servo/components/script/dom/customelementregistry.rs:986:25
41: script::dom::element::Element::invoke_reactions
at /Users/jdm/src/servo/components/script/dom/element.rs:416:17
42: script::dom::customelementregistry::ElementQueue::invoke_reactions
at /Users/jdm/src/servo/components/script/dom/customelementregistry.rs:1232:13
43: script::dom::customelementregistry::CustomElementReactionStack::pop_current_element_queue
at /Users/jdm/src/servo/components/script/dom/customelementregistry.rs:1036:13
44: script::script_thread::ScriptThread::pop_current_element_queue::{{closure}}
at /Users/jdm/src/servo/components/script/script_thread.rs:1211:17
45: std::thread::local::LocalKey<T>::try_with
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/thread/local.rs:284:16
46: std::thread::local::LocalKey<T>::with
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/thread/local.rs:260:9
47: script::script_thread::ScriptThread::pop_current_element_queue
at /Users/jdm/src/servo/components/script/script_thread.rs:1208:9
48: script::dom::servoparser::insert
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:1041:17
49: <script::dom::servoparser::Sink as markup5ever::interface::tree_builder::TreeSink>::append
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:1206:9
50: html5ever::tree_builder::TreeBuilder<Handle,Sink>::insert_at
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tree_builder/mod.rs:431:34
51: html5ever::tree_builder::TreeBuilder<Handle,Sink>::insert_element
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tree_builder/mod.rs:1318:9
52: html5ever::tree_builder::TreeBuilder<Handle,Sink>::insert_element_for
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tree_builder/mod.rs:1329:9
53: html5ever::tree_builder::TreeBuilder<Handle,Sink>::step
at /Users/jdm/src/servo/target/debug/build/html5ever-d05ea9841cf9e4c6/out/rules.rs:774:36
54: html5ever::tree_builder::TreeBuilder<Handle,Sink>::process_to_completion
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tree_builder/mod.rs:335:17
55: <html5ever::tree_builder::TreeBuilder<Handle,Sink> as html5ever::tokenizer::interface::TokenSink>::process_token
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tree_builder/mod.rs:522:9
56: html5ever::tokenizer::Tokenizer<Sink>::process_token
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tokenizer/mod.rs:237:13
57: html5ever::tokenizer::Tokenizer<Sink>::emit_current_tag
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tokenizer/mod.rs:443:15
58: html5ever::tokenizer::Tokenizer<Sink>::step
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tokenizer/mod.rs:810:28
59: html5ever::tokenizer::Tokenizer<Sink>::run
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tokenizer/mod.rs:373:23
60: html5ever::tokenizer::Tokenizer<Sink>::feed
at /Users/jdm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/html5ever-0.27.0/src/tokenizer/mod.rs:224:9
61: script::dom::servoparser::html::Tokenizer::feed
at /Users/jdm/src/servo/components/script/dom/servoparser/html.rs:82:15
62: script::dom::servoparser::Tokenizer::feed
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:703:51
63: script::dom::servoparser::ServoParser::do_parse_sync::{{closure}}
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:568:35
64: script::dom::servoparser::ServoParser::tokenize
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:606:32
65: script::dom::servoparser::ServoParser::do_parse_sync
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:568:9
66: script::dom::servoparser::ServoParser::parse_sync::{{closure}}
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:550:16
67: profile_traits::time::profile
at /Users/jdm/src/servo/components/shared/profile/time.rs:147:15
68: script::dom::servoparser::ServoParser::parse_sync
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:542:9
69: script::dom::servoparser::ServoParser::parse_bytes_chunk
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:593:13
70: <script::dom::servoparser::ParserContext as net_traits::FetchResponseListener>::process_response_chunk
at /Users/jdm/src/servo/components/script/dom/servoparser/mod.rs:940:9
71: script::script_thread::ScriptThread::handle_fetch_chunk
at /Users/jdm/src/servo/components/script/script_thread.rs:3972:13
72: script::script_thread::ScriptThread::handle_msg_from_constellation
at /Users/jdm/src/servo/components/script/script_thread.rs:2227:25
73: script::script_thread::ScriptThread::handle_msgs::{{closure}}
at /Users/jdm/src/servo/components/script/script_thread.rs:1941:53
74: script::script_thread::ScriptThread::profile_event
at /Users/jdm/src/servo/components/script/script_thread.rs:2195:13
75: script::script_thread::ScriptThread::handle_msgs
at /Users/jdm/src/servo/components/script/script_thread.rs:1935:26
76: script::script_thread::ScriptThread::start
at /Users/jdm/src/servo/components/script/script_thread.rs:1448:15
77: <script::script_thread::ScriptThread as script_layout_interface::ScriptThreadFactory>::create::{{closure}}::{{closure}}
at /Users/jdm/src/servo/components/script/script_thread.rs:846:25
78: profile_traits::mem::ProfilerChan::run_with_memory_reporting
at /Users/jdm/src/servo/components/shared/profile/mem.rs:91:9
79: <script::script_thread::ScriptThread as script_layout_interface::ScriptThreadFactory>::create::{{closure}}
at /Users/jdm/src/servo/components/script/script_thread.rs:844:17
80: std::sys_common::backtrace::__rust_begin_short_backtrace
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/sys_common/backtrace.rs:155:18
81: std::thread::Builder::spawn_unchecked_::{{closure}}::{{closure}}
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/thread/mod.rs:528:17
82: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panic/unwind_safe.rs:272:9
83: std::panicking::try::do_call
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:552:40
84: ___rust_try
85: std::panicking::try
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:516:19
86: std::panic::catch_unwind
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panic.rs:146:14
std::thread::Builder::spawn_unchecked_::{{closure}}
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/thread/mod.rs:527:30
87: core::ops::function::FnOnce::call_once{{vtable.shim}}
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/ops/function.rs:250:5
88: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/alloc/src/boxed.rs:2020:9
<alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/alloc/src/boxed.rs:2020:9
std::sys::pal::unix::thread::Thread::new::thread_start
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/sys/pal/unix/thread.rs:108:17
89: __pthread_joiner_wake
from servo.
jdm
commented on June 26, 2024
I think it's not just the treebuilder sink and tokenizer interfaces that need to change—BufferQueue also needs to be converted to use internal mutability instead of &mut self, because we use extended mutable borrows of the various input queues across each parser invocation right now, so that's not safe for reentrancy.
The alternative is to change the tokenizer interface to accept a trait that can obtain a &mut BufferQueue on demand, which would allow passing the &DomRefCell<BufferQueue> and only borrowing it when necessary.
from servo.
Related Issues (20)
- globalscope should hold weakref to `GPUDevice`
- Dependabot merging PRs that fail servo-tidy HOT 1
- Carefully consider security of local directory listings
- window.HTMLDocument is not implemented
- Update Servo's Android apps `org` name HOT 1
- Update the rust edition to 2021
- Improve mach build or documentation for android HOT 1
- Fix DOMRect calc in ResizeObserverEntry HOT 4
- test-tidy: TypeError: 'NoneType' object is not iterable HOT 2
- `shell.nix` broken after migrating crown to use workspace root's package info
- Investigate GPUDevice not being GCed if configured with context on page reload. HOT 5
- Servo panics when launched in multiprocess mode. HOT 1
- Separate build/run time deps HOT 2
- Intermittent PASS in /html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-reload-location-reload.html
- Intermittent FAIL in /html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-nav-window-open.html
- warning: unknown lint: `crown_is_not_used` HOT 3
- script: Safe way to create `Function` HOT 2
- Intermittent FAIL in /html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-nav-location-replace.html
- Intermittent FAIL in /html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-nav-form-submit.html
- Intermittent TIMEOUT in /html/semantics/document-metadata/the-base-element/base_target_does_not_affect_location_assignment.html
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from servo.