servo: Merge #20339 - style: Remove unsound Atom From implementations (from emilio:atom-from-dead-beef); r=nox

Fixes #20158

Source-Repo: https://github.com/servo/servo
Source-Revision: b47224dbcce2974e6f572b284cba1e2f5393b95d

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 68a732c7b4fa6564e34427438a019385391585b9

servo/components/style/gecko/conversions.rs

@@ -413,7 +413,7 @@ impl nsStyleImage {
             nsStyleImageType::eStyleImageType_Element => {
                 use gecko_string_cache::Atom;
                 let atom = Gecko_GetImageElement(self);
-                Some(GenericImage::Element(Atom::from(atom)))
+                Some(GenericImage::Element(Atom::from_raw(atom)))
             },
             _ => panic!("Unexpected image type")
         }

servo/components/style/gecko/media_queries.rs

@@ -176,7 +176,7 @@ impl Device {
             context.mMedium
         };
 
-        MediaType(CustomIdent(Atom::from(medium_to_use)))
+        MediaType(CustomIdent(unsafe { Atom::from_raw(medium_to_use) }))
     }
 
     /// Returns the current viewport size in app units.
@@ -262,7 +262,7 @@ impl ToCss for Expression {
         }
 
         // NB: CssStringWriter not needed, feature names are under control.
