Return Option for Window's layout channel #27163

alarsyo · 2020-07-03T15:33:22Z

Window::layout_chan() now returns an Option<Sender<Msg>>, returning None if the window is dead.

FIX #26969
FIX #26429
FIX #21208
FIX #19092
FIX #22559
FIX #22584
FIX #22652

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Layout queries on elements in disconnected frames panics #23053

There are tests for these changes

This is my first contribution, I'm trying to figure things out!

This fix passes the test case shown in #23053, however I don't know what the behavior should be in Document and ScriptThread if Window::is_alive() is false : simply ignore it, don't do anything ? Or is this something that should not happen now that we return false in Window::force_reflow() ?

I'm not sure about the directory where the test case should go, any advice?

highfive · 2020-07-03T15:33:31Z

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

highfive · 2020-07-03T15:33:32Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/script_thread.rs, components/script/dom/document.rs, components/script/dom/window.rs
@KiChjang: components/script/script_thread.rs, components/script/dom/document.rs, components/script/dom/window.rs

highfive · 2020-07-03T15:33:34Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

servo-wpt-sync · 2020-07-03T15:34:02Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#24436.

components/script/dom/window.rs

servo-wpt-sync · 2020-07-03T16:05:07Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#24436.

jdm · 2020-07-03T16:09:59Z

tests/wpt/web-platform-tests/fixme/detached-layout.html

@@ -0,0 +1,23 @@
+<!doctype html>


This test can be moved to tests/wpt/mozilla/tests/mozilla. These are WPT tests that we don't upstream and are used for checking regressions in specific cases in our engine, rather than conformance tests for all engines.

jdm · 2020-07-03T16:09:59Z

components/script/script_thread.rs

@@ -2472,7 +2473,7 @@ impl ScriptThread {
            .borrow()
            .iter()
            .next()
-            .map(|(_, document)| document.window().layout_chan().clone())
+            .map(|(_, document)| document.window().layout_chan().unwrap().clone())


This map can become and_then and we can remove the unwrap.

jdm · 2020-07-03T16:09:59Z

components/script/dom/window.rs

@@ -1553,7 +1556,8 @@ impl Window {
        // TODO Step 1
        // TODO(mrobinson, #18709): Add smooth scrolling support to WebRender so that we can
        // properly process ScrollBehavior here.
-        self.layout_chan
+        self.layout_chan()
+            .unwrap()


We'll want to replace these unwraps in this PR with warn! statements and return from the code as soon as possible.

servo-wpt-sync · 2020-07-03T17:52:11Z

No upstreamable changes; closed existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#24436.

alarsyo · 2020-07-03T18:09:09Z

components/script/script_thread.rs

-        let _ = window
-            .layout_chan()
-            .send(Msg::RegisterPaint(name, properties, painter));
+
+        match window.layout_chan() {
+            Some(chan) => chan
+                .send(Msg::RegisterPaint(name, properties, painter))
+                .unwrap(),
+            None => warn!("Layout channel unavailable"),
+        }


Not sure why the Result was ignored here and not unwrapped like everywhere else, I've changed it to match with other uses of this channel

jdm · 2020-07-03T18:11:03Z

@bors-servo try=wpt

…es-panic, r=<try> Return Option for Window's layout channel  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #23053  - [x] There are tests for these changes   This is my first contribution, I'm trying to figure things out! This fix passes the test case shown in #23053, however I don't know what the behavior should be in `Document` and `ScriptThread` if `Window::is_alive()` is false : simply ignore it, don't do anything ? Or is this something that should not happen now that we return false in `Window::force_reflow()` ? I'm not sure about the directory where the test case should go, any advice?

bors-servo · 2020-07-03T18:11:09Z

⌛ Trying commit 36fbbea with merge 9047dcf...

bors-servo · 2020-07-03T19:17:45Z

☀️ Test successful - status-taskcluster
State: approved= try=True

gterzian · 2020-07-04T03:56:17Z

I think we can make this PR also close all the open issues mentioned at #22507 (comment)

While leaving #22507 itself open, since we still have the underlying problem of perhaps closing the layout thread too early(or in other words exiting the pipeline, while the related browsing context is still alive even though not fully active), which requires some further investigation, such as what happens if a disconnected iframe is re-connected into the document.

alarsyo · 2020-07-04T04:07:32Z

Have I done this right? :)

gterzian · 2020-07-04T04:27:51Z

Have I done this right? :)

Looks like it, Github as added them to list of closing issues on the right panel..

Great work, thanks!

@bors-servo r=jdm

bors-servo · 2020-07-04T04:27:52Z

📌 Commit 36fbbea has been approved by jdm

bors-servo · 2020-07-04T04:27:57Z

⌛ Testing commit 36fbbea with merge a8f9ab7...

