Share script threads by tab and by eTLD+1 #14211

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 #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:54:17Z

components/script_traits/lib.rs

@@ -162,19 +162,20 @@ impl LoadData {
    }
 }

-/// 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:54:17Z

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:54:17Z

components/constellation/constellation.rs

                    is_private: bool) {
        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:54:17Z

components/constellation/constellation.rs

        let pipeline_url = pipeline_id.and_then(|id| self.pipelines.get(&id).map(|pipeline| pipeline.url.clone()));
        let parent_info = pipeline_id.and_then(|id| self.pipelines.get(&id).and_then(|pipeline| pipeline.parent_info));
        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:54:17Z

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:54:17Z

components/constellation/constellation.rs

@@ -133,6 +134,9 @@ pub struct Constellation<Message, LTF, STF> {
    /// 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."


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

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


        Auto merge of #14211 - asajeffrey:constellation-share-more-script-thr…

…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 label Nov 14, 2016

highfive assigned emilio Nov 14, 2016

highfive added the S-awaiting-review 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 label Nov 14, 2016

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

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

asajeffrey mentioned this pull request Nov 17, 2016

Purge pipelines from distant history. #11893

Closed

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

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

KiChjang removed the S-needs-rebase label Nov 18, 2016

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

highfive assigned jdm and unassigned emilio Nov 21, 2016

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

jdm removed the S-blocked-on-external label Nov 22, 2016

jdm requested changes Nov 22, 2016

View changes

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

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

asajeffrey added S-needs-code-changes S-needs-squash and removed S-awaiting-review labels Nov 22, 2016

asajeffrey added S-awaiting-review and removed S-needs-code-changes labels Nov 22, 2016

jdm approved these changes Nov 22, 2016

View changes

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

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

Loading status checks…

7ae19c0

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

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

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

highfive added S-awaiting-merge and removed S-awaiting-review 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
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge 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

servo / servo

Share script threads by tab and by eTLD+1 #14211

Share script threads by tab and by eTLD+1 #14211

asajeffrey commented Nov 14, 2016 •

edited

highfive commented Nov 14, 2016

asajeffrey commented Nov 14, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 17, 2016

asajeffrey commented Nov 21, 2016

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

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.

This comment has been minimized.

asajeffrey commented Nov 22, 2016

bors-servo commented Nov 22, 2016

bors-servo commented Nov 22, 2016

bors-servo commented Nov 22, 2016

servo / servo

Join GitHub today

Share script threads by tab and by eTLD+1 #14211

Share script threads by tab and by eTLD+1 #14211

Conversation

asajeffrey commented Nov 14, 2016 • edited

highfive commented Nov 14, 2016

asajeffrey commented Nov 14, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 17, 2016

asajeffrey commented Nov 21, 2016

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

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.

This comment has been minimized.

asajeffrey commented Nov 22, 2016

bors-servo commented Nov 22, 2016

bors-servo commented Nov 22, 2016

bors-servo commented Nov 22, 2016

asajeffrey commented Nov 14, 2016 •

edited