/rust-mozjs

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make runtime creation safe #450

Merged
merged 2 commits into from Jan 14, 2019

Conversation

Reviewers

@asajeffrey asajeffrey

@emilio emilio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jdm @asajeffrey @bors-servo @emilio
@jdm
Copy link
Member

jdm commented Dec 2, 2018 •
edited by larsbergstrom

The fundamental problem exposed in servo/servo#22342 is that our concept of a parent runtime did not match reality. Using the first JSContext's runtime as the global parent for all subsequent contexts only makes sense if that JSContext outlives every other context. This is not guaranteed, leading to crashes when trying to use those contexts if the first context (and therefore its runtime) was destroyed.

The new design incorporates several changes for safer, more clear context and runtime management:

  • in order to create a new context, either a handle to an initialized JS engine is required or a handle to an existing runtime
  • all runtimes track outstanding handles that have been created, and assert if a runtime is destroyed before all of its child runtimes
  • overall initialization and shutdown of the engine is controlled by the lifetime of a JSEngine value, so creating a Runtime value is now infallible

This change is Reviewable

@jdm jdm referenced this pull request Dec 2, 2018

Merged

Update rust-mozjs #22353

4 of 4 tasks complete

bors-servo added a commit to servo/servo that referenced this pull request Dec 2, 2018

@bors-servo
Auto merge of #22353 - jdm:runtime-parent, r=<try> 
Runtime parent

These changes adjust our uses of the rust-mozjs APIs to accommodate the changes in servo/rust-mozjs#450.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22342.
- [x] There are tests for these changes
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5fe9b42

@jdm jdm force-pushed the jdm:parent-refactor branch from bdd6e09 to d0a6288 Dec 2, 2018

@jdm

This comment has been minimized.

Sign in to view
Copy link
Member Author

jdm commented Dec 2, 2018

There is one hole that I've come up with so far - while the assertion in the Runtime drop implementation catches the point at which a parent runtime is dropped before all of its children are dropped, that only interrupts the thread on which the parent runtime is executing. The threads on which any child runtimes are executing have no signal that their parent runtime is now invalid. The best choice at this point would be to abort the whole program for the sake of safety, but this PR does not do that right now.

@jdm jdm force-pushed the jdm:parent-refactor branch 2 times, most recently from 5c26260 to ac7f350 Dec 2, 2018

@emilio

emilio reviewed Dec 2, 2018
View changes

Show resolved Hide resolved src/rust.rs

@jdm jdm force-pushed the jdm:parent-refactor branch from ac7f350 to 69cfd4a Dec 3, 2018

@jdm jdm force-pushed the jdm:parent-refactor branch 2 times, most recently from d10b181 to 6e9cc73 Dec 21, 2018

@jdm

This comment has been minimized.

Sign in to view
Copy link
Member Author

jdm commented Dec 21, 2018

I fixed the racing issues by switching to a single mutex around an enum that represents the engine state. This is ready for review.
@jdm

This comment has been minimized.

Sign in to view
Copy link
Member Author

jdm commented Dec 21, 2018

r? @asajeffrey
@asajeffrey

asajeffrey reviewed Dec 21, 2018
View changes

Copy link
Member

asajeffrey left a comment

Mostly nits, the main thing is it looks like you're assuming the JSEngine outlives all the JSContexts, but I'm not sure why that should be the case.
Cargo.toml
@@ -42,9 +44,9 @@ doctest = false

[features]
debugmozjs = ['mozjs_sys/debugmozjs']
init_once = []

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Dec 21, 2018

Member

Yay!
Cargo.toml Outdated

[dependencies]
integer-atomics = "1"

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Dec 21, 2018

Member

