mirror of
https://github.com/servo/servo.git
synced 2025-07-30 18:50:36 +01:00
Auto merge of #28560 - yvt:fix-nosrc-iframe-load-2, r=jdm
The "process the iframe attributes" steps shouldn't enqueue the iframe load event steps (anymore) Fixes `HtmlIFrameElement::process_the_iframe_attributes` (which implements [the "process the `iframe` attributes" steps][1]) queueing an element task to execute [the iframe load event steps][2], which causes another `load` event to be fired on the iframe in addition to the one asynchronously generated by [the "create a new nested browsing context" steps][3]. The following timeline shows what happens when a `src`-less `iframe` element is parser-inserted and which step this PR intends to remove. (This does not necessarily match Servo's current behavior exactly - Most notably, `T2`'s steps might run late because the "create a nested browsing context" steps' implementation actually [parses][5] a blank HTML document.) ```diff T0 (networking task source) | Insert the `iframe` element, causing the following steps to be executed: | | Create a new nested browsing context | | | Create a new browsing context | | | | Completely finish loading the `iframe` element's `Document` | | | | | Queue element task T2. | | | | Process the `iframe` attributes - | | | Queue element task T1 - T1 (DOM manipulation task source) - | Execute the `iframe` load event steps - | | Fire `load` for the `iframe` element T2 (DOM manipulation task source) | Execute the `iframe` load event steps | | Fire `load` for the `iframe` element ``` This bug likely originates from [a bug][4] in the specification. [1]: https://html.spec.whatwg.org/multipage/#process-the-iframe-attributes [2]: https://html.spec.whatwg.org/multipage/#iframe-load-event-steps [3]: https://html.spec.whatwg.org/multipage/#creating-a-new-nested-browsing-context [4]: https://github.com/whatwg/html/pull/5797 [5]: https://html.spec.whatwg.org/multipage/#the-end --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24501, #15727, and the `cross-origin-objects-on-new-window.html` part of #26299 --- - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___
This commit is contained in:
commit
cc1f89863c
6 changed files with 42 additions and 44 deletions
|
@ -25,7 +25,6 @@ use crate::dom::virtualmethods::VirtualMethods;
|
||||||
use crate::dom::window::ReflowReason;
|
use crate::dom::window::ReflowReason;
|
||||||
use crate::dom::windowproxy::WindowProxy;
|
use crate::dom::windowproxy::WindowProxy;
|
||||||
use crate::script_thread::ScriptThread;
|
use crate::script_thread::ScriptThread;
|
||||||
use crate::task_source::TaskSource;
|
|
||||||
use dom_struct::dom_struct;
|
use dom_struct::dom_struct;
|
||||||
use html5ever::{LocalName, Prefix};
|
use html5ever::{LocalName, Prefix};
|
||||||
use ipc_channel::ipc;
|
use ipc_channel::ipc;
|
||||||
|
@ -57,9 +56,9 @@ bitflags! {
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(PartialEq)]
|
#[derive(PartialEq)]
|
||||||
pub enum NavigationType {
|
enum PipelineType {
|
||||||
InitialAboutBlank,
|
InitialAboutBlank,
|
||||||
Regular,
|
Navigation,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(PartialEq)]
|
#[derive(PartialEq)]
|
||||||
|
@ -105,9 +104,17 @@ impl HTMLIFrameElement {
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn navigate_or_reload_child_browsing_context(
|
pub fn navigate_or_reload_child_browsing_context(
|
||||||
|
&self,
|
||||||
|
load_data: LoadData,
|
||||||
|
replace: HistoryEntryReplacement,
|
||||||
|
) {
|
||||||
|
self.start_new_pipeline(load_data, PipelineType::Navigation, replace);
|
||||||
|
}
|
||||||
|
|
||||||
|
fn start_new_pipeline(
|
||||||
&self,
|
&self,
|
||||||
mut load_data: LoadData,
|
mut load_data: LoadData,
|
||||||
nav_type: NavigationType,
|
pipeline_type: PipelineType,
|
||||||
replace: HistoryEntryReplacement,
|
replace: HistoryEntryReplacement,
|
||||||
) {
|
) {
|
||||||
let sandboxed = if self.is_sandboxed() {
|
let sandboxed = if self.is_sandboxed() {
|
||||||
|
@ -117,12 +124,12 @@ impl HTMLIFrameElement {
|
||||||
};
|
};
|
||||||
|
|
||||||
let browsing_context_id = match self.browsing_context_id() {
|
let browsing_context_id = match self.browsing_context_id() {
|
||||||
None => return warn!("Navigating unattached iframe."),
|
None => return warn!("Attempted to start a new pipeline on an unattached iframe."),
|
||||||
Some(id) => id,
|
Some(id) => id,
|
||||||
};
|
};
|
||||||
|
|
||||||
let top_level_browsing_context_id = match self.top_level_browsing_context_id() {
|
let top_level_browsing_context_id = match self.top_level_browsing_context_id() {
|
||||||
None => return warn!("Navigating unattached iframe."),
|
None => return warn!("Attempted to start a new pipeline on an unattached iframe."),
|
||||||
Some(id) => id,
|
Some(id) => id,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
@ -181,8 +188,8 @@ impl HTMLIFrameElement {
|
||||||
device_pixel_ratio: window.device_pixel_ratio(),
|
device_pixel_ratio: window.device_pixel_ratio(),
|
||||||
};
|
};
|
||||||
|
|
||||||
match nav_type {
|
match pipeline_type {
|
||||||
NavigationType::InitialAboutBlank => {
|
PipelineType::InitialAboutBlank => {
|
||||||
let (pipeline_sender, pipeline_receiver) = ipc::channel().unwrap();
|
let (pipeline_sender, pipeline_receiver) = ipc::channel().unwrap();
|
||||||
|
|
||||||
self.about_blank_pipeline_id.set(Some(new_pipeline_id));
|
self.about_blank_pipeline_id.set(Some(new_pipeline_id));
|
||||||
|
@ -213,7 +220,7 @@ impl HTMLIFrameElement {
|
||||||
self.pipeline_id.set(Some(new_pipeline_id));
|
self.pipeline_id.set(Some(new_pipeline_id));
|
||||||
ScriptThread::process_attach_layout(new_layout_info, document.origin().clone());
|
ScriptThread::process_attach_layout(new_layout_info, document.origin().clone());
|
||||||
},
|
},
|
||||||
NavigationType::Regular => {
|
PipelineType::Navigation => {
|
||||||
let load_info = IFrameLoadInfoWithData {
|
let load_info = IFrameLoadInfoWithData {
|
||||||
info: load_info,
|
info: load_info,
|
||||||
load_data: load_data,
|
load_data: load_data,
|
||||||
|
@ -231,6 +238,7 @@ impl HTMLIFrameElement {
|
||||||
|
|
||||||
/// <https://html.spec.whatwg.org/multipage/#process-the-iframe-attributes>
|
/// <https://html.spec.whatwg.org/multipage/#process-the-iframe-attributes>
|
||||||
fn process_the_iframe_attributes(&self, mode: ProcessingMode) {
|
fn process_the_iframe_attributes(&self, mode: ProcessingMode) {
|
||||||
|
// > 1. If `element`'s `srcdoc` attribute is specified, then:
|
||||||
if self
|
if self
|
||||||
.upcast::<Element>()
|
.upcast::<Element>()
|
||||||
.has_attribute(&local_name!("srcdoc"))
|
.has_attribute(&local_name!("srcdoc"))
|
||||||
|
@ -251,7 +259,6 @@ impl HTMLIFrameElement {
|
||||||
load_data.srcdoc = String::from(element.get_string_attribute(&local_name!("srcdoc")));
|
load_data.srcdoc = String::from(element.get_string_attribute(&local_name!("srcdoc")));
|
||||||
self.navigate_or_reload_child_browsing_context(
|
self.navigate_or_reload_child_browsing_context(
|
||||||
load_data,
|
load_data,
|
||||||
NavigationType::Regular,
|
|
||||||
HistoryEntryReplacement::Disabled,
|
HistoryEntryReplacement::Disabled,
|
||||||
);
|
);
|
||||||
return;
|
return;
|
||||||
|
@ -273,22 +280,16 @@ impl HTMLIFrameElement {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// https://github.com/whatwg/html/issues/490
|
|
||||||
if mode == ProcessingMode::FirstTime &&
|
if mode == ProcessingMode::FirstTime &&
|
||||||
!self.upcast::<Element>().has_attribute(&local_name!("src"))
|
!self.upcast::<Element>().has_attribute(&local_name!("src"))
|
||||||
{
|
{
|
||||||
let this = Trusted::new(self);
|
|
||||||
let pipeline_id = self.pipeline_id().unwrap();
|
|
||||||
// FIXME(nox): Why are errors silenced here?
|
|
||||||
let _ = window.task_manager().dom_manipulation_task_source().queue(
|
|
||||||
task!(iframe_load_event_steps: move || {
|
|
||||||
this.root().iframe_load_event_steps(pipeline_id);
|
|
||||||
}),
|
|
||||||
window.upcast(),
|
|
||||||
);
|
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// > 2. Otherwise, if `element` has a `src` attribute specified, or
|
||||||
|
// > `initialInsertion` is false, then run the shared attribute
|
||||||
|
// > processing steps for `iframe` and `frame` elements given
|
||||||
|
// > `element`.
|
||||||
let url = self.get_url();
|
let url = self.get_url();
|
||||||
|
|
||||||
// TODO(#25748):
|
// TODO(#25748):
|
||||||
|
@ -342,11 +343,25 @@ impl HTMLIFrameElement {
|
||||||
} else {
|
} else {
|
||||||
HistoryEntryReplacement::Disabled
|
HistoryEntryReplacement::Disabled
|
||||||
};
|
};
|
||||||
self.navigate_or_reload_child_browsing_context(load_data, NavigationType::Regular, replace);
|
self.navigate_or_reload_child_browsing_context(load_data, replace);
|
||||||
}
|
}
|
||||||
|
|
||||||
fn create_nested_browsing_context(&self) {
|
fn create_nested_browsing_context(&self) {
|
||||||
// Synchronously create a new context and navigate it to about:blank.
|
// Synchronously create a new browsing context, which will present
|
||||||
|
// `about:blank`. (This is not a navigation.)
|
||||||
|
//
|
||||||
|
// The pipeline started here will synchronously "completely finish
|
||||||
|
// loading", which will then asynchronously call
|
||||||
|
// `iframe_load_event_steps`.
|
||||||
|
//
|
||||||
|
// The precise event timing differs between implementations and
|
||||||
|
// remains controversial:
|
||||||
|
//
|
||||||
|
// - [Unclear "iframe load event steps" for initial load of about:blank
|
||||||
|
// in an iframe #490](https://github.com/whatwg/html/issues/490)
|
||||||
|
// - [load event handling for iframes with no src may not be web
|
||||||
|
// compatible #4965](https://github.com/whatwg/html/issues/4965)
|
||||||
|
//
|
||||||
let url = ServoUrl::parse("about:blank").unwrap();
|
let url = ServoUrl::parse("about:blank").unwrap();
|
||||||
let document = document_from_node(self);
|
let document = document_from_node(self);
|
||||||
let window = window_from_node(self);
|
let window = window_from_node(self);
|
||||||
|
@ -366,9 +381,9 @@ impl HTMLIFrameElement {
|
||||||
self.top_level_browsing_context_id
|
self.top_level_browsing_context_id
|
||||||
.set(Some(top_level_browsing_context_id));
|
.set(Some(top_level_browsing_context_id));
|
||||||
self.browsing_context_id.set(Some(browsing_context_id));
|
self.browsing_context_id.set(Some(browsing_context_id));
|
||||||
self.navigate_or_reload_child_browsing_context(
|
self.start_new_pipeline(
|
||||||
load_data,
|
load_data,
|
||||||
NavigationType::InitialAboutBlank,
|
PipelineType::InitialAboutBlank,
|
||||||
HistoryEntryReplacement::Disabled,
|
HistoryEntryReplacement::Disabled,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
|
@ -45,7 +45,7 @@ use crate::dom::element::Element;
|
||||||
use crate::dom::event::{Event, EventBubbles, EventCancelable};
|
use crate::dom::event::{Event, EventBubbles, EventCancelable};
|
||||||
use crate::dom::globalscope::GlobalScope;
|
use crate::dom::globalscope::GlobalScope;
|
||||||
use crate::dom::htmlanchorelement::HTMLAnchorElement;
|
use crate::dom::htmlanchorelement::HTMLAnchorElement;
|
||||||
use crate::dom::htmliframeelement::{HTMLIFrameElement, NavigationType};
|
use crate::dom::htmliframeelement::HTMLIFrameElement;
|
||||||
use crate::dom::identityhub::Identities;
|
use crate::dom::identityhub::Identities;
|
||||||
use crate::dom::mutationobserver::MutationObserver;
|
use crate::dom::mutationobserver::MutationObserver;
|
||||||
use crate::dom::node::{window_from_node, Node, ShadowIncluding};
|
use crate::dom::node::{window_from_node, Node, ShadowIncluding};
|
||||||
|
@ -3705,11 +3705,7 @@ impl ScriptThread {
|
||||||
.borrow()
|
.borrow()
|
||||||
.find_iframe(parent_pipeline_id, browsing_context_id);
|
.find_iframe(parent_pipeline_id, browsing_context_id);
|
||||||
if let Some(iframe) = iframe {
|
if let Some(iframe) = iframe {
|
||||||
iframe.navigate_or_reload_child_browsing_context(
|
iframe.navigate_or_reload_child_browsing_context(load_data, replace);
|
||||||
load_data,
|
|
||||||
NavigationType::Regular,
|
|
||||||
replace,
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -5,9 +5,6 @@
|
||||||
[Following a noreferrer link with a named target should not cause creation of a window that can be targeted by another noreferrer link with the same named target]
|
[Following a noreferrer link with a named target should not cause creation of a window that can be targeted by another noreferrer link with the same named target]
|
||||||
expected: TIMEOUT
|
expected: TIMEOUT
|
||||||
|
|
||||||
[Targeting a rel=noreferrer link at an existing named subframe should work]
|
|
||||||
expected: FAIL
|
|
||||||
|
|
||||||
[Targeting a rel=noreferrer link at an existing named window should work]
|
[Targeting a rel=noreferrer link at an existing named window should work]
|
||||||
expected: FAIL
|
expected: FAIL
|
||||||
|
|
||||||
|
|
|
@ -1,4 +0,0 @@
|
||||||
[form-double-submit-2.html]
|
|
||||||
[preventDefault should allow onclick submit() to succeed]
|
|
||||||
expected: FAIL
|
|
||||||
|
|
|
@ -1,4 +0,0 @@
|
||||||
[form-double-submit.html]
|
|
||||||
[default submit action should supersede onclick submit()]
|
|
||||||
expected: FAIL
|
|
||||||
|
|
|
@ -1,6 +1,4 @@
|
||||||
[open-url-multi-window-6.htm]
|
[open-url-multi-window-6.htm]
|
||||||
type: testharness
|
|
||||||
expected: TIMEOUT
|
|
||||||
[XMLHttpRequest: open() in document that is not fully active (but may be active) should throw]
|
[XMLHttpRequest: open() in document that is not fully active (but may be active) should throw]
|
||||||
expected: TIMEOUT
|
expected: FAIL
|
||||||
|
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue