Skip to content

Commit

Permalink
Make FlowRef a newtype
Browse files Browse the repository at this point in the history 
This creates a sharp distinction between `Arc<Flow>`s, which may be
owned by anyone, and `FlowRef`s, which may only be owned by the
traversal code. By checking the reference count, we ensure that a `Flow`
cannot be pointed to by `Arc`s and `FlowRef`s simultaneously.

This is not a complete fix for #6503, though it is a necessary start
(enforcing the no-aliasing rule of `FlowRef::deref_mut` will require far
more work).

Fixes #14014
  • Loading branch information
@notriddle
notriddle committed Nov 4, 2016
1 parent 5b4cc95 commit 5f320bd
Show file tree
Hide file tree
Showing 10 changed files with 146 additions and 102 deletions.
9 changes: 4 additions & 5 deletions components/layout/block.rs
View file
Expand Up @@ -40,7 +40,6 @@ use flow::{FragmentationContext, MARGINS_CANNOT_COLLAPSE, PreorderFlowTraversal}
use flow::{ImmutableFlowUtils, LateAbsolutePositionInfo, MutableFlowUtils, OpaqueFlow};
use flow::IS_ABSOLUTELY_POSITIONED;
use flow_list::FlowList;
use flow_ref::FlowRef;
use fragment::{CoordinateSystem, Fragment, FragmentBorderBoxIterator, Overflow};
use fragment::SpecificFragmentInfo;
use gfx::display_list::{ClippingRegion, StackingContext};
Expand Down Expand Up @@ -791,7 +790,7 @@ impl BlockFlow {
layout_context: &'a LayoutContext<'a>,
mut fragmentation_context: Option<FragmentationContext>,
margins_may_collapse: MarginsMayCollapseFlag)
-> Option<FlowRef> {
-> Option<Arc<Flow>> {
let _scope = layout_debug_scope!("assign_block_size_block_base {:x}",
self.base.debug_id());

Expand Down Expand Up @@ -1101,9 +1100,9 @@ impl BlockFlow {
} else {
let mut children = self.base.children.split_off(i);
if let Some(child) = child_remaining {
children.push_front(child);
children.push_front_arc(child);
}
Some(Arc::new(self.clone_with_children(children)) as FlowRef)
Some(Arc::new(self.clone_with_children(children)) as Arc<Flow>)
}
})
}
Expand Down Expand Up @@ -1898,7 +1897,7 @@ impl Flow for BlockFlow {

fn fragment(&mut self, layout_context: &LayoutContext,
fragmentation_context: Option<FragmentationContext>)
-> Option<FlowRef> {
-> Option<Arc<Flow>> {
if self.is_replaced_content() {
let _scope = layout_debug_scope!("assign_replaced_block_size_if_necessary {:x}",
self.base.debug_id());
Expand Down
68 changes: 34 additions & 34 deletions components/layout/construct.rs
View file
Expand Up @@ -22,7 +22,7 @@ use floats::FloatKind;
use flow::{self, AbsoluteDescendants, Flow, FlowClass, ImmutableFlowUtils};
use flow::{CAN_BE_FRAGMENTED, IS_ABSOLUTELY_POSITIONED, MARGINS_CANNOT_COLLAPSE};
use flow::{MutableFlowUtils, MutableOwnedFlowUtils};
use flow_ref::{self, FlowRef};
use flow_ref::FlowRef;
use fragment::{CanvasFragmentInfo, ImageFragmentInfo, InlineAbsoluteFragmentInfo, SvgFragmentInfo};
use fragment::{Fragment, GeneratedContentInfo, IframeFragmentInfo};
use fragment::{InlineAbsoluteHypotheticalFragmentInfo, TableColumnFragmentInfo};
Expand Down Expand Up @@ -397,9 +397,9 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
let scanned_fragments =
TextRunScanner::new().scan_for_runs(&mut self.layout_context.font_context(),
fragments.fragments);
let mut inline_flow_ref: FlowRef =
Arc::new(InlineFlow::from_fragments(scanned_fragments,
node.style(self.style_context()).writing_mode));
let mut inline_flow_ref =
FlowRef::new(Arc::new(InlineFlow::from_fragments(scanned_fragments,
node.style(self.style_context()).writing_mode)));

// Add all the inline-block fragments as children of the inline flow.
for inline_block_flow in &inline_block_flows {
Expand All @@ -423,7 +423,7 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>

{
// FIXME(#6503): Use Arc::get_mut().unwrap() here.
let inline_flow = flow_ref::deref_mut(&mut inline_flow_ref).as_mut_inline();
let inline_flow = FlowRef::deref_mut(&mut inline_flow_ref).as_mut_inline();
inline_flow.minimum_line_metrics =
inline_flow.minimum_line_metrics(&mut self.layout_context.font_context(),
&node.style(self.style_context()))
Expand Down Expand Up @@ -697,8 +697,8 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
}

let fragment = self.build_fragment_for_block(node);
let flow: FlowRef =
Arc::new(BlockFlow::from_fragment_and_float_kind(fragment, float_kind));
let flow =
FlowRef::new(Arc::new(BlockFlow::from_fragment_and_float_kind(fragment, float_kind)));
self.build_flow_for_block_like(flow, node)
}

Expand Down Expand Up @@ -1023,10 +1023,10 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
float_kind: Option<FloatKind>)
-> ConstructionResult {
let fragment = Fragment::new(node, SpecificFragmentInfo::Multicol, self.layout_context);
let mut flow: FlowRef = Arc::new(MulticolFlow::from_fragment(fragment, float_kind));
let mut flow = FlowRef::new(Arc::new(MulticolFlow::from_fragment(fragment, float_kind)));

let column_fragment = Fragment::new(node, SpecificFragmentInfo::MulticolColumn, self.layout_context);
let column_flow = Arc::new(MulticolColumnFlow::from_fragment(column_fragment));
let column_flow = FlowRef::new(Arc::new(MulticolColumnFlow::from_fragment(column_fragment)));

// First populate the column flow with its children.
let construction_result = self.build_flow_for_block_like(column_flow, node);
Expand Down Expand Up @@ -1078,12 +1078,12 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
node.restyle_damage(),
SpecificFragmentInfo::TableWrapper);
let wrapper_float_kind = FloatKind::from_property(float_value);
let mut wrapper_flow: FlowRef =
Arc::new(TableWrapperFlow::from_fragment_and_float_kind(wrapper_fragment,
wrapper_float_kind));
let mut wrapper_flow =
FlowRef::new(Arc::new(TableWrapperFlow::from_fragment_and_float_kind(wrapper_fragment,
wrapper_float_kind)));

let table_fragment = Fragment::new(node, SpecificFragmentInfo::Table, self.layout_context);
let table_flow = Arc::new(TableFlow::from_fragment(table_fragment));
let table_flow = FlowRef::new(Arc::new(TableFlow::from_fragment(table_fragment)));

// First populate the table flow with its children.
let construction_result = self.build_flow_for_block_like(table_flow, node);
Expand Down Expand Up @@ -1134,7 +1134,7 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
/// with possibly other `BlockFlow`s or `InlineFlow`s underneath it.
fn build_flow_for_table_caption(&mut self, node: &ConcreteThreadSafeLayoutNode) -> ConstructionResult {
let fragment = self.build_fragment_for_block(node);
let flow = Arc::new(TableCaptionFlow::from_fragment(fragment));
let flow = FlowRef::new(Arc::new(TableCaptionFlow::from_fragment(fragment)));
self.build_flow_for_block_like(flow, node)
}

Expand All @@ -1143,15 +1143,15 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
fn build_flow_for_table_rowgroup(&mut self, node: &ConcreteThreadSafeLayoutNode)
-> ConstructionResult {
let fragment = Fragment::new(node, SpecificFragmentInfo::TableRow, self.layout_context);
let flow = Arc::new(TableRowGroupFlow::from_fragment(fragment));
let flow = FlowRef::new(Arc::new(TableRowGroupFlow::from_fragment(fragment)));
self.build_flow_for_block_like(flow, node)
}

/// Builds a flow for a node with `display: table-row`. This yields a `TableRowFlow` with
/// possibly other `TableCellFlow`s underneath it.
fn build_flow_for_table_row(&mut self, node: &ConcreteThreadSafeLayoutNode) -> ConstructionResult {
let fragment = Fragment::new(node, SpecificFragmentInfo::TableRow, self.layout_context);
let flow = Arc::new(TableRowFlow::from_fragment(fragment));
let flow = FlowRef::new(Arc::new(TableRowFlow::from_fragment(fragment)));
self.build_flow_for_block_like(flow, node)
}

Expand All @@ -1171,8 +1171,8 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
position == position::T::fixed
});

let flow = Arc::new(
TableCellFlow::from_node_fragment_and_visibility_flag(node, fragment, !hide));
let flow = FlowRef::new(Arc::new(
TableCellFlow::from_node_fragment_and_visibility_flag(node, fragment, !hide)));
self.build_flow_for_block_like(flow, node)
}

