Constellation manages script thread lifetime by asajeffrey · Pull Request #14052 · servo/servo

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:25:33Z

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:26:33Z

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:28:49Z

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-07T18:34:23Z

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-07T18:46:03Z

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-07T19:39:44Z

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

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

@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 No impact; the issue is one of maintainability or tidiness. Proposed solution requires refactoring. A-content/script Related to the script thread A-constellation Involves the constellation labels Nov 3, 2016

highfive assigned nox Nov 3, 2016

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

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

nox suggested changes Nov 4, 2016

View reviewed changes

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

asajeffrey added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Nov 4, 2016

highfive assigned jdm and unassigned nox Nov 4, 2016

jdm requested changes Nov 7, 2016

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 7, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Nov 8, 2016

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

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 8, 2016

asajeffrey added the S-needs-squash Some (or all) of the commits in the PR should be combined. label Nov 8, 2016

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 8, 2016

jdm approved these changes Nov 8, 2016

View reviewed changes

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

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

asajeffrey removed the S-needs-squash Some (or all) of the commits in the PR should be combined. label Nov 8, 2016

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 Nov 8, 2016

Script thread lifetime is now managed by the constellation.

0e7ffaf

notriddle force-pushed the constellation-manages-script-thread-lifetime branch from 08effa7 to 0e7ffaf Compare November 9, 2016 01:29

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

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 Nov 9, 2016

bors-servo merged commit 0e7ffaf into servo:master Nov 9, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 9, 2016

Uh oh!

Conversation

asajeffrey commented Nov 3, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Nov 3, 2016

Uh oh!

highfive commented Nov 3, 2016

Uh oh!

asajeffrey commented Nov 3, 2016

Uh oh!

nox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asajeffrey commented Nov 4, 2016

Uh oh!

nox commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

jdm commented Nov 8, 2016

Uh oh!

asajeffrey commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

notriddle commented Nov 9, 2016

Uh oh!

bors-servo commented Nov 9, 2016

Uh oh!

bors-servo commented Nov 9, 2016

Uh oh!

bors-servo commented Nov 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asajeffrey commented Nov 3, 2016 •

edited by larsbergstrom

Loading