layout: Destroy flows properly under display: none.

Fixes a crash on http://en.wikipedia.org/wiki/South_China_Sea

It plants a destructor bomb on flows so that this can't happen again.

src/components/main/layout/construct.rs

@@ -26,7 +26,7 @@ use layout::box_::{Box, GenericBox, IframeBox, IframeBoxInfo, ImageBox, ImageBox
 use layout::box_::{UnscannedTextBox, UnscannedTextBoxInfo, InlineInfo, InlineParentInfo};
 use layout::context::LayoutContext;
 use layout::float_context::FloatType;
-use layout::flow::{BaseFlow, Flow, MutableOwnedFlowUtils};
+use layout::flow::{BaseFlow, Flow, LeafSet, MutableOwnedFlowUtils};
 use layout::inline::InlineFlow;
 use layout::text::TextRunScanner;
 use layout::util::LayoutDataAccess;
@@ -56,6 +56,16 @@ pub enum ConstructionResult {
     ConstructionItemConstructionResult(ConstructionItem),
 }
 
+impl ConstructionResult {
+    fn destroy(&mut self, leaf_set: &mut LeafSet) {
+        match *self {
+            NoConstructionResult => {}
+            FlowConstructionResult(ref mut flow) => flow.destroy(leaf_set),
+            ConstructionItemConstructionResult(ref mut item) => item.destroy(leaf_set),
+        }
+    }
+}
+
 /// Represents the output of flow construction for a DOM node that has not yet resulted in a
 /// complete flow. Construction items bubble up the tree until they find a `Flow` to be
 /// attached to.
@@ -64,6 +74,20 @@ enum ConstructionItem {
     InlineBoxesConstructionItem(InlineBoxesConstructionResult),
 }
 
+impl ConstructionItem {
+    fn destroy(&mut self, leaf_set: &mut LeafSet) {
+        match *self {
+            InlineBoxesConstructionItem(ref mut result) => {
+                for splits in result.splits.mut_iter() {
+                    for split in splits.mut_iter() {
+                        split.destroy(leaf_set)
+                    }
+                }
+            }
+        }
+    }
+}
+
 /// Represents inline boxes and {ib} splits that are bubbling up from an inline.
 struct InlineBoxesConstructionResult {
     /// Any {ib} splits that we're bubbling up.
@@ -107,6 +131,12 @@ struct InlineBlockSplit {
     flow: ~Flow,
 }
 
+impl InlineBlockSplit {
+    fn destroy(&mut self, leaf_set: &mut LeafSet) {
+        self.flow.destroy(leaf_set)
+    }
+}
+
 /// Methods on optional vectors.
 ///
 /// TODO(pcwalton): I think this will no longer be necessary once Rust #8981 lands.
@@ -539,9 +569,12 @@ impl<'a> PostorderNodeMutTraversal for FlowConstructor<'a> {
             // `display: none` contributes no flow construction result. Nuke the flow construction
             // results of children.
             (display::none, _, _) => {
+                self.layout_context.shared.leaf_set.access(|leaf_set| {
                     for child in node.children() {
-                    child.set_flow_construction_result(NoConstructionResult)
+                        let mut old_result = child.swap_out_construction_result();
+                        old_result.destroy(leaf_set)
                     }
+                })
             }
 
             // Inline items contribute inline box construction results.

src/components/main/layout/flow.rs

@@ -219,6 +219,9 @@ pub trait MutableOwnedFlowUtils {
     /// Adds a new flow as a child of this flow. Removes the flow from the given leaf set if
     /// it's present.
     fn add_new_child(&mut self, new_child: ~Flow, leaf_set: &mut LeafSet);
+
+    /// Destroys the flow.
+    fn destroy(&mut self, leaf_set: &mut LeafSet);
 }
 
 pub enum FlowClass {
@@ -470,9 +473,6 @@ impl FlowFlags {
 }
 
 /// Data common to all flows.
-///
-/// TODO(pcwalton): Plant a destructor bomb on this type. It is bad if it goes out of scope,
-/// because of the leaf list.
 pub struct BaseFlow {
     restyle_damage: RestyleDamage,
 
@@ -506,10 +506,24 @@ pub struct BaseFlow {
     num_floats: uint,
     abs_position: Point2D<Au>,
 
+    /// Whether this flow has been destroyed.
+    ///
+    /// TODO(pcwalton): Pack this into the flags? Need to be careful because manipulation of this
+    /// flag can have memory safety implications.
+    priv destroyed: bool,
+
     /// Various flags for flows and some info
     flags_info: FlowFlagsInfo,
 }
 
+impl Drop for BaseFlow {
+    fn drop(&mut self) {
+        if !self.destroyed {
+            fail!("Flow destroyed by going out of scope—this is unsafe! Use `destroy()` instead!")
+        }
+    }
+}
+
 pub struct BoxIterator {
     priv boxes: ~[@Box],
     priv index: uint,
@@ -550,6 +564,8 @@ impl BaseFlow {
             num_floats: 0,
             abs_position: Point2D(Au::new(0), Au::new(0)),
 
+            destroyed: false,
+
             flags_info: FlowFlagsInfo::new(style.get()),
         }
     }
@@ -722,6 +738,7 @@ impl<'a> MutableFlowUtils for &'a mut Flow {
         });
         true
     }
+
 }
 
 impl MutableOwnedFlowUtils for ~Flow {
@@ -741,6 +758,25 @@ impl MutableOwnedFlowUtils for ~Flow {
         base.children.push_back(new_child);
         let _ = base.parallel.children_count.fetch_add(1, Relaxed);
     }
+
+    /// Destroys the flow.
+    fn destroy(&mut self, leaf_set: &mut LeafSet) {
+        let is_leaf = {
+            let base = mut_base(*self);
+            base.children.len() == 0
+        };
+
+        if is_leaf {
+            leaf_set.remove(self);
+        } else {
+            for kid in child_iter(*self) {
+                kid.destroy(leaf_set)
+            }
+        }
+
+        let base = mut_base(*self);
+        base.destroyed = true
+    }
 }
 
 /// Keeps track of the leaves of the flow tree. This is used to efficiently start bottom-up
@@ -766,13 +802,18 @@ impl LeafSet {
     /// Removes a flow from the leaf set. Asserts that the flow was indeed in the leaf set. (This
     /// invariant is needed for memory safety, as there must always be exactly one leaf set.)
     fn remove(&mut self, flow: &~Flow) {
-        let flow = parallel::owned_flow_to_unsafe_flow(flow);
+        if !self.contains(flow) {
-        if !self.set.contains(&flow) {
             fail!("attempted to remove a flow from the leaf set that wasn't in the set!")
         }
+        let flow = parallel::owned_flow_to_unsafe_flow(flow);
         self.set.remove(&flow);
     }
 
+    pub fn contains(&mut self, flow: &~Flow) -> bool {
+        let flow = parallel::owned_flow_to_unsafe_flow(flow);
+        self.set.contains(&flow)
+    }
+
     pub fn clear(&mut self) {
         self.set.clear()
     }

src/components/main/layout/layout_task.rs

@@ -12,8 +12,8 @@ use layout::construct::{FlowConstructionResult, FlowConstructor, NoConstructionR
 use layout::context::{LayoutContext, SharedLayoutInfo};
 use layout::display_list_builder::{DisplayListBuilder, ToGfxColor};
 use layout::extra::LayoutAuxMethods;
-use layout::flow::{Flow, ImmutableFlowUtils, LeafSet, MutableFlowUtils, PreorderFlowTraversal};
+use layout::flow::{Flow, ImmutableFlowUtils, LeafSet, MutableFlowUtils, MutableOwnedFlowUtils};
-use layout::flow::{PostorderFlowTraversal};
+use layout::flow::{PreorderFlowTraversal, PostorderFlowTraversal};
 use layout::flow;
 use layout::incremental::RestyleDamage;
 use layout::parallel::{AssignHeightsAndStoreOverflowTraversalKind, BubbleWidthsTraversalKind};
@@ -590,10 +590,7 @@ impl LayoutTask {
             });
         }
 
-        // FIXME(pcwalton): Hack because we don't yet reference count flows. When we do reference
+        self.leaf_set.access(|leaf_set| layout_root.destroy(leaf_set));
-        // count them, then the destructor should remove the flow from the leaf set once the count
-        // hits zero.
-        self.leaf_set.access(|leaf_set| leaf_set.clear());
 
         // Tell script that we're done.
         //