Script thread with no root document by asajeffrey · Pull Request #14013 · servo/servo

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

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

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

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

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

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

@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

@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 Related to the script thread label Nov 1, 2016

highfive assigned ghost Nov 1, 2016

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

frewsxcv reviewed Nov 1, 2016

View reviewed changes

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

highfive added the S-tests-failed The changes caused existing tests to fail. label Nov 3, 2016

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

asajeffrey mentioned this pull request Nov 3, 2016

Constellation manages script thread lifetime #14052

Merged

3 tasks

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

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

highfive removed the S-tests-failed The changes caused existing tests to fail. label Nov 3, 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. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Nov 8, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 8, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 8, 2016

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

90e9c91

…nts.

asajeffrey force-pushed the script-thread-no-root-document branch from b779e69 to 90e9c91 Compare November 8, 2016 16:49

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

asajeffrey mentioned this pull request Nov 8, 2016

Storage notifications routed via the constellation. #14023

Merged

4 tasks

bors-servo merged commit 90e9c91 into servo:master Nov 8, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. 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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

highfive commented Nov 1, 2016

Uh oh!

highfive commented Nov 1, 2016

Uh oh!

frewsxcv Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

emilio commented Nov 1, 2016

Uh oh!

bors-servo commented Nov 1, 2016

Uh oh!

bors-servo commented Nov 1, 2016

Uh oh!

asajeffrey commented Nov 2, 2016

Uh oh!

bors-servo commented Nov 2, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

jdm commented Nov 3, 2016

Uh oh!

jdm commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

asajeffrey commented Nov 3, 2016

Uh oh!

asajeffrey commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

asajeffrey commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

asajeffrey commented Nov 8, 2016

Uh oh!

asajeffrey commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

bors-servo commented Nov 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

asajeffrey commented Nov 1, 2016 •

edited by larsbergstrom

Loading