Expand Down Expand Up @@ -1236,7 +1236,7 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
}
};

self.build_flow_for_block_starting_with_fragments(flow, node, initial_fragments)
self.build_flow_for_block_starting_with_fragments(FlowRef::new(flow), node, initial_fragments)
}

/// Creates a fragment for a node with `display: table-column`.
Expand Down Expand Up @@ -1276,7 +1276,7 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
let specific = SpecificFragmentInfo::TableColumn(TableColumnFragmentInfo::new(node));
col_fragments.push(Fragment::new(node, specific, self.layout_context));
}
let mut flow: FlowRef = Arc::new(TableColGroupFlow::from_fragments(fragment, col_fragments));
let mut flow = FlowRef::new(Arc::new(TableColGroupFlow::from_fragments(fragment, col_fragments)));
flow.finish();

ConstructionResult::Flow(flow, AbsoluteDescendants::new())
Expand All @@ -1288,7 +1288,7 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
float_kind: Option<FloatKind>)
-> ConstructionResult {
let fragment = self.build_fragment_for_block(node);
let flow = Arc::new(FlexFlow::from_fragment(fragment, float_kind));
let flow = FlowRef::new(Arc::new(FlexFlow::from_fragment(fragment, float_kind)));
self.build_flow_for_block_like(flow, node)
}

