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

@@ -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();

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::<GlobalScope>())
+                    .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();
     }
 
     /// <https://drafts.csswg.org/cssom-view/#dom-window-scroll>

components/script/dom/windowproxy.rs

@@ -96,6 +96,9 @@ pub struct WindowProxy {
     /// Has the browsing context been disowned?
     disowned: Cell<bool>,
 
+    /// https://html.spec.whatwg.org/multipage/#is-closing
+    is_closing: Cell<bool>,
+
     /// The containing iframe element, if this is a same-origin iframe
     frame_element: Option<Dom<Element>>,
 
@@ -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();
     }
 

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);

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
-

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
 

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