Why do we need this? Can we use AtomicUsize rather than AtomicU32?
Show resolved Hide resolved src/rust.rs
src/rust.rs Outdated
impl Drop for JSEngine {
fn drop(&mut self) {
*ENGINE_STATE.lock().unwrap() = EngineState::ShutDown;
unsafe {

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Dec 21, 2018

Member

Check to see what the current state is before calling shutdown?
src/rust.rs Outdated
/// runtime can be dropped.
pub struct ParentRuntime {
parent: *mut JSRuntime,
count: Arc<AtomicU32>,

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Dec 21, 2018

Member

Say what this is counting?

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Dec 21, 2018

Member

Also, looking through the code it looks like the counter is incremented when the arc is cloned, and decremented when the arc is dropped, so the counter is always the same as the refcount. This makes me suspect that there's a simplification, where ParentRuntime just contains an Arc<JSEngine> or some such.
src/rust.rs
/// Unsafety:
/// If panicking does not abort the program, any threads with child runtimes will
/// continue executing after the thread with the parent runtime panics, but they
/// will be in an invalid and undefined state.

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Dec 21, 2018

Member

Hmm, this might be an issue for Servo when we do crash reporting? Not sure what we can do though, other than not put any JS on our crash reporting page and hope for the best.

@jdm jdm referenced this pull request Dec 21, 2018

Closed

Add some tests for default-initialized roots #411

@jdm

This comment has been minimized.

Sign in to view
Copy link
Member Author

jdm commented Jan 14, 2019

@asajeffrey Review comments addressed. I've added more comments around the adjusted refcounting bits to hopefully make clear why it looks weird.
@asajeffrey

This comment has been minimized.

Sign in to view
Copy link
Member

asajeffrey commented Jan 14, 2019

LGTM. Squash and r=me?

@jdm jdm force-pushed the jdm:parent-refactor branch from 60ab9de to 5e3c3ba Jan 14, 2019

@jdm

This comment has been minimized.

Sign in to view
Copy link
Member Author

jdm commented Jan 14, 2019

@bors-servo r=asajeffrey
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Jan 14, 2019

📌 Commit 5e3c3ba has been approved by asajeffrey
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Jan 14, 2019

⌛️ Testing commit 5e3c3ba with merge 35193f1...

bors-servo added a commit that referenced this pull request Jan 14, 2019

@bors-servo
Auto merge of #450 - jdm:parent-refactor, r=asajeffrey 
Make runtime creation safe

The fundamental problem exposed in servo/servo#22342 is that our concept of a parent runtime did not match reality. Using the first JSContext's runtime as the global parent for all subsequent contexts only makes sense if that JSContext outlives every other context. This is not guaranteed, leading to crashes when trying to use those contexts if the first context (and therefore its runtime) was destroyed.

The new design incorporates several changes for safer, more clear context and runtime management:
* in order to create a new context, either a handle to an initialized JS engine is required or a handle to an existing runtime
* all runtimes track outstanding handles that have been created, and assert if a runtime is destroyed before all of its child runtimes
* overall initialization and shutdown of the engine is controlled by the lifetime of a JSEngine value, so creating a Runtime value is now infallible

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/450)
<!-- Reviewable:end -->
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
35193f1

@jdm jdm force-pushed the jdm:parent-refactor branch from 5e3c3ba to 3e1ef3c Jan 14, 2019

@jdm jdm force-pushed the jdm:parent-refactor branch from 3e1ef3c to 981e266 Jan 14, 2019

@jdm

This comment has been minimized.

Sign in to view
Copy link
Member Author

jdm commented Jan 14, 2019

@bors-servo r=asajeffrey
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Jan 14, 2019

📌 Commit 981e266 has been approved by asajeffrey
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Jan 14, 2019

⌛️ Testing commit 981e266 with merge 7811094...

bors-servo added a commit that referenced this pull request Jan 14, 2019

@bors-servo
Auto merge of #450 - jdm:parent-refactor, r=asajeffrey 
Make runtime creation safe

The fundamental problem exposed in servo/servo#22342 is that our concept of a parent runtime did not match reality. Using the first JSContext's runtime as the global parent for all subsequent contexts only makes sense if that JSContext outlives every other context. This is not guaranteed, leading to crashes when trying to use those contexts if the first context (and therefore its runtime) was destroyed.

The new design incorporates several changes for safer, more clear context and runtime management:
* in order to create a new context, either a handle to an initialized JS engine is required or a handle to an existing runtime
* all runtimes track outstanding handles that have been created, and assert if a runtime is destroyed before all of its child runtimes
* overall initialization and shutdown of the engine is controlled by the lifetime of a JSEngine value, so creating a Runtime value is now infallible

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/450)
<!-- Reviewable:end -->
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7811094
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Jan 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: asajeffrey
Pushing 7811094 to master...

@bors-servo bors-servo merged commit 981e266 into servo:master Jan 14, 2019
2 of 3 checks passed

2 of 3 checks passed

code-review/reviewable 16 files, 6 discussions left (asajeffrey, emilio)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

bors-servo added a commit to servo/servo that referenced this pull request Jan 17, 2019

@bors-servo
Auto merge of #22353 - jdm:runtime-parent, r=nox 
Update rust-mozjs

These changes adjust our uses of the rust-mozjs APIs to accommodate the changes in servo/rust-mozjs#450.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22342.
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22353)
<!-- Reviewable:end -->
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
fb95f9d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment