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

SwagColoredKitteh · 2017-01-22T15:31:32Z

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 <link> 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

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15101 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-01-22T15:31:38Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

highfive · 2017-01-22T15:31:38Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmllinkelement.rs
@KiChjang: components/script/dom/htmllinkelement.rs

jdm · 2017-01-22T15:44:39Z

The way we handle the timing issue in other code like XMLHttpRequest is to store a monotonically-increasing id in the element and the network load context, and compare them in the implementation of PreInvoke.

SwagColoredKitteh · 2017-01-22T15:48:11Z

Alright, should I add that to this PR? Is there also a way to write a WPT that checks for these out-of-order responses?

jdm · 2017-01-22T17:24:56Z

You could write a test that first loads a resource that has a 3 second response delay, then immediately loads a second one with no delay, then waits for 4 seconds and queries the computed style of an element that would be affected.

emilio · 2017-01-23T12:55:45Z

components/script/dom/htmllinkelement.rs

-        assert!(self.stylesheet.borrow().is_none());
-        *self.stylesheet.borrow_mut() = Some(s);
+        let mut sheet = self.stylesheet.borrow_mut();
+        let must_invalidate = sheet.is_some();


I think we don't need to call invalidate_stylesheets here, since the stylesheet loader will do it right after calling set_stylesheet.

Remove this code and maybe add a note about it?

It might be better to just leave the assert there as it is and do what @jdm suggested, which would fix the underlying issue instead of clear the symptoms. That assert will continue to be useful to find timing issues such as what caused this issue in the first place.

Note that we still need to support setting a stylesheet when it is loaded and the href changes, or at least (probably preferably?) add support for unsetting it and invalidate_stylesheets after the href changes.

So yeah, let's keep the assertion there, let's add the load counter, and also let's move this must_invalidate logic to handle_stylesheet_url, and unset the stylesheet there.

SwagColoredKitteh · 2017-01-23T18:24:59Z

Did what @jdm suggested, out-of-order stylesheet responses for the same element should be handled correctly now. There are also 2 tests which verify that this works correctly. These tests do time out, even though they should not, though. Added exceptions for those timeouts in the inis files.

jdm · 2017-01-23T18:35:08Z

#15159 was filed for followup investigation into the timeouts.

emilio · 2017-01-23T19:14:49Z

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 GenerationId(u32);


nit: Maybe RequestGenerationId?

emilio · 2017-01-23T19:14:49Z

components/script/stylesheet_loader.rs

+    fn should_invoke(&self) -> bool {
+        // 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 gen = self.elem.root()


We should invoke the callback always, just not set the stylesheet if the generation doesn't match. Otherwise we will lose track of the pending load count, which means we'll never fire the load event.

Also, elem may be a <style> element where there's an @import, or the load could be for an @import rule in general, in which case the generation id doesn't matter. So we should do the check in process_response_eof, an we should check that StylesheetContextSource is a LinkElement.

Note that the imports can theoretically have the same race if mutated from CSSOM before the load finishes, but we don't expose that right now to the web, so it can't happen, so no need to worry about this in this PR.

Actually, the above comment may be the reason why the tests timeout :)

emilio · 2017-01-23T19:14:49Z

components/script/stylesheet_loader.rs

@@ -215,13 +227,17 @@ impl<'a> StylesheetLoader<'a> {
                integrity_metadata: String) {
        let url = source.url();
        let document = document_from_node(self.elem);
+        let gen = self.elem.downcast::<HTMLLinkElement>()


Similarly, you can't rely on this being a HTMLLinkElement.

SwagColoredKitteh · 2017-01-24T13:08:59Z

Fixed issue #15159 and made sure that style elements with imports do not crash servo by not assuming that some downcasts always succeed, also did a few cleanups.

emilio

Looks great! I have one minor request, after that this is good to go :)

Thanks for fixing this @SwagColoredKitteh!

emilio · 2017-01-24T13:39:09Z

components/script/stylesheet_loader.rs

