Script thread with no root document #14013

asajeffrey · 2016-11-01T20:19:52Z

This PR removes the single root document from the script thread, and replaces it by a lookup table from PipelineIds to Documents. This is needed if we're going to share script threads, as per #633.

The last commit is the one that matters, the ones before are #13646.

cc @jdm @Ms2ger @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-01T20:19:56Z

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-01T20:19:57Z

Warning

These commits modify unsafe code. Please review it carefully!

frewsxcv · 2016-11-01T20:36:11Z

components/script/script_thread.rs

-            for it_context in root_context.iter() {
-                let current_url = it_context.active_document().url().to_string();
+        for document in self.documents.borrow().iter() {
+            let current_url = document.url().to_string();


You might be able to get by with let current_url = document.url().as_str(); here?

It was a bit trickier than that, but I did manage to avoid double-copying the urls here.

emilio · 2016-11-01T20:47:28Z

@bors-servo try

bors-servo · 2016-11-01T20:47:31Z

⌛ Trying commit d8de7de with merge 2d7d730...


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…

@jdm

…try> Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @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/14013)

bors-servo · 2016-11-01T21:46:27Z

☀️ 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 · 2016-11-02T20:43:51Z

Rebased. @bors-servo try

bors-servo · 2016-11-02T20:43:54Z

⌛ Trying commit 18b7b5d with merge 2a6c3bb...


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…

@jdm

…try> Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @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/14013)

bors-servo · 2016-11-03T07:19:39Z

💥 Test timed out

jdm · 2016-11-03T12:47:22Z

@bors-servo: try

jdm · 2016-11-03T12:47:40Z

@bors-servo: retry

bors-servo · 2016-11-03T12:47:43Z

⌛ Trying commit 18b7b5d with merge 4fe3e6f...


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…

@jdm

…try> Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @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/14013)

bors-servo · 2016-11-03T13:31:32Z

💔 Test failed - linux-rel-css

asajeffrey · 2016-11-03T13:51:32Z

  ▶ CRASH [expected FAIL] /css-text-3_dev/html/line-break-strict-017.htm
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ called `Result::unwrap()` on an `Err` value: IoError(Error { repr: Os { code: 104, message: "Connection reset by peer" } }) (thread main, at ../src/libcore/result.rs:837)
  │ stack backtrace:
  │    0:     0x7ffc0078f3ad - backtrace::backtrace::trace::h16372ee7bf1517e5
  │    1:     0x7ffc0078fa42 - backtrace::capture::Backtrace::new::hcc43c50c4b11c693
  │    2:     0x7ffbff126e7d - servo::main::_{{closure}}::h993bd385ff269be0
  │    3:     0x7ffc00d52b74 - std::panicking::rust_panic_with_hook
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:452
  │    4:     0x7ffc00d52a34 - std::panicking::begin_panic<collections::string::String>
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:413
  │    5:     0x7ffc00d52959 - std::panicking::begin_panic_fmt
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:397
  │    6:     0x7ffc00d528e7 - std::panicking::rust_begin_panic
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:373
  │    7:     0x7ffc00d8f34d - core::panicking::panic_fmt
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libcore/panicking.rs:69
  │    8:     0x7ffc008202df - core::result::unwrap_failed::h97cf4d1c2597280e
  │    9:     0x7ffc008213d2 - webrender_traits::api::_<impl webrender_traits..RenderApiSender>::create_api::h8aae9b868a4fcac9
  │   10:     0x7ffbff1248fd - servo::main::hf0cef208a66e3646
  │   11:     0x7ffc00d5b1da - panic_unwind::__rust_maybe_catch_panic
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libpanic_unwind/lib.rs:97
  │   12:     0x7ffc00d51b9a - std::panicking::try<(),fn()>
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:332
  │                          - std::panic::catch_unwind<fn(),()>
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panic.rs:351
  │                          - std::rt::lang_start
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/rt.rs:57
  │   13:     0x7ffbfca39f44 - __libc_start_main
  │   14:     0x7ffbff0f9909 - <unknown>
  │   15:                0x0 - <unknown>
  │ thread panicked while processing panic. aborting.
  └ thread panicked while processing panic. aborting.

