Share script threads by tab and by eTLD+1 by asajeffrey · Pull Request #14211 · servo/servo

asajeffrey · 2016-11-14T20:36:19Z

This PR shares script threads among all similar-origin documents in the same tab. This allows DOM object to be shared among same-origin same-tab documents.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Share Script Threads by eTLD+1, not by parent origin #633.
These changes do not require tests because refactoring.

This change is

highfive · 2016-11-14T20:36:23Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/dedicatedworkerglobalscope.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
@KiChjang: components/script/dom/dedicatedworkerglobalscope.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

asajeffrey · 2016-11-14T20:46:03Z

There's two commits in this PR. The first one is #14173, the second one is the subject of this PR.

bors-servo · 2016-11-15T18:47:37Z

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

bors-servo · 2016-11-17T23:46:49Z

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

asajeffrey · 2016-11-21T23:23:53Z

This is now ready for review, since #14173 landed. @bors-servo r? @jdm

jdm

Excellent work! This feels like a sensible design, and it's nice that new_pipeline is completely in control of selecting a script thread now.

jdm · 2016-11-22T18:08:34Z

components/script_traits/lib.rs

 }

-/// The initial data associated with a newly-created framed pipeline.
+/// The initial data associated to create a new layout attached to an existing script thread.


s/associated/required/

jdm · 2016-11-22T18:20:09Z

components/script/script_thread.rs

+        match self.documents.borrow().iter().next() {
+            None => panic!("Layout attached to empty script thread."),
+            // Tell the layout thread factory to actually spawn the thread.
+            Some((_, document)) => document.window().layout_chan().send(msg).unwrap(),


We could keep trying other threads if sending to the first one fails. Maybe file as a followup?

Really we should provide the script thread with a factory method for building layout threads, this would also deal with the corner case where a pipeline is asked to create a layout thread despite not having any current layout threads.

That does sound better. A followup would be good.

Filed #14328

jdm · 2016-11-22T18:27:39Z

components/constellation/constellation.rs

        if self.shutting_down { return; }

+        // TODO: think about the case where the child pipeline is created
+        // before the parent is part of the frame tree.


I don't understand what this means. It doesn't sound possible as written.

I made this comment into a question. You're right, I don't think this can happen, but I'm not sure we should be relying on that.

jdm · 2016-11-22T18:46:01Z

components/constellation/constellation.rs

        let window_size = pipeline_id.and_then(|id| self.pipelines.get(&id).and_then(|pipeline| pipeline.size));

-        self.close_frame_children(frame_id, ExitPipelineMode::Force);
+        self.close_frame_children(top_level_frame_id, ExitPipelineMode::Force);


Should we remove any entries from the script channels map?

Once we've got document discarding, that should all happen automatically, because the old page will be discarded, which will remove the script channel map entry.

jdm · 2016-11-22T18:47:49Z

components/constellation/constellation.rs

+    /// Returns None if the URL has no host name.
+    /// Returns the registered suffix for the host name if it is a domain.
+    /// Leaves the host name alone if it is an IP address.
+    fn reg_host<'a>(&self, url: &'a ServoUrl) -> Option<&'a str> {


This can be a free function instead, since it never uses self.

jdm · 2016-11-22T18:51:03Z

components/constellation/constellation.rs

    /// to receive sw manager message
    swmanager_receiver: Receiver<SWManagerMsg>,

+    /// A map from top-level frame id and registered domain name to script channels.


Let's add a comment to the effect of "This double indirection ensures that separate tabs do not share script threads, even if the same domain is loaded in each."

asajeffrey · 2016-11-22T21:04:35Z

@bors-servo r=jdm

bors-servo · 2016-11-22T21:04:36Z

📌 Commit 7ae19c0 has been approved by jdm

bors-servo · 2016-11-22T22:41:47Z

⌛ Testing commit 7ae19c0 with merge 1535f84...

…eads, r=jdm Share script threads by tab and by eTLD+1  This PR shares script threads among all similar-origin documents in the same tab. This allows DOM object to be shared among same-origin same-tab documents. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #633. - [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/14211)

bors-servo · 2016-11-22T23:59:43Z

☀️ 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 the A-constellation Involves the constellation label Nov 14, 2016

highfive assigned emilio Nov 14, 2016

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

asajeffrey changed the title S Share script threads by tab and by eTLD+1 Nov 14, 2016

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

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

asajeffrey force-pushed the constellation-share-more-script-threads branch 2 times, most recently from 8db72d1 to 0f92143 Compare November 15, 2016 19:58

asajeffrey mentioned this pull request Nov 17, 2016

Purge pipelines from distant history. #11893

Closed

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

asajeffrey force-pushed the constellation-share-more-script-threads branch from 0f92143 to daaf77a Compare November 18, 2016 00:09

KiChjang removed the S-needs-rebase There are merge conflict errors. label Nov 18, 2016

asajeffrey force-pushed the constellation-share-more-script-threads branch from daaf77a to 4ef42c1 Compare November 21, 2016 23:22

highfive assigned jdm and unassigned emilio Nov 21, 2016

asajeffrey force-pushed the constellation-share-more-script-threads branch from 4ef42c1 to 5c472e9 Compare November 21, 2016 23:29

jdm removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Nov 22, 2016

jdm requested changes Nov 22, 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 22, 2016

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

asajeffrey added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 22, 2016

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

jdm approved these changes Nov 22, 2016

View reviewed changes

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

Share script threads per-tab and per-eTLD+1.

7ae19c0

asajeffrey force-pushed the constellation-share-more-script-threads branch from 5ab118f to 7ae19c0 Compare November 22, 2016 21:02

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

asajeffrey removed the S-needs-squash Some (or all) of the commits in the PR should be combined. label Nov 22, 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 22, 2016

jdm closed this Nov 22, 2016

jdm reopened this Nov 22, 2016

bors-servo merged commit 7ae19c0 into servo:master Nov 22, 2016

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

This was referenced Nov 23, 2016

Implement synchronous about:blank loading. #13996

Merged

Rename ScriptChan in constellation to EventLoop #14260

Merged

Implement discarding Document objects to reclaim space. #14312

Merged

This was referenced Dec 1, 2016

Make same-origin navigations use the same script task #5239

Closed

Store a map of eTLDs to script tasks in the constellation #5322

Closed

Support multiple root pages in a script task #5481

Closed

asajeffrey mentioned this pull request Dec 23, 2016

Constellation creates a new script task for same-origin navigations #5095

Closed

Uh oh!

Conversation

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

Uh oh!

Uh oh!

highfive commented Nov 14, 2016

Uh oh!

asajeffrey commented Nov 14, 2016

Uh oh!

bors-servo commented Nov 15, 2016

Uh oh!

bors-servo commented Nov 17, 2016

Uh oh!

asajeffrey commented Nov 21, 2016

Uh oh!

jdm 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!

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!

Choose a reason for hiding this comment

Uh oh!

asajeffrey commented Nov 22, 2016

Uh oh!

bors-servo commented Nov 22, 2016

Uh oh!

bors-servo commented Nov 22, 2016

Uh oh!

bors-servo commented Nov 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asajeffrey commented Nov 14, 2016 •

edited

Loading