Various fixes for cleaning up iframes, compositor layers, pipelines and threads.

This allows most of the jquery test suite to run without exhausting thread resources.

components/compositing/compositor.rs

@@ -431,9 +431,7 @@ impl<Window: WindowMethods> IOCompositor<Window> {
             }
 
             (Msg::PaintTaskExited(pipeline_id), ShutdownState::NotShuttingDown) => {
-                if self.pipeline_details.remove(&pipeline_id).is_none() {
+                self.remove_pipeline_root_layer(pipeline_id);
-                    panic!("Saw PaintTaskExited message from an unknown pipeline!");
-                }
             }
 
             (Msg::ViewportConstrained(pipeline_id, constraints), ShutdownState::NotShuttingDown) => {
@@ -591,6 +589,16 @@ impl<Window: WindowMethods> IOCompositor<Window> {
         self.find_layer_with_pipeline_and_layer_id(pipeline_id, LayerId::null())
     }
 
+    fn remove_pipeline_root_layer(&mut self, pipeline_id: PipelineId) {
+        if let Some(ref root_layer) = self.scene.root {
+            // Remove all the compositor layers for this pipeline
+            // and send any owned buffers back to the paint task.
+            root_layer.remove_root_layer_with_pipeline_id(self, pipeline_id);
+
+            self.pipeline_details.remove(&pipeline_id);
+        }
+    }
+
     fn update_layer_if_exists(&mut self, pipeline_id: PipelineId, properties: LayerProperties) -> bool {
         match self.find_layer_with_pipeline_and_layer_id(pipeline_id, properties.id) {
             Some(existing_layer) => {

components/compositing/compositor_layer.rs

@@ -89,6 +89,14 @@ pub trait CompositorLayer {
     /// painter to be destroyed or reused.
     fn clear_all_tiles<Window>(&self, compositor: &IOCompositor<Window>) where Window: WindowMethods;
 
+    /// Removes the root layer (and any children) for a given pipeline from the
+    /// compositor. Buffers that the compositor is holding are returned to the
+    /// owning paint task.
+    fn remove_root_layer_with_pipeline_id<Window>(&self,
+                                                  compositor: &IOCompositor<Window>,
+                                                  pipeline_id: PipelineId)
+                                                  where Window: WindowMethods;
+
     /// Destroys all tiles of all layers, including children, *without* sending them back to the
     /// painter. You must call this only when the paint task is destined to be going down;
     /// otherwise, you will leak tiles.
@@ -248,6 +256,31 @@ impl CompositorLayer for Layer<CompositorData> {
         }
     }
 
+    fn remove_root_layer_with_pipeline_id<Window>(&self,
+                                                  compositor: &IOCompositor<Window>,
+                                                  pipeline_id: PipelineId)
+                                                  where Window: WindowMethods {
+        // Find the child that is the root layer for this pipeline.
+        let index = self.children().iter().position(|kid| {
+            let extra_data = kid.extra_data.borrow();
+            extra_data.pipeline_id == pipeline_id && extra_data.id == LayerId::null()
+        });
+
+        match index {
+            Some(index) => {
+                // Remove the root layer, and return buffers to the paint task
+                let child = self.children().remove(index);
+                child.clear_all_tiles(compositor);
+            }
+            None => {
+                // Wasn't found, recurse into child layers
+                for kid in self.children().iter() {
+                    kid.remove_root_layer_with_pipeline_id(compositor, pipeline_id);
+                }
+            }
+        }
+    }
+
     /// Destroys all tiles of all layers, including children, *without* sending them back to the
     /// painter. You must call this only when the paint task is destined to be going down;
     /// otherwise, you will leak tiles.

components/compositing/compositor_task.rs

@@ -114,6 +114,10 @@ impl PaintListener for Box<CompositorProxy+'static+Send> {
         // `position: fixed` but will not be sufficient to handle `overflow: scroll` or transforms.
         self.send(Msg::InitializeLayersForPipeline(pipeline_id, epoch, properties));
     }
+
+    fn notify_paint_task_exiting(&mut self, pipeline_id: PipelineId) {
+        self.send(Msg::PaintTaskExited(pipeline_id))
+    }
 }
 
 /// Messages from the painting task and the constellation task to the compositor task.

components/compositing/constellation.rs

@@ -443,6 +443,10 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
                 let is_ready = self.handle_is_ready_to_save_image(pipeline_states);
                 self.compositor_proxy.send(CompositorMsg::IsReadyToSaveImageReply(is_ready));
             }
+            ConstellationMsg::RemoveIFrame(containing_pipeline_id, subpage_id) => {
+                debug!("constellation got remove iframe message");
+                self.handle_remove_iframe_msg(containing_pipeline_id, subpage_id);
+            }
         }
         true
     }