asajeffrey · 2016-11-03T13:56:53Z

Looks like #13480.

bors-servo · 2016-11-03T20:06:27Z

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

asajeffrey · 2016-11-03T20:57:38Z

Rebased and squashed. @bors-servo try

bors-servo · 2016-11-08T03:30:06Z

⌛ Testing commit ae9268d with merge 8236d19...


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=jdm

@jdm

Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @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/14013)

bors-servo · 2016-11-08T04:04:26Z

💔 Test failed - linux-rel-wpt

asajeffrey · 2016-11-08T14:45:48Z

Hmm, failing tests. Puts on debugging hat...


        Replace script thread root browsing context by a collection of docume…

…nts.

asajeffrey · 2016-11-08T16:50:14Z

@bors-servo r=jdm

bors-servo · 2016-11-08T16:50:20Z

📌 Commit 90e9c91 has been approved by jdm

bors-servo · 2016-11-08T17:31:34Z

⌛ Testing commit 90e9c91 with merge d1b7f19...


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=jdm

@jdm

Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @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/14013)

bors-servo · 2016-11-08T18:23:54Z

☀️ 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


        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)

asajeffrey added the A-content/script label Nov 1, 2016

highfive assigned wafflespeanut Nov 1, 2016

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

frewsxcv reviewed Nov 1, 2016

View changes

asajeffrey force-pushed the asajeffrey:script-thread-no-root-document branch from 35a6929 to 7bc34d0 Nov 2, 2016

highfive added the S-tests-failed label Nov 3, 2016

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

asajeffrey mentioned this pull request Nov 3, 2016

Constellation manages script thread lifetime #14052

Merged

3 of 3 tasks complete

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

asajeffrey force-pushed the asajeffrey:script-thread-no-root-document branch from 18b7b5d to 1080827 Nov 3, 2016

highfive removed the S-tests-failed label Nov 3, 2016

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

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

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

asajeffrey added S-needs-squash S-awaiting-merge and removed S-awaiting-merge labels Nov 8, 2016

Replace script thread root browsing context by a collection of docume…

Loading status checks…

90e9c91

…nts.

asajeffrey force-pushed the asajeffrey:script-thread-no-root-document branch from b779e69 to 90e9c91 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

asajeffrey mentioned this pull request Nov 8, 2016

Storage notifications routed via the constellation. #14023

Merged

4 of 4 tasks complete

bors-servo merged commit 90e9c91 into servo:master Nov 8, 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 8, 2016

This was referenced Nov 8, 2016

Make document url mutable and implement location.replace() #13418

Merged

[WIP] Implement History State #13781

Closed

servo / servo

Script thread with no root document #14013

Script thread with no root document #14013

asajeffrey commented Nov 1, 2016 •

edited by larsbergstrom

highfive commented Nov 1, 2016

highfive commented Nov 1, 2016

This comment has been minimized.

This comment has been minimized.

emilio commented Nov 1, 2016

bors-servo commented Nov 1, 2016

bors-servo commented Nov 1, 2016

asajeffrey commented Nov 2, 2016

bors-servo commented Nov 2, 2016

bors-servo commented Nov 3, 2016

jdm commented Nov 3, 2016

jdm commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

bors-servo commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

asajeffrey commented Nov 8, 2016

asajeffrey commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

servo / servo

Join GitHub today

Script thread with no root document #14013

Script thread with no root document #14013

Conversation

asajeffrey commented Nov 1, 2016 • edited by larsbergstrom

highfive commented Nov 1, 2016

highfive commented Nov 1, 2016

This comment has been minimized.

frewsxcv Nov 1, 2016

This comment has been minimized.

asajeffrey Nov 2, 2016

emilio commented Nov 1, 2016

bors-servo commented Nov 1, 2016

bors-servo commented Nov 1, 2016

asajeffrey commented Nov 2, 2016

bors-servo commented Nov 2, 2016

bors-servo commented Nov 3, 2016

jdm commented Nov 3, 2016

jdm commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

bors-servo commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

asajeffrey commented Nov 8, 2016

asajeffrey commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

asajeffrey commented Nov 1, 2016 •

edited by larsbergstrom