script: Start implementation of shared attribute processing for iframes#42254
Merged
TimvdLippe merged 1 commit intoservo:mainfrom Feb 7, 2026
Merged
Conversation
servo-highfive
added
the
S-awaiting-review
|
🔨 Triggering try run (#21513151315) for Linux (WPT) |
|
|
TimvdLippe
force-pushed
the
start-implementation-shared-loading-steps
branch
from
561de28 to
1d92643
Compare
|
🔨 Triggering try run (#21518061886) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21518061886): Flaky unexpected result (34)
Stable unexpected results that are known to be intermittent (35)
Stable unexpected results (52)
|
|
|
TimvdLippe
force-pushed
the
start-implementation-shared-loading-steps
branch
from
1d92643 to
419e2bf
Compare
Contributor
Author
|
@jdm I got pretty close with this one, but would appreciate your debugging skills again. These I get the sense that it is related to how pipelines are stored for iframes. I think it reuses iframe pipelines and then they get stuck in an invalid state. However, this is purely guess and I don't really know where I would need to start investigating. Overall the test improvements look hopeful that this is another step in the right direction. So I think we are just missing one small part of the puzzle to make these test not start to fail. |
TimvdLippe
commented
Jan 30, 2026
| [load & pageshow events do not fire on contentWindow of <iframe> element created with src='about:blank#foo'] | ||
| expected: FAIL | ||
|
|
||
| [load & pageshow events do not fire on contentWindow of <iframe> element created with src=''] |
Contributor
Author
There was a problem hiding this comment.
I don't think we are firing pageshow events yet when finalizing a document, so these are failing just like the other ones.
Member
There was a problem hiding this comment.
The page show event is fired in components/script/dom/document.rs
I think the main point is firing the load event on the document, versus the load event on the iframe element in the load steps.
So when a blank frame is created, the load should fire on the frame and return; we should not continue with a navigation. But what makes this hard in servo is that we always create a first blank frame, and then navigate in most cases. The way the spec is written is that in such a case we should only create the frame and not navigate it. But last time I tried, not navigating it results in all sorts of timeouts.
Contributor
Author
There was a problem hiding this comment.
I think the note at https://html.spec.whatwg.org/multipage/document-lifecycle.html#loading-documents:create-a-new-child-navigable explains what's going on here.
Turns out that pageshow isn't the issue, load is. We have a frame_element() that is Some(iframe), whereas it should be None given it is about:blank. Still, I think this the case for the other subsets in this file as well, which is why I think it is acceptable for the new failures. Once we fix frame_element() to be spec-compliant, I expect all subtsets in this file to start passing.
Member
There was a problem hiding this comment.
Your're right that in the intial about blank case, the load event steps are fired earlier as part of the synchronous steps you now implemented.
But the tests here is about the event fired not on the iframe but on the window inside the iframe. The problem is that when we create the nested BC for the initial about blank, we still do the whole document load thing.
still not completely sure why they started failing now, but anyway the fact they were passing was sort of random I would say.
jdm
reviewed
Feb 1, 2026
TimvdLippe
force-pushed
the
start-implementation-shared-loading-steps
branch
from
419e2bf to
c01a792
Compare
|
🔨 Triggering try run (#21623497132) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21623497132): Flaky unexpected result (23)
Stable unexpected results that are known to be intermittent (38)
Stable unexpected results (9)
|
|
|
Contributor
Author
|
Okay, I figured out why The reason is that servo/components/script/dom/html/htmlformelement.rs Lines 1064 to 1073 in 1abe258 frame_element() is None. We encountered this before, where this is None, even though it is a same-origin element.
It isn't related to my changes, but my changes uncover the issue. The reason is that with my changes, the Another issue with this patch: the load events can run after the browsing context is destroyed, after which self.about_blank_pipeline_id.get().is_some_and(|about_blank_pipeline_id| self.pending_pipeline_id.get() == Some(about_blank_pipeline_id))This is required for debugging the tests, but doesn't fix them per the |
Contributor
Author
|
Here are my logs of a timeout on the fourth one (the first three logs are the expected values): DetailsDebugging diff: Detailsdiff --git a/components/script/dom/html/htmliframeelement.rs b/components/script/dom/html/htmliframeelement.rs
index 203c1934598..f6e4ba3859d 100644
--- a/components/script/dom/html/htmliframeelement.rs
+++ b/components/script/dom/html/htmliframeelement.rs
@@ -137,6 +137,7 @@ impl HTMLIFrameElement {
history_handling: NavigationHistoryBehavior,
can_gc: CanGc,
) {
+ println!("Resetting for navigation");
// In case we fired a synchronous load event, but navigate away
// in the event listener of that event, then we should still
// fire a second asynchronous load event when that navigation
@@ -287,7 +288,7 @@ impl HTMLIFrameElement {
/// This initial synchronous load should have no noticeable effect in script.
/// See the note in `iframe_load_event_steps`.
pub(crate) fn is_initial_blank_document(&self) -> bool {
- self.pending_pipeline_id.get() == self.about_blank_pipeline_id.get()
+ self.about_blank_pipeline_id.get().is_some_and(|about_blank_pipeline_id| self.pending_pipeline_id.get() == Some(about_blank_pipeline_id))
}
/// <https://html.spec.whatwg.org/multipage/#process-the-iframe-attributes>
@@ -344,12 +345,15 @@ impl HTMLIFrameElement {
// for iframe and frame elements given element and initialInsertion.
let Some(url) = self.shared_attribute_processing_steps_for_iframe_and_frame_elements()
else {
+ println!("No url computed");
// Step 2.2. If url is null, then return.
return;
};
+ println!("Loading url {}", url);
// Step 2.3. If url matches about:blank and initialInsertion is true, then:
if url.matches_about_blank() && mode == ProcessingMode::FirstTime {
+ println!("Synchronously firing load event");
// We should **not** send a load event in `iframe_load_event_steps`.
self.already_fired_synchronous_load_event.set(true);
// Step 2.3.1. Run the iframe load event steps given element.
@@ -490,6 +494,8 @@ impl HTMLIFrameElement {
self.about_blank_pipeline_id.set(None);
self.webview_id.set(None);
self.browsing_context_id.set(None);
+ self.already_fired_synchronous_load_event.set(false);
+ self.pending_navigation.set(false);
}
pub(crate) fn update_pipeline_id(
@@ -594,6 +600,7 @@ impl HTMLIFrameElement {
/// This is used to ignore the async load event steps for
/// the initial blank document if those haven't run yet.
pub(crate) fn note_pending_navigation(&self) {
+ println!("Form noting pending navigation, was: {}", self.pending_navigation.get());
self.pending_navigation.set(true);
}
@@ -634,21 +641,23 @@ impl HTMLIFrameElement {
//
// TODO: run these step synchronously as part of processing the iframe attributes.
let should_fire_event = if self.is_initial_blank_document() {
+ println!("Is initial about blank document");
// If this is the initial blank doc:
// do not fire if there is a pending navigation,
// or if the iframe has an src.
- !self.pending_navigation.get() &&
+ !self.pending_navigation.replace(false) &&
!self.upcast::<Element>().has_attribute(&local_name!("src"))
} else {
// If this is not the initial blank doc:
// do not fire if there is a pending navigation.
- !self.pending_navigation.get()
+ !self.pending_navigation.replace(false)
};
// If we already fired a synchronous load event, we shouldn't fire another
// one in this method.
- let should_fire_event =
+ let should_really_fire_event =
!self.already_fired_synchronous_load_event.replace(false) && should_fire_event;
- if should_fire_event {
+ println!("Should fire event: {} really: {}", should_fire_event, should_really_fire_event);
+ if should_really_fire_event {
// Step 6. Fire an event named load at element.
self.upcast::<EventTarget>()
.fire_event(atom!("load"), can_gc);
@@ -1056,7 +1065,7 @@ impl<'a> IframeContext<'a> {
element,
url: element
.shared_attribute_processing_steps_for_iframe_and_frame_elements()
- .unwrap_or_else("about:blank".into()),
+ .unwrap_or_else(|| ServoUrl::parse("about:blank").unwrap()),
}
}
} |
Member
|
The reason for the timeouts is very silly—each subtest creates an iframe and gives it a name, then uses the name to target that iframe when submitting the form, but the names are not unique. The test that times out tries to submit in an iframe that has already been cleaned up, instead of the one created for that subtest. The following diff: diff --git a/tests/wpt/tests/FileAPI/support/send-file-form-helper.js b/tests/wpt/tests/FileAPI/support/send-file-form-helper.js
index d6adf21ec33..39784cd98fb 100644
--- a/tests/wpt/tests/FileAPI/support/send-file-form-helper.js
+++ b/tests/wpt/tests/FileAPI/support/send-file-form-helper.js
@@ -114,6 +114,8 @@ const kTestFallbackXUserDefined = kTestChars.replace(
(x) => `&#${x.codePointAt(0)};`,
);
+let frameSuffix = 0;
+
// formPostFileUploadTest - verifies multipart upload structure and
// numeric character reference replacement for filenames, field names,
// and field values using form submission.
@@ -152,7 +154,7 @@ const formPostFileUploadTest = ({
}
const formTargetFrame = Object.assign(document.createElement('iframe'), {
- name: 'formtargetframe',
+ name: 'formtargetframe' + frameSuffix++,
});
document.body.append(formTargetFrame);
testCase.add_cleanup(() => {yields the following results: |
Member
|
We don't implement the target selection for named targets according to the spec. https://html.spec.whatwg.org/multipage/document-sequences.html#find-a-navigable-by-target-name has a bunch of steps that describe a tree-order based lookup, but we just invoke servo/components/script/script_window_proxies.rs Lines 37 to 47 in 7bf00ec filed #42343. |
Member
|
Oh, there's another reason why this occurs: our map of window proxies never has elements removed from it: #15258 |
Member
|
#42344 causes the same tests to pass without any changes to the tests. |
TimvdLippe
commented
Feb 6, 2026
| [open-url-multi-window-6.htm] | ||
| [XMLHttpRequest: open() in document that is not fully active (but may be active) should throw] | ||
| expected: FAIL | ||
| expected: CRASH |
Contributor
Author
There was a problem hiding this comment.
Seems like there is something funky going on with XMLHttpRequest and documents that aren't fully active. Given that the test was already failing, I don't think this is a big issue and we need to investigate further in a follow-up PR, given that the test fails in all browsers: https://wpt.fyi/results/xhr/open-url-multi-window-6.htm?label=master&product=chrome%5Bexperimental%5D&product=edge%5Bexperimental%5D&product=firefox%5Bexperimental%5D&product=safari%5Bexperimental%5D&product=servo&aligned
Details
pid:14349 assertion `left == right` failed: Attempt to use script or layout while DOM not in a stable state
0:01.64 pid:14349 left: 1
0:01.64 pid:14349 right: 0 (thread Script#4, at components/script/dom/document.rs:3984)
0:04.49 pid:14349 0: servoshell::backtrace::print
0:04.49 pid:14349 at /home/tvanderlippe/Projects/servo/ports/servoshell/backtrace.rs:18:5
0:04.49 pid:14349 1: servoshell::panic_hook::panic_hook
0:04.49 pid:14349 at /home/tvanderlippe/Projects/servo/ports/servoshell/panic_hook.rs:40:17
0:04.54 pid:14349 2: core::ops::function::Fn::call
0:04.54 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:79:5
0:04.61 pid:14349 3: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
0:04.61 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/alloc/src/boxed.rs:1999:9
0:04.61 pid:14349 std::panicking::panic_with_hook
0:04.61 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/std/src/panicking.rs:842:13
0:04.61 pid:14349 4: std::panicking::panic_handler::{{closure}}
0:04.61 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/std/src/panicking.rs:707:13
0:04.61 pid:14349 5: std::sys::backtrace::__rust_end_short_backtrace
0:04.61 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/std/src/sys/backtrace.rs:174:18
0:04.61 pid:14349 6: __rustc::rust_begin_unwind
0:04.61 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/std/src/panicking.rs:698:5
0:04.63 pid:14349 7: core::panicking::panic_fmt
0:04.63 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/core/src/panicking.rs:75:14
0:04.63 pid:14349 8: core::panicking::assert_failed_inner
0:04.63 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/core/src/panicking.rs:434:23
0:04.63 pid:14349 9: core::panicking::assert_failed
0:04.63 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:394:5
0:04.68 pid:14349 10: script::dom::document::Document::ensure_safe_to_run_script_or_layout
0:04.68 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/document.rs:3984:9
0:04.68 pid:14349 11: <script::dom::document::Document as script_bindings::interfaces::DocumentHelpers>::ensure_safe_to_run_script_or_layout
0:04.68 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/document.rs:6733:9
0:04.72 pid:14349 12: script_bindings::callback::CallSetup<D>::new
0:04.72 pid:14349 at /home/tvanderlippe/Projects/servo/components/script_bindings/callback.rs:282:31
0:04.77 pid:14349 13: script_bindings::codegen::GenericBindings::EventHandlerBinding::EventHandlerNonNull<D>::Call_
0:04.77 pid:14349 at /home/tvanderlippe/Projects/servo/target/debug/build/script_bindings-a9454c66c35db46d/out/Bindings/EventHandlerBinding.rs:35:17
0:04.82 pid:14349 14: script::dom::eventtarget::CompiledEventListener::call_or_handle_event
0:04.82 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/eventtarget.rs:401:49
0:04.87 pid:14349 15: script::dom::event::inner_invoke
0:04.87 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/event.rs:1371:27
0:04.87 pid:14349 16: script::dom::event::invoke
0:04.87 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/event.rs:1254:17
0:04.87 pid:14349 17: script::dom::event::Event::dispatch
0:04.87 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/event.rs:588:17
0:04.87 pid:14349 18: script::dom::eventtarget::EventTarget::dispatch_event
0:04.87 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/eventtarget.rs:556:15
0:04.87 pid:14349 19: script::dom::event::Event::fire
0:04.87 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/event.rs:731:16
0:04.87 pid:14349 20: script::dom::eventtarget::EventTarget::fire_event_with_params
0:04.87 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/eventtarget.rs:941:15
0:04.87 pid:14349 21: script::dom::eventtarget::EventTarget::fire_event
0:04.87 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/eventtarget.rs:888:14
0:04.92 pid:14349 22: script::dom::html::htmliframeelement::HTMLIFrameElement::process_the_iframe_attributes
0:04.92 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/html/htmliframeelement.rs:363:18
0:04.92 pid:14349 23: <script::dom::html::htmliframeelement::HTMLIFrameElement as script::dom::virtualmethods::VirtualMethods>::post_connection_steps
0:04.92 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/html/htmliframeelement.rs:1024:14
0:04.97 pid:14349 24: script::dom::node::Node::insert::{{closure}}
0:04.97 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/node.rs:2723:39
0:05.44 pid:14349 25: core::ops::function::FnOnce::call_once
0:05.44 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
0:05.44 pid:14349 26: <script::dom::node::Node::insert::PostConnectionSteps<F> as script::task::NonSendTaskOnce>::run_once
0:05.44 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/task.rs:81:17
0:05.44 pid:14349 27: <T as script::task::NonSendTaskBox>::run_box
0:05.44 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/task.rs:141:14
0:05.44 pid:14349 28: script::dom::document::Document::remove_script_and_layout_blocker
0:05.44 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/document.rs:3972:18
0:05.48 pid:14349 29: script::dom::node::Node::insert
0:05.48 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/node.rs:2728:25
0:05.48 pid:14349 30: script::dom::node::Node::pre_insert
0:05.48 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/node.rs:2528:9
0:05.48 pid:14349 31: <script::dom::node::Node as script_bindings::codegen::GenericBindings::NodeBinding::Node_Binding::NodeMethods<script::dom::bindings::codegen::DomTypeHolder::DomTypeHolder>>::AppendChild
0:05.48 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/node.rs:3575:9
0:05.53 pid:14349 32: script_bindings::codegen::GenericBindings::NodeBinding::Node_Binding::appendChild::{{closure}}::{{closure}}
0:05.53 pid:14349 at /home/tvanderlippe/Projects/servo/target/debug/build/script_bindings-a9454c66c35db46d/out/Bindings/NodeBinding.rs:1678:60
0:05.53 pid:14349 33: script_bindings::codegen::GenericBindings::NodeBinding::Node_Binding::appendChild::{{closure}}
0:05.53 pid:14349 at /home/tvanderlippe/Projects/servo/target/debug/build/script_bindings-a9454c66c35db46d/out/Bindings/NodeBinding.rs:1651:33
0:05.53 pid:14349 34: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &mut F>::call_once
0:05.53 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:310:21
0:05.53 pid:14349 35: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
0:05.53 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:274:9
0:05.53 pid:14349 36: std::panicking::catch_unwind::do_call
0:05.53 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:590:40
0:05.53 pid:14349 37: __rust_try
0:05.54 pid:14349 38: std::panicking::catch_unwind
0:05.54 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:553:19
0:05.54 pid:14349 std::panic::catch_unwind
0:05.54 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:359:14
0:05.54 pid:14349 39: mozjs::panic::wrap_panic
0:05.54 pid:14349 at /home/tvanderlippe/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/mozjs-0.14.8/src/panic.rs:22:11
0:05.54 pid:14349 40: script_bindings::codegen::GenericBindings::NodeBinding::Node_Binding::appendChild
0:05.54 pid:14349 at /home/tvanderlippe/Projects/servo/target/debug/build/script_bindings-a9454c66c35db46d/out/Bindings/NodeBinding.rs:1651:5
0:05.56 pid:14349 41: CallJitMethodOp
0:05.56 pid:14349 at /home/runner/work/mozjs/mozjs/mozjs-sys/./src/jsglue.cpp:682:22
0:05.57 pid:14349 42: script_bindings::utils::generic_call
0:05.57 pid:14349 at /home/tvanderlippe/Projects/servo/components/script_bindings/utils.rs:402:5
0:05.62 pid:14349 43: script_bindings::utils::generic_method
0:05.62 pid:14349 at /home/tvanderlippe/Projects/servo/components/script_bindings/utils.rs:422:5
0:05.62 pid:14349 44: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructENS_10CallReasonE
0:05.62 pid:14349 45: _ZN2js9InterpretEP9JSContextRNS_8RunStateE
0:05.62 pid:14349 46: _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
0:05.62 pid:14349 47: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructENS_10CallReasonE
0:05.62 pid:14349 48: _ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EENS_10CallReasonE
0:05.62 pid:14349 49: _ZN2js9fun_applyEP9JSContextjPN2JS5ValueE
0:05.62 pid:14349 50: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructENS_10CallReasonE
0:05.62 pid:14349 51: _ZN2js9InterpretEP9JSContextRNS_8RunStateE
0:05.62 pid:14349 52: _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
0:05.62 pid:14349 53: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructENS_10CallReasonE
0:05.62 pid:14349 54: _ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EENS_10CallReasonE
0:05.62 pid:14349 55: _ZN2JS4CallEP9JSContextNS_6HandleINS_5ValueEEES4_RKNS_16HandleValueArrayENS_13MutableHandleIS3_EE
0:05.67 pid:14349 56: mozjs::rust::wrappers::Call
0:05.67 pid:14349 at /home/tvanderlippe/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/mozjs-0.14.8/src/rust.rs:1272:51
0:05.71 pid:14349 57: script_bindings::codegen::GenericBindings::FunctionBinding::Function<D>::Call
0:05.71 pid:14349 at /home/tvanderlippe/Projects/servo/target/debug/build/script_bindings-a9454c66c35db46d/out/Bindings/FunctionBinding.rs:57:18
0:05.71 pid:14349 58: script_bindings::codegen::GenericBindings::FunctionBinding::Function<D>::Call_
0:05.71 pid:14349 at /home/tvanderlippe/Projects/servo/target/debug/build/script_bindings-a9454c66c35db46d/out/Bindings/FunctionBinding.rs:35:23
0:05.75 pid:14349 59: script::timers::JsTimerTask::invoke
0:05.75 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/timers.rs:826:34
0:05.75 pid:14349 60: script::timers::OneshotTimerCallback::invoke
0:05.75 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/timers.rs:150:57
0:05.80 pid:14349 61: script::timers::OneshotTimers::fire_timer
0:05.80 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/timers.rs:424:24
0:05.84 pid:14349 62: script::dom::globalscope::GlobalScope::fire_timer
0:05.84 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/dom/globalscope.rs:2935:23
0:05.84 pid:14349 63: script::timers::TimerListener::handle::{{closure}}
0:05.84 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/timers.rs:897:24
0:05.84 pid:14349 64: core::ops::function::FnOnce::call_once
0:05.84 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
0:05.84 pid:14349 65: <script::timers::TimerListener::handle::timer_event<F> as script::task::TaskOnce>::run_once
0:05.84 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/task.rs:54:17
0:05.91 pid:14349 66: <script::task::CancellableTask<T> as script::task::TaskOnce>::run_once
0:05.91 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/task.rs:203:24
0:05.91 pid:14349 67: <T as script::task::TaskBox>::run_box
0:05.91 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/task.rs:154:14
0:05.95 pid:14349 68: script::script_thread::ScriptThread::handle_msg_from_script
0:05.95 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/script_thread.rs:2067:22
0:05.95 pid:14349 69: script::script_thread::ScriptThread::handle_msgs::{{closure}}
0:05.95 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/script_thread.rs:1477:30
0:05.95 pid:14349 70: script::script_thread::ScriptThread::profile_event
0:05.95 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/script_thread.rs:1712:13
0:05.95 pid:14349 71: script::script_thread::ScriptThread::handle_msgs
0:05.95 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/script_thread.rs:1467:32
0:05.95 pid:14349 72: script::script_thread::ScriptThread::start
0:05.95 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/script_thread.rs:1082:20
0:05.95 pid:14349 73: <script::script_thread::ScriptThread as layout_api::ScriptThreadFactory>::create::{{closure}}::{{closure}}
0:05.95 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/script_thread.rs:504:38
0:05.99 pid:14349 74: profile_traits::mem::ProfilerChan::run_with_memory_reporting
0:05.99 pid:14349 at /home/tvanderlippe/Projects/servo/components/shared/profile/mem.rs:143:9
0:05.99 pid:14349 75: <script::script_thread::ScriptThread as layout_api::ScriptThreadFactory>::create::{{closure}}
0:05.99 pid:14349 at /home/tvanderlippe/Projects/servo/components/script/script_thread.rs:503:40
0:06.04 pid:14349 76: std::sys::backtrace::__rust_begin_short_backtrace
0:06.04 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/backtrace.rs:158:18
0:06.08 pid:14349 77: std::thread::Builder::spawn_unchecked_::{{closure}}::{{closure}}
0:06.08 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/mod.rs:559:17
0:06.12 pid:14349 78: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
0:06.12 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:274:9
0:06.17 pid:14349 79: std::panicking::catch_unwind::do_call
0:06.17 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:590:40
0:06.17 pid:14349 80: __rust_try
0:06.17 pid:14349 81: std::panicking::catch_unwind
0:06.17 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:553:19
0:06.17 pid:14349 std::panic::catch_unwind
0:06.17 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:359:14
0:06.17 pid:14349 std::thread::Builder::spawn_unchecked_::{{closure}}
0:06.17 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/mod.rs:557:30
0:06.17 pid:14349 82: core::ops::function::FnOnce::call_once{{vtable.shim}}
0:06.17 pid:14349 at /home/tvanderlippe/.rustup/toolchains/1.91.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
0:06.17 pid:14349 83: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
0:06.17 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/alloc/src/boxed.rs:1985:9
0:06.17 pid:14349 std::sys::thread::unix::Thread::new::thread_start
0:06.17 pid:14349 at /rustc/f8297e351a40c1439a467bbbb6879088047f50b3/library/std/src/sys/thread/unix.rs:126:17
0:06.17 pid:14349 84: <unknown>
0:06.17 pid:14349 85: <unknown>
TimvdLippe
commented
Feb 6, 2026
| [synchronously navigate iframe with initial src == "".] | ||
| expected: FAIL | ||
|
|
||
| [after the first iframe load event, navigate iframe with initial src == "".] |
Contributor
Author
There was a problem hiding this comment.
This test is considered broken:
|
🔨 Triggering try run (#21746202097) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21746202097): Flaky unexpected result (31)
Stable unexpected results that are known to be intermittent (41)
Stable unexpected results (1)
|
|
|
gterzian
reviewed
Feb 6, 2026
Member
gterzian
left a comment
gterzian
left a comment
There was a problem hiding this comment.
Quick note on one of the newly failing test.
gterzian
reviewed
Feb 6, 2026
| // If we already fired a synchronous load event, we shouldn't fire another | ||
| // one in this method. | ||
| let should_fire_event = | ||
| !self.already_fired_synchronous_load_event.replace(false) && should_fire_event; |
Member
There was a problem hiding this comment.
So I think the point is not so much to prevent a second load event on the frame, but rather to prevent a load to fire on the document inside the frame.
So this method here is called as part of a message sent to the constellation from the document load steps, and the thing is we should call into this method synchronously and then return, so skipping the whole navigation and document load part. But last time I tried this causes lots of timeouts.
So in this PR you are firing the event synchronously and then skipping it later, but actually we should skip the whole navigation part in that case.
Contributor
Author
There was a problem hiding this comment.
I am confused about your comment
but actually we should skip the whole navigation part in that case.
We are skipping the navigation part. It early-returns, so it never calls navigate_or_reload_child_browsing_context in process_the_iframe_attributes.
The reason we need to have this here is that in post_connection_steps we are calling create_nested_browsing_context when then eventually calls iframe_load_event_steps. That's the part where it does the InitialAboutBlank which should be unobservable. Since it must be unobservable, we should suppress the load event.
A prior version of this PR actually did an early-return in iframe_load_event_steps, but that doesn't work since we still need to call LoadBlocker::terminate and such. Otherwise, it would never finish the WPT test.
Member
There was a problem hiding this comment.
Yes you do an early return so you prevent the navigation, which is good(I realized this later, see my other comment).
But, this is only after having created the nested browsing context, which does the whole maybe_queue_document_completion thing.
So ideally we should only create the browsing context and run the iframe load event steps.
gterzian
reviewed
Feb 6, 2026
| self.upcast::<EventTarget>() | ||
| .fire_event(atom!("load"), can_gc); | ||
| // Step 2.3.2. Return. | ||
| return; |
Member
There was a problem hiding this comment.
So this is good because it means we are indeed not navigating further.
Not sure why that one test started failing, since it means the load event is fires on window where previously it wasn't
gterzian
reviewed
Feb 6, 2026
| } | ||
| } | ||
|
|
||
| if mode == ProcessingMode::FirstTime && !element.has_attribute(&local_name!("src")) { |
Member
There was a problem hiding this comment.
Ok so this removal might what got those two load event tests to start failing.
Spec wise I'm not sure what to do. You could leave a separate early return here.
The new method always returns some, where this early return checks if src is set, so you're not hitting it now.
| expected: FAIL | ||
|
|
||
| [load event does not fire on window.open('about:blank')] | ||
| expected: FAIL |
Member
There was a problem hiding this comment.
I don't understand how the changes in this pr would affect window.open
Contributor
Author
|
The computed center point is @mrobinson Can you help me out here? This particular WPT test was enabled 2 days ago, so unfortunate timing for this PR 😅 Still, I would like to fix it, but I can't seem to trigger a reflow. Things I tried: self.upcast::<Node>().dirty(NodeDamage::Other);
self.GetContentDocument().expect("Must always have a document initialized").update_the_rendering(can_gc);
if let Some(document) = self.GetContentDocument() {
document.window().allow_layout_if_necessary();
} |
gterzian
reviewed
Feb 6, 2026
gterzian
reviewed
Feb 6, 2026
| if mode == ProcessingMode::FirstTime && !element.has_attribute(&local_name!("src")) { | ||
| // Step 2.1. Let url be the result of running the shared attribute processing steps | ||
| // for iframe and frame elements given element and initialInsertion. | ||
| let Some(url) = self.shared_attribute_processing_steps_for_iframe_and_frame_elements() |
Member
There was a problem hiding this comment.
Ok so I would say your implementation is closer to the spec than what there was before, but the tests at load-pageshow-events-iframe-contentWindow.htm started failing because here you are always returning a url, whereas before there would be an early return if there was no src.
So to pass the tests in load-pageshow-events-iframe-contentWindow.htm, we would need to not run the document load steps for the about blank initial iframe, but that is for another day.
gterzian
approved these changes
Feb 6, 2026
Contributor
Author
|
Okay, I now know why
I don't know if it is possible to say: "hey, wait for the constellation to have finished processing the message"? I can send a message to the constellation and wait for its receiver, but in almost all cases that seems redundant. |
Contributor
Author
|
Also, do we consider this PR blocked on |
Member
|
I'm ok with filing an issue with the investigation work that you've done and accepting the known failure for the time being. |
Start with implementing the new algorithms per the spec. This fixes the case where the load event should be fired, but only if a src exists and it is about:blank and it is connected for the very first time. Part of servo#31973 Signed-off-by: Tim van der Lippe <[email protected]>
TimvdLippe
force-pushed
the
start-implementation-shared-loading-steps
branch
from
303547d to
2691762
Compare
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Start with implementing the new algorithms per the spec. This fixes the case where the load event should be fired, but only if a src exists and it is about:blank and it is connected for the very first time.
Part of #31973