@@ -511,7 +515,14 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
                              rect: TypedRect<PagePx, f32>) {
         // Store the new rect inside the pipeline
         let (pipeline_id, script_chan) = {
-            let pipeline = self.find_subpage(containing_pipeline_id, subpage_id);
+            // Find the pipeline that corresponds to this rectangle. It's possible that this
+            // pipeline may have already exited before we process this message, so just
+            // early exit if that occurs.
+            let pipeline_id = self.subpage_map.get(&(containing_pipeline_id, subpage_id)).map(|id| *id);
+            let pipeline = match pipeline_id {
+                Some(pipeline_id) => self.mut_pipeline(pipeline_id),
+                None => return,
+            };
             pipeline.rect = Some(rect);
             (pipeline.id, pipeline.script_chan.clone())
         };
@@ -786,6 +797,23 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
         self.focus_parent_pipeline(pipeline_id);
     }
 
+    fn handle_remove_iframe_msg(&mut self, containing_pipeline_id: PipelineId, subpage_id: SubpageId) {
+        let pipeline_id = self.find_subpage(containing_pipeline_id, subpage_id).id;
+        let frame_id = self.pipeline_to_frame_map.get(&pipeline_id).map(|id| *id);
+        match frame_id {
+            Some(frame_id) => {
+                // This iframe has already loaded and been added to the frame tree.
+                self.close_frame(frame_id, ExitPipelineMode::Normal);
+            }
+            None => {
+                // This iframe is currently loading / painting for the first time.
+                // In this case, it doesn't exist in the frame tree, but the pipeline
+                // still needs to be shut down.
+                self.close_pipeline(pipeline_id, ExitPipelineMode::Normal);
+            }
+        }
+    }
+
     fn handle_webdriver_command_msg(&mut self,
                                     pipeline_id: PipelineId,
                                     msg: WebDriverScriptCommand) {
@@ -1026,24 +1054,56 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
 
     // Close a frame (and all children)
     fn close_frame(&mut self, frame_id: FrameId, exit_mode: ExitPipelineMode) {
-        let frame = self.frames.remove(&frame_id).unwrap();
+        // Store information about the pipelines to be closed. Then close the
+        // pipelines, before removing ourself from the frames hash map. This
+        // ordering is vital - so that if close_pipeline() ends up closing
+        // any child frames, they can be removed from the parent frame correctly.
+        let parent_info = self.pipeline(self.frame(frame_id).current).parent_info;
+        let pipelines_to_close = {
+            let mut pipelines_to_close = vec!();
 
-        self.close_pipeline(frame.current, exit_mode);
+            let frame = self.frame(frame_id);
+            pipelines_to_close.push_all(&frame.next);
+            pipelines_to_close.push(frame.current);
+            pipelines_to_close.push_all(&frame.prev);
 
-        for pipeline_id in frame.prev.iter().chain(frame.next.iter()) {
+            pipelines_to_close
+        };
+
+        for pipeline_id in &pipelines_to_close {
             self.close_pipeline(*pipeline_id, exit_mode);
         }
+
+        self.frames.remove(&frame_id).unwrap();
+
+        if let Some((parent_pipeline_id, _)) = parent_info {
+            let parent_pipeline = self.mut_pipeline(parent_pipeline_id);
+            parent_pipeline.remove_child(frame_id);
+        }
     }
 
     // Close all pipelines at and beneath a given frame
     fn close_pipeline(&mut self, pipeline_id: PipelineId, exit_mode: ExitPipelineMode) {
-        let pipeline = self.pipelines.remove(&pipeline_id).unwrap();
+        // Store information about the frames to be closed. Then close the
+        // frames, before removing ourself from the pipelines hash map. This
+        // ordering is vital - so that if close_frames() ends up closing
+        // any child pipelines, they can be removed from the parent pipeline correctly.
+        let frames_to_close = {
+            let mut frames_to_close = vec!();
+
+            let pipeline = self.pipeline(pipeline_id);
+            frames_to_close.push_all(&pipeline.children);
+
+            frames_to_close
+        };
 
         // Remove any child frames
-        for child in &pipeline.children {
+        for child_frame in &frames_to_close {
-            self.close_frame(*child, exit_mode);
+            self.close_frame(*child_frame, exit_mode);
         }
 
+        let pipeline = self.pipelines.remove(&pipeline_id).unwrap();
+
         // If a child pipeline, remove from subpage map
         if let Some(info) = pipeline.parent_info {
             self.subpage_map.remove(&info);
@@ -1052,12 +1112,19 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
         // Remove assocation between this pipeline and its holding frame
         self.pipeline_to_frame_map.remove(&pipeline_id);
 
+        // Remove this pipeline from pending frames if it hasn't loaded yet.
+        let pending_index = self.pending_frames.iter().position(|frame_change| {
+            frame_change.new_pipeline_id == pipeline_id
+        });
+        if let Some(pending_index) = pending_index {
+            self.pending_frames.remove(pending_index);
+        }
+
         // Inform script, compositor that this pipeline has exited.
         match exit_mode {
             ExitPipelineMode::Normal => pipeline.exit(PipelineExitType::PipelineOnly),
             ExitPipelineMode::Force => pipeline.force_exit(),
         }
-        self.compositor_proxy.send(CompositorMsg::PaintTaskExited(pipeline_id));
     }
 
     // Convert a frame to a sendable form to pass to the compositor

components/compositing/lib.rs

@@ -3,6 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #![feature(box_syntax)]
+#![feature(collections)]
 #![feature(core)]
 #![feature(rustc_private)]
 

components/compositing/pipeline.rs

@@ -253,6 +253,11 @@ impl Pipeline {
         self.children.push(frame_id);
     }
 
+    pub fn remove_child(&mut self, frame_id: FrameId) {
+        let index = self.children.iter().position(|id| *id == frame_id).unwrap();
+        self.children.remove(index);
+    }
+
     pub fn trigger_mozbrowser_event(&self,
                                      subpage_id: SubpageId,
                                      event: MozBrowserEvent) {

components/gfx/paint_task.rs

@@ -208,6 +208,11 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                         continue;
                     }
 
+                    // If waiting to exit, ignore any more paint commands
+                    if waiting_for_compositor_buffers_to_exit {
+                        continue;
+                    }
+
                     self.initialize_layers();
                 }
                 Msg::Paint(requests, frame_tree_id) => {
@@ -218,6 +223,11 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                         continue;
                     }
 
+                    // If waiting to exit, ignore any more paint commands
+                    if waiting_for_compositor_buffers_to_exit {
+                        continue;
+                    }
+
                     let mut replies = Vec::new();
                     for PaintRequest { buffer_requests, scale, layer_id, epoch }
                           in requests.into_iter() {
@@ -237,7 +247,7 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                     self.compositor.assign_painted_buffers(self.id, self.current_epoch.unwrap(), replies, frame_tree_id);
                 }
                 Msg::UnusedBuffer(unused_buffers) => {
-                    debug!("PaintTask: Received {} unused buffers", unused_buffers.len());
+                    debug!("PaintTask {:?}: Received {} unused buffers", self.id, unused_buffers.len());
                     self.used_buffer_count -= unused_buffers.len();
 
                     for buffer in unused_buffers.into_iter().rev() {
@@ -261,6 +271,13 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                     self.paint_permission = false;
                 }
                 Msg::Exit(response_channel, exit_type) => {
+                    // Ask the compositor to return any used buffers it
+                    // is holding for this paint task. This previously was
+                    // sent from the constellation. However, it needs to be sent
+                    // from here to avoid a race condition with the paint
+                    // messages above.
+                    self.compositor.notify_paint_task_exiting(self.id);
+
                     let should_wait_for_compositor_buffers = match exit_type {
                         PipelineExitType::Complete => false,
                         PipelineExitType::PipelineOnly => self.used_buffer_count != 0
@@ -275,7 +292,7 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                     // If we own buffers in the compositor and we are not exiting completely, wait
                     // for the compositor to return buffers, so that we can release them properly.
                     // When doing a complete exit, the compositor lets all buffers leak.
-                    println!("PaintTask: Saw ExitMsg, {} buffers in use", self.used_buffer_count);
+                    println!("PaintTask {:?}: Saw ExitMsg, {} buffers in use", self.id, self.used_buffer_count);
                     waiting_for_compositor_buffers_to_exit = true;
                     exit_response_channel = response_channel;
                 }

components/msg/compositor_msg.rs

@@ -93,6 +93,8 @@ pub trait PaintListener {
                               replies: Vec<(LayerId, Box<LayerBufferSet>)>,
                               frame_tree_id: FrameTreeId);
 
+    // Notification that the paint task wants to exit.
+    fn notify_paint_task_exiting(&mut self, pipeline_id: PipelineId);
 }
 
 /// The interface used by the script task to tell the compositor to update its ready state,

components/msg/constellation_msg.rs

@@ -244,6 +244,8 @@ pub enum Msg {
     CompositePng(Sender<Option<png::Image>>),
     /// Query the constellation to see if the current compositor output is stable
     IsReadyToSaveImage(HashMap<PipelineId, Epoch>),
+    /// Notification that this iframe should be removed.
+    RemoveIFrame(PipelineId, SubpageId),
 }
 
 #[derive(Clone, Eq, PartialEq)]

components/script/dom/htmliframeelement.rs

@@ -385,5 +385,32 @@ impl<'a> VirtualMethods for JSRef<'a, HTMLIFrameElement> {
             self.process_the_iframe_attributes();
         }
     }
+
+    fn unbind_from_tree(&self, tree_in_doc: bool) {
+        if let Some(ref s) = self.super_type() {
+            s.unbind_from_tree(tree_in_doc);
+        }
+
+        // https://html.spec.whatwg.org/multipage/#a-browsing-context-is-discarded
+        match (self.containing_page_pipeline_id(), self.subpage_id()) {
+            (Some(containing_pipeline_id), Some(subpage_id)) => {
+                let window = window_from_node(*self).root();
+                let window = window.r();
+
+                let ConstellationChan(ref chan) = window.constellation_chan();
+                let msg = ConstellationMsg::RemoveIFrame(containing_pipeline_id,
+                                                         subpage_id);
+                chan.send(msg).unwrap();
+
+                // Resetting the subpage id to None is required here so that
+                // if this iframe is subsequently re-added to the document
+                // the load doesn't think that it's a navigation, but instead
+                // a new iframe. Without this, the constellation gets very
+                // confused.
+                self.subpage_id.set(None);
+            }
+            _ => {}
+        }
+    }
 }
 

components/script/script_task.rs

@@ -1078,6 +1078,13 @@ impl ScriptTask {
     /// Handles a request to exit the script task and shut down layout.
     /// Returns true if the script task should shut down and false otherwise.
     fn handle_exit_pipeline_msg(&self, id: PipelineId, exit_type: PipelineExitType) -> bool {
+        if self.page.borrow().is_none() {
+            // The root page doesn't even exist yet, so it
+            // is safe to exit this script task.
+            // TODO(gw): This probably leaks resources!
+            return true;
+        }
+
         // If root is being exited, shut down all pages
         let page = self.root_page();
         let window = page.window().root();
@@ -1452,7 +1459,11 @@ impl ScriptTask {
             }, LoadConsumer::Channel(input_chan))).unwrap();
 
             let load_response = input_port.recv().unwrap();
-            script_chan.send(ScriptMsg::PageFetchComplete(id, subpage, load_response)).unwrap();
+            if script_chan.send(ScriptMsg::PageFetchComplete(id, subpage, load_response)).is_err() {
+                // TODO(gw): This should be handled by aborting
+                // the load before the script task exits.
+                debug!("PageFetchComplete: script channel has exited");
+            }
         });
 
         self.incomplete_loads.borrow_mut().push(incomplete);