Auto merge of #6677 - jdm:iframeblockonload, r=Ms2ger

Make iframes block the enclosing document's load event

It occurs to me as I write this that this doesn't handle the case of removing the iframe from the document before it's finished loading. Consider this an early feedback release!

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6677)
<!-- Reviewable:end -->

components/compositing/constellation.rs

@@ -832,7 +832,7 @@ impl<LTF: LayoutThreadFactory, STF: ScriptThreadFactory> Constellation<LTF, STF>
         let parent_pipeline = self.pipeline(subframe_parent.0);
         let msg = ConstellationControlMsg::DispatchFrameLoadEvent {
             target: pipeline_id,
-            parent: subframe_parent.0
+            parent: subframe_parent.0,
         };
         parent_pipeline.script_chan.send(msg).unwrap();
     }

components/script/document_loader.rs

@@ -5,10 +5,13 @@
 //! Tracking of pending loads in a document.
 //! https://html.spec.whatwg.org/multipage/#the-end
 
+use dom::bindings::js::JS;
+use dom::document::Document;
 use msg::constellation_msg::PipelineId;
 use net_traits::AsyncResponseTarget;
 use net_traits::{PendingAsyncLoad, ResourceThread, LoadContext};
 use std::sync::Arc;
+use std::thread;
 use url::Url;
 
 #[derive(JSTraceable, PartialEq, Clone, Debug, HeapSizeOf)]
@@ -41,6 +44,50 @@ impl LoadType {
     }
 }
 
+/// Canary value ensuring that manually added blocking loads (ie. ones that weren't
+/// created via DocumentLoader::prepare_async_load) are always removed by the time
+/// that the owner is destroyed.
+#[derive(JSTraceable, HeapSizeOf)]
+#[must_root]
+pub struct LoadBlocker {
+    /// The document whose load event is blocked by this object existing.
+    doc: JS<Document>,
+    /// The load that is blocking the document's load event.
+    load: Option<LoadType>,
+}
+
+impl LoadBlocker {
+    /// Mark the document's load event as blocked on this new load.
+    pub fn new(doc: &Document, load: LoadType) -> LoadBlocker {
+        doc.add_blocking_load(load.clone());
+        LoadBlocker {
+            doc: JS::from_ref(doc),
+            load: Some(load),
+        }
+    }
+
+    /// Remove this load from the associated document's list of blocking loads.
+    pub fn terminate(blocker: &mut Option<LoadBlocker>) {
+        if let Some(this) = blocker.as_mut() {
+            this.doc.finish_load(this.load.take().unwrap());
+        }
+        *blocker = None;
+    }
+
+    /// Return the url associated with this load.
+    pub fn url(&self) -> Option<&Url> {
+        self.load.as_ref().map(LoadType::url)
+    }
+}
+
+impl Drop for LoadBlocker {
+    fn drop(&mut self) {
+        if !thread::panicking() {
+            assert!(self.load.is_none());
+        }
+    }
+}
+
 #[derive(JSTraceable, HeapSizeOf)]
 pub struct DocumentLoader {
     /// We use an `Arc<ResourceThread>` here in order to avoid file descriptor exhaustion when there
@@ -73,12 +120,17 @@ impl DocumentLoader {
         }
     }
 
+    /// Add a load to the list of blocking loads.
+    pub fn add_blocking_load(&mut self, load: LoadType) {
+        self.blocking_loads.push(load);
+    }
+
     /// Create a new pending network request, which can be initiated at some point in
     /// the future.
     pub fn prepare_async_load(&mut self, load: LoadType) -> PendingAsyncLoad {
         let context = load.to_load_context();
         let url = load.url().clone();
-        self.blocking_loads.push(load);
+        self.add_blocking_load(load);
         PendingAsyncLoad::new(context, (*self.resource_thread).clone(), url, self.pipeline)
     }
 

components/script/dom/document.rs

@@ -1231,6 +1231,12 @@ impl Document {
                            ReflowReason::RequestAnimationFrame);
     }
 
