Constellation manages script thread lifetime #14052

asajeffrey · 2016-11-03T19:49:56Z

Yet another step towards fixing #633...

At the moment, script threads are responsible for killing themselves when they have no more pipelines to manage. This is fine right now, because script threads have a root pipeline: once it is closed, everything is closed. It's not fine once we fix #633 because there's a race condition where the constellation could kill the last pipeline in a script thread, then open a new one in the same script thread.

We can't have the constellation block on the script thread, waiting to make sure a pipeline is created, since this could introduce deadlock. The easiest solution is to have the constellation manage the script thread lifetime: when the constellation discovers that a script thread is not managing any live pipelines, it closes the script thread, and updates its own state.

Most of the commits are from #14013, its just "Script thread lifetime is now managed by the constellation." (9ac6e4d) that's new.

cc @jdm @ConnorGBrewster

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes do not require tests because refactoring

This change is

highfive · 2016-11-03T19:50:02Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script/webdriver_handlers.rs, components/script/dom/node.rs, components/script/dom/browsingcontext.rs, components/script/dom/storage.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/document_loader.rs, components/script/dom/nodelist.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/devtools.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script/webdriver_handlers.rs, components/net/http_loader.rs, components/script/dom/node.rs, components/script/dom/browsingcontext.rs, components/script/dom/storage.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/document_loader.rs, components/script/dom/nodelist.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/devtools.rs

highfive · 2016-11-03T19:50:03Z

Warning

These commits modify unsafe code. Please review it carefully!

asajeffrey · 2016-11-03T21:09:33Z

Rebased.

nox

Could you write in the second commit message why switching to a set of documents was needed?

nox · 2016-11-04T11:29:59Z

components/script/dom/nodelist.rs

+        self.offset = self.offset + 1;
+        result
+    }
+}


Why do you need this iterator? We shouldn't use NodeList outside from other DOM stuff IMO, it's quite inefficient.

Also, this iterator continues to increment offset for nothing if self.nodes.Item(self.offset) returned None, and it doesn't provide a size hint so it's probably used inefficiently.

nox · 2016-11-04T11:29:59Z

components/script/script_thread.rs

            },
            DevtoolScriptControlMsg::GetRootNode(id, reply) =>
-                devtools::handle_get_root_node(&context, id, reply),
+                devtools::handle_get_root_node(&*documents, id, reply),


Nit: why is the * suddenly needed?

nox · 2016-11-04T11:29:59Z

components/constellation/constellation.rs

+pub struct ScriptChan {
+    chan: IpcSender<ConstellationControlMsg>,
+    dont_send_or_sync: PhantomData<Rc<()>>,
+}


This seems quite dangerous to me. Could we not rely on Drop? What happens if that value is forgotten?

I disagree that it seems dangerous, but I don't understand why it's needed. Whether it's Sync or not seems immaterial, since we just seem to care that ownership can't be transferred to some non-constellation thread. However, if it's only constructed via the new method and that only returns Rc, this seems to be redundant.

The dont_send_or_sync isn't necessary, it's just there to make sure that any messages that are sent on this channel come from the constellation thread. I could just remove it.

asajeffrey · 2016-11-04T14:19:57Z

@nox: are you commenting on the changes in #14013? This PR is just the last commit. I love dependent PRs, I love dependent PRs...

nox · 2016-11-04T14:57:18Z

r? @jdm

jdm · 2016-11-07T20:01:14Z

components/script/script_thread.rs

+        debug!("Exiting script thread.");
+
+        while let Some(pipeline_id) = self.incomplete_loads.borrow().iter().next().map(|load| load.pipeline_id)
+            .or_else(|| self.documents.borrow().iter().next().map(|doc| doc.global().pipeline_id()))


let mut pipelines = self.incomplete_loads.borrow().iter().map(|load| load.pipeline_id); let mut pipelines = self.documents.borrow().iter().map(|doc| doc.global().pipeline_id()).zip(pipelines); while let Some(pipeline_id) = pipelines.next() {

Also, if we add the ability to iterate over the keys of the documents object, the second iterator becomes clearer.

The problem is we can't have the iterator borrowed while we call handle_exit_pipeline_msg, since it mutably borrows self.documents and self.incomplete_loads.

I'm actually surprised that this works as written, since calling iter() should cause a new iterator to be created every time...

A more workable option is to create a vector from the original iterators and iterate over that.

There is new iterator each time, but that's okay because handle_exit_pipeline_msg removes the entry, so we don't end up infinitely looping. We can just collect the iterators into a vector though, that is probably easier to read.

jdm · 2016-11-07T20:01:14Z

components/constellation/constellation.rs

+    pub fn send(&self, msg: ConstellationControlMsg) -> Result<(), IOError> {
+        self.chan.send(msg)
+    }
+    pub fn new(chan: IpcSender<ConstellationControlMsg>) -> ScriptChan {


Why not make this return Rc<ScriptChan>?

jdm · 2016-11-07T20:01:14Z

components/constellation/constellation.rs

+pub struct ScriptChan {
+    chan: IpcSender<ConstellationControlMsg>,
+    dont_send_or_sync: PhantomData<Rc<()>>,
+}


I disagree that it seems dangerous, but I don't understand why it's needed. Whether it's Sync or not seems immaterial, since we just seem to care that ownership can't be transferred to some non-constellation thread. However, if it's only constructed via the new method and that only returns Rc, this seems to be redundant.

bors-servo · 2016-11-08T18:25:45Z

☔ The latest upstream changes (presumably #14013) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-11-08T21:38:18Z

Looks good!

asajeffrey · 2016-11-08T21:43:15Z

Squashed. @bors-servo r=jdm

bors-servo · 2016-11-08T21:43:16Z

📌 Commit 08effa7 has been approved by jdm


        Script thread lifetime is now managed by the constellation.

notriddle · 2016-11-09T01:32:25Z

@bors-servo r=jdm

bors-servo · 2016-11-09T01:32:25Z

📌 Commit 0e7ffaf has been approved by jdm

bors-servo · 2016-11-09T02:09:22Z

⌛ Testing commit 0e7ffaf with merge 13009fe...


        Auto merge of #14052 - asajeffrey:constellation-manages-script-thread…

@jdm

…-lifetime, r=jdm Constellation manages script thread lifetime  Yet another step towards fixing #633... At the moment, script threads are responsible for killing themselves when they have no more pipelines to manage. This is fine right now, because script threads have a root pipeline: once it is closed, everything is closed. It's not fine once we fix #633 because there's a race condition where the constellation could kill the last pipeline in a script thread, then open a new one in the same script thread. We can't have the constellation block on the script thread, waiting to make sure a pipeline is created, since this could introduce deadlock. The easiest solution is to have the constellation manage the script thread lifetime: when the constellation discovers that a script thread is not managing any live pipelines, it closes the script thread, and updates its own state. Most of the commits are from #14013, its just "Script thread lifetime is now managed by the constellation." (9ac6e4d) that's new. cc @jdm @ConnorGBrewster ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14052)

bors-servo · 2016-11-09T03:09:39Z

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

asajeffrey added I-refactor A-content/script A-constellation labels Nov 3, 2016

highfive assigned nox Nov 3, 2016

highfive added the S-awaiting-review label Nov 3, 2016

asajeffrey force-pushed the asajeffrey:constellation-manages-script-thread-lifetime branch from 9ac6e4d to 774172b Nov 3, 2016

nox requested changes Nov 4, 2016

View changes

asajeffrey force-pushed the asajeffrey:constellation-manages-script-thread-lifetime branch from 774172b to 354501b Nov 4, 2016

asajeffrey added the S-blocked-on-external label Nov 4, 2016

highfive assigned jdm and unassigned nox Nov 4, 2016

jdm requested changes Nov 7, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 7, 2016

highfive added the S-needs-rebase label Nov 8, 2016

asajeffrey force-pushed the asajeffrey:constellation-manages-script-thread-lifetime branch from 354501b to 7624ea7 Nov 8, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 8, 2016

asajeffrey added S-needs-code-changes and removed S-awaiting-review S-blocked-on-external S-needs-rebase labels Nov 8, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 8, 2016

asajeffrey added the S-needs-squash label Nov 8, 2016

jdm removed the S-awaiting-review label Nov 8, 2016

jdm approved these changes Nov 8, 2016

View changes

asajeffrey force-pushed the asajeffrey:constellation-manages-script-thread-lifetime branch from d27a481 to 08effa7 Nov 8, 2016

highfive added the S-awaiting-review label Nov 8, 2016

asajeffrey removed the S-needs-squash label Nov 8, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 8, 2016

Script thread lifetime is now managed by the constellation.

Loading status checks…

0e7ffaf

notriddle force-pushed the asajeffrey:constellation-manages-script-thread-lifetime branch from 08effa7 to 0e7ffaf Nov 9, 2016

highfive added S-awaiting-review and removed S-awaiting-merge labels Nov 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 9, 2016

bors-servo merged commit 0e7ffaf into servo:master Nov 9, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 9, 2016

servo / servo

Constellation manages script thread lifetime #14052

Constellation manages script thread lifetime #14052

asajeffrey commented Nov 3, 2016 •

edited by larsbergstrom

highfive commented Nov 3, 2016

highfive commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

nox left a comment

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.

asajeffrey commented Nov 4, 2016

nox commented Nov 4, 2016

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 8, 2016

jdm commented Nov 8, 2016

asajeffrey commented Nov 8, 2016

bors-servo commented Nov 8, 2016

notriddle commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

servo / servo

Join GitHub today

Constellation manages script thread lifetime #14052

Constellation manages script thread lifetime #14052

Conversation

asajeffrey commented Nov 3, 2016 • edited by larsbergstrom

highfive commented Nov 3, 2016

highfive commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

nox left a comment

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.

asajeffrey commented Nov 4, 2016

nox commented Nov 4, 2016

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 8, 2016

jdm commented Nov 8, 2016

asajeffrey commented Nov 8, 2016

bors-servo commented Nov 8, 2016

notriddle commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

asajeffrey commented Nov 3, 2016 •

edited by larsbergstrom