+                    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 should_continue = match self.request_generation_id {


let's give this variable a meaningful name, something like:

let is_applicable_stylesheet_load = self.request_generation_id.map_or(true, |id| id == link.get_request_generation_id());

(or other name if you want, I'm pretty bad at it :))

emilio · 2017-01-24T13:39:09Z

components/script/stylesheet_loader.rs

+                    // else we risk applying the wrong stylesheet when responses come out-of-order.
+                    let should_continue = match self.request_generation_id {
+                        Some(gen) => gen == link.get_request_generation_id(),
+                        None      => true,


nit: I don't think we use to indent fat arrows visually. In any case, you may be able to write this in a more compact way with map_or, as I did above.

emilio · 2017-01-24T13:40:02Z

Also, travis seems to be complaining about the manifest order, you need to fix that I think: You can run etc/ci/manifest_changed.sh locally.

emilio · 2017-01-24T14:30:57Z

@bors-servo r+

Thanks for doing this! :)

bors-servo · 2017-01-24T14:30:58Z

📌 Commit abebf34 has been approved by emilio

SwagColoredKitteh · 2017-01-24T14:37:37Z

@emilio My pleasure! I had a lot of fun searching around the codebase and figuring out what went wrong. :D


        fix issue #15101 and make sure out-of-order stylesheet loads work cor…

…rectly for the same element


        add tests for stylesheet responses coming out-of-order for the same e…

…lement

jdm · 2017-01-24T14:59:18Z

@bors-servo: r=emilio

bors-servo · 2017-01-24T14:59:19Z

📌 Commit 068f960 has been approved by emilio

bors-servo · 2017-01-24T15:06:53Z

⌛ Testing commit 068f960 with merge c3f0c90...


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

@emilio

Prevent crashing when a link tag has two or more in-flight requests (fix for issue #15101)  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 <link> 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 ---  - [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).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15145)

bors-servo · 2017-01-24T16:02:08Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned emilio Jan 22, 2017

highfive added the S-awaiting-review label Jan 22, 2017

emilio requested changes Jan 23, 2017

View changes

SwagColoredKitteh force-pushed the SwagColoredKitteh:issue-15101 branch from 367c10d to 23a5fdc Jan 23, 2017

emilio requested changes Jan 23, 2017

View changes

emilio mentioned this pull request Jan 23, 2017

WPTs for PR #15145 pass, but the harness times out #15159

Closed

SwagColoredKitteh force-pushed the SwagColoredKitteh:issue-15101 branch from 23a5fdc to 86f3209 Jan 24, 2017

emilio approved these changes Jan 24, 2017

View changes

SwagColoredKitteh force-pushed the SwagColoredKitteh:issue-15101 branch from 86f3209 to abebf34 Jan 24, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 24, 2017

SwagColoredKitteh added 2 commits Jan 23, 2017

fix issue #15101 and make sure out-of-order stylesheet loads work cor…

bb2826e

…rectly for the same element

add tests for stylesheet responses coming out-of-order for the same e…

Loading status checks…

068f960

…lement

SwagColoredKitteh force-pushed the SwagColoredKitteh:issue-15101 branch from abebf34 to 068f960 Jan 24, 2017

highfive added S-awaiting-review and removed S-awaiting-merge labels Jan 24, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 24, 2017

bors-servo merged commit 068f960 into servo:master Jan 24, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 24, 2017

servo / servo

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

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

SwagColoredKitteh commented Jan 22, 2017 •

edited by larsbergstrom

highfive commented Jan 22, 2017

highfive commented Jan 22, 2017

jdm commented Jan 22, 2017

SwagColoredKitteh commented Jan 22, 2017

jdm commented Jan 22, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SwagColoredKitteh commented Jan 23, 2017

jdm commented Jan 23, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SwagColoredKitteh commented Jan 24, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 24, 2017 •

edited

emilio commented Jan 24, 2017

bors-servo commented Jan 24, 2017

SwagColoredKitteh commented Jan 24, 2017

jdm commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

servo / servo

Join GitHub today

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

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

Conversation

SwagColoredKitteh commented Jan 22, 2017 • edited by larsbergstrom

highfive commented Jan 22, 2017

highfive commented Jan 22, 2017

jdm commented Jan 22, 2017

SwagColoredKitteh commented Jan 22, 2017

jdm commented Jan 22, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SwagColoredKitteh commented Jan 23, 2017

jdm commented Jan 23, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SwagColoredKitteh commented Jan 24, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 24, 2017 • edited

emilio commented Jan 24, 2017

bors-servo commented Jan 24, 2017

SwagColoredKitteh commented Jan 24, 2017

jdm commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

SwagColoredKitteh commented Jan 22, 2017 •

edited by larsbergstrom

emilio commented Jan 24, 2017 •

edited