Expand Down Expand Up @@ -1345,7 +1345,7 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
return false
}

let flow = flow_ref::deref_mut(flow);
let flow = FlowRef::deref_mut(flow);
flow::mut_base(flow).restyle_damage.insert(damage);
flow.repair_style_and_bubble_inline_sizes(&style);
true
Expand All @@ -1370,21 +1370,21 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>

match fragment.specific {
SpecificFragmentInfo::InlineBlock(ref mut inline_block_fragment) => {
let flow_ref = flow_ref::deref_mut(&mut inline_block_fragment.flow_ref);
let flow_ref = FlowRef::deref_mut(&mut inline_block_fragment.flow_ref);
flow::mut_base(flow_ref).restyle_damage.insert(damage);
// FIXME(pcwalton): Fragment restyle damage too?
flow_ref.repair_style_and_bubble_inline_sizes(&style);
}
SpecificFragmentInfo::InlineAbsoluteHypothetical(
ref mut inline_absolute_hypothetical_fragment) => {
let flow_ref = flow_ref::deref_mut(
let flow_ref = FlowRef::deref_mut(
&mut inline_absolute_hypothetical_fragment.flow_ref);
flow::mut_base(flow_ref).restyle_damage.insert(damage);
// FIXME(pcwalton): Fragment restyle damage too?
flow_ref.repair_style_and_bubble_inline_sizes(&style);
}
SpecificFragmentInfo::InlineAbsolute(ref mut inline_absolute_fragment) => {
let flow_ref = flow_ref::deref_mut(
let flow_ref = FlowRef::deref_mut(
&mut inline_absolute_fragment.flow_ref);
flow::mut_base(flow_ref).restyle_damage.insert(damage);
// FIXME(pcwalton): Fragment restyle damage too?
Expand Down Expand Up @@ -1631,7 +1631,7 @@ impl<ConcreteThreadSafeLayoutNode> NodeUtils for ConcreteThreadSafeLayoutNode
fn set_flow_construction_result(self, mut result: ConstructionResult) {
if self.can_be_fragmented() {
if let ConstructionResult::Flow(ref mut flow, _) = result {
flow::mut_base(flow_ref::deref_mut(flow)).flags.insert(CAN_BE_FRAGMENTED);
flow::mut_base(FlowRef::deref_mut(flow)).flags.insert(CAN_BE_FRAGMENTED);
}
}

Expand Down Expand Up @@ -1703,11 +1703,11 @@ impl FlowConstructionUtils for FlowRef {
/// Adds a new flow as a child of this flow. Fails if this flow is marked as a leaf.
fn add_new_child(&mut self, mut new_child: FlowRef) {
{
let kid_base = flow::mut_base(flow_ref::deref_mut(&mut new_child));
let kid_base = flow::mut_base(FlowRef::deref_mut(&mut new_child));
kid_base.parallel.parent = parallel::mut_owned_flow_to_unsafe_flow(self);
}

let base = flow::mut_base(flow_ref::deref_mut(self));
let base = flow::mut_base(FlowRef::deref_mut(self));
base.children.push_back(new_child);
let _ = base.parallel.children_count.fetch_add(1, Ordering::Relaxed);
}
Expand All @@ -1720,8 +1720,8 @@ impl FlowConstructionUtils for FlowRef {
/// properly computed. (This is not, however, a memory safety problem.)
fn finish(&mut self) {
if !opts::get().bubble_inline_sizes_separately {
flow_ref::deref_mut(self).bubble_inline_sizes();
flow::mut_base(flow_ref::deref_mut(self)).restyle_damage.remove(BUBBLE_ISIZES);
FlowRef::deref_mut(self).bubble_inline_sizes();
flow::mut_base(FlowRef::deref_mut(self)).restyle_damage.remove(BUBBLE_ISIZES);
}
}
}
Expand Down Expand Up @@ -1921,14 +1921,14 @@ impl Legalizer {
}

(FlowClass::Flex, FlowClass::Inline) => {
flow::mut_base(flow_ref::deref_mut(child)).flags.insert(MARGINS_CANNOT_COLLAPSE);
flow::mut_base(FlowRef::deref_mut(child)).flags.insert(MARGINS_CANNOT_COLLAPSE);
let mut block_wrapper =
Legalizer::create_anonymous_flow(stylist,
parent,
&[PseudoElement::ServoAnonymousBlock],
SpecificFragmentInfo::Generic,
BlockFlow::from_fragment);
flow::mut_base(flow_ref::deref_mut(&mut
flow::mut_base(FlowRef::deref_mut(&mut
block_wrapper)).flags
.insert(MARGINS_CANNOT_COLLAPSE);
block_wrapper.add_new_child((*child).clone());
Expand All @@ -1938,7 +1938,7 @@ impl Legalizer {
}

(FlowClass::Flex, _) => {
flow::mut_base(flow_ref::deref_mut(child)).flags.insert(MARGINS_CANNOT_COLLAPSE);
flow::mut_base(FlowRef::deref_mut(child)).flags.insert(MARGINS_CANNOT_COLLAPSE);
parent.add_new_child((*child).clone());
true
}
Expand Down Expand Up @@ -2030,7 +2030,7 @@ impl Legalizer {
let fragment = reference_block.fragment
.create_similar_anonymous_fragment(new_style,
specific_fragment_info);
Arc::new(constructor(fragment))
FlowRef::new(Arc::new(constructor(fragment)))
}
}

6 changes: 3 additions & 3 deletions components/layout/display_list_builder.rs
View file
Expand Up @@ -18,7 +18,7 @@ use context::SharedLayoutContext;
use euclid::{Matrix4D, Point2D, Radians, Rect, SideOffsets2D, Size2D};
use flex::FlexFlow;
use flow::{BaseFlow, Flow, IS_ABSOLUTELY_POSITIONED};
use flow_ref;
use flow_ref::FlowRef;
use fragment::{CoordinateSystem, Fragment, ImageFragmentInfo, ScannedTextFragmentInfo};
use fragment::SpecificFragmentInfo;
use gfx::display_list::{BLUR_INFLATION_FACTOR, BaseDisplayItem, BorderDisplayItem};
Expand Down Expand Up @@ -1994,11 +1994,11 @@ impl InlineFlowDisplayListBuilding for InlineFlow {
for mut fragment in self.fragments.fragments.iter_mut() {
match fragment.specific {
SpecificFragmentInfo::InlineBlock(ref mut block_flow) => {
let block_flow = flow_ref::deref_mut(&mut block_flow.flow_ref);
let block_flow = FlowRef::deref_mut(&mut block_flow.flow_ref);
block_flow.collect_stacking_contexts(parent, parent_scroll_root_id);
}
SpecificFragmentInfo::InlineAbsoluteHypothetical(ref mut block_flow) => {
let block_flow = flow_ref::deref_mut(&mut block_flow.flow_ref);
let block_flow = FlowRef::deref_mut(&mut block_flow.flow_ref);
block_flow.collect_stacking_contexts(parent, parent_scroll_root_id);
}
_ if fragment.establishes_stacking_context() => {
Expand Down
14 changes: 7 additions & 7 deletions components/layout/flow.rs
View file
Expand Up @@ -32,7 +32,7 @@ use display_list_builder::DisplayListBuildState;
use euclid::{Point2D, Size2D};
use floats::{Floats, SpeculatedFloatPlacement};
use flow_list::{FlowList, MutFlowListIterator};
use flow_ref::{self, FlowRef, WeakFlowRef};
use flow_ref::{FlowRef, WeakFlowRef};
use fragment::{Fragment, FragmentBorderBoxIterator, Overflow};
use gfx::display_list::{ClippingRegion, StackingContext};
use gfx_traits::{ScrollRootId, StackingContextId};
Expand Down Expand Up @@ -212,7 +212,7 @@ pub trait Flow: fmt::Debug + Sync + Send + 'static {
fn fragment(&mut self,
layout_context: &LayoutContext,
_fragmentation_context: Option<FragmentationContext>)
-> Option<FlowRef> {
-> Option<Arc<Flow>> {
fn recursive_assign_block_size<F: ?Sized + Flow>(flow: &mut F, ctx: &LayoutContext) {
for child in mut_base(flow).children.iter_mut() {
recursive_assign_block_size(child, ctx)
Expand Down Expand Up @@ -791,7 +791,7 @@ pub struct AbsoluteDescendantIter<'a> {
impl<'a> Iterator for AbsoluteDescendantIter<'a> {
type Item = &'a mut Flow;
fn next(&mut self) -> Option<&'a mut Flow> {
self.iter.next().map(|info| flow_ref::deref_mut(&mut info.flow))
self.iter.next().map(|info| FlowRef::deref_mut(&mut info.flow))
}
}

Expand Down Expand Up @@ -1403,11 +1403,11 @@ impl MutableOwnedFlowUtils for FlowRef {
/// construction is allowed to possess.
fn set_absolute_descendants(&mut self, abs_descendants: AbsoluteDescendants) {
let this = self.clone();
let base = mut_base(flow_ref::deref_mut(self));
let base = mut_base(FlowRef::deref_mut(self));
base.abs_descendants = abs_descendants;
for descendant_link in base.abs_descendants.descendant_links.iter_mut() {
debug_assert!(!descendant_link.has_reached_containing_block);
let descendant_base = mut_base(flow_ref::deref_mut(&mut descendant_link.flow));
let descendant_base = mut_base(FlowRef::deref_mut(&mut descendant_link.flow));
descendant_base.absolute_cb.set(this.clone());
}
}
Expand All @@ -1433,7 +1433,7 @@ impl MutableOwnedFlowUtils for FlowRef {
});

let this = self.clone();
let base = mut_base(flow_ref::deref_mut(self));
let base = mut_base(FlowRef::deref_mut(self));
base.abs_descendants = applicable_absolute_descendants;
for descendant_link in base.abs_descendants.iter() {
let descendant_base = mut_base(descendant_link);
Expand Down Expand Up @@ -1464,7 +1464,7 @@ impl ContainingBlockLink {
}

fn set(&mut self, link: FlowRef) {
self.link = Some(Arc::downgrade(&link))
self.link = Some(FlowRef::downgrade(&link))
}

#[inline]
Expand Down

0 comments on commit 5f320bd

Please sign in to comment.