Skip to content
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] Make DOM bindings methods not take raw JSContext pointers #20424

Closed
wants to merge 3 commits into from

Conversation

@csmoe
Copy link
Contributor

csmoe commented Mar 25, 2018

address #20377

  • create a struct JSContext(*mut jsapi::JSContext) type
  • implement Deref for this new type so it's easy to obtain the inner context pointer
  • add an unsafe from_ptr static method that returns a new instance of the type
  • make the code generator declare trait methods that accept the new type instead of *mut JSContext
  • adjust generated code to pass JSContext::from_ptr(cx) when necessary
  • update all of the methods defined in components/script/dom that need to be changed
  • remove any unnecessary uses of #[allow(unsafe_code)] on methods that no longer accept raw pointers

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Mar 25, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

@highfive
Copy link

highfive commented Mar 25, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/script_runtime.rs
  • @fitzgen: components/script/script_runtime.rs
  • @KiChjang: components/script/script_runtime.rs
@highfive
Copy link

highfive commented Mar 25, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
pub struct JSContext(*mut JSContext);

impl Deref for JSContext {
type Target = JSContext;

This comment has been minimized.

@KiChjang

KiChjang Mar 26, 2018

Member

This looks like it would not compile, since there's a naming collision.

@csmoe csmoe force-pushed the csmoe:safe_dom_jsctxt branch from 3dc1038 to cbaba5b Mar 28, 2018
@@ -473,3 +473,13 @@ unsafe fn set_gc_zeal_options(rt: *mut JSRuntime) {
#[allow(unsafe_code)]
#[cfg(not(feature = "debugmozjs"))]
unsafe fn set_gc_zeal_options(_: *mut JSRuntime) {}

pub struct RawJSContext(*mut JSContext);

This comment has been minimized.

@KiChjang

KiChjang Mar 28, 2018

Member

I believe this is a misnomer -- the struct that wraps the *mut JSContext is actually the safe JSContext, since we make sure that we don't have a null pointer reference when creating the struct. I would instead rename the JSContext import to RawJSContext.

This comment has been minimized.

@csmoe

csmoe Mar 28, 2018

Author Contributor

thanks, I'll address your advice in next commit.

@csmoe csmoe force-pushed the csmoe:safe_dom_jsctxt branch 2 times, most recently from 7c15665 to 95d76b1 Mar 28, 2018
@csmoe
Copy link
Contributor Author

csmoe commented Mar 28, 2018

there are lots of raw JSContext waiting to be wrapped in dom, introduction hasn't been done, more commits will be pushed one day later.

@jdm
Copy link
Member

jdm commented Mar 28, 2018

Yes, that is definitely something we can do incrementally over time.

@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2018

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

@csmoe csmoe force-pushed the csmoe:safe_dom_jsctxt branch from 190c36a to c62176e Mar 29, 2018
@csmoe
Copy link
Contributor Author

csmoe commented Mar 29, 2018

There are some methods defined locally but declared from outside crates, for example:

impl<T: Float + ToJSValConvertible> ToJSValConvertible for Finite<T> {
#[inline]
unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue) {
let value = **self;
value.to_jsval(cx, rval);
}
}

the to_jsval method is declared in mozjs crate:

pub trait ToJSValConvertible {
    /// Convert `self` to a `JSVal`. JSAPI failure causes a panic.
    #[inline]
    unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue);
}

this prevents me to introduce safe JSContext here. So can I leave that kinds of methods unchanged?

@jdm
Copy link
Member

jdm commented Mar 29, 2018

Yes. We can migrate this type into the upstream rust-mozjs crate at a later time if we decide it's worthwhile.

@KiChjang
Copy link
Member

KiChjang commented Mar 29, 2018

I'm not sure if the migration is possible, since I believe that these FromJSValConvertible implementations are implemented on types that only exist in servo and not rust-mozjs.

@jdm
Copy link
Member

jdm commented Mar 30, 2018

I meant migrating the new wrapper struct.

@csmoe
Copy link
Contributor Author

csmoe commented Apr 4, 2018

I suspect that it'll be better if the safe JSContext were introduced into rust-mozjs before this commit, it's really trival to wrap RawJSContext in dom, there are many methods called directly from rust-mozjs, I have been working on this task, but the mixture makes codegen and other dom::components so hard to maintain.

If it's ok, I'll volunteer to create a PR for rust-mozjs and the related introductions for servo.

@jdm
Copy link
Member

jdm commented Jun 19, 2018

@csmoe Are you still planning to continue working on this?

@csmoe
Copy link
Contributor Author

csmoe commented Jun 22, 2018

@jdm I'll restart this

@csmoe csmoe force-pushed the csmoe:safe_dom_jsctxt branch from c62176e to 24c7982 Jun 22, 2018
@bors-servo
Copy link
Contributor

bors-servo commented Jul 6, 2018

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

@jdm jdm closed this Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.