Auto merge of #24143 - gterzian:improve_spec_comp_bc_discarding, r=asajeffrey

Improve spec compliance of discarding BCs

<!-- Please describe your changes on the following line: -->

Came-up at https://github.com/servo/servo/pull/23637#issuecomment-528254988

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24143)
<!-- Reviewable:end -->

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
+                let exited_window = exited_document.window();
-                    .window()
+                exited_window.discard_browsing_context();
-                    .window_proxy()
-                    .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();
+                // 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 doc.prompt_to_unload(false) {
+                    if document.prompt_to_unload(false) {
                         // Step 2, unload.
-                    doc.unload(false);
+                        document.unload(false);
                         // Step 3, remove from the user interface
-                    let _ = self.send_to_embedder(EmbedderMsg::CloseBrowser);
+                        let _ = window.send_to_embedder(EmbedderMsg::CloseBrowser);
                         // Step 4, discard browsing context.
-                    let _ = self.send_to_constellation(ScriptMsg::DiscardTopLevelBrowsingContext);
+                        // 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
-  [Test that iframe navigations are not observable by the parent, even after history navigations by the parent]
   expected: TIMEOUT
+  [Test that iframe navigations are not observable by the parent, even after history navigations by the parent]
+    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