-        write!(dest, "{}", Atom::from(unsafe { *self.feature.mName }))?;
+        write!(dest, "{}", unsafe { Atom::from_static(*self.feature.mName) })?;
 
         if let Some(ref val) = self.value {
             dest.write_str(": ")?;
@@ -394,9 +394,9 @@ impl MediaExpressionValue {
             }
             nsMediaFeature_ValueType::eIdent => {
                 debug_assert_eq!(css_value.mUnit, nsCSSUnit::eCSSUnit_AtomIdent);
-                Some(MediaExpressionValue::Ident(Atom::from(unsafe {
+                Some(MediaExpressionValue::Ident(unsafe {
-                    *css_value.mValue.mAtom.as_ref()
+                    Atom::from_raw(*css_value.mValue.mAtom.as_ref())
-                })))
+                }))
             }
             nsMediaFeature_ValueType::eIntRatio => {
                 let array = unsafe { css_value.array_unchecked() };

servo/components/style/gecko/values.rs

@@ -507,7 +507,7 @@ impl CounterStyleOrNone {
 
         let name = unsafe { bindings::Gecko_CounterStyle_GetName(gecko_value) };
         if !name.is_null() {
-            let name = Atom::from(name);
+            let name = unsafe { Atom::from_raw(name) };
             if name == atom!("none") {
                 Either::First(CounterStyleOrNone::None)
             } else {

servo/components/style/gecko_string_cache/mod.rs

@@ -99,7 +99,9 @@ impl WeakAtom {
     /// Clone this atom, bumping the refcount if the atom is not static.
     #[inline]
     pub fn clone(&self) -> Atom {
-        Atom::from(self.as_ptr())
+        unsafe {
+            Atom::from_raw(self.as_ptr())
+        }
     }
 
     /// Get the atom hash.
@@ -267,15 +269,25 @@ impl Atom {
     ///
     /// Right now it's only used by the atom macro, and ideally it should keep
     /// that way, now we have sugar for is_static, creating atoms using
-    /// Atom::from should involve almost no overhead.
+    /// Atom::from_raw should involve almost no overhead.
     #[inline]
-    unsafe fn from_static(ptr: *mut nsStaticAtom) -> Self {
+    pub unsafe fn from_static(ptr: *mut nsStaticAtom) -> Self {
         let atom = Atom(ptr as *mut WeakAtom);
         debug_assert!(atom.is_static(),
                       "Called from_static for a non-static atom!");
         atom
     }
 
+    /// Creates an atom from an atom pointer.
+    #[inline(always)]
+    pub unsafe fn from_raw(ptr: *mut nsAtom) -> Self {
+        let atom = Atom(ptr as *mut WeakAtom);
+        if !atom.is_static() {
+            Gecko_AddRefAtom(ptr);
+        }
+        atom
+    }
+
     /// Creates an atom from a dynamic atom pointer that has already had AddRef
     /// called on it.
     #[inline]
@@ -308,7 +320,9 @@ impl Hash for WeakAtom {
 impl Clone for Atom {
     #[inline(always)]
     fn clone(&self) -> Atom {
-        Atom::from(self.as_ptr())
+        unsafe {
+            Atom::from_raw(self.as_ptr())
+        }
     }
 }
 
@@ -388,28 +402,4 @@ impl From<String> for Atom {
     }
 }
 
-impl From<*mut nsAtom> for Atom {
-    #[inline]
-    fn from(ptr: *mut nsAtom) -> Atom {
-        assert!(!ptr.is_null());
-        unsafe {
-            let ret = Atom(WeakAtom::new(ptr));
-            if !ret.is_static() {
-                Gecko_AddRefAtom(ptr);
-            }
-            ret
-        }
-    }
-}
-
-impl From<*mut nsStaticAtom> for Atom {
-    #[inline]
-    fn from(ptr: *mut nsStaticAtom) -> Atom {
-        assert!(!ptr.is_null());
-        unsafe {
-            Atom::from_static(ptr)
-        }
-    }
-}
-
 malloc_size_of_is_0!(Atom);

servo/components/style/properties/gecko.mako.rs

@@ -131,7 +131,7 @@ impl ComputedValues {
             return None;
         }
 
-        let atom = Atom::from(atom);
+        let atom = unsafe { Atom::from_raw(atom) };
         PseudoElement::from_atom(&atom)
     }
 
@@ -3248,12 +3248,15 @@ fn static_assert() {
         use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_no_properties;
         use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_variable;
         use gecko_bindings::structs::nsCSSPropertyID::eCSSProperty_UNKNOWN;
+        use Atom;
 
         let property = self.gecko.mTransitions[index].mProperty;
         if property == eCSSProperty_UNKNOWN || property == eCSSPropertyExtra_variable {
             let atom = self.gecko.mTransitions[index].mUnknownProperty.mRawPtr;
             debug_assert!(!atom.is_null());
-            TransitionProperty::Unsupported(CustomIdent(atom.into()))
+            TransitionProperty::Unsupported(CustomIdent(unsafe{
+                Atom::from_raw(atom)
+            }))
         } else if property == eCSSPropertyExtra_no_properties {
             // Actually, we don't expect TransitionProperty::Unsupported also represents "none",
             // but if the caller wants to convert it, it is fine. Please use it carefully.
@@ -3343,13 +3346,13 @@ fn static_assert() {
     pub fn animation_name_at(&self, index: usize)
         -> longhands::animation_name::computed_value::SingleComputedValue {
         use properties::longhands::animation_name::single_value::SpecifiedValue as AnimationName;
+        use Atom;
 
         let atom = self.gecko.mAnimations[index].mName.mRawPtr;
         if atom == atom!("").as_ptr() {
-            AnimationName(None)
+            return AnimationName(None)
-        } else {
-            AnimationName(Some(KeyframesName::from_atom(atom.into())))
         }
+        AnimationName(Some(KeyframesName::from_atom(unsafe { Atom::from_raw(atom) })))
     }
     pub fn copy_animation_name_from(&mut self, other: &Self) {
         self.gecko.mAnimationNameCount = other.gecko.mAnimationNameCount;
@@ -3549,16 +3552,19 @@ fn static_assert() {
         use properties::longhands::will_change::computed_value::T;
         use gecko_bindings::structs::nsAtom;
         use values::CustomIdent;
+        use Atom;
 
         if self.gecko.mWillChange.len() == 0 {
-            T::Auto
+            return T::Auto
-        } else {
-            let custom_idents: Vec<CustomIdent> = self.gecko.mWillChange.iter().map(|gecko_atom| {
-                CustomIdent((gecko_atom.mRawPtr as *mut nsAtom).into())
-            }).collect();
-
-            T::AnimateableFeatures(custom_idents.into_boxed_slice())
         }
+
+        let custom_idents: Vec<CustomIdent> = self.gecko.mWillChange.iter().map(|gecko_atom| {
+            unsafe {
+                CustomIdent(Atom::from_raw(gecko_atom.mRawPtr as *mut nsAtom))
+            }
+        }).collect();
+
+        T::AnimateableFeatures(custom_idents.into_boxed_slice())
     }
 
     <% impl_shape_source("shape_outside", "mShapeOutside") %>

servo/components/style/stylist.rs

@@ -1636,7 +1636,7 @@ impl ExtraStyleData {
         guard: &SharedRwLockReadGuard,
         rule: &Arc<Locked<CounterStyleRule>>,
     ) {
-        let name = rule.read_with(guard).mName.mRawPtr.into();
+        let name = unsafe { Atom::from_raw(rule.read_with(guard).mName.mRawPtr) };
         self.counter_styles.insert(name, rule.clone());
     }
 

servo/ports/geckolib/glue.rs

@@ -2020,9 +2020,9 @@ pub extern "C" fn Servo_KeyframesRule_GetName(rule: RawServoKeyframesRuleBorrowe
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_KeyframesRule_SetName(rule: RawServoKeyframesRuleBorrowed, name: *mut nsAtom) {
+pub unsafe extern "C" fn Servo_KeyframesRule_SetName(rule: RawServoKeyframesRuleBorrowed, name: *mut nsAtom) {
     write_locked_arc(rule, |rule: &mut KeyframesRule| {
-        rule.name = KeyframesName::Ident(CustomIdent(unsafe { Atom::from_addrefed(name) }));
+        rule.name = KeyframesName::Ident(CustomIdent(Atom::from_addrefed(name)));
     })
 }
 
@@ -2167,7 +2167,7 @@ pub extern "C" fn Servo_FontFeatureValuesRule_GetValueText(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(
+pub unsafe extern "C" fn Servo_ComputedValues_GetForAnonymousBox(
     parent_style_or_null: ServoStyleContextBorrowedOrNull,
     pseudo_tag: *mut nsAtom,
     raw_data: RawServoStyleSetBorrowed,
@@ -2176,7 +2176,7 @@ pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(
     let guard = global_style_data.shared_lock.read();
     let guards = StylesheetGuards::same(&guard);
     let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
-    let atom = Atom::from(pseudo_tag);
+    let atom = Atom::from_raw(pseudo_tag);
     let pseudo = PseudoElement::from_anon_box_atom(&atom)
         .expect("Not an anon box pseudo?");
 
@@ -2478,7 +2478,7 @@ fn get_pseudo_style(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_ComputedValues_Inherit(
+pub unsafe extern "C" fn Servo_ComputedValues_Inherit(
     raw_data: RawServoStyleSetBorrowed,
     pseudo_tag: *mut nsAtom,
     parent_style_context: ServoStyleContextBorrowedOrNull,
@@ -2487,7 +2487,7 @@ pub extern "C" fn Servo_ComputedValues_Inherit(
     let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
 
     let for_text = target == structs::InheritTarget::Text;
-    let atom = Atom::from(pseudo_tag);
+    let atom = Atom::from_raw(pseudo_tag);
     let pseudo = PseudoElement::from_anon_box_atom(&atom)
         .expect("Not an anon-box? Gah!");
 
@@ -3210,7 +3210,7 @@ pub extern "C" fn Servo_DeclarationBlock_PropertyIsSet(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_DeclarationBlock_SetIdentStringValue(
+pub unsafe extern "C" fn Servo_DeclarationBlock_SetIdentStringValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
     value: *mut nsAtom,
@@ -3220,7 +3220,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetIdentStringValue(
 
     let long = get_longhand_from_id!(property);
     let prop = match_wrap_declared! { long,
-        XLang => Lang(Atom::from(value)),
+        XLang => Lang(Atom::from_raw(value)),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
         decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
@@ -4138,7 +4138,7 @@ fn fill_in_missing_keyframe_values(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_StyleSet_GetKeyframesForName(
+pub unsafe extern "C" fn Servo_StyleSet_GetKeyframesForName(
     raw_data: RawServoStyleSetBorrowed,
     name: *mut nsAtom,
     inherited_timing_function: nsTimingFunctionBorrowed,
@@ -4148,7 +4148,7 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(
                   "keyframes should be initially empty");
 
     let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
-    let name = Atom::from(name);
+    let name = Atom::from_raw(name);
 
     let animation = match data.stylist.get_animation(&name) {
         Some(animation) => animation,
@@ -4183,11 +4183,11 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(
         // Look for an existing keyframe with the same offset and timing
         // function or else add a new keyframe at the beginning of the keyframe
         // array.
-        let keyframe = unsafe {
+        let keyframe = Gecko_GetOrCreateKeyframeAtStart(
-            Gecko_GetOrCreateKeyframeAtStart(keyframes,
+            keyframes,
-                                             step.start_percentage.0 as f32,
+            step.start_percentage.0 as f32,
-                                             &timing_function)
+            &timing_function,
-        };
+        );
 
         match step.value {
             KeyframesStepValue::ComputedValues => {
@@ -4197,12 +4197,10 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(
                 // animation should be set to the underlying computed value for
                 // that keyframe.
                 for property in animation.properties_changed.iter() {
-                    unsafe {
+                    Gecko_AppendPropertyValuePair(
-                        Gecko_AppendPropertyValuePair(
+                        &mut (*keyframe).mPropertyValues,
-                            &mut (*keyframe).mPropertyValues,
+                        property.to_nscsspropertyid(),
-                            property.to_nscsspropertyid(),
+                    );
-                        );
-                    }
                 }
                 if current_offset == 0.0 {
                     has_complete_initial_keyframe = true;
@@ -4249,23 +4247,19 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(
                         continue;
                     }
 
-                    let pair = unsafe {
+                    let pair = Gecko_AppendPropertyValuePair(
-                        Gecko_AppendPropertyValuePair(
+                        &mut (*keyframe).mPropertyValues,
-                            &mut (*keyframe).mPropertyValues,
+                        id.to_nscsspropertyid(),
-                            id.to_nscsspropertyid(),
+                    );
-                        )
-                    };
 
-                    unsafe {
+                    (*pair).mServoDeclarationBlock.set_arc_leaky(
-                        (*pair).mServoDeclarationBlock.set_arc_leaky(
+                        Arc::new(global_style_data.shared_lock.wrap(
-                            Arc::new(global_style_data.shared_lock.wrap(
+                            PropertyDeclarationBlock::with_one(
-                                PropertyDeclarationBlock::with_one(
+                                declaration.clone(),
-                                    declaration.clone(),
+                                Importance::Normal,
-                                    Importance::Normal,
+                            )
-                                )
+                        ))
-                            ))
+                    );
-                        );
-                    }
 
                     if current_offset == 0.0 {
                         properties_set_at_start.insert(id);
@@ -4276,19 +4270,14 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(
                 }
 
                 if custom_properties.any_normal() {
-                    let pair = unsafe {
+                    let pair = Gecko_AppendPropertyValuePair(
-                        Gecko_AppendPropertyValuePair(
+                        &mut (*keyframe).mPropertyValues,
-                            &mut (*keyframe).mPropertyValues,
+                        nsCSSPropertyID::eCSSPropertyExtra_variable,
-                            nsCSSPropertyID::eCSSPropertyExtra_variable,
+                    );
-                        )
-                    };
-
-                    unsafe {
-                        (*pair).mServoDeclarationBlock.set_arc_leaky(Arc::new(
-                            global_style_data.shared_lock.wrap(custom_properties)
-                        ));
-                    }
 
+                    (*pair).mServoDeclarationBlock.set_arc_leaky(Arc::new(
+                        global_style_data.shared_lock.wrap(custom_properties)
+                    ));
                 }
             },
         }
@@ -4498,7 +4487,7 @@ pub extern "C" fn Servo_StyleSet_HasDocumentStateDependency(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_GetCustomPropertyValue(
+pub unsafe extern "C" fn Servo_GetCustomPropertyValue(
     computed_values: ServoStyleContextBorrowed,
     name: *const nsAString,
     value: *mut nsAString,
@@ -4508,13 +4497,13 @@ pub extern "C" fn Servo_GetCustomPropertyValue(
         None => return false,
     };
 
-    let name = unsafe { Atom::from(&*name) };
+    let name = Atom::from(&*name);
     let computed_value = match custom_properties.get(&name) {
         Some(v) => v,
         None => return false,
     };
 
-    computed_value.to_css(&mut CssWriter::new(unsafe { value.as_mut().unwrap() })).unwrap();
+    computed_value.to_css(&mut CssWriter::new(&mut *value)).unwrap();
     true
 }