Review fixes and bindings resync

Manishearth · Manishearth · commit f5503e29116c · 2016-09-01T11:13:50.000+05:30
diff --git a/ports/geckolib/binding_tools/regen.py b/ports/geckolib/binding_tools/regen.py
@@ -164,6 +164,7 @@
             "RawGeckoNode",
             "RawGeckoElement",
             "RawGeckoDocument",
+            "StyleChildrenIterator",
         ],
     },
 
@@ -283,6 +284,22 @@ def build(objdir, target_name, debug, debugger, kind_name=None,
 
     flags = []
 
+    # This makes an FFI-safe void type that can't be matched on
+    # &VoidType is UB to have, because you can match on it
+    # to produce a reachable unreachable. If it's wrapped in
+    # a struct as a private field it becomes okay again
+    #
+    # Not 100% sure of how safe this is, but it's what we're using
+    # in the XPCOM ffi too
+    # https://github.com/nikomatsakis/rust-memory-model/issues/2
+    def zero_size_type(ty, flags):
+        flags.append("--blacklist-type")
+        flags.append(ty)
+        flags.append("--raw-line")
+        flags.append("pub enum {0}Void{{ }}".format(ty))
+        flags.append("--raw-line")
+        flags.append("pub struct {0}({0}Void);".format(ty))
+
     if "flags" in current_target:
         flags.extend(current_target["flags"])
 
@@ -334,18 +351,20 @@ def build(objdir, target_name, debug, debugger, kind_name=None,
             flags.append("pub type {0}Strong = ::sugar::ownership::Strong<{0}>;".format(ty))
             flags.append("--blacklist-type")
             flags.append("{}MaybeBorrowed".format(ty))
-            flags.append("-raw-line")
+            flags.append("--raw-line")
             flags.append("pub type {0}MaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, {0}>;".format(ty))
+            zero_size_type(ty, flags)
     if "servo_immutable_borrow_types" in current_target:
         for ty in current_target["servo_immutable_borrow_types"]:
-            flags.append("-blacklist-type")
+            flags.append("--blacklist-type")
             flags.append("{}Borrowed".format(ty))
             flags.append("--raw-line")
             flags.append("pub type {0}Borrowed<'a> = &'a {0};".format(ty))
             flags.append("--blacklist-type")
             flags.append("{}MaybeBorrowed".format(ty))
             flags.append("--raw-line")
             flags.append("pub type {0}MaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, {0}>;".format(ty))
+            zero_size_type(ty, flags)
     if "servo_owned_types" in current_target:
         for ty in current_target["servo_owned_types"]:
             flags.append("--blacklist-type")
@@ -360,6 +379,7 @@ def build(objdir, target_name, debug, debugger, kind_name=None,
             flags.append("{}Owned".format(ty))
             flags.append("--raw-line")
             flags.append("pub type {0}Owned = ::sugar::ownership::Owned<{0}>;".format(ty))
+            zero_size_type(ty, flags)
     if "servo_maybe_owned_types" in current_target:
         for ty in current_target["servo_maybe_owned_types"]:
             flags.append("--blacklist-type")
@@ -376,6 +396,7 @@ def build(objdir, target_name, debug, debugger, kind_name=None,
             flags.append("{}MaybeOwned".format(ty))
             flags.append("--raw-line")
             flags.append("pub type {0}MaybeOwned = ::sugar::ownership::MaybeOwned<{0}>;".format(ty))
+            zero_size_type(ty, flags)
     if "structs_types" in current_target:
         for ty in current_target["structs_types"]:
             ty_fragments = ty.split("::")
diff --git a/ports/geckolib/gecko_bindings/bindings.rs b/ports/geckolib/gecko_bindings/bindings.rs
@@ -7,22 +7,42 @@ pub enum nsIPrincipal {}
 pub enum nsIURI {}
 pub type ServoComputedValuesStrong = ::sugar::ownership::Strong<ServoComputedValues>;
 pub type ServoComputedValuesMaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, ServoComputedValues>;
+pub enum ServoComputedValuesVoid{ }
+pub struct ServoComputedValues(ServoComputedValuesVoid);
 pub type RawServoStyleSheetStrong = ::sugar::ownership::Strong<RawServoStyleSheet>;
 pub type RawServoStyleSheetMaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, RawServoStyleSheet>;
+pub enum RawServoStyleSheetVoid{ }
+pub struct RawServoStyleSheet(RawServoStyleSheetVoid);
 pub type ServoDeclarationBlockStrong = ::sugar::ownership::Strong<ServoDeclarationBlock>;
 pub type ServoDeclarationBlockMaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, ServoDeclarationBlock>;
+pub enum ServoDeclarationBlockVoid{ }
+pub struct ServoDeclarationBlock(ServoDeclarationBlockVoid);
 pub type RawGeckoNodeBorrowed<'a> = &'a RawGeckoNode;
 pub type RawGeckoNodeMaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, RawGeckoNode>;
+pub enum RawGeckoNodeVoid{ }
+pub struct RawGeckoNode(RawGeckoNodeVoid);
 pub type RawGeckoElementBorrowed<'a> = &'a RawGeckoElement;
 pub type RawGeckoElementMaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, RawGeckoElement>;
+pub enum RawGeckoElementVoid{ }
+pub struct RawGeckoElement(RawGeckoElementVoid);
 pub type RawGeckoDocumentBorrowed<'a> = &'a RawGeckoDocument;
 pub type RawGeckoDocumentMaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, RawGeckoDocument>;
+pub enum RawGeckoDocumentVoid{ }
+pub struct RawGeckoDocument(RawGeckoDocumentVoid);
+pub type StyleChildrenIteratorBorrowed<'a> = &'a StyleChildrenIterator;
+pub type StyleChildrenIteratorMaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, StyleChildrenIterator>;
+pub enum StyleChildrenIteratorVoid{ }
+pub struct StyleChildrenIterator(StyleChildrenIteratorVoid);
 pub type RawServoStyleSetBorrowed<'a> = &'a RawServoStyleSet;
 pub type RawServoStyleSetBorrowedMut<'a> = &'a mut RawServoStyleSet;
 pub type RawServoStyleSetOwned = ::sugar::ownership::Owned<RawServoStyleSet>;
+pub enum RawServoStyleSetVoid{ }
+pub struct RawServoStyleSet(RawServoStyleSetVoid);
 pub type ServoNodeDataMaybeBorrowed<'a> = ::sugar::ownership::Borrowed<'a, ServoNodeData>;
 pub type ServoNodeDataMaybeBorrowedMut<'a> = ::sugar::ownership::BorrowedMut<'a, ServoNodeData>;
 pub type ServoNodeDataMaybeOwned = ::sugar::ownership::MaybeOwned<ServoNodeData>;
+pub enum ServoNodeDataVoid{ }
+pub struct ServoNodeData(ServoNodeDataVoid);
 use structs::nsStyleFont;
 unsafe impl Send for nsStyleFont {}
 unsafe impl Sync for nsStyleFont {}
@@ -171,17 +191,9 @@ use structs::StyleBasicShapeType;
 use structs::StyleBasicShape;
 use structs::nsCSSShadowArray;
 
-pub type RawGeckoNode = nsINode;
 pub enum Element { }
-pub type RawGeckoElement = Element;
-pub type RawGeckoDocument = nsIDocument;
-pub enum ServoNodeData { }
-pub enum ServoComputedValues { }
-pub enum RawServoStyleSheet { }
-pub enum RawServoStyleSet { }
 pub enum nsHTMLCSSStyleSheet { }
-pub enum ServoDeclarationBlock { }
-pub enum StyleChildrenIterator { }
+pub type StyleChildrenIteratorBorrowedMut = *mut StyleChildrenIterator;
 pub type ThreadSafePrincipalHolder = nsMainThreadPtrHolder<nsIPrincipal>;
 pub type ThreadSafeURIHolder = nsMainThreadPtrHolder<nsIURI>;
 extern "C" {
@@ -210,12 +222,12 @@ extern "C" {
     pub fn Gecko_GetDocumentElement(document: RawGeckoDocumentBorrowed)
      -> RawGeckoElementMaybeBorrowed;
     pub fn Gecko_MaybeCreateStyleChildrenIterator(node: RawGeckoNodeBorrowed)
-     -> *mut StyleChildrenIterator;
+     -> StyleChildrenIteratorMaybeBorrowed;
     pub fn Gecko_DropStyleChildrenIterator(it: *mut StyleChildrenIterator);
-    pub fn Gecko_GetNextStyleChild(it: *mut StyleChildrenIterator)
-     -> *mut RawGeckoNode;
-    pub fn Gecko_ElementState(element: *mut RawGeckoElement) -> u8;
-    pub fn Gecko_IsHTMLElementInHTMLDocument(element: *mut RawGeckoElement)
+    pub fn Gecko_GetNextStyleChild(it: StyleChildrenIteratorBorrowed)
+     -> RawGeckoNodeMaybeBorrowed;
+    pub fn Gecko_ElementState(element: RawGeckoElementBorrowed) -> u8;
+    pub fn Gecko_IsHTMLElementInHTMLDocument(element: RawGeckoElementBorrowed)
      -> bool;
     pub fn Gecko_IsLink(element: RawGeckoElementBorrowed) -> bool;
     pub fn Gecko_IsTextNode(node: RawGeckoNodeBorrowed) -> bool;
@@ -494,13 +506,16 @@ extern "C" {
     pub fn Servo_StyleSet_Init() -> RawServoStyleSetOwned;
     pub fn Servo_StyleSet_Drop(set: RawServoStyleSetOwned);
     pub fn Servo_StyleSet_AppendStyleSheet(set: RawServoStyleSetBorrowedMut,
-                                           sheet: RawServoStyleSheetMaybeBorrowed);
+                                           sheet:
+                                               RawServoStyleSheetMaybeBorrowed);
     pub fn Servo_StyleSet_PrependStyleSheet(set: RawServoStyleSetBorrowedMut,
                                             sheet:
                                                 RawServoStyleSheetMaybeBorrowed);
     pub fn Servo_StyleSet_RemoveStyleSheet(set: RawServoStyleSetBorrowedMut,
-                                           sheet: RawServoStyleSheetMaybeBorrowed);
-    pub fn Servo_StyleSet_InsertStyleSheetBefore(set: RawServoStyleSetBorrowedMut,
+                                           sheet:
+                                               RawServoStyleSheetMaybeBorrowed);
+    pub fn Servo_StyleSet_InsertStyleSheetBefore(set:
+                                                     RawServoStyleSetBorrowedMut,
                                                  sheet:
                                                      RawServoStyleSheetMaybeBorrowed,
                                                  reference:
@@ -526,12 +541,13 @@ extern "C" {
     pub fn Servo_ComputedValues_GetForAnonymousBox(parent_style_or_null:
                                                        ServoComputedValuesMaybeBorrowed,
                                                    pseudoTag: *mut nsIAtom,
-                                                   set: RawServoStyleSetBorrowedMut)
+                                                   set:
+                                                       RawServoStyleSetBorrowedMut)
      -> ServoComputedValuesStrong;
     pub fn Servo_ComputedValues_GetForPseudoElement(parent_style:
                                                         ServoComputedValuesMaybeBorrowed,
                                                     match_element:
-                                                        *mut RawGeckoElement,
+                                                        RawGeckoElementBorrowed,
                                                     pseudo_tag: *mut nsIAtom,
                                                     set:
                                                         RawServoStyleSetBorrowedMut,
diff --git a/ports/geckolib/gecko_bindings/sugar/ownership.rs b/ports/geckolib/gecko_bindings/sugar/ownership.rs
@@ -3,7 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use std::marker::PhantomData;
-use std::mem::{forget, transmute, transmute_copy};
+use std::mem::{forget, transmute};
 use std::ops::{Deref, DerefMut};
 use std::ptr;
 use std::sync::Arc;
@@ -73,7 +73,7 @@ pub unsafe trait HasArcFFI : HasFFI {
     // impl parameter
     /// Artificially increments the refcount of a borrowed Arc over FFI.
     unsafe fn addref(ptr: Borrowed<Self::FFIType>) {
-        forget(ptr.as_arc::<Self>().clone())
+        forget(ptr.as_arc_opt::<Self>().clone())
     }
 
     /// Given a (possibly null) borrowed FFI reference, decrements the refcount.
@@ -113,7 +113,7 @@ impl<'a, T> Clone for Borrowed<'a, T> {
 
 impl<'a, T> Borrowed<'a, T> {
     #[inline]
-    pub fn is_null(&self) -> bool {
+    pub fn is_null(self) -> bool {
         self.ptr == ptr::null()
     }
 
@@ -150,7 +150,7 @@ impl<'a, T> Borrowed<'a, T> {
 
     #[inline]
     /// Borrowed<ServoType> -> Borrowed<GeckoType>
-    pub fn as_ffi(&self) -> Borrowed<<Self as HasFFI>::FFIType> where Self: HasSimpleFFI {
+    pub fn as_ffi(self) -> Borrowed<'a, <Self as HasFFI>::FFIType> where Self: HasSimpleFFI {
         unsafe { transmute(self) }
     }
 
@@ -165,6 +165,13 @@ impl<'a, T> Borrowed<'a, T> {
     pub fn as_servo_ref<U>(self) -> Option<&'a U> where U: HasSimpleFFI<FFIType = T> {
         self.borrow_opt().map(HasSimpleFFI::from_ffi)
     }
+
+    pub fn null() -> Borrowed<'static, T> {
+        Borrowed {
+            ptr: ptr::null_mut(),
+            _marker: PhantomData
+        }
+    }
 }
 
 impl<'a, T> BorrowedMut<'a, T> {
@@ -196,9 +203,17 @@ impl<'a, T> BorrowedMut<'a, T> {
     pub fn as_servo_mut_ref<U>(self) -> Option<&'a mut U> where U: HasSimpleFFI<FFIType = T> {
         self.borrow_mut_opt().map(HasSimpleFFI::from_ffi_mut)
     }
+
+    pub fn null_mut() -> BorrowedMut<'static, T> {
+        BorrowedMut {
+            ptr: ptr::null_mut(),
+            _marker: PhantomData
+        }
+    }
 }
 
-// technically not
+// technically not how we're supposed to use
+// Deref, but that's a minor style issue
 impl<'a, T> Deref for BorrowedMut<'a, T> {
     type Target = Borrowed<'a, T>;
     fn deref(&self) -> &Self::Target {
@@ -209,6 +224,8 @@ impl<'a, T> Deref for BorrowedMut<'a, T> {
 #[repr(C)]
 /// Gecko-FFI-safe Arc (T is an ArcInner).
 /// This can be null.
+/// Leaks on drop. Please don't drop this.
+/// TODO: Add destructor bomb once drop flags are gone
 pub struct Strong<T> {
     ptr: *const T,
     _marker: PhantomData<T>,
@@ -227,13 +244,26 @@ impl<T> Strong<T> {
     ///
     /// Strong<GeckoType> -> Arc<ServoType>
     pub fn into_arc<U>(self) -> Arc<U> where U: HasArcFFI<FFIType = T> {
-        assert!(!self.is_null());
-        unsafe { transmute(self) }
+        self.into_arc_opt().unwrap()
+    }
+
+    #[inline]
+    /// Given a strong FFI reference,
+    /// converts it into a servo-side Arc
+    /// Returns None on null.
+    ///
+    /// Strong<GeckoType> -> Arc<ServoType>
+    pub fn into_arc_opt<U>(self) -> Option<Arc<U>> where U: HasArcFFI<FFIType = T> {
+        if self.is_null() {
+            None
+        } else {
+            unsafe { Some(transmute(self)) }
+        }
     }
 
     #[inline]
     /// Produces a null strong FFI reference
-    pub fn null_strong() -> Self {
+    pub fn null() -> Self {
         unsafe { transmute(ptr::null::<T>()) }
     }
 }
@@ -266,6 +296,7 @@ unsafe impl<T: HasArcFFI> FFIArcHelpers for Arc<T> {
 #[repr(C)]
 /// Gecko-FFI-safe owned pointer
 /// Cannot be null
+/// Leaks on drop. Please don't drop this.
 pub struct Owned<T> {
     ptr: *mut T,
     _marker: PhantomData<T>,
diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs
@@ -264,7 +264,7 @@ pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(parent_style_or_null:
 
     let maybe_parent = parent_style_or_null.as_arc_opt();
     let new_computed = data.stylist.precomputed_values_for_pseudo(&pseudo, maybe_parent);
-    new_computed.map_or(Strong::null_strong(), |c| c.into_strong())
+    new_computed.map_or(Strong::null(), |c| c.into_strong())
 }
 
 #[no_mangle]
@@ -278,7 +278,7 @@ pub extern "C" fn Servo_ComputedValues_GetForPseudoElement(parent_style: ServoCo
 
     let parent_or_null = || {
         if is_probe {
-            Strong::null_strong()
+            Strong::null()
         } else {
             parent_style.as_arc::<ComputedValues>().clone().into_strong()
         }
diff --git a/ports/geckolib/wrapper.rs b/ports/geckolib/wrapper.rs
@@ -165,8 +165,8 @@ impl<'ln> TNode for GeckoNode<'ln> {
 
     fn children(self) -> GeckoChildrenIterator<'ln> {
         let maybe_iter = unsafe { Gecko_MaybeCreateStyleChildrenIterator(self.0) };
-        if !maybe_iter.is_null() {
-            GeckoChildrenIterator::GeckoIterator(maybe_iter)
+        if let Some(iter) = maybe_iter.borrow_opt() {
+            GeckoChildrenIterator::GeckoIterator(iter)
         } else {
             GeckoChildrenIterator::Current(self.first_child())
         }
@@ -346,14 +346,14 @@ impl<'ln> TNode for GeckoNode<'ln> {
 // (heavier-weight) Gecko-implemented iterator.
 pub enum GeckoChildrenIterator<'a> {
     Current(Option<GeckoNode<'a>>),
-    GeckoIterator(*mut bindings::StyleChildrenIterator),
+    GeckoIterator(bindings::StyleChildrenIteratorBorrowed<'a>),
 }
 
 impl<'a> Drop for GeckoChildrenIterator<'a> {
     fn drop(&mut self) {
-        if let GeckoChildrenIterator::GeckoIterator(it) = *self {
+        if let GeckoChildrenIterator::GeckoIterator(ref it) = *self {
             unsafe {
-                Gecko_DropStyleChildrenIterator(it);
+                Gecko_DropStyleChildrenIterator(*it as *const _ as *mut _);
             }
         }
     }
@@ -369,7 +369,7 @@ impl<'a> Iterator for GeckoChildrenIterator<'a> {
                 curr
             },
             GeckoChildrenIterator::GeckoIterator(it) => unsafe {
-                Gecko_GetNextStyleChild(it).as_ref().map(|n| GeckoNode::from_ref(n))
+                Gecko_GetNextStyleChild(it).borrow_opt().map(GeckoNode)
             }
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -264,7 +264,7 @@ pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(parent_style_or_null:`
`264`	`264`
`265`	`265`	`let maybe_parent = parent_style_or_null.as_arc_opt();`
`266`	`266`	`let new_computed = data.stylist.precomputed_values_for_pseudo(&pseudo, maybe_parent);`
`267`		`- new_computed.map_or(Strong::null_strong(), \|c\| c.into_strong())`
	`267`	`+ new_computed.map_or(Strong::null(), \|c\| c.into_strong())`
`268`	`268`	`}`
`269`	`269`
`270`	`270`	`#[no_mangle]`
`@@ -278,7 +278,7 @@ pub extern "C" fn Servo_ComputedValues_GetForPseudoElement(parent_style: ServoCo`
`278`	`278`
`279`	`279`	`let parent_or_null = \|\| {`
`280`	`280`	`if is_probe {`
`281`		`- Strong::null_strong()`
	`281`	`+ Strong::null()`
`282`	`282`	`} else {`
`283`	`283`	`parent_style.as_arc::<ComputedValues>().clone().into_strong()`
`284`	`284`	`}`
Original file line number	Diff line number	Diff line change
`@@ -165,8 +165,8 @@ impl<'ln> TNode for GeckoNode<'ln> {`
`165`	`165`
`166`	`166`	`fn children(self) -> GeckoChildrenIterator<'ln> {`
`167`	`167`	`let maybe_iter = unsafe { Gecko_MaybeCreateStyleChildrenIterator(self.0) };`
`168`		`- if !maybe_iter.is_null() {`
`169`		`- GeckoChildrenIterator::GeckoIterator(maybe_iter)`
	`168`	`+ if let Some(iter) = maybe_iter.borrow_opt() {`
	`169`	`+ GeckoChildrenIterator::GeckoIterator(iter)`
`170`	`170`	`} else {`
`171`	`171`	`GeckoChildrenIterator::Current(self.first_child())`
`172`	`172`	`}`
`@@ -346,14 +346,14 @@ impl<'ln> TNode for GeckoNode<'ln> {`
`346`	`346`	`// (heavier-weight) Gecko-implemented iterator.`
`347`	`347`	`pub enum GeckoChildrenIterator<'a> {`
`348`	`348`	`Current(Option<GeckoNode<'a>>),`
`349`		`- GeckoIterator(*mut bindings::StyleChildrenIterator),`
	`349`	`+ GeckoIterator(bindings::StyleChildrenIteratorBorrowed<'a>),`
`350`	`350`	`}`
`351`	`351`
`352`	`352`	`impl<'a> Drop for GeckoChildrenIterator<'a> {`
`353`	`353`	`fn drop(&mut self) {`
`354`		`- if let GeckoChildrenIterator::GeckoIterator(it) = *self {`
	`354`	`+ if let GeckoChildrenIterator::GeckoIterator(ref it) = *self {`
`355`	`355`	`unsafe {`
`356`		`- Gecko_DropStyleChildrenIterator(it);`
	`356`	`+ Gecko_DropStyleChildrenIterator(it as const _ as *mut _);`
`357`	`357`	`}`
`358`	`358`	`}`
`359`	`359`	`}`
`@@ -369,7 +369,7 @@ impl<'a> Iterator for GeckoChildrenIterator<'a> {`
`369`	`369`	`curr`
`370`	`370`	`},`
`371`	`371`	`GeckoChildrenIterator::GeckoIterator(it) => unsafe {`
`372`		`- Gecko_GetNextStyleChild(it).as_ref().map(\|n\| GeckoNode::from_ref(n))`
	`372`	`+ Gecko_GetNextStyleChild(it).borrow_opt().map(GeckoNode)`
`373`	`373`	`}`
`374`	`374`	`}`
`375`	`375`	`}`