Make all DOM manipulation wait until it's safe to do so (ie. all reflows for the page have completed). Fix a race where a newly-initiated reflow would be considered complete when receiving the completion notice for the previous reflow.

src/components/main/layout/layout_task.rs

@@ -360,7 +360,7 @@ impl LayoutTask {
         // FIXME(pcwalton): This should probably be *one* channel, but we can't fix this without
         // either select or a filtered recv() that only looks for messages of a given type.
         data.script_join_chan.send(());
-        data.script_chan.send(ReflowCompleteMsg(self.id));
+        data.script_chan.send(ReflowCompleteMsg(self.id, data.id));
     }
 
     /// Handles a query from the script task. This is the main routine that DOM functions like

src/components/script/dom/bindings/codegen/CodegenRust.py

@@ -4020,7 +4020,8 @@ def finalizeHook(descriptor, hookName, context):
         assert descriptor.nativeIsISupports
         release = """let val = JS_GetReservedSlot(obj, 0);
 let _: @mut %s = cast::transmute(RUST_JSVAL_TO_PRIVATE(val));
-""" % descriptor.concreteType
+debug!("%s finalize: %%p", this);
+""" % (descriptor.concreteType, descriptor.concreteType)
     #return clearWrapper + release
     return release
 

src/components/script/dom/document.rs

@@ -458,6 +458,12 @@ impl Document {
             window.content_changed()
         }
     }
+
+    pub fn wait_until_safe_to_modify_dom(&self) {
+        for window in self.window.iter() {
+            window.wait_until_safe_to_modify_dom();
+        }
+    }
 }
 
 impl Traceable for Document {

src/components/script/dom/node.rs

@@ -625,6 +625,8 @@ impl Node<ScriptView> {
             self.replace_all(abstract_self, node);
           }
           CommentNodeTypeId | TextNodeTypeId => {
+            self.wait_until_safe_to_modify_dom();
+
             do abstract_self.with_mut_characterdata() |characterdata| {
                 characterdata.data = value.to_str();
 
@@ -644,6 +646,14 @@ impl Node<ScriptView> {
         fail!("stub")
     }
 
+    fn wait_until_safe_to_modify_dom(&self) {
+        for doc in self.owner_doc.iter() {
+            do doc.with_base |doc| {
+                doc.wait_until_safe_to_modify_dom();
+            }
+        }
+    }
+
     pub fn AppendChild(&mut self,
                        abstract_self: AbstractNode<ScriptView>,
                        node: AbstractNode<ScriptView>,
@@ -676,6 +686,8 @@ impl Node<ScriptView> {
         // TODO: Should we handle WRONG_DOCUMENT_ERR here?
 
         if rv.is_ok() {
+            self.wait_until_safe_to_modify_dom();
+
             // If the node already exists it is removed from current parent node.
             node.parent_node().map(|parent| parent.remove_child(node));
             abstract_self.add_child(node);
@@ -709,6 +721,8 @@ impl Node<ScriptView> {
             *rv = Err(NotFound);
         }
         if rv.is_ok() {
+            self.wait_until_safe_to_modify_dom();
+
             abstract_self.remove_child(node);
             self.remove_from_doc();
         }

src/components/script/dom/window.rs

@@ -173,6 +173,12 @@ impl Window {
         self.page.reflow_all(ReflowForDisplay, self.script_chan.clone(), self.compositor);
     }
 
+    pub fn wait_until_safe_to_modify_dom(&self) {
+        // FIXME: This disables concurrent layout while we are modifying the DOM, since
+        //        our current architecture is entirely unsafe in the presence of races.
+        self.page.join_layout();
+    }
+
     #[fixed_stack_segment]
     pub fn new(cx: *JSContext,
                page: @mut Page,

src/components/script/layout_interface.rs

@@ -105,6 +105,8 @@ pub struct Reflow {
     window_size: Size2D<uint>,
     /// The channel that we send a notification to.
     script_join_chan: Chan<()>,
+    /// Unique identifier
+    id: uint
 }
 
 /// Encapsulates a channel to the layout task.

src/components/script/script_task.rs

@@ -68,7 +68,7 @@ pub enum ScriptMsg {
     /// Fires a JavaScript timeout.
     FireTimerMsg(PipelineId, ~TimerData),
     /// Notifies script that reflow is finished.
-    ReflowCompleteMsg(PipelineId),
+    ReflowCompleteMsg(PipelineId, uint),
     /// Notifies script that window has been resized but to not take immediate action.
     ResizeInactiveMsg(PipelineId, Size2D<uint>),
     /// Exits the constellation.
@@ -106,6 +106,9 @@ pub struct Page {
     /// Pipeline id associated with this page.
     id: PipelineId,
 
+    /// Unique id for last reflow request; used for confirming completion reply.
+    last_reflow_id: uint,
+
     /// The outermost frame containing the document, window, and page URL.
     frame: Option<Frame>,
 
@@ -158,6 +161,7 @@ impl PageTree {
                 url: None,
                 next_subpage_id: SubpageId(0),
                 resize_event: None,
+                last_reflow_id: 0
             },
             inner: ~[],
         }
@@ -216,7 +220,7 @@ impl Page {
 
     /// Sends a ping to layout and waits for the response. The response will arrive when the
     /// layout task has finished any pending request messages.
-    fn join_layout(&mut self) {
+    pub fn join_layout(&mut self) {
         if self.layout_join_port.is_some() {
             let join_port = replace(&mut self.layout_join_port, None);
             match join_port {
@@ -263,6 +267,8 @@ impl Page {
         let (join_port, join_chan) = comm::stream();
         self.layout_join_port = Some(join_port);
 
+        self.last_reflow_id += 1;
+
         match self.frame {
             None => fail!(~"Tried to relayout with no root frame!"),
             Some(ref frame) => {
@@ -275,6 +281,7 @@ impl Page {
                     script_chan: script_chan,
                     script_join_chan: join_chan,
                     damage: replace(&mut self.damage, None).unwrap(),
+                    id: self.last_reflow_id,
                 };
 
                 self.layout_chan.send(ReflowMsg(reflow))
@@ -498,7 +505,7 @@ impl ScriptTask {
                 SendEventMsg(id, event) => self.handle_event(id, event),
                 FireTimerMsg(id, timer_data) => self.handle_fire_timer_msg(id, timer_data),
                 NavigateMsg(direction) => self.handle_navigate_msg(direction),
-                ReflowCompleteMsg(id) => self.handle_reflow_complete_msg(id),
+                ReflowCompleteMsg(id, reflow_id) => self.handle_reflow_complete_msg(id, reflow_id),
                 ResizeInactiveMsg(id, new_size) => self.handle_resize_inactive_msg(id, new_size),
                 ExitMsg => {
                     self.handle_exit_msg();
@@ -576,11 +583,14 @@ impl ScriptTask {
     }
 
     /// Handles a notification that reflow completed.
-    fn handle_reflow_complete_msg(&mut self, pipeline_id: PipelineId) {
+    fn handle_reflow_complete_msg(&mut self, pipeline_id: PipelineId, reflow_id: uint) {
-        debug!("Script: Reflow complete for %?", pipeline_id);
+        debug!("Script: Reflow %? complete for %?", reflow_id, pipeline_id);
-        self.page_tree.find(pipeline_id).expect("ScriptTask: received a load
+        let page_tree = self.page_tree.find(pipeline_id).expect(
-            message for a layout channel that is not associated with this script task. This
+            "ScriptTask: received a load message for a layout channel that is not associated \
-            is a bug.").page.layout_join_port = None;
+             with this script task. This is a bug.");
+        if page_tree.page.last_reflow_id == reflow_id {
+            page_tree.page.layout_join_port = None;
+        }
         self.constellation_chan.send(RendererReadyMsg(pipeline_id));
         self.compositor.set_ready_state(FinishedLoading);
     }