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

#14902 Write a MIR lint for rooting analysis #20264

Closed
wants to merge 15 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -13,12 +13,13 @@ use std::sync::mpsc::{Receiver, Sender};
/// common event loop messages. While this SendableWorkerScriptChan is alive, the associated
/// Worker object will remain alive.
#[derive(Clone, JSTraceable)]
pub struct SendableWorkerScriptChan<T: DomObject> {
#[must_root]
pub struct SendableWorkerScriptChan<#[must_root] T: DomObject> {

This comment has been minimized.

Copy link
@jdm

jdm May 31, 2018

Member

I'm confused by these changes - Trusted<T> is a type that does not require rooting (due to the allow_unrooted_interior attribute), so it's not clear why T needs to be marked must_root here.

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 4, 2018

Author

Sorry, I don't understand the relation between Trusted<T> and this place. I was putting #[must_root] in some places I thought it should be, without further understanding all of the code (there were too many errors).

For methods generic over T that we want to be able to accept #[must_root] we must specify explicitly that it is #[must_root] or mark the method with allow_unrooted_interior and/or allow(unrooted_must_root) (not sure which one).

This comment has been minimized.

Copy link
@jdm

jdm Jun 4, 2018

Member

The reason I brought it up is that T is only used as an argument for the Trusted<T> member that is stored in SendableWorkerScriptChan. Since Trusted<T> is marked #[allow_unrooted_interior] it is allowed to be stored anywhere. SendableWorkerScriptChan should not be marked #[must_root] in that case. Is that case possible to represent with the current plugin?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 5, 2018

Author

Oh, I see. I didn't notice the field 2 lines below. I got your point.

I don't remember how exactly all of these checks work. As I wrote somewhere else, here we are dealing with struct definition which are handled only on the HIR level if I remember correctly.

The intention with using #[must_root] is that it should be properly and explicitly propagated. So if we want T to be able to represent a #[must_root] type, then it should be marked as a #[must_root] type. Then, if we pass T to Trusted<T> and it is marked with allow_unrooted_interior, it means that Trusted<T> isn't #[must_root], so the whole SendableWorkerScriptChan structure doesn't have to be #[must_root], because it consists of regular fields.

Why is SendableWorkerScriptChan a #[must_root]? I marked it because there were some compilation errors, I believe. It is the heuristics I wrote about here and there.

There is also one side note: I am not sure if having sth<T: U> where U is #[must_root] enforces T to also be #[must_root]. But it should be possible to mute the error with allow_unrooted_interior and/or allow(unrooted_must_root).

pub sender: Sender<(Trusted<T>, CommonScriptMsg)>,
pub worker: Trusted<T>,
}

impl<T: JSTraceable + DomObject + 'static> ScriptChan for SendableWorkerScriptChan<T> {
impl<#[must_root] T: JSTraceable + DomObject + 'static> ScriptChan for SendableWorkerScriptChan<T> {
fn send(&self, msg: CommonScriptMsg) -> Result<(), ()> {
self.sender.send((self.worker.clone(), msg)).map_err(|_| ())
}
@@ -35,12 +36,13 @@ impl<T: JSTraceable + DomObject + 'static> ScriptChan for SendableWorkerScriptCh
/// worker event loop messages. While this SendableWorkerScriptChan is alive, the associated
/// Worker object will remain alive.
#[derive(Clone, JSTraceable)]
pub struct WorkerThreadWorkerChan<T: DomObject> {
#[must_root]
pub struct WorkerThreadWorkerChan<#[must_root] T: DomObject> {
pub sender: Sender<(Trusted<T>, WorkerScriptMsg)>,
pub worker: Trusted<T>,
}

impl<T: JSTraceable + DomObject + 'static> ScriptChan for WorkerThreadWorkerChan<T> {
impl<#[must_root] T: JSTraceable + DomObject + 'static> ScriptChan for WorkerThreadWorkerChan<T> {
fn send(&self, msg: CommonScriptMsg) -> Result<(), ()> {
self.sender
.send((self.worker.clone(), WorkerScriptMsg::Common(msg)))
@@ -55,7 +57,7 @@ impl<T: JSTraceable + DomObject + 'static> ScriptChan for WorkerThreadWorkerChan
}
}

