-
Notifications
You must be signed in to change notification settings - Fork 110
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
WIP smup #119
WIP smup #119
Conversation
This needs to make its way upstream
This needs to be ported over to the m-c patch queue.
This fixes safe_extern_statics warnings; see <rust-lang/rust#36247>.
by changing it to use pointers instead. Fixes servo#311
We should be able to create a default value for any Heap<*mut T> where *mut T implements GCMethods<*mut T> and Copy, not just for Heap<*mut JSObject>.
Currently, to create a Heap<T>, one first has to create a Heap<T>::default, and then call Heap<T>::set. This is a common pattern and therefore should be abstracted behind a method.
We expose several JSAPI functions that are used for tracing. By abstracting these functions behind a trait we can overload them on the type to be traced.
This is what's required for catch_unwind; there's no need to be stricter.
The only remaining OOL calls are ToWindowIfWindowProxy and JS_WrapValue, like in the equivalent Gecko function.
Rust-mozjs currently does not define any methods to create instances of HandleValueArray. Consequently, consumers have to create these instances manually. This is unnecessary boilerplate, and therefore should be abstracted behind a function.
This should make it harder to use Rooted in an unsound way (when not using the rooted!() macro).
This ensures no unrooted values linger in the Rooted when it is no longer rooted. It should be impossible to access the value after that, but Handles currently lack the lifetime to prevent that (servo#153).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally ignored the first two commits. The rest of the changes all look pretty sensible.
@@ -1,12 +0,0 @@ | |||
[package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this being upstreamed? Does it cause problems for us somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cargo will complain about malformed toml files and refuse to look any further.
I upstreamed this into the js/src/make-source-package.sh
bundling script that creates the source release tarballs. If we ever want to point directly at a revision of m-c, rather than snapshots in this repo, then we will need another fix.
@@ -1,15 +0,0 @@ | |||
[package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question - do we need to automate the removal of these each time we update SpiderMonkey?
mozjs/js/rust/build.rs
Outdated
@@ -25,7 +26,16 @@ fn build_jsglue_cpp() { | |||
|
|||
/// Find the public include directory within our mozjs-sys crate dependency. | |||
fn get_mozjs_include_dir() -> path::PathBuf { | |||
let entries = glob::glob("./target/*/build/mozjs_sys-*/out/dist/include") | |||
let out_dir = env::var("OUT_DIR") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message mentions the need for upstreaming. Has this happened? Is this still WIP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upstreaming is still theoretical because none of js/rust
has actually landed in m-c yet. The m-c patches I had are fairly out of date now, and need to be recreated anyways, so this won't get lost.
mozjs/js/rust/build.rs
Outdated
@@ -122,6 +122,8 @@ const WHITELIST_TYPES: &'static [&'static str] = &[ | |||
"js::Class", | |||
"JS::CompartmentOptions", | |||
"JS::ContextOptions", | |||
"js::DOMCallbacks", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mozjs/js/rust/build.rs
Outdated
@@ -160,6 +160,7 @@ const WHITELIST_TYPES: &'static [&'static str] = &[ | |||
"JSType", | |||
"JSValueTag", | |||
"JSValueType", | |||
"JSVersion", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squash this with earlier whitelisting and subsequent similar commits?
mozjs/js/rust/src/sc.rs
Outdated
callbacks: &jsapi::JSStructuredCloneCallbacks) | ||
-> bool { | ||
unsafe { | ||
(*self.raw).read(Runtime::get(), vp, callbacks as *const _, ptr::null_mut()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast should not be necessary.
mozjs/js/rust/src/sc.rs
Outdated
callbacks: &jsapi::JSStructuredCloneCallbacks) | ||
-> bool { | ||
unsafe { | ||
(*self.raw).write(Runtime::get(), v, callbacks as *const _, ptr::null_mut()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cast should not be necessary.
mozjs/js/rust/src/sc.rs
Outdated
pub fn write(&mut self, | ||
v: jsapi::JS::HandleValue, | ||
callbacks: &jsapi::JSStructuredCloneCallbacks) | ||
-> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's return Result<(), ()>
instead.
mozjs/js/rust/src/sc.rs
Outdated
|
||
/// Copy the given slice into this buffer. Returns false when an underlying | ||
/// JSAPI call fails. | ||
pub fn write_bytes(&mut self, bytes: &[u8]) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's return Result<(), ()>
.
mozjs/js/src/build.rs
Outdated
@@ -5,7 +5,7 @@ | |||
extern crate num_cpus; | |||
|
|||
use std::env; | |||
use std::path; | |||
// use std::path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this?
@jdm thanks for the initial pass on review! I pushed some new commits that should address your feedback. |
Regarding squashing: I can squash everything that isn't a rebase from servo/rust-mozjs into a single commit before landing. That's probably easiest. |
Approval conditional on getting CI working again, of course. |
@UK992, hello! I've been informed that you have helped out with Servo + windows build issues in the past before. If you have the cycles, I'd appreciate some help here (no worries if not!). I can build SpiderMonkey on Windows with MSVC just fine from Thanks for your help!
|
☔ The latest upstream changes (presumably #121) made this pull request unmergeable. Please resolve the merge conflicts. |
Can this be closed now? |
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)