From 45ec250b0a989643865880734ff898de4b15bd7d Mon Sep 17 00:00:00 2001 From: Gregory Terzian Date: Thu, 5 Sep 2019 16:12:52 +0800 Subject: [PATCH] improve spec compliance of discarding BCs do not handle compositor input events when BC is being discarded prevent firing of timers for discarded BCs return null for opener is BC has been discarded bundle discard BC steps into window method return null in window.opener, if BC has already been discarded move the window closed check pre-event to script-thread --- components/script/dom/htmliframeelement.rs | 7 +- components/script/dom/window.rs | 97 +++++++++++++++---- components/script/dom/windowproxy.rs | 21 +++- components/script/script_thread.rs | 31 +++++- .../close-method.window.js.ini | 4 - ...nested-context-navigations-iframe.html.ini | 12 +-- .../dedicated-worker-import-csp.html.ini | 14 +-- 7 files changed, 133 insertions(+), 53 deletions(-) diff --git a/components/script/dom/htmliframeelement.rs b/components/script/dom/htmliframeelement.rs index a3a246c4edc..f5a820458c2 100644 --- a/components/script/dom/htmliframeelement.rs +++ b/components/script/dom/htmliframeelement.rs @@ -668,15 +668,14 @@ impl VirtualMethods for HTMLIFrameElement { // so we need to discard the browsing contexts now, rather than // when the `PipelineExit` message arrives. for exited_pipeline_id in exited_pipeline_ids { + // https://html.spec.whatwg.org/multipage/#a-browsing-context-is-discarded if let Some(exited_document) = ScriptThread::find_document(exited_pipeline_id) { debug!( "Discarding browsing context for pipeline {}", exited_pipeline_id ); - exited_document - .window() - .window_proxy() - .discard_browsing_context(); + let exited_window = exited_document.window(); + exited_window.discard_browsing_context(); for exited_iframe in exited_document.iter_iframes() { debug!("Discarding nested browsing context"); exited_iframe.destroy_nested_browsing_context(); diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index a79669d6c2e..8e139158427 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -63,7 +63,7 @@ use crate::script_runtime::{ use crate::script_thread::{ImageCacheMsg, MainThreadScriptChan, MainThreadScriptMsg}; use crate::script_thread::{ScriptThread, SendableMainThreadScriptChan}; use crate::task_manager::TaskManager; -use crate::task_source::TaskSourceName; +use crate::task_source::{TaskSource, TaskSourceName}; use crate::timers::{IsInterval, TimerCallback}; use crate::webdriver_handlers::jsval_to_webdriver; use app_units::Au; @@ -82,8 +82,8 @@ use ipc_channel::router::ROUTER; use js::jsapi::JSAutoRealm; use js::jsapi::JSPROP_ENUMERATE; use js::jsapi::{GCReason, JS_GC}; -use js::jsval::JSVal; use js::jsval::UndefinedValue; +use js::jsval::{JSVal, NullValue}; use js::rust::wrappers::JS_DefineProperty; use js::rust::HandleValue; use media::WindowGLContext; @@ -352,11 +352,26 @@ impl Window { *self.js_runtime.borrow_for_script_deallocation() = None; self.window_proxy.set(None); self.current_state.set(WindowState::Zombie); - self.ignore_all_events(); + self.ignore_all_tasks(); } } - fn ignore_all_events(&self) { + /// A convenience method for + /// https://html.spec.whatwg.org/multipage/#a-browsing-context-is-discarded + pub fn discard_browsing_context(&self) { + let proxy = match self.window_proxy.get() { + Some(proxy) => proxy, + None => panic!("Discarding a BC from a window that has none"), + }; + proxy.discard_browsing_context(); + // Step 4 of https://html.spec.whatwg.org/multipage/#discard-a-document + // Other steps performed when the `PipelineExit` message + // is handled by the ScriptThread. + self.ignore_all_tasks(); + } + + /// Cancel all current, and ignore all subsequently queued, tasks. + pub fn ignore_all_tasks(&self) { let mut ignore_flags = self.task_manager.task_cancellers.borrow_mut(); for task_source_name in TaskSourceName::all() { let flag = ignore_flags @@ -623,10 +638,23 @@ impl WindowMethods for Window { self.window_proxy().open(url, target, features) } - #[allow(unsafe_code)] // https://html.spec.whatwg.org/multipage/#dom-opener fn Opener(&self, cx: JSContext) -> JSVal { - unsafe { self.window_proxy().opener(*cx) } + // Step 1, Let current be this Window object's browsing context. + let current = match self.window_proxy.get() { + Some(proxy) => proxy, + // Step 2, If current is null, then return null. + None => return NullValue(), + }; + // Still step 2, since the window's BC is the associated doc's BC, + // see https://html.spec.whatwg.org/multipage/#window-bc + // and a doc's BC is null if it has been discarded. + // see https://html.spec.whatwg.org/multipage/#concept-document-bc + if current.is_browsing_context_discarded() { + return NullValue(); + } + // Step 3 to 5. + current.opener(*cx) } #[allow(unsafe_code)] @@ -653,31 +681,60 @@ impl WindowMethods for Window { fn Closed(&self) -> bool { self.window_proxy .get() - .map(|ref proxy| proxy.is_browsing_context_discarded()) + .map(|ref proxy| proxy.is_browsing_context_discarded() || proxy.is_closing()) .unwrap_or(true) } // https://html.spec.whatwg.org/multipage/#dom-window-close fn Close(&self) { - let window_proxy = self.window_proxy(); + // Step 1, Let current be this Window object's browsing context. + // Step 2, If current is null or its is closing is true, then return. + let window_proxy = match self.window_proxy.get() { + Some(proxy) => proxy, + None => return, + }; + if window_proxy.is_closing() { + return; + } // Note: check the length of the "session history", as opposed to the joint session history? // see https://github.com/whatwg/html/issues/3734 if let Ok(history_length) = self.History().GetLength() { let is_auxiliary = window_proxy.is_auxiliary(); + // https://html.spec.whatwg.org/multipage/#script-closable let is_script_closable = (self.is_top_level() && history_length == 1) || is_auxiliary; + + // TODO: rest of Step 3: + // Is the incumbent settings object's responsible browsing context familiar with current? + // Is the incumbent settings object's responsible browsing context allowed to navigate current? if is_script_closable { - let doc = self.Document(); - // https://html.spec.whatwg.org/multipage/#closing-browsing-contexts - // Step 1, prompt to unload. - if doc.prompt_to_unload(false) { - // Step 2, unload. - doc.unload(false); - // Step 3, remove from the user interface - let _ = self.send_to_embedder(EmbedderMsg::CloseBrowser); - // Step 4, discard browsing context. - let _ = self.send_to_constellation(ScriptMsg::DiscardTopLevelBrowsingContext); - } + // Step 3.1, set current's is closing to true. + window_proxy.close(); + + // Step 3.2, queue a task on the DOM manipulation task source to close current. + let this = Trusted::new(self); + let task = task!(window_close_browsing_context: move || { + let window = this.root(); + let document = window.Document(); + // https://html.spec.whatwg.org/multipage/#closing-browsing-contexts + // Step 1, prompt to unload. + if document.prompt_to_unload(false) { + // Step 2, unload. + document.unload(false); + // Step 3, remove from the user interface + let _ = window.send_to_embedder(EmbedderMsg::CloseBrowser); + // Step 4, discard browsing context. + // https://html.spec.whatwg.org/multipage/#a-browsing-context-is-discarded + // which calls into https://html.spec.whatwg.org/multipage/#discard-a-document. + window.discard_browsing_context(); + + let _ = window.send_to_constellation(ScriptMsg::DiscardTopLevelBrowsingContext); + } + }); + self.task_manager() + .dom_manipulation_task_source() + .queue(task, &self.upcast::()) + .expect("Queuing window_close_browsing_context task to work"); } } } @@ -1302,7 +1359,7 @@ impl Window { if let Some(performance) = self.performance.get() { performance.clear_and_disable_performance_entry_buffer(); } - self.ignore_all_events(); + self.ignore_all_tasks(); } /// diff --git a/components/script/dom/windowproxy.rs b/components/script/dom/windowproxy.rs index 3ff326c683a..7fea1728c12 100644 --- a/components/script/dom/windowproxy.rs +++ b/components/script/dom/windowproxy.rs @@ -96,6 +96,9 @@ pub struct WindowProxy { /// Has the browsing context been disowned? disowned: Cell, + /// https://html.spec.whatwg.org/multipage/#is-closing + is_closing: Cell, + /// The containing iframe element, if this is a same-origin iframe frame_element: Option>, @@ -126,6 +129,7 @@ impl WindowProxy { currently_active: Cell::new(currently_active), discarded: Cell::new(false), disowned: Cell::new(false), + is_closing: Cell::new(false), frame_element: frame_element.map(Dom::from_ref), parent: parent.map(Dom::from_ref), delaying_load_events_mode: Cell::new(false), @@ -352,9 +356,20 @@ impl WindowProxy { self.disowned.set(true); } + /// https://html.spec.whatwg.org/multipage/#dom-window-close + /// Step 3.1, set BCs `is_closing` to true. + pub fn close(&self) { + self.is_closing.set(true); + } + + /// https://html.spec.whatwg.org/multipage/#is-closing + pub fn is_closing(&self) -> bool { + self.is_closing.get() + } + #[allow(unsafe_code)] // https://html.spec.whatwg.org/multipage/#dom-opener - pub unsafe fn opener(&self, cx: *mut JSContext) -> JSVal { + pub fn opener(&self, cx: *mut JSContext) -> JSVal { if self.disowned.get() { return NullValue(); } @@ -371,7 +386,7 @@ impl WindowProxy { opener_id, ) { Some(opener_top_id) => { - let global_to_clone_from = GlobalScope::from_context(cx); + let global_to_clone_from = unsafe { GlobalScope::from_context(cx) }; WindowProxy::new_dissimilar_origin( &*global_to_clone_from, opener_id, @@ -388,7 +403,7 @@ impl WindowProxy { return NullValue(); } rooted!(in(cx) let mut val = UndefinedValue()); - opener_proxy.to_jsval(cx, val.handle_mut()); + unsafe { opener_proxy.to_jsval(cx, val.handle_mut()) }; return val.get(); } diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 440de991ceb..b4363f5ada3 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1973,7 +1973,15 @@ impl ScriptThread { let window = self.documents.borrow().find_window(pipeline_id); let window = match window { - Some(w) => w, + Some(w) => { + if w.Closed() { + return warn!( + "Received fire timer msg for a discarded browsing context whose pipeline is pending exit {}.", + pipeline_id + ); + } + w + }, None => { return warn!( "Received fire timer msg for a closed pipeline {}.", @@ -2848,7 +2856,7 @@ impl ScriptThread { // to avoid running layout on detached iframes. let window = document.window(); if discard_bc == DiscardBrowsingContext::Yes { - window.window_proxy().discard_browsing_context(); + window.discard_browsing_context(); } window.clear_js_runtime(); } @@ -3378,9 +3386,26 @@ impl ScriptThread { /// /// TODO: Actually perform DOM event dispatch. fn handle_event(&self, pipeline_id: PipelineId, event: CompositorEvent) { + // Do not handle events if the pipeline exited. + let window = match { self.documents.borrow().find_window(pipeline_id) } { + Some(win) => win, + None => { + return warn!( + "Compositor event sent to a pipeline that already exited {}.", + pipeline_id + ) + }, + }; + // Do not handle events if the BC has been, or is being, discarded + if window.Closed() { + return warn!( + "Compositor event sent to a pipeline with a closed window {}.", + pipeline_id + ); + } + // Assuming all CompositionEvent are generated by user interactions. ScriptThread::set_user_interacting(true); - match event { ResizeEvent(new_size, size_type) => { self.handle_resize_event(pipeline_id, new_size, size_type); diff --git a/tests/wpt/metadata/html/browsers/the-window-object/close-method.window.js.ini b/tests/wpt/metadata/html/browsers/the-window-object/close-method.window.js.ini index 7af149760cd..0e9dfd0ea04 100644 --- a/tests/wpt/metadata/html/browsers/the-window-object/close-method.window.js.ini +++ b/tests/wpt/metadata/html/browsers/the-window-object/close-method.window.js.ini @@ -1,7 +1,3 @@ [close-method.window.html] [window.close() affects name targeting immediately] expected: FAIL - - [window.close() queues a task to discard, but window.closed knows immediately] - expected: FAIL - diff --git a/tests/wpt/metadata/resource-timing/nested-context-navigations-iframe.html.ini b/tests/wpt/metadata/resource-timing/nested-context-navigations-iframe.html.ini index 87d3c09909f..ec371a1bf72 100644 --- a/tests/wpt/metadata/resource-timing/nested-context-navigations-iframe.html.ini +++ b/tests/wpt/metadata/resource-timing/nested-context-navigations-iframe.html.ini @@ -1,20 +1,20 @@ [nested-context-navigations-iframe.html] - expected: CRASH + expected: TIMEOUT [Test that iframe navigations are not observable by the parent, even after history navigations by the parent] - expected: TIMEOUT + expected: FAIL [Test that crossorigin iframe navigations are not observable by the parent, even after history navigations by the parent] - expected: NOTRUN + expected: FAIL [Test that iframe refreshes are not observable by the parent] - expected: NOTRUN + expected: TIMEOUT [Test that crossorigin iframe navigations are not observable by the parent] - expected: NOTRUN + expected: FAIL [Test that crossorigin iframe refreshes are not observable by the parent] expected: NOTRUN [Test that iframe navigations are not observable by the parent] - expected: NOTRUN + expected: FAIL diff --git a/tests/wpt/metadata/workers/modules/dedicated-worker-import-csp.html.ini b/tests/wpt/metadata/workers/modules/dedicated-worker-import-csp.html.ini index 48e41ffe093..ad9025141b3 100644 --- a/tests/wpt/metadata/workers/modules/dedicated-worker-import-csp.html.ini +++ b/tests/wpt/metadata/workers/modules/dedicated-worker-import-csp.html.ini @@ -1,29 +1,17 @@ [dedicated-worker-import-csp.html] - expected: CRASH + expected: OK [DedicatedWorker: CSP for ES Modules] expected: FAIL - [worker-src 'self' directive should disallow cross origin static import.] - expected: FAIL - [worker-src * directive should allow cross origin static import.] expected: FAIL - [script-src 'self' directive should disallow cross origin static import.] - expected: FAIL - [script-src * directive should allow cross origin static import.] expected: FAIL [worker-src * directive should override script-src 'self' directive and allow cross origin static import.] expected: FAIL - [worker-src 'self' directive should override script-src * directive and disallow cross origin static import.] - expected: FAIL - - [script-src 'self' directive should disallow cross origin dynamic import.] - expected: FAIL - [script-src * directive should allow cross origin dynamic import.] expected: FAIL