…es-panic, r=jdm Return Option for Window's layout channel  `Window::layout_chan()` now returns an `Option<Sender<Msg>>`, returning `None` if the window is dead. FIX #26969 FIX #26429 FIX #21208 FIX #19092 FIX #22559 FIX #22584 FIX #22652 ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #23053  - [x] There are tests for these changes   This is my first contribution, I'm trying to figure things out! This fix passes the test case shown in #23053, however I don't know what the behavior should be in `Document` and `ScriptThread` if `Window::is_alive()` is false : simply ignore it, don't do anything ? Or is this something that should not happen now that we return false in `Window::force_reflow()` ? I'm not sure about the directory where the test case should go, any advice?

bors-servo · 2020-07-04T05:24:41Z

💔 Test failed - status-taskcluster

CYBAI · 2020-07-04T05:30:24Z

@bors-servo retry

Intermittent mac timeout due to missing font URLs (No URL for Core Text font!) #24622

bors-servo · 2020-07-04T05:30:30Z

⌛ Testing commit 36fbbea with merge 8916a42...

bors-servo · 2020-07-04T05:41:52Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 8916a42 to master...

highfive assigned jdm Jul 3, 2020

highfive added the S-awaiting-review label Jul 3, 2020

servo-wpt-sync mentioned this pull request Jul 3, 2020

Return Option for Window's layout channel web-platform-tests/wpt#24436

Closed

alarsyo reviewed Jul 3, 2020

View changes

components/script/dom/window.rs Show resolved Hide resolved

alarsyo force-pushed the 23053-layout-queries-disconnected-frames-panic branch from 8768532 to 9483686 Compare Jul 3, 2020

jdm requested changes Jul 3, 2020

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 3, 2020

alarsyo force-pushed the 23053-layout-queries-disconnected-frames-panic branch from 9483686 to d6ba755 Compare Jul 3, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 3, 2020

Return Option for Window's layout channel

36fbbea

alarsyo force-pushed the 23053-layout-queries-disconnected-frames-panic branch from d6ba755 to 36fbbea Compare Jul 3, 2020

alarsyo marked this pull request as ready for review Jul 3, 2020

alarsyo reviewed Jul 3, 2020

View changes

highfive removed the S-awaiting-review label Jul 4, 2020

highfive added the S-awaiting-merge label Jul 4, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 4, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 4, 2020

bors-servo merged commit 8916a42 into servo:master Jul 4, 2020
2 checks passed

highfive removed the S-awaiting-merge label Jul 4, 2020

Return Option for Window's layout channel #27163

Return Option for Window's layout channel #27163

alarsyo commented Jul 3, 2020 •

edited

highfive commented Jul 3, 2020

highfive commented Jul 3, 2020

highfive commented Jul 3, 2020

servo-wpt-sync commented Jul 3, 2020

servo-wpt-sync commented Jul 3, 2020

jdm Jul 3, 2020

jdm Jul 3, 2020

jdm Jul 3, 2020

servo-wpt-sync commented Jul 3, 2020

alarsyo Jul 3, 2020

jdm commented Jul 3, 2020

bors-servo commented Jul 3, 2020

bors-servo commented Jul 3, 2020

gterzian commented Jul 4, 2020 •

edited

alarsyo commented Jul 4, 2020

gterzian commented Jul 4, 2020

bors-servo commented Jul 4, 2020

bors-servo commented Jul 4, 2020

bors-servo commented Jul 4, 2020

CYBAI commented Jul 4, 2020

bors-servo commented Jul 4, 2020

bors-servo commented Jul 4, 2020

Return Option for Window's layout channel #27163

Return Option for Window's layout channel #27163

Conversation

alarsyo commented Jul 3, 2020 • edited

highfive commented Jul 3, 2020

highfive commented Jul 3, 2020

highfive commented Jul 3, 2020

servo-wpt-sync commented Jul 3, 2020

servo-wpt-sync commented Jul 3, 2020

jdm Jul 3, 2020

Choose a reason for hiding this comment

jdm Jul 3, 2020

Choose a reason for hiding this comment

jdm Jul 3, 2020

Choose a reason for hiding this comment

servo-wpt-sync commented Jul 3, 2020

alarsyo Jul 3, 2020

Choose a reason for hiding this comment

jdm commented Jul 3, 2020

bors-servo commented Jul 3, 2020

bors-servo commented Jul 3, 2020

gterzian commented Jul 4, 2020 • edited

alarsyo commented Jul 4, 2020

gterzian commented Jul 4, 2020

bors-servo commented Jul 4, 2020

bors-servo commented Jul 4, 2020

bors-servo commented Jul 4, 2020

CYBAI commented Jul 4, 2020

bors-servo commented Jul 4, 2020

bors-servo commented Jul 4, 2020

alarsyo commented Jul 3, 2020 •

edited

gterzian commented Jul 4, 2020 •

edited