change parse_own_css to queue event not fire synchronously #19307

avadacatavra · 2017-11-21T00:41:08Z

fixes a panic and aligns with spec

I've also added checks around each mutable borrow of the tokenizer to see if we can catch any other panics

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix DomRefCell<T> already borrowed: BorrowMutError #18930 (github issue number if applicable).

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

This change is

highfive · 2017-11-21T00:41:15Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmlstyleelement.rs, components/script/dom/servoparser/mod.rs
@KiChjang: components/script/dom/htmlstyleelement.rs, components/script/dom/servoparser/mod.rs

highfive · 2017-11-21T00:41:16Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

avadacatavra · 2017-11-21T00:41:24Z

r? jdm

jdm · 2017-11-21T00:50:01Z

components/script/dom/servoparser/mod.rs

@@ -175,6 +175,7 @@ impl ServoParser {
        document.set_current_parser(Some(&parser));
        if !type_.eq_ignore_ascii_case("text/html") {
            parser.parse_string_chunk("<pre>\n".to_owned());
+            assert!(parser.tokenizer.try_borrow_mut().is_ok());


Asserting here is no different than performing the infallible borrow. Rather than adding these extra asserts everywhere, I would like a single API on ServoParser called something like parser_is_not_active (and an API on Document named something like can_invoke_script, which has access to the parser), and we should assert that that API returns true when we're asked to dispatch an event in EventTarget.

avadacatavra · 2017-11-21T12:03:36Z

@jdm not sure if i should queue the events if the parser is in use?

jdm · 2017-11-21T12:57:15Z

No, we don't want to change behaviour based on this. We simply want to expose potential problems that could end up interacting with the parser if they invoke document.write.

jdm

To make sure this works locally, please run ./mach test-wpt workers/. This would have caught the problem with unwrapping the the downcast to Window, which doesn't work in a non-window global.

jdm · 2017-11-21T13:00:29Z

components/script/dom/eventtarget.rs

@@ -315,11 +315,35 @@ impl EventTarget {
    pub fn dispatch_event_with_target(&self,
                                      target: &EventTarget,
                                      event: &Event) -> EventStatus {
-        event.dispatch(self, Some(target))


Assert here, and let's match on target.global().downcast::<Window>() instead of trying Element (keeping in mind that None is valid).

jdm · 2017-11-21T13:01:18Z

components/script/dom/eventtarget.rs

    }

    pub fn dispatch_event(&self, event: &Event) -> EventStatus {
-        event.dispatch(self, None)


Assert here, and do the same as the previous comment but using self.

jdm · 2017-11-21T19:58:13Z

@bors-servo: try

bors-servo · 2017-11-21T19:58:18Z

⌛ Trying commit 0269db1 with merge 553e4ea...

change parse_own_css to queue event not fire synchronously  fixes a panic and aligns with spec I've also added checks around each mutable borrow of the tokenizer to see if we can catch any other panics ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18930 (github issue number if applicable).  - [ ] 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/19307)

bors-servo · 2017-11-21T20:55:30Z

💔 Test failed - mac-rel-wpt3

avadacatavra · 2017-11-21T21:07:14Z

@bors-servo retry

change parse_own_css to queue event not fire synchronously  fixes a panic and aligns with spec I've also added checks around each mutable borrow of the tokenizer to see if we can catch any other panics ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18930 (github issue number if applicable).  - [ ] 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/19307)

bors-servo · 2017-11-21T21:07:19Z

⌛ Trying commit 0269db1 with merge 167c884...

bors-servo · 2017-11-21T21:46:14Z

💔 Test failed - mac-rel-wpt3

jdm

Looks good!

jdm · 2017-11-29T22:39:16Z

.../web-platform-tests/html/semantics/document-metadata/the-style-element/style_load_async.html

+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style onload="document.blah = true">
+</style>


This style block can be removed now.

jdm · 2017-11-29T22:40:11Z

tests/wpt/metadata/custom-elements/parser/parser-fallsback-to-unknown-element.html.ini

@@ -0,0 +1,3 @@
+;bug: https://github.com/servo/servo/issues/19392


This (and the other ini file) should be:

[parser-fallsback-to-unknown-element.html] expected: CRASH bug: https://github.com/servo/servo/issues/19392

jdm · 2017-11-29T22:42:10Z

components/script/dom/servoparser/mod.rs

@@ -102,6 +102,10 @@ enum LastChunkState {
 }

 impl ServoParser {
+    pub fn parser_is_not_active(&self) -> bool {
+        self.tokenizer.try_borrow_mut().is_ok()


Let's add || self.can_write(), so that this is handled correctly when #19393 is fixed.

jdm · 2017-11-29T22:43:03Z

components/script/dom/eventtarget.rs

@@ -315,10 +315,23 @@ impl EventTarget {
    pub fn dispatch_event_with_target(&self,
                                      target: &EventTarget,
                                      event: &Event) -> EventStatus {
+        match target.global().downcast::<Window>() {
+            Some(window) => if window.has_document() {


Let's use if let

jdm · 2017-11-29T22:43:08Z

components/script/dom/eventtarget.rs

        event.dispatch(self, Some(target))
    }

    pub fn dispatch_event(&self, event: &Event) -> EventStatus {
+        match self.global().downcast::<Window>() {


created checks to see if parser is in use before event dispatch changed tests to expect crash and added async style test

jdm · 2017-12-01T17:17:31Z

@bors-servo: r+

bors-servo · 2017-12-01T17:17:32Z

📌 Commit 79d896d has been approved by jdm

bors-servo · 2017-12-01T17:41:49Z

⌛ Testing commit 79d896d with merge 712d75a...

change parse_own_css to queue event not fire synchronously  fixes a panic and aligns with spec I've also added checks around each mutable borrow of the tokenizer to see if we can catch any other panics ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18930 (github issue number if applicable).  - [ ] 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/19307)

bors-servo · 2017-12-01T18:58:06Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 712d75a to master...

created checks to see if parser is in use before event dispatch changed tests to expect crash and added async style test Upstreamed from servo/servo#19307 [ci skip]

highfive assigned wafflespeanut Nov 21, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 21, 2017

highfive assigned jdm and unassigned wafflespeanut Nov 21, 2017

jdm requested changes Nov 21, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 21, 2017

avadacatavra force-pushed the domrefcellpanic branch from c6a1549 to d8e0dba Compare November 21, 2017 12:02

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 21, 2017

jdm requested changes Nov 21, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 21, 2017

avadacatavra force-pushed the domrefcellpanic branch from d8e0dba to 0269db1 Compare November 21, 2017 18:39

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 21, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Nov 21, 2017

jdm added S-needs-tests New tests have been requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 27, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 28, 2017

avadacatavra force-pushed the domrefcellpanic branch 3 times, most recently from 7eafa4f to 62caa26 Compare November 28, 2017 17:44

avadacatavra removed the S-needs-tests New tests have been requested by a reviewer. label Nov 28, 2017

avadacatavra force-pushed the domrefcellpanic branch 3 times, most recently from d10cd00 to a6ecc75 Compare November 29, 2017 16:37

jdm requested changes Nov 29, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 29, 2017

avadacatavra force-pushed the domrefcellpanic branch from a6ecc75 to a1abe94 Compare November 30, 2017 14:43

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 30, 2017

queue event instead of immediately fire

79d896d

created checks to see if parser is in use before event dispatch changed tests to expect crash and added async style test

avadacatavra force-pushed the domrefcellpanic branch from a1abe94 to 79d896d Compare November 30, 2017 16:19

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 1, 2017

bors-servo merged commit 79d896d into servo:master Dec 1, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 1, 2017

servo-wpt-sync mentioned this pull request Jan 4, 2018

queue event instead of immediately fire web-platform-tests/wpt#8909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change parse_own_css to queue event not fire synchronously #19307

change parse_own_css to queue event not fire synchronously #19307

avadacatavra commented Nov 21, 2017 •

edited by SimonSapin

highfive commented Nov 21, 2017

highfive commented Nov 21, 2017

avadacatavra commented Nov 21, 2017

jdm Nov 21, 2017

avadacatavra commented Nov 21, 2017

jdm commented Nov 21, 2017 •

edited

jdm left a comment

jdm Nov 21, 2017

jdm Nov 21, 2017

jdm commented Nov 21, 2017

bors-servo commented Nov 21, 2017

bors-servo commented Nov 21, 2017

avadacatavra commented Nov 21, 2017

bors-servo commented Nov 21, 2017

bors-servo commented Nov 21, 2017

jdm left a comment

jdm Nov 29, 2017

jdm Nov 29, 2017

jdm Nov 29, 2017

jdm Nov 29, 2017

jdm Nov 29, 2017

jdm commented Dec 1, 2017

bors-servo commented Dec 1, 2017

bors-servo commented Dec 1, 2017

bors-servo commented Dec 1, 2017

		@@ -0,0 +1,3 @@
		;bug: https://github.com/servo/servo/issues/19392

change parse_own_css to queue event not fire synchronously #19307

change parse_own_css to queue event not fire synchronously #19307

Conversation

avadacatavra commented Nov 21, 2017 • edited by SimonSapin

highfive commented Nov 21, 2017

highfive commented Nov 21, 2017

avadacatavra commented Nov 21, 2017

Choose a reason for hiding this comment

avadacatavra commented Nov 21, 2017

jdm commented Nov 21, 2017 • edited

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Nov 21, 2017

bors-servo commented Nov 21, 2017

bors-servo commented Nov 21, 2017

avadacatavra commented Nov 21, 2017

bors-servo commented Nov 21, 2017

bors-servo commented Nov 21, 2017

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Dec 1, 2017

bors-servo commented Dec 1, 2017

bors-servo commented Dec 1, 2017

bors-servo commented Dec 1, 2017

avadacatavra commented Nov 21, 2017 •

edited by SimonSapin

jdm commented Nov 21, 2017 •

edited