impl<T: DomObject> ScriptPort for Receiver<(Trusted<T>, WorkerScriptMsg)> {
impl<#[must_root] T: DomObject> ScriptPort for Receiver<(Trusted<T>, WorkerScriptMsg)> {
fn recv(&self) -> Result<CommonScriptMsg, ()> {
match self.recv().map(|(_, msg)| msg) {
Ok(WorkerScriptMsg::Common(script_msg)) => Ok(script_msg),
@@ -12,14 +12,15 @@ use style::thread_state::{self, ThreadState};
/// This extends the API of `std::cell::RefCell` to allow unsafe access in
/// certain situations, with dynamic checking in debug builds.
#[derive(Clone, Debug, Default, MallocSizeOf, PartialEq)]
pub struct DomRefCell<T> {
value: RefCell<T>,
//#[must_root] // TODO should it be #[must_root]? then -> there are usages with #[must_root] types and with normal ones; two different structs for them? or hacking plugin compiler further to mark structure as #[must_root] if it generic type is #[must_root]?

This comment has been minimized.

Copy link
@jdm

jdm May 31, 2018

Member

This is an interesting question. What we want to represent is that DomRefCell<T> is must_root if and only if T is must_root for each particular instance of T. Is this something that is possible with the current plugin?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 4, 2018

Author

I see the intention. It is what I came up with, but didn't implement so far. After short recalling I am not sure if it was very problematic in some way. It could potentially require some changes to part of the plugin that checks only HIR (check_struct_def method if I remember the name correctly) and maybe some cooperation with the method that analyzes the MIR.

pub struct DomRefCell<#[must_root] T> {
value: RefCell<T>, // TODO currently a little bit hacky since RefCell is currently on the exception list
}

// Functionality specific to Servo's `DomRefCell` type
// ===================================================

impl<T> DomRefCell<T> {
impl<#[must_root] T> DomRefCell<T> {
/// Return a reference to the contents.
///
/// For use in the layout thread only.
@@ -47,8 +48,9 @@ impl<T> DomRefCell<T> {

// Functionality duplicated with `std::cell::RefCell`
// ===================================================
impl<T> DomRefCell<T> {
impl<#[must_root] T> DomRefCell<T> {
/// Create a new `DomRefCell` containing `value`.
#[allow(unrooted_must_root)] // TODO don't know if it is okay; T is unrooted argument
pub fn new(value: T) -> DomRefCell<T> {
DomRefCell {
value: RefCell::new(value),
@@ -5629,7 +5629,7 @@ def contains_unsafe_arg(arguments):

if methods:
self.cgRoot = CGWrapper(CGIndenter(CGList(methods, "")),
pre="pub trait %sMethods {\n" % descriptor.interface.identifier.name,
pre="#[must_root] pub trait %sMethods {\n" % descriptor.interface.identifier.name,

This comment has been minimized.

Copy link
@jdm

jdm May 31, 2018

Member

Why is this change required?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 4, 2018

Author

Because of this. We want traits to be #[must_root].

post="}")
else:
self.cgRoot = CGGeneric("")
@@ -62,6 +62,7 @@ use servo_config::opts;
use std::{char, ffi, ptr, slice};

/// A trait to check whether a given `JSObject` implements an IDL interface.
#[must_root]

This comment has been minimized.

Copy link
@jdm

jdm May 31, 2018

Member

Why is this change required?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 4, 2018

Author

Like mentioned somewhere before - some heuristics. It is a trait that is implemented by #[must_root] types, I believe.

pub trait IDLInterface {
/// Returns whether the given DOM class derives that interface.
fn derives(&'static DOMClass) -> bool;
@@ -105,7 +106,7 @@ impl<T: Float + FromJSValConvertible<Config=()>> FromJSValConvertible for Finite
}
}

impl <T: DomObject + IDLInterface> FromJSValConvertible for DomRoot<T> {
impl <#[must_root] T: DomObject + IDLInterface> FromJSValConvertible for DomRoot<T> {
type Config = ();

unsafe fn from_jsval(_cx: *mut JSContext,
@@ -420,7 +421,7 @@ pub unsafe fn private_from_proto_check<F>(mut obj: *mut JSObject,
}

/// Get a `*const T` for a DOM object accessible from a `JSObject`.
pub fn native_from_object<T>(obj: *mut JSObject) -> Result<*const T, ()>
pub fn native_from_object<#[must_root] T>(obj: *mut JSObject) -> Result<*const T, ()>
where T: DomObject + IDLInterface
{
unsafe {
@@ -434,15 +435,15 @@ pub fn native_from_object<T>(obj: *mut JSObject) -> Result<*const T, ()>
/// Returns Err(()) if `obj` is an opaque security wrapper or if the object is
/// not a reflector for a DOM object of the given type (as defined by the
/// proto_id and proto_depth).
pub fn root_from_object<T>(obj: *mut JSObject) -> Result<DomRoot<T>, ()>
pub fn root_from_object<#[must_root] T>(obj: *mut JSObject) -> Result<DomRoot<T>, ()>
where T: DomObject + IDLInterface
{
native_from_object(obj).map(|ptr| unsafe { DomRoot::from_ref(&*ptr) })
}

/// Get a `*const T` for a DOM object accessible from a `HandleValue`.
/// Caller is responsible for throwing a JS exception if needed in case of error.
pub fn native_from_handlevalue<T>(v: HandleValue) -> Result<*const T, ()>
pub fn native_from_handlevalue<#[must_root] T>(v: HandleValue) -> Result<*const T, ()>
where T: DomObject + IDLInterface
{
if !v.get().is_object() {
@@ -453,7 +454,7 @@ pub fn native_from_handlevalue<T>(v: HandleValue) -> Result<*const T, ()>

/// Get a `DomRoot<T>` for a DOM object accessible from a `HandleValue`.
/// Caller is responsible for throwing a JS exception if needed in case of error.
pub fn root_from_handlevalue<T>(v: HandleValue) -> Result<DomRoot<T>, ()>
pub fn root_from_handlevalue<#[must_root] T>(v: HandleValue) -> Result<DomRoot<T>, ()>
where T: DomObject + IDLInterface
{
if !v.get().is_object() {
@@ -463,13 +464,13 @@ pub fn root_from_handlevalue<T>(v: HandleValue) -> Result<DomRoot<T>, ()>
}

/// Get a `DomRoot<T>` for a DOM object accessible from a `HandleObject`.
pub fn root_from_handleobject<T>(obj: HandleObject) -> Result<DomRoot<T>, ()>
pub fn root_from_handleobject<#[must_root] T>(obj: HandleObject) -> Result<DomRoot<T>, ()>
where T: DomObject + IDLInterface
{
root_from_object(obj.get())
}

impl<T: DomObject> ToJSValConvertible for DomRoot<T> {
impl<#[must_root] T: DomObject> ToJSValConvertible for DomRoot<T> {
unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue) {
self.reflector().to_jsval(cx, rval);
}
@@ -13,25 +13,26 @@ use std::mem;

/// A trait to hold the cast functions of IDL interfaces that either derive
/// or are derived from other interfaces.
#[must_root]
pub trait Castable: IDLInterface + DomObject + Sized {
/// Check whether a DOM object implements one of its deriving interfaces.
fn is<T>(&self) -> bool
fn is<#[must_root] T>(&self) -> bool
where T: DerivedFrom<Self>
{
let class = unsafe { get_dom_class(self.reflector().get_jsobject().get()).unwrap() };
T::derives(class)
}

/// Cast a DOM object upwards to one of the interfaces it derives from.
fn upcast<T>(&self) -> &T
fn upcast<#[must_root] T>(&self) -> &T
where T: Castable,
Self: DerivedFrom<T>
{
unsafe { mem::transmute(self) }
}

/// Cast a DOM object downwards to one of the interfaces it might implement.
fn downcast<T>(&self) -> Option<&T>
fn downcast<#[must_root] T>(&self) -> Option<&T>
where T: DerivedFrom<Self>
{
if self.is::<T>() {
@@ -56,7 +56,7 @@ pub struct TrustedReference(*const libc::c_void);
unsafe impl Send for TrustedReference {}

impl TrustedReference {
fn new<T: DomObject>(ptr: *const T) -> TrustedReference {
fn new<#[must_root] T: DomObject>(ptr: *const T) -> TrustedReference {
TrustedReference(ptr as *const libc::c_void)
}
}
@@ -146,17 +146,18 @@ impl TrustedPromise {
/// DOM object is guaranteed to live at least as long as the last outstanding
/// `Trusted<T>` instance.
#[allow_unrooted_interior]
pub struct Trusted<T: DomObject> {
//#[must_root] // TODO hm, allow_unrooted_interior is not checked well enough
pub struct Trusted<#[must_root] T: DomObject> {
/// A pointer to the Rust DOM object of type T, but void to allow
/// sending `Trusted<T>` between threads, regardless of T's sendability.
refcount: Arc<TrustedReference>,
owner_thread: *const libc::c_void,
phantom: PhantomData<T>,
}

unsafe impl<T: DomObject> Send for Trusted<T> {}
unsafe impl<#[must_root] T: DomObject> Send for Trusted<T> {}

impl<T: DomObject> Trusted<T> {
impl<#[must_root] T: DomObject> Trusted<T> {
/// Create a new `Trusted<T>` instance from an existing DOM pointer. The DOM object will
/// be prevented from being GCed for the duration of the resulting `Trusted<T>` object's
/// lifetime.
@@ -188,7 +189,7 @@ impl<T: DomObject> Trusted<T> {
}
}

impl<T: DomObject> Clone for Trusted<T> {
impl<#[must_root] T: DomObject> Clone for Trusted<T> {
fn clone(&self) -> Trusted<T> {
Trusted {
refcount: self.refcount.clone(),
@@ -224,7 +225,7 @@ impl LiveDOMReferences {
table.entry(&*promise).or_insert(vec![]).push(promise)
}

fn addref<T: DomObject>(&self, ptr: *const T) -> Arc<TrustedReference> {
fn addref<#[must_root] T: DomObject>(&self, ptr: *const T) -> Arc<TrustedReference> {
let mut table = self.reflectable_table.borrow_mut();
let capacity = table.capacity();
let len = table.len();
@@ -13,7 +13,7 @@ use std::default::Default;

/// Create the reflector for a new DOM object and yield ownership to the
/// reflector.
pub fn reflect_dom_object<T, U>(
pub fn reflect_dom_object<#[must_root] T, #[must_root] U>(

This comment has been minimized.

Copy link
@jdm

jdm May 31, 2018

Member

I think the right solution for Box<T> where T is actually a rooted type is creating a #[allow_unrooted_interior] struct Unrooted<#[must_root] T>(Box<T>) and using that instead of Box<T>.

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 4, 2018

Author

Sounds reasonable. If I remember correctly, this is the only usage of Box<T> with #[must_root] T, though. At least, the only usage reported by the compiler when I was gradually fixing the violations. So I can be wrong.

obj: Box<T>,
global: &U,
wrap_fn: unsafe fn(*mut JSContext, &GlobalScope, Box<T>) -> DomRoot<T>)
@@ -74,6 +74,7 @@ impl Reflector {
}

/// A trait to provide access to the `Reflector` for a DOM object.
#[must_root]

This comment has been minimized.

Copy link
@jdm

jdm May 31, 2018

Member

Why is this necessary?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Jun 4, 2018

Author

Like mentioned before - it is just some heuristics of mine.

pub trait DomObject: 'static {
/// Returns the receiver's reflector.
fn reflector(&self) -> &Reflector;
@@ -91,6 +92,7 @@ impl DomObject for Reflector {
}

/// A trait to initialize the `Reflector` for a DOM object.
#[must_root]
pub trait MutDomObject: DomObject {
/// Initializes the Reflector
fn init_reflector(&mut self, obj: *mut JSObject);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.