+    /// Add a load to the list of loads blocking this document's load.
+    pub fn add_blocking_load(&self, load: LoadType) {
+        let mut loader = self.loader.borrow_mut();
+        loader.add_blocking_load(load)
+    }
+
     pub fn prepare_async_load(&self, load: LoadType) -> PendingAsyncLoad {
         let mut loader = self.loader.borrow_mut();
         loader.prepare_async_load(load)
@@ -1291,8 +1297,8 @@ impl Document {
         };
 
         if self.script_blocking_stylesheets_count.get() == 0 && script.is_ready_to_be_executed() {
-            script.execute();
             self.pending_parsing_blocking_script.set(None);
+            script.execute();
             return ParserBlockedByScript::Unblocked;
         }
         ParserBlockedByScript::Blocked

components/script/dom/htmliframeelement.rs

@@ -2,7 +2,9 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+use document_loader::{LoadType, LoadBlocker};
 use dom::attr::{Attr, AttrValue};
+use dom::bindings::cell::DOMRefCell;
 use dom::bindings::codegen::Bindings::BrowserElementBinding::BrowserElementIconChangeEventDetail;
 use dom::bindings::codegen::Bindings::BrowserElementBinding::BrowserElementSecurityChangeDetail;
 use dom::bindings::codegen::Bindings::BrowserElementBinding::BrowserShowModalPromptEventDetail;
@@ -21,7 +23,7 @@ use dom::element::{AttributeMutation, Element, RawLayoutElementHelpers};
 use dom::event::Event;
 use dom::eventtarget::EventTarget;
 use dom::htmlelement::HTMLElement;
-use dom::node::{Node, UnbindContext, window_from_node};
+use dom::node::{Node, UnbindContext, window_from_node, document_from_node};
 use dom::urlhelper::UrlHelper;
 use dom::virtualmethods::VirtualMethods;
 use dom::window::{ReflowReason, Window};
@@ -64,6 +66,7 @@ pub struct HTMLIFrameElement {
     subpage_id: Cell<Option<SubpageId>>,
     containing_page_pipeline_id: Cell<Option<PipelineId>>,
     sandbox: Cell<Option<u8>>,
+    load_blocker: DOMRefCell<Option<LoadBlocker>>,
 }
 
 impl HTMLIFrameElement {
@@ -101,6 +104,20 @@ impl HTMLIFrameElement {
             IFrameUnsandboxed
         };
 
+        let document = document_from_node(self);
+
+        let mut load_blocker = self.load_blocker.borrow_mut();
+        // Any oustanding load is finished from the point of view of the blocked
+        // document; the new navigation will continue blocking it.
+        LoadBlocker::terminate(&mut load_blocker);
+
+        //TODO(#9592): Deal with the case where an iframe is being reloaded so url is None.
+        //      The iframe should always have access to the nested context's active
+        //      document URL through the browsing context.
+        if let Some(ref url) = url {
+            *load_blocker = Some(LoadBlocker::new(&*document, LoadType::Subframe(url.clone())));
+        }
+
         let window = window_from_node(self);
         let window = window.r();
         let (new_subpage_id, old_subpage_id) = self.generate_new_subpage_id();
@@ -173,6 +190,7 @@ impl HTMLIFrameElement {
             subpage_id: Cell::new(None),
             containing_page_pipeline_id: Cell::new(None),
             sandbox: Cell::new(None),
+            load_blocker: DOMRefCell::new(None),
         }
     }
 
@@ -204,7 +222,11 @@ impl HTMLIFrameElement {
     }
 
     /// https://html.spec.whatwg.org/multipage/#iframe-load-event-steps steps 1-4
-    pub fn iframe_load_event_steps(&self) {
+    pub fn iframe_load_event_steps(&self, loaded_pipeline: PipelineId) {
+        // TODO(#9592): assert that the load blocker is present at all times when we
+        //              can guarantee that it's created for the case of iframe.reload().
+        assert_eq!(loaded_pipeline, self.pipeline().unwrap());
+
         // TODO A cross-origin child document would not be easily accessible
         //      from this script thread. It's unclear how to implement
         //      steps 2, 3, and 5 efficiently in this case.
@@ -213,6 +235,10 @@ impl HTMLIFrameElement {
 
         // Step 4
         self.upcast::<EventTarget>().fire_simple_event("load");
+
+        let mut blocker = self.load_blocker.borrow_mut();
+        LoadBlocker::terminate(&mut blocker);
+
         // TODO Step 5 - unset child document `mut iframe load` flag
 
         let window = window_from_node(self);
@@ -510,6 +536,9 @@ impl VirtualMethods for HTMLIFrameElement {
     fn unbind_from_tree(&self, context: &UnbindContext) {
         self.super_type().unwrap().unbind_from_tree(context);
 
+        let mut blocker = self.load_blocker.borrow_mut();
+        LoadBlocker::terminate(&mut blocker);
+
         // https://html.spec.whatwg.org/multipage/#a-browsing-context-is-discarded
         if let Some(pipeline_id) = self.pipeline_id.get() {
             let window = window_from_node(self);

components/script/script_thread.rs

@@ -1662,7 +1662,7 @@ impl ScriptThread {
         let page = get_page(&self.root_page(), containing_pipeline);
         let document = page.document();
         if let Some(iframe) = document.find_iframe_by_pipeline(id) {
-            iframe.iframe_load_event_steps();
+            iframe.iframe_load_event_steps(id);
         }
     }
 

components/script_traits/lib.rs

@@ -142,7 +142,7 @@ pub enum ConstellationControlMsg {
         /// The pipeline that has been marked as loaded.
         target: PipelineId,
         /// The pipeline that contains a frame loading the target pipeline.
-        parent: PipelineId
+        parent: PipelineId,
     },
     /// Notifies a parent frame that one of its child frames is now active.
     FramedContentChanged(PipelineId, SubpageId),

tests/wpt/metadata/XMLHttpRequest/send-entity-body-document.htm.ini

@@ -1,4 +1,23 @@
 [send-entity-body-document.htm]
   type: testharness
-  disabled: https://github.com/servo/servo/issues/8157
+  [XML document, windows-1252]
+    expected: FAIL
+
+  [HTML document, invalid UTF-8]
+    expected: FAIL
+
+  [HTML document, shift-jis]
+    expected: FAIL
+
+  [plain text file]
+    expected: FAIL
+
+  [image file]
+    expected: FAIL
+
+  [img tag]
+    expected: FAIL
+
+  [empty div]
+    expected: FAIL
 

tests/wpt/metadata/dom/ranges/Range-deleteContents.html.ini

@@ -35,4 +35,3 @@
 
   [Resulting cursor position for range 51 [paras[3\], 1, comment, 8\]]
     expected: FAIL
-

tests/wpt/metadata/dom/ranges/Range-extractContents.html.ini

@@ -14,4 +14,3 @@
 
   [Resulting cursor position for range 51 [paras[3\], 1, comment, 8\]]
     expected: FAIL
-

tests/wpt/metadata/html/infrastructure/terminology/plugins/text-plain.html.ini

@@ -1,3 +0,0 @@
-[text-plain.html]
-  type: testharness
-  disabled: https://github.com/servo/servo/issues/9286

tests/wpt/metadata/html/semantics/document-metadata/the-base-element/base_multiple.html.ini

@@ -1,6 +1,5 @@
 [base_multiple.html]
   type: testharness
-  expected: ERROR
   [The attributes of the a element must be affected by the first base element]
     expected: FAIL
 

tests/wpt/metadata/html/semantics/embedded-content/the-iframe-element/move_iframe_in_dom_03.html.ini

@@ -1,3 +0,0 @@
-[move_iframe_in_dom_03.html]
-  type: testharness
-  expected: TIMEOUT

tests/wpt/metadata/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html.ini

@@ -1,3 +1,109 @@
 [viewport-change.html]
   type: testharness
-  expected: TIMEOUT
+  disabled: unsupported feature and prone to intermittent incorrect results
+  bug: 9560
+  [img (no src), onload, narrow]
+    expected: FAIL
+
+  [img (empty src), onload, narrow]
+    expected: FAIL
+
+  [img (src only) broken image, onload, narrow]
+    expected: FAIL
+
+  [img (src only) valid image, onload, narrow]
+    expected: FAIL
+
+  [img (srcset 1 cand) broken image, onload, narrow]
+    expected: FAIL
+
+  [img (srcset 1 cand) valid image, onload, narrow]
+    expected: FAIL
+
+  [picture: source (max-width:500px) broken image, img broken image, onload, narrow]
+    expected: FAIL
+
+  [picture: source (max-width:500px) broken image, img broken image, resize to wide]
+    expected: FAIL
+
+  [picture: source (max-width:500px) broken image, img valid image, onload, narrow]
+    expected: FAIL
+
+  [picture: source (max-width:500px) broken image, img valid image, resize to wide]
+    expected: FAIL
+
+  [picture: source (max-width:500px) valid image, img broken image, onload, narrow]
+    expected: FAIL
+
+  [picture: source (max-width:500px) valid image, img broken image, resize to wide]
+    expected: FAIL
+
+  [picture: source (max-width:500px) valid image, img valid image, onload, narrow]
+    expected: FAIL
+
+  [picture: source (max-width:500px) valid image, img valid image, resize to wide]
+    expected: FAIL
+
+  [picture: same URL in source (max-width:500px) and img, onload, narrow]
+    expected: FAIL
+
+  [img (no src), onload, wide]
+    expected: FAIL
+
+  [img (empty src), onload, wide]
+    expected: FAIL
+
+  [img (src only) broken image, onload, wide]
+    expected: FAIL
+
+  [img (src only) valid image, onload, wide]
+    expected: FAIL
+
+  [img (srcset 1 cand) broken image, onload, wide]
+    expected: FAIL
+
+  [img (srcset 1 cand) valid image, onload, wide]
+    expected: FAIL
+
+  [picture: source (max-width:500px) broken image, img broken image, onload, wide]
+    expected: FAIL
+
+  [picture: source (max-width:500px) broken image, img broken image, resize to narrow]
+    expected: FAIL
+
+  [picture: source (max-width:500px) broken image, img valid image, onload, wide]
+    expected: FAIL
+
+  [picture: source (max-width:500px) broken image, img valid image, resize to narrow]
+    expected: FAIL
+
+  [picture: source (max-width:500px) valid image, img broken image, onload, wide]
+    expected: FAIL
+
+  [picture: source (max-width:500px) valid image, img broken image, resize to narrow]
+    expected: FAIL
+
+  [picture: source (max-width:500px) valid image, img valid image, onload, wide]
+    expected: FAIL
+
+  [picture: source (max-width:500px) valid image, img valid image, resize to narrow]
+    expected: FAIL
+
+  [picture: same URL in source (max-width:500px) and img, onload, wide]
+    expected: FAIL
+
+  [img (empty src), resize to wide]
+    expected: FAIL
+
+  [img (src only) broken image, resize to wide]
+    expected: FAIL
+
+  [img (src only) valid image, resize to wide]
+    expected: FAIL
+
+  [picture: same URL in source (max-width:500px) and img, resize to wide]
+    expected: FAIL
+
+  [picture: same URL in source (max-width:500px) and img, resize to narrow]
+    expected: FAIL
+

tests/wpt/metadata/html/semantics/selectors/pseudo-classes/focus.html.ini

@@ -6,3 +6,6 @@
   [editable elements are focusable]
     expected: FAIL
 
+  [':focus' doesn't match focused elements in iframe]
+    expected: FAIL
+

tests/wpt/metadata/workers/nested_worker.worker.js.ini

@@ -3,4 +3,6 @@
   expected: TIMEOUT
   [Nested worker]
     expected: FAIL
+  [Checking contents for text file]
+    expected: FAIL
 

tests/wpt/mozilla/meta/MANIFEST.json

@@ -5850,6 +5850,12 @@
             "url": "/_mozilla/mozilla/htmlspacechars.html"
           }
         ],
+        "mozilla/iframe-unblock-onload.html": [
+          {
+            "path": "mozilla/iframe-unblock-onload.html",
+            "url": "/_mozilla/mozilla/iframe-unblock-onload.html"
+          }
+        ],
         "mozilla/iframe_contentDocument.html": [
           {
             "path": "mozilla/iframe_contentDocument.html",
@@ -5910,6 +5916,12 @@
             "url": "/_mozilla/mozilla/mozbrowser/iframe_goback.html"
           }
         ],
+        "mozilla/mozbrowser/iframe_reload_twice.html": [
+          {
+            "path": "mozilla/mozbrowser/iframe_reload_twice.html",
+            "url": "/_mozilla/mozilla/mozbrowser/iframe_reload_twice.html"
+          }
+        ],
         "mozilla/mozbrowser/mozbrowsericonchange_event.html": [
           {
             "path": "mozilla/mozbrowser/mozbrowsericonchange_event.html",

tests/wpt/mozilla/tests/mozilla/iframe-unblock-onload.html

@@ -0,0 +1,18 @@
+<html>
+<head>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+</head>
+<body>
+<iframe src="resources/iframe_contentDocument_inner.html?pipe=trickle(d20)"></iframe>
+<script>
+  async_test(function(t) {
+    window.addEventListener('load', t.step_func(function() {
+      t.done();
+    }, false));
+
+    document.querySelector('iframe').remove();
+  }, "Document load is unblocked by iframe removal");
+</script>
+</body>
+</html>

tests/wpt/mozilla/tests/mozilla/mozbrowser/iframe_reload_twice.html

@@ -0,0 +1,23 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Reloading an iframe twice doesn't panic</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<iframe></iframe>
+<script>
+async_test(function(t) {
+  var iframe = document.querySelector('iframe');
+  var loaded = false;
+  iframe.onload = t.step_func(function() {
+    if (!loaded) {
+      loaded = true;
+      iframe.reload();
+      iframe.reload();
+    } else {
+      t.done();
+    }
+  });
+  iframe.mozbrowser = true;
+  iframe.src = "about:blank";
+});
+</script>

tests/wpt/web-platform-tests/html/browsers/history/the-location-interface/location_reload.html

@@ -21,6 +21,7 @@
           assert_equals(url, innerURL, "iframe url (" + pingCount + ")");
           pingCount++;
           if (pingCount == 5) {
+            iframe.src = 'about:blank';
             t.done();
           }
       });

tests/wpt/web-platform-tests/html/semantics/document-metadata/the-base-element/base_multiple.html

@@ -5,33 +5,25 @@
 <link rel="help" href="https://html.spec.whatwg.org/multipage/#the-base-element">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
-<body onload="on_load()">
+<body>
   <div id="log"></div>
   <iframe id="test1" src="example.html" style="width:0;height:0" frameborder="0"></iframe>
   <iframe id="test2" src="example.html" name="targetWin" style="width:0;height:0" frameborder="0"></iframe>
   <script>
-    var t = async_test("The attributes of the a element must be affected by the first base element"),
+    async_test(function() {
-        doc1,
+      window.onload = this.step_func(function() {
-        fr2,
+        var fr1 = document.getElementById("test1");
-        a1;
+        fr1.addEventListener("load", this.unreached_func("loaded in the wrong iframe"));
 
-    function on_load() {
+        var fr2 = document.getElementById("test2");
-      setup(function (){
+        fr2.addEventListener("load", this.step_func_done(function () {
-        doc1 = document.getElementById("test1").contentDocument;
-        fr2 = document.getElementById("test2");
-        a1 = doc1.getElementById("a1");
-      });
-
-      fr2.addEventListener("load", function () {
-        t.step(function () {
           var doc2 = fr2.contentDocument;
           assert_not_equals(doc2.location.href.indexOf("example2.html"), -1, "The target attribute does not impact the a element.");
           assert_equals(doc2.getElementById("d1").innerHTML, "PASS", "The opend page should be the example2.html.");
-        });
+        }), true);
-        t.done();
-      }, true);
 
-      a1.click();
+        fr1.contentDocument.getElementById("a1").click();
-    }
+      });
+    }, "The attributes of the a element must be affected by the first base element");
   </script>
 </body>