Auto merge of #17025 - emilio:layout-data, r=jdm

script/layout: Stop doing unsafe transmutes between refcell references.

This commit splits the style and layout data in two separate refcells.

These transmutes have been a source of trouble (for example on Android), and
they feel like a hack anyway.

Fixes #16982

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17025)
<!-- Reviewable:end -->

components/layout/construct.rs

@@ -17,7 +17,7 @@ use StyleArc;
 use app_units::Au;
 use block::BlockFlow;
 use context::{LayoutContext, with_thread_local_font_context};
-use data::{HAS_NEWLY_CONSTRUCTED_FLOW, PersistentLayoutData};
+use data::{HAS_NEWLY_CONSTRUCTED_FLOW, LayoutData};
 use flex::FlexFlow;
 use floats::FloatKind;
 use flow::{self, AbsoluteDescendants, Flow, FlowClass, ImmutableFlowUtils};
@@ -1621,7 +1621,7 @@ trait NodeUtils {
     /// Returns true if this node doesn't render its kids and false otherwise.
     fn is_replaced_content(&self) -> bool;
 
-    fn construction_result_mut(self, layout_data: &mut PersistentLayoutData) -> &mut ConstructionResult;
+    fn construction_result_mut(self, layout_data: &mut LayoutData) -> &mut ConstructionResult;
 
     /// Sets the construction result of a flow.
     fn set_flow_construction_result(self, result: ConstructionResult);
@@ -1645,7 +1645,7 @@ impl<ConcreteThreadSafeLayoutNode> NodeUtils for ConcreteThreadSafeLayoutNode
         }
     }
 
-    fn construction_result_mut(self, data: &mut PersistentLayoutData) -> &mut ConstructionResult {
+    fn construction_result_mut(self, data: &mut LayoutData) -> &mut ConstructionResult {
         match self.get_pseudo_element_type() {
             PseudoElementType::Before(_) => &mut data.before_flow_construction_result,
             PseudoElementType::After(_) => &mut data.after_flow_construction_result,

components/layout/data.rs

@@ -2,16 +2,32 @@
  * 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 atomic_refcell::AtomicRefCell;
 use construct::ConstructionResult;
-use script_layout_interface::PartialPersistentLayoutData;
+use script_layout_interface::StyleData;
 
-/// Data that layout associates with a node.
 #[repr(C)]
-pub struct PersistentLayoutData {
+pub struct StyleAndLayoutData {
     /// Data accessed by script_layout_interface. This must be first to allow
-    /// casting between PersistentLayoutData and PartialPersistentLayoutData.
-    pub base: PartialPersistentLayoutData,
+    /// casting between StyleAndLayoutData and StyleData.
+    pub style_data: StyleData,
+    /// The layout data associated with a node.
+    pub layout_data: AtomicRefCell<LayoutData>,
+}
 
+impl StyleAndLayoutData {
+    pub fn new() -> Self {
+        Self {
+            style_data: StyleData::new(),
+            layout_data: AtomicRefCell::new(LayoutData::new()),
+        }
+    }
+}
+
+
+/// Data that layout associates with a node.
+#[repr(C)]
+pub struct LayoutData {
     /// The current results of flow construction for this node. This is either a
     /// flow or a `ConstructionItem`. See comments in `construct.rs` for more
     /// details.
@@ -29,11 +45,10 @@ pub struct PersistentLayoutData {
     pub flags: LayoutDataFlags,
 }
 
-impl PersistentLayoutData {
+impl LayoutData {
     /// Creates new layout data.
-    pub fn new() -> PersistentLayoutData {
-        PersistentLayoutData {
-            base: PartialPersistentLayoutData::new(),
+    pub fn new() -> LayoutData {
+        Self {
             flow_construction_result: ConstructionResult::None,
             before_flow_construction_result: ConstructionResult::None,
             after_flow_construction_result: ConstructionResult::None,

components/layout/lib.rs

@@ -93,7 +93,7 @@ pub mod wrapper;
 // For unit tests:
 pub use fragment::Fragment;
 pub use fragment::SpecificFragmentInfo;
-pub use self::data::PersistentLayoutData;
+pub use self::data::LayoutData;
 
 /// Returns whether the two arguments point to the same value.
 ///

components/layout/wrapper.rs

@@ -30,39 +30,37 @@
 
 #![allow(unsafe_code)]
 
-use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
+use atomic_refcell::{AtomicRef, AtomicRefMut};
 use core::nonzero::NonZero;
-use data::{LayoutDataFlags, PersistentLayoutData};
-use script_layout_interface::{OpaqueStyleAndLayoutData, PartialPersistentLayoutData};
+use data::{LayoutData, LayoutDataFlags, StyleAndLayoutData};
+use script_layout_interface::{OpaqueStyleAndLayoutData, StyleData};
 use script_layout_interface::wrapper_traits::{LayoutNode, ThreadSafeLayoutElement, ThreadSafeLayoutNode};
 use script_layout_interface::wrapper_traits::GetLayoutData;
 use style::computed_values::content::{self, ContentItem};
 use style::dom::{NodeInfo, TNode};
 use style::selector_parser::RestyleDamage;
 
-pub type NonOpaqueStyleAndLayoutData = AtomicRefCell<PersistentLayoutData>;
-
 pub unsafe fn drop_style_and_layout_data(data: OpaqueStyleAndLayoutData) {
-    let ptr: *mut AtomicRefCell<PartialPersistentLayoutData> = data.ptr.get();
-    let non_opaque: *mut NonOpaqueStyleAndLayoutData = ptr as *mut _;
+    let ptr: *mut StyleData = data.ptr.get();
+    let non_opaque: *mut StyleAndLayoutData = ptr as *mut _;
     let _ = Box::from_raw(non_opaque);
 }
 
 pub trait LayoutNodeLayoutData {
     /// Similar to borrow_data*, but returns the full PersistentLayoutData rather
     /// than only the style::data::ElementData.
-    fn borrow_layout_data(&self) -> Option<AtomicRef<PersistentLayoutData>>;
-    fn mutate_layout_data(&self) -> Option<AtomicRefMut<PersistentLayoutData>>;
+    fn borrow_layout_data(&self) -> Option<AtomicRef<LayoutData>>;
+    fn mutate_layout_data(&self) -> Option<AtomicRefMut<LayoutData>>;
     fn flow_debug_id(self) -> usize;
 }
 
 impl<T: GetLayoutData> LayoutNodeLayoutData for T {
-    fn borrow_layout_data(&self) -> Option<AtomicRef<PersistentLayoutData>> {
-        self.get_raw_data().map(|d| d.borrow())
+    fn borrow_layout_data(&self) -> Option<AtomicRef<LayoutData>> {
+        self.get_raw_data().map(|d| d.layout_data.borrow())
     }
 
-    fn mutate_layout_data(&self) -> Option<AtomicRefMut<PersistentLayoutData>> {
-        self.get_raw_data().map(|d| d.borrow_mut())
+    fn mutate_layout_data(&self) -> Option<AtomicRefMut<LayoutData>> {
+        self.get_raw_data().map(|d| d.layout_data.borrow_mut())
     }
 
     fn flow_debug_id(self) -> usize {
@@ -71,13 +69,13 @@ impl<T: GetLayoutData> LayoutNodeLayoutData for T {
 }
 
 pub trait GetRawData {
-    fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData>;
+    fn get_raw_data(&self) -> Option<&StyleAndLayoutData>;
 }
 
 impl<T: GetLayoutData> GetRawData for T {
-    fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData> {
+    fn get_raw_data(&self) -> Option<&StyleAndLayoutData> {
         self.get_style_and_layout_data().map(|opaque| {
-            let container = opaque.ptr.get() as *mut NonOpaqueStyleAndLayoutData;
+            let container = opaque.ptr.get() as *mut StyleAndLayoutData;
             unsafe { &*container }
         })
     }
@@ -91,10 +89,10 @@ pub trait LayoutNodeHelpers {
 impl<T: LayoutNode> LayoutNodeHelpers for T {
     fn initialize_data(&self) {
         if self.get_raw_data().is_none() {
-            let ptr: *mut NonOpaqueStyleAndLayoutData =
-                Box::into_raw(box AtomicRefCell::new(PersistentLayoutData::new()));
+            let ptr: *mut StyleAndLayoutData =
+                Box::into_raw(Box::new(StyleAndLayoutData::new()));
             let opaque = OpaqueStyleAndLayoutData {
-                ptr: unsafe { NonZero::new(ptr as *mut AtomicRefCell<PartialPersistentLayoutData>) }
+                ptr: unsafe { NonZero::new(ptr as *mut StyleData) }
             };
             unsafe { self.init_style_and_layout_data(opaque) };
         };
@@ -171,21 +169,25 @@ impl<T: ThreadSafeLayoutNode> ThreadSafeLayoutNodeHelpers for T {
             debug_assert!(node.is_element());
         }
 
-        let data = node.borrow_layout_data().unwrap();
-        if let Some(r) = data.base.style_data.get_restyle() {
-            // We're reflowing a node that just got a restyle, and so the
-            // damage has been computed and stored in the RestyleData.
-            r.damage
-        } else if !data.flags.contains(::data::HAS_BEEN_TRAVERSED) {
-            // We're reflowing a node that was styled for the first time and
-            // has never been visited by layout. Return rebuild_and_reflow,
-            // because that's what the code expects.
-            RestyleDamage::rebuild_and_reflow()
-        } else {
-            // We're reflowing a node whose style data didn't change, but whose
-            // layout may change due to changes in ancestors or descendants.
-            RestyleDamage::empty()
-        }
+        let damage = {
+            let data = node.get_raw_data().unwrap();
+            if let Some(r) = data.style_data.element_data.borrow().get_restyle() {
+                // We're reflowing a node that just got a restyle, and so the
+                // damage has been computed and stored in the RestyleData.
+                r.damage
+            } else if !data.layout_data.borrow().flags.contains(::data::HAS_BEEN_TRAVERSED) {
+                // We're reflowing a node that was styled for the first time and
+                // has never been visited by layout. Return rebuild_and_reflow,
+                // because that's what the code expects.
+                RestyleDamage::rebuild_and_reflow()
+            } else {
+                // We're reflowing a node whose style data didn't change, but whose
+                // layout may change due to changes in ancestors or descendants.
+                RestyleDamage::empty()
+            }
+        };
+
+        damage
     }
 
 }

components/layout_thread/lib.rs

@@ -1173,7 +1173,7 @@ impl LayoutThread {
 
             // If we haven't styled this node yet, we don't need to track a
             // restyle.
-            let mut data = match el.mutate_layout_data() {
+            let style_data = match el.get_data() {
                 Some(d) => d,
                 None => {
                     unsafe { el.unset_snapshot_flags() };
@@ -1186,7 +1186,7 @@ impl LayoutThread {
                 map.insert(el.as_node().opaque(), s);
             }
 
-            let mut style_data = &mut data.base.style_data;
+            let mut style_data = style_data.borrow_mut();
             let mut restyle_data = style_data.ensure_restyle();
 
             // Stash the data on the element for processing by the style system.

components/script/layout_wrapper.rs

@@ -47,7 +47,7 @@ use html5ever::{LocalName, Namespace};
 use msg::constellation_msg::{BrowsingContextId, PipelineId};
 use range::Range;
 use script_layout_interface::{HTMLCanvasData, LayoutNodeType, SVGSVGData, TrustedNodeAddress};
-use script_layout_interface::{OpaqueStyleAndLayoutData, PartialPersistentLayoutData};
+use script_layout_interface::{OpaqueStyleAndLayoutData, StyleData};
 use script_layout_interface::wrapper_traits::{DangerousThreadSafeLayoutNode, GetLayoutData, LayoutNode};
 use script_layout_interface::wrapper_traits::{PseudoElementType, ThreadSafeLayoutElement, ThreadSafeLayoutNode};
 use selectors::attr::{AttrSelectorOperation, NamespaceConstraint};
@@ -452,12 +452,12 @@ impl<'le> TElement for ServoLayoutElement<'le> {
     }
 
     fn store_children_to_process(&self, n: isize) {
-        let data = self.get_partial_layout_data().unwrap().borrow();
+        let data = self.get_style_data().unwrap();
         data.parallel.children_to_process.store(n, Ordering::Relaxed);
     }
 
     fn did_process_child(&self) -> isize {
-        let data = self.get_partial_layout_data().unwrap().borrow();
+        let data = self.get_style_data().unwrap();
         let old_value = data.parallel.children_to_process.fetch_sub(1, Ordering::Relaxed);
         debug_assert!(old_value >= 1);
         old_value - 1
@@ -466,9 +466,7 @@ impl<'le> TElement for ServoLayoutElement<'le> {
     fn get_data(&self) -> Option<&AtomicRefCell<ElementData>> {
         unsafe {
             self.get_style_and_layout_data().map(|d| {
-                let ppld: &AtomicRefCell<PartialPersistentLayoutData> = &*d.ptr.get();
-                let psd: &AtomicRefCell<ElementData> = transmute(ppld);
-                psd
+                &(*d.ptr.get()).element_data
             })
         }
     }
@@ -537,7 +535,7 @@ impl<'le> ServoLayoutElement<'le> {
         }
     }
 
-    fn get_partial_layout_data(&self) -> Option<&AtomicRefCell<PartialPersistentLayoutData>> {
+    fn get_style_data(&self) -> Option<&StyleData> {
         unsafe {
             self.get_style_and_layout_data().map(|d| &*d.ptr.get())
         }

components/script_layout_interface/lib.rs

@@ -51,35 +51,32 @@ use std::sync::atomic::AtomicIsize;
 use style::data::ElementData;
 
 #[repr(C)]
-pub struct PartialPersistentLayoutData {
+pub struct StyleData {
     /// Data that the style system associates with a node. When the
     /// style system is being used standalone, this is all that hangs
     /// off the node. This must be first to permit the various
     /// transmutations between ElementData and PersistentLayoutData.
-    pub style_data: ElementData,
+    pub element_data: AtomicRefCell<ElementData>,
 
     /// Information needed during parallel traversals.
     pub parallel: DomParallelInfo,
-
-    // Required alignment for safe transmutes between PersistentLayoutData and PartialPersistentLayoutData.
-    _align: [u64; 0]
 }
 
-impl PartialPersistentLayoutData {
+impl StyleData {
     pub fn new() -> Self {
-        PartialPersistentLayoutData {
-            style_data: ElementData::new(None),
+        Self {
+            element_data: AtomicRefCell::new(ElementData::new(None)),
             parallel: DomParallelInfo::new(),
-            _align: [],
         }
     }
 }
 
 #[derive(Copy, Clone, HeapSizeOf)]
 pub struct OpaqueStyleAndLayoutData {
+    // NB: We really store a `StyleAndLayoutData` here, so be careful!
     #[ignore_heap_size_of = "TODO(#6910) Box value that should be counted but \
                              the type lives in layout"]
-    pub ptr: NonZero<*mut AtomicRefCell<PartialPersistentLayoutData>>
+    pub ptr: NonZero<*mut StyleData>
 }
 
 #[allow(unsafe_code)]

tests/unit/layout/lib.rs

@@ -7,5 +7,4 @@ extern crate layout;
 extern crate script_layout_interface;
 #[macro_use] extern crate size_of_test;
 
-#[cfg(test)] mod align_of;
 #[cfg(all(test, target_pointer_width = "64"))] mod size_of;