Bug 1309848 - Add safer bindings for RefPtr; r?emilio,mystor draft
authorManish Goregaokar <manishsmail@gmail.com>
Thu, 13 Oct 2016 16:59:10 +0530
changeset 424911 95f63160723529c641bbce27a68324f1e04b4b25
parent 424794 5f5840fb9d22069c5ba311732e647356891b7e86
child 424912 f7901ac35a4255aaf0ca39b7847fa697c8031d82
push id32286
push userbmo:manishearth@gmail.com
push dateThu, 13 Oct 2016 19:02:26 +0000
reviewersemilio, mystor
bugs1309848
milestone52.0a1
Bug 1309848 - Add safer bindings for RefPtr; r?emilio,mystor MozReview-Commit-ID: HaXN0Y8upBG
servo/components/style/gecko_bindings/mod.rs
servo/components/style/gecko_bindings/ptr.rs
servo/components/style/gecko_bindings/sugar/mod.rs
servo/components/style/gecko_bindings/sugar/refptr.rs
servo/components/style/parser.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/longhand/box.mako.rs
servo/components/style/values/specified/mod.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/gecko_bindings/mod.rs
+++ b/servo/components/style/gecko_bindings/mod.rs
@@ -1,15 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #[allow(dead_code, improper_ctypes, non_camel_case_types)]
 pub mod bindings;
-pub mod ptr;
 
 // FIXME: We allow `improper_ctypes` (for now), because the lint doesn't allow
 // foreign structs to have `PhantomData`. We should remove this once the lint
 // ignores this case.
 
 #[cfg(debug_assertions)]
 #[allow(dead_code, improper_ctypes, non_camel_case_types, non_snake_case, non_upper_case_globals)]
 pub mod structs {
deleted file mode 100644
--- a/servo/components/style/gecko_bindings/ptr.rs
+++ /dev/null
@@ -1,64 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-use gecko_bindings::bindings::*;
-use heapsize::HeapSizeOf;
-use std::fmt::{self, Debug};
-
-// Defines an Arc-like type that manages a refcounted Gecko object stored
-// in a ThreadSafeFooHolder smart pointer (for those Gecko classes that
-// do not have thread-safe refcounting support) or as raw pointers (for
-// those that do have thread-safe refcounting support).  Used in tandem
-// with the NS_DECL_(HOLDER|THREADSAFE)_FFI_REFCOUNTING-defined types and
-// functions in Gecko.
-macro_rules! define_arc {
-    ($arc_type:ident, $name:ident, $gecko_type:ident, $addref: ident, $release: ident) => (
-        #[derive(PartialEq)]
-        pub struct $arc_type {
-            ptr: *mut $gecko_type,
-        }
-
-        impl $arc_type {
-            pub fn new(data: *mut $gecko_type) -> $arc_type {
-                debug_assert!(!data.is_null());
-                unsafe { $addref(data); }
-                $arc_type {
-                    ptr: data
-                }
-            }
-
-            pub fn as_raw(&self) -> *mut $gecko_type { self.ptr }
-        }
-
-        unsafe impl Send for $arc_type {}
-        unsafe impl Sync for $arc_type {}
-
-        impl Clone for $arc_type {
-            fn clone(&self) -> $arc_type {
-                $arc_type::new(self.ptr)
-            }
-        }
-
-        impl Drop for $arc_type {
-            fn drop(&mut self) {
-                unsafe { $release(self.ptr); }
-            }
-        }
-
-        impl HeapSizeOf for $arc_type {
-            fn heap_size_of_children(&self) -> usize { 0 }
-        }
-
-        impl Debug for $arc_type {
-            fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-                write!(f, stringify!($name))
-            }
-        }
-    )
-}
-
-define_arc!(GeckoArcPrincipal, Principal, ThreadSafePrincipalHolder,
-            Gecko_AddRefPrincipalArbitraryThread, Gecko_ReleasePrincipalArbitraryThread);
-define_arc!(GeckoArcURI, URI, ThreadSafeURIHolder,
-            Gecko_AddRefURIArbitraryThread, Gecko_ReleaseURIArbitraryThread);
--- a/servo/components/style/gecko_bindings/sugar/mod.rs
+++ b/servo/components/style/gecko_bindings/sugar/mod.rs
@@ -3,9 +3,10 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 mod ns_com_ptr;
 mod ns_css_shadow_array;
 mod ns_style_auto_array;
 pub mod ns_style_coord;
 mod ns_t_array;
 pub mod ownership;
+pub mod refptr;
 mod style_complex_color;
new file mode 100644
--- /dev/null
+++ b/servo/components/style/gecko_bindings/sugar/refptr.rs
@@ -0,0 +1,238 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+use heapsize::HeapSizeOf;
+use gecko_bindings::structs;
+use std::marker::PhantomData;
+use std::{mem, ptr};
+use std::ops::{Deref, DerefMut};
+
+/// Trait for all objects that have Addref() and Release
+/// methods and can be placed inside RefPtr<T>
+pub unsafe trait RefCounted {
+    fn addref(&self);
+    unsafe fn release(&self);
+}
+
+/// Trait for types which can be shared across threads in RefPtr
+pub unsafe trait ThreadSafeRefCounted: RefCounted {}
+
+#[derive(Debug)]
+pub struct RefPtr<T: RefCounted> {
+    ptr: *mut T,
+    _marker: PhantomData<T>,
+}
+
+/// A RefPtr that we know is uniquely owned
+///
+/// This is basically Box<T>, with the additional
+/// guarantee that the box can be safely interpreted
+/// as a RefPtr<T> (with refcount 1)
+///
+/// This is useful when you wish to create a refptr
+/// and mutate it temporarily, while it is still
+/// uniquely owned.
+pub struct UniqueRefPtr<T: RefCounted>(RefPtr<T>);
+
+// There is no safe conversion from &T to RefPtr<T> (like Gecko has)
+// because this lets you break UniqueRefPtr's guarantee
+
+impl<T: RefCounted> RefPtr<T> {
+    /// Create a new RefPtr from an already addrefed
+    /// pointer obtained from FFI. Pointer
+    /// must be valid, non-null and have been addrefed
+    pub unsafe fn from_addrefed(ptr: *mut T) -> Self {
+        debug_assert!(!ptr.is_null());
+        RefPtr {
+            ptr: ptr,
+            _marker: PhantomData,
+        }
+    }
+
+    /// Create a new RefPtr from a pointer obtained
+    /// from FFI. Pointer must be valid and non null.
+    /// This method calls addref() internally
+    pub unsafe fn new(ptr: *mut T) -> Self {
+        debug_assert!(!ptr.is_null());
+        let ret = RefPtr {
+            ptr: ptr,
+            _marker: PhantomData,
+        };
+        ret.addref();
+        ret
+    }
+
+    /// Produces an FFI-compatible RefPtr that can be stored in
+    /// style structs.
+    ///
+    /// structs::RefPtr does not have a destructor, so this may leak
+    pub fn forget(self) -> structs::RefPtr<T> {
+        let ret = structs::RefPtr {
+            mRawPtr: self.ptr,
+        };
+        mem::forget(self);
+        ret
+    }
+
+    /// Returns the raw inner pointer
+    /// to be fed back into FFI
+    pub fn get(&self) -> *mut T {
+        self.ptr
+    }
+
+    /// Addref the inner data
+    ///
+    /// Leaky on its own
+    pub fn addref(&self) {
+        unsafe { (*self.ptr).addref(); }
+    }
+
+    /// Release the inner data
+    ///
+    /// Call only when the data actuall needs releasing
+    pub unsafe fn release(&self) {
+        (*self.ptr).release();
+    }
+}
+
+impl<T: RefCounted> UniqueRefPtr<T> {
+    /// Create a unique refptr from an already addrefed
+    /// pointer obtained from FFI. The refcount must be one.
+    /// The pointer must be valid and non null
+    pub unsafe fn from_addrefed(ptr: *mut T) -> Self {
+        UniqueRefPtr(RefPtr::from_addrefed(ptr))
+    }
+
+    /// Convert to a RefPtr so that it can be used
+    pub fn get(self) -> RefPtr<T> {
+        self.0
+    }
+}
+
+impl<T: RefCounted> Deref for RefPtr<T> {
+    type Target = T;
+    fn deref(&self) -> &T {
+        unsafe { &*self.ptr }
+    }
+}
+
+impl<T: RefCounted> Deref for UniqueRefPtr<T> {
+    type Target = T;
+    fn deref(&self) -> &T {
+        unsafe { &*self.0.ptr }
+    }
+}
+
+impl<T: RefCounted> DerefMut for UniqueRefPtr<T> {
+    fn deref_mut(&mut self) -> &mut T {
+        unsafe { &mut *self.0.ptr }
+    }
+}
+
+impl<T: RefCounted> structs::RefPtr<T> {
+    /// Produces a Rust-side RefPtr from an FFI RefPtr, bumping the refcount
+    ///
+    /// Must be called on a valid, non-null structs::RefPtr<T>
+    pub unsafe fn to_safe(&self) -> RefPtr<T> {
+        debug_assert!(!self.mRawPtr.is_null());
+        let r = RefPtr {
+            ptr: self.mRawPtr,
+            _marker: PhantomData,
+        };
+        r.addref();
+        r
+    }
+    /// Produces a Rust-side RefPtr, consuming the existing one (and not bumping the refcount)
+    pub unsafe fn into_safe(self) -> RefPtr<T> {
+        debug_assert!(!self.mRawPtr.is_null());
+        RefPtr {
+            ptr: self.mRawPtr,
+            _marker: PhantomData,
+        }
+    }
+
+    /// Replace a structs::RefPtr<T> with a different one, appropriately addref/releasing
+    ///
+    /// Both `self` and `other` must be valid, but can be null
+    pub unsafe fn set(&mut self, other: &Self) {
+        if !self.mRawPtr.is_null() {
+            (*self.mRawPtr).release();
+            self.mRawPtr = ptr::null_mut();
+        }
+        if !other.mRawPtr.is_null() {
+            *self = other.to_safe().forget();
+        }
+    }
+
+    /// Replace a `structs::RefPtr<T>` with a `RefPtr<T>`,
+    /// consuming the `RefPtr<T>`, and releasing the old
+    /// value in `self` if necessary.
+    ///
+    /// `self` must be valid, possibly null
+    pub fn set_move(&mut self, other: RefPtr<T>) {
+        if !self.mRawPtr.is_null() {
+            unsafe { (*self.mRawPtr).release(); }
+        }
+        *self = other.forget();
+    }
+}
+
+impl<T: RefCounted> Drop for RefPtr<T> {
+    fn drop(&mut self) {
+        unsafe { self.release() }
+    }
+}
+
+impl<T: RefCounted> Clone for RefPtr<T> {
+    fn clone(&self) -> Self {
+        self.addref();
+        RefPtr {
+            ptr: self.ptr,
+            _marker: PhantomData,
+        }
+    }
+}
+
+impl<T: RefCounted> HeapSizeOf for RefPtr<T> {
+    fn heap_size_of_children(&self) -> usize { 0 }
+}
+
+impl<T: RefCounted> PartialEq for RefPtr<T> {
+    fn eq(&self, other: &Self) -> bool {
+        self.ptr == other.ptr
+    }
+}
+
+unsafe impl<T: ThreadSafeRefCounted> Send for RefPtr<T> {}
+unsafe impl<T: ThreadSafeRefCounted> Sync for RefPtr<T> {}
+
+// Companion of NS_DECL_THREADSAFE_FFI_REFCOUNTING
+//
+// Gets you a free RefCounted impl
+macro_rules! impl_threadsafe_refcount {
+    ($t:ty, $addref:ident, $release:ident) => (
+        unsafe impl RefCounted for $t {
+            fn addref(&self) {
+                unsafe { ::gecko_bindings::bindings::$addref(self as *const _ as *mut _) }
+            }
+            unsafe fn release(&self) {
+                ::gecko_bindings::bindings::$release(self as *const _ as *mut _)
+            }
+        }
+        unsafe impl ThreadSafeRefCounted for $t {}
+    );
+}
+
+impl_threadsafe_refcount!(::gecko_bindings::bindings::ThreadSafePrincipalHolder,
+                          Gecko_AddRefPrincipalArbitraryThread,
+                          Gecko_ReleasePrincipalArbitraryThread);
+impl_threadsafe_refcount!(::gecko_bindings::bindings::ThreadSafeURIHolder,
+                          Gecko_AddRefURIArbitraryThread,
+                          Gecko_ReleaseURIArbitraryThread);
+impl_threadsafe_refcount!(::gecko_bindings::structs::nsStyleQuoteValues,
+                          Gecko_AddRefQuoteValuesArbitraryThread,
+                          Gecko_ReleaseQuoteValuesArbitraryThread);
+pub type GeckoArcPrincipal = RefPtr<::gecko_bindings::bindings::ThreadSafePrincipalHolder>;
+pub type GeckoArcURI = RefPtr<::gecko_bindings::bindings::ThreadSafeURIHolder>;
+
--- a/servo/components/style/parser.rs
+++ b/servo/components/style/parser.rs
@@ -2,17 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! The context within which CSS code is parsed.
 
 use cssparser::{Parser, SourcePosition};
 use error_reporting::ParseErrorReporter;
 #[cfg(feature = "gecko")]
-use gecko_bindings::ptr::{GeckoArcPrincipal, GeckoArcURI};
+use gecko_bindings::sugar::refptr::{GeckoArcPrincipal, GeckoArcURI};
 use selector_impl::TheSelectorImpl;
 use selectors::parser::ParserContext as SelectorParserContext;
 use stylesheets::Origin;
 use url::Url;
 
 #[cfg(not(feature = "gecko"))]
 pub struct ParserContextExtraData;
 
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -891,19 +891,19 @@ fn static_assert() {
         use properties::longhands::_moz_binding::SpecifiedValue as BindingValue;
         match v {
             BindingValue::None => debug_assert!(self.gecko.mBinding.mRawPtr.is_null()),
             BindingValue::Url(ref url, ref extra_data) => {
                 unsafe {
                     Gecko_SetMozBinding(&mut self.gecko,
                                         url.as_str().as_ptr(),
                                         url.as_str().len() as u32,
-                                        extra_data.base.as_raw(),
-                                        extra_data.referrer.as_raw(),
-                                        extra_data.principal.as_raw());
+                                        extra_data.base.get(),
+                                        extra_data.referrer.get(),
+                                        extra_data.principal.get());
                 }
             }
         }
     }
     #[allow(non_snake_case)]
     pub fn copy__moz_binding_from(&mut self, other: &Self) {
         unsafe { Gecko_CopyMozBindingFrom(&mut self.gecko, &other.gecko); }
     }
--- a/servo/components/style/properties/longhand/box.mako.rs
+++ b/servo/components/style/properties/longhand/box.mako.rs
@@ -917,17 +917,17 @@
                          gecko_ffi_name="mAppearance",
                          gecko_constant_prefix="NS_THEME",
                          products="gecko",
                          animatable=False)}
 
 // Non-standard: https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-binding
 <%helpers:longhand name="-moz-binding" products="gecko" animatable="False" disable_when_testing="True">
     use cssparser::{CssStringWriter, ToCss};
-    use gecko_bindings::ptr::{GeckoArcPrincipal, GeckoArcURI};
+    use gecko_bindings::sugar::refptr::{GeckoArcPrincipal, GeckoArcURI};
     use std::fmt::{self, Write};
     use url::Url;
     use values::specified::UrlExtraData;
     use values::computed::ComputedValueAsSpecified;
     use values::NoViewportPercentage;
 
     #[derive(PartialEq, Clone, Debug)]
     #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
--- a/servo/components/style/values/specified/mod.rs
+++ b/servo/components/style/values/specified/mod.rs
@@ -1,17 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use app_units::Au;
 use cssparser::{self, Parser, ToCss, Token};
 use euclid::size::Size2D;
 #[cfg(feature = "gecko")]
-use gecko_bindings::ptr::{GeckoArcPrincipal, GeckoArcURI};
+use gecko_bindings::sugar::refptr::{GeckoArcPrincipal, GeckoArcURI};
 use parser::{Parse, ParserContext};
 #[cfg(feature = "gecko")]
 use parser::ParserContextExtraData;
 use std::ascii::AsciiExt;
 use std::cmp;
 use std::f32::consts::PI;
 use std::fmt;
 use std::ops::Mul;
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -26,23 +26,23 @@ use style::gecko::wrapper::{GeckoElement
 use style::gecko_bindings::bindings::{RawGeckoElementBorrowed, RawGeckoNodeBorrowed};
 use style::gecko_bindings::bindings::{RawServoStyleSetBorrowed, RawServoStyleSetOwned};
 use style::gecko_bindings::bindings::{RawServoStyleSheetBorrowed, ServoComputedValuesBorrowed};
 use style::gecko_bindings::bindings::{RawServoStyleSheetStrong, ServoComputedValuesStrong};
 use style::gecko_bindings::bindings::{ServoDeclarationBlockBorrowed, ServoDeclarationBlockStrong};
 use style::gecko_bindings::bindings::{ThreadSafePrincipalHolder, ThreadSafeURIHolder};
 use style::gecko_bindings::bindings::{nsHTMLCSSStyleSheet, ServoComputedValuesBorrowedOrNull};
 use style::gecko_bindings::bindings::Gecko_Utf8SliceToString;
-use style::gecko_bindings::ptr::{GeckoArcPrincipal, GeckoArcURI};
 use style::gecko_bindings::structs::{SheetParsingMode, nsIAtom};
 use style::gecko_bindings::structs::ServoElementSnapshot;
 use style::gecko_bindings::structs::nsRestyleHint;
 use style::gecko_bindings::structs::nsString;
 use style::gecko_bindings::sugar::ownership::{FFIArcHelpers, HasArcFFI, HasBoxFFI};
 use style::gecko_bindings::sugar::ownership::{HasSimpleFFI, Strong};
+use style::gecko_bindings::sugar::refptr::{GeckoArcPrincipal, GeckoArcURI};
 use style::parallel;
 use style::parser::{ParserContext, ParserContextExtraData};
 use style::properties::{ComputedValues, Importance, PropertyDeclaration};
 use style::properties::{PropertyDeclarationParseResult, PropertyDeclarationBlock};
 use style::properties::{cascade, parse_one_declaration};
 use style::selector_impl::PseudoElementCascadeType;
 use style::selector_matching::ApplicableDeclarationBlock;
 use style::sequential;
@@ -181,21 +181,21 @@ pub extern "C" fn Servo_StyleSheet_FromU
     let origin = match mode {
         SheetParsingMode::eAuthorSheetFeatures => Origin::Author,
         SheetParsingMode::eUserSheetFeatures => Origin::User,
         SheetParsingMode::eAgentSheetFeatures => Origin::UserAgent,
     };
 
     let base_str = unsafe { from_utf8_unchecked(slice::from_raw_parts(base_bytes, base_length as usize)) };
     let url = Url::parse(base_str).unwrap();
-    let extra_data = ParserContextExtraData {
+    let extra_data = unsafe { ParserContextExtraData {
         base: Some(GeckoArcURI::new(base)),
         referrer: Some(GeckoArcURI::new(referrer)),
         principal: Some(GeckoArcPrincipal::new(principal)),
-    };
+    }};
     let sheet = Arc::new(Stylesheet::from_str(input, url, origin, Box::new(StdoutErrorReporter),
                                               extra_data));
     unsafe {
         transmute(sheet)
     }
 }
 
 #[no_mangle]
@@ -385,21 +385,21 @@ pub extern "C" fn Servo_ParseProperty(pr
     // All this string wrangling is temporary until the Gecko string bindings land (bug 1294742).
     let name = unsafe { from_utf8_unchecked(slice::from_raw_parts(property_bytes,
                                                                   property_length as usize)) };
     let value_str = unsafe { from_utf8_unchecked(slice::from_raw_parts(value_bytes,
                                                                        value_length as usize)) };
     let base_str = unsafe { from_utf8_unchecked(slice::from_raw_parts(base_bytes,
                                                                       base_length as usize)) };
     let base_url = Url::parse(base_str).unwrap();
-    let extra_data = ParserContextExtraData {
+    let extra_data = unsafe { ParserContextExtraData {
         base: Some(GeckoArcURI::new(base)),
         referrer: Some(GeckoArcURI::new(referrer)),
         principal: Some(GeckoArcPrincipal::new(principal)),
-    };
+    }};
 
     let context = ParserContext::new_with_extra_data(Origin::Author, &base_url,
                                                      Box::new(StdoutErrorReporter),
                                                      extra_data);
 
     let mut results = vec![];
     match PropertyDeclaration::parse(name, &context, &mut Parser::new(value_str),
                                      &mut results, false) {