Auto merge of #15145 - SwagColoredKitteh:issue-15101, r=emilio

Prevent crashing when a link tag has two or more in-flight requests (fix for issue #15101)

<!-- Please describe your changes on the following line: -->
The `HTMLLinkElement::set_stylesheet` function now checks whether there already is a stylesheet, and if there is, calls `Document::invalidate_stylesheets` after modifying `self.stylesheet`.

This PR also includes a minimal WPT that causes the panic.

This is fundamentally a timing issue, so while this fix prevents the crash, it does not fix the underlying issue. Making a &lt;link&gt; element send a second request before the first can finish and then getting the two stylesheet responses out-of-order will apply the wrong stylesheet, as demonstrated with https://gist.github.com/SwagColoredKitteh/2c24c7fac635445042eda4a30e10420e.

r? @emilio

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

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

<!-- 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/15145)
<!-- Reviewable:end -->

components/script/dom/htmllinkelement.rs

@@ -37,6 +37,15 @@ use stylesheet_loader::{StylesheetLoader, StylesheetContextSource, StylesheetOwn
 
 unsafe_no_jsmanaged_fields!(Stylesheet);
 
+#[derive(JSTraceable, PartialEq, Clone, Copy, HeapSizeOf)]
+pub struct RequestGenerationId(u32);
+
+impl RequestGenerationId {
+    fn increment(self) -> RequestGenerationId {
+        RequestGenerationId(self.0 + 1)
+    }
+}
+
 #[dom_struct]
 pub struct HTMLLinkElement {
     htmlelement: HTMLElement,
@@ -52,6 +61,8 @@ pub struct HTMLLinkElement {
     pending_loads: Cell<u32>,
     /// Whether any of the loads have failed.
     any_failed_load: Cell<bool>,
+    /// A monotonically increasing counter that keeps track of which stylesheet to apply.
+    request_generation_id: Cell<RequestGenerationId>,
 }
 
 impl HTMLLinkElement {
@@ -65,6 +76,7 @@ impl HTMLLinkElement {
             cssom_stylesheet: MutNullableJS::new(None),
             pending_loads: Cell::new(0),
             any_failed_load: Cell::new(false),
+            request_generation_id: Cell::new(RequestGenerationId(0)),
         }
     }
 
@@ -78,11 +90,14 @@ impl HTMLLinkElement {
                            HTMLLinkElementBinding::Wrap)
     }
 
-    pub fn set_stylesheet(&self, s: Arc<Stylesheet>) {
-        assert!(self.stylesheet.borrow().is_none());
-        *self.stylesheet.borrow_mut() = Some(s);
+    pub fn get_request_generation_id(&self) -> RequestGenerationId {
+        self.request_generation_id.get()
     }
 
+    pub fn set_stylesheet(&self, s: Arc<Stylesheet>) {
+        assert!(self.stylesheet.borrow().is_none()); // Useful for catching timing issues.
+        *self.stylesheet.borrow_mut() = Some(s);
+    }
 
     pub fn get_stylesheet(&self) -> Option<Arc<Stylesheet>> {
         self.stylesheet.borrow().clone()
@@ -260,6 +275,8 @@ impl HTMLLinkElement {
             None => "",
         };
 
+        self.request_generation_id.set(self.request_generation_id.get().increment());
+
         // TODO: #8085 - Don't load external stylesheets if the node's mq
         // doesn't match.
         let loader = StylesheetLoader::for_element(self.upcast());

components/script/stylesheet_loader.rs

@@ -10,7 +10,7 @@ use dom::document::Document;
 use dom::element::Element;
 use dom::eventtarget::EventTarget;
 use dom::htmlelement::HTMLElement;
-use dom::htmllinkelement::HTMLLinkElement;
+use dom::htmllinkelement::{RequestGenerationId, HTMLLinkElement};
 use dom::node::{document_from_node, window_from_node};
 use encoding::EncodingRef;
 use encoding::all::UTF_8;
@@ -85,6 +85,9 @@ pub struct StylesheetContext {
     /// The node document for elem when the load was initiated.
     document: Trusted<Document>,
     origin_clean: bool,
+    /// A token which must match the generation id of the `HTMLLinkElement` for it to load the stylesheet.
+    /// This is ignored for `HTMLStyleElement` and imports.
+    request_generation_id: Option<RequestGenerationId>,
 }
 
 impl PreInvoke for StylesheetContext {}
@@ -143,24 +146,30 @@ impl FetchResponseListener for StylesheetContext {
             let loader = StylesheetLoader::for_element(&elem);
             match self.source {
                 StylesheetContextSource::LinkElement { ref mut media, .. } => {
-                    let sheet =
-                        Arc::new(Stylesheet::from_bytes(&data, final_url,
-                                                        protocol_encoding_label,
-                                                        Some(environment_encoding),
-                                                        Origin::Author,
-                                                        media.take().unwrap(),
-                                                        Some(&loader),
-                                                        win.css_error_reporter(),
-                                                        ParserContextExtraData::default()));
-                    if elem.downcast::<HTMLLinkElement>().unwrap().is_alternate() {
-                        sheet.set_disabled(true);
-                    }
-                    elem.downcast::<HTMLLinkElement>()
-                        .unwrap()
-                        .set_stylesheet(sheet.clone());
+                    let link = elem.downcast::<HTMLLinkElement>().unwrap();
+                    // We must first check whether the generations of the context and the element match up,
+                    // else we risk applying the wrong stylesheet when responses come out-of-order.
+                    let is_stylesheet_load_applicable =
+                        self.request_generation_id.map_or(true, |gen| gen == link.get_request_generation_id());
+                    if is_stylesheet_load_applicable {
+                        let sheet =
+                            Arc::new(Stylesheet::from_bytes(&data, final_url,
+                                                            protocol_encoding_label,
+                                                            Some(environment_encoding),
+                                                            Origin::Author,
+                                                            media.take().unwrap(),
+                                                            Some(&loader),
+                                                            win.css_error_reporter(),
+                                                            ParserContextExtraData::default()));
 
-                    let win = window_from_node(&*elem);
-                    win.layout_chan().send(Msg::AddStylesheet(sheet)).unwrap();
+                        if link.is_alternate() {
+                            sheet.set_disabled(true);
+                        }
+
+                        link.set_stylesheet(sheet.clone());
+
+                        win.layout_chan().send(Msg::AddStylesheet(sheet)).unwrap();
+                    }
                 }
                 StylesheetContextSource::Import(ref import) => {
                     let import = import.read();
@@ -215,6 +224,8 @@ impl<'a> StylesheetLoader<'a> {
                 integrity_metadata: String) {
         let url = source.url();
         let document = document_from_node(self.elem);
+        let gen = self.elem.downcast::<HTMLLinkElement>()
+                           .map(HTMLLinkElement::get_request_generation_id);
         let context = Arc::new(Mutex::new(StylesheetContext {
             elem: Trusted::new(&*self.elem),
             source: source,
@@ -222,6 +233,7 @@ impl<'a> StylesheetLoader<'a> {
             data: vec![],
             document: Trusted::new(&*document),
             origin_clean: true,
+            request_generation_id: gen,
         }));
 
         let (action_sender, action_receiver) = ipc::channel().unwrap();

tests/wpt/mozilla/meta/MANIFEST.json

@@ -8762,6 +8762,18 @@
             "url": "/_mozilla/mozilla/node_replaceChild.html"
           }
         ],
+        "mozilla/out-of-order-stylesheet-loads-and-imports.html": [
+          {
+            "path": "mozilla/out-of-order-stylesheet-loads-and-imports.html",
+            "url": "/_mozilla/mozilla/out-of-order-stylesheet-loads-and-imports.html"
+          }
+        ],
+        "mozilla/out-of-order-stylesheet-loads.html": [
+          {
+            "path": "mozilla/out-of-order-stylesheet-loads.html",
+            "url": "/_mozilla/mozilla/out-of-order-stylesheet-loads.html"
+          }
+        ],
         "mozilla/parentNode_querySelector.html": [
           {
             "path": "mozilla/parentNode_querySelector.html",

tests/wpt/mozilla/tests/mozilla/out-of-order-stylesheet-loads-and-imports.html

@@ -0,0 +1,18 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Out-of-order stylesheet loads for the same element happen correctly, even with imports (issue #15101)</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+async_test(function(t) {
+  var link = document.createElement("link");
+  link.rel = "stylesheet";
+  link.href = "resources/imports-background-red.css?pipe=trickle(d3)";
+  document.head.appendChild(link);
+  link.href = "resources/imports-background-green.css";
+  t.step_timeout(function() {
+    assert_equals(getComputedStyle(document.body).getPropertyValue("background-color"), "rgb(0, 128, 0)");
+    t.done();
+  }, 4000);
+}, "out-of-order stylesheet loads for the same element happen correctly, even with imports");
+</script>

tests/wpt/mozilla/tests/mozilla/out-of-order-stylesheet-loads.html

@@ -0,0 +1,18 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Out-of-order stylesheet loads for the same element happen correctly (issue #15101)</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+async_test(function(t) {
+  var link = document.createElement("link");
+  link.rel = "stylesheet";
+  link.href = "resources/background-red.css?pipe=trickle(d3)";
+  document.head.appendChild(link);
+  link.href = "resources/background-green.css";
+  t.step_timeout(function() {
+    assert_equals(getComputedStyle(document.body).getPropertyValue("background-color"), "rgb(0, 128, 0)");
+    t.done();
+  }, 4000);
+}, "out-of-order stylesheet loads for the same element happen correctly");
+</script>

tests/wpt/mozilla/tests/mozilla/resources/background-green.css

@@ -0,0 +1,3 @@
+body {
+  background-color: green;
+}

tests/wpt/mozilla/tests/mozilla/resources/background-red.css

@@ -0,0 +1,3 @@
+body {
+  background-color: red;
+}

tests/wpt/mozilla/tests/mozilla/resources/imports-background-green.css

@@ -0,0 +1 @@
+@import url("background-green.css");

tests/wpt/mozilla/tests/mozilla/resources/imports-background-red.css

@@ -0,0 +1 @@
+@import url("background-red.css");