diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index d5cebea2aaf..4ba546fbe92 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -431,9 +431,7 @@ impl IOCompositor { } (Msg::PaintTaskExited(pipeline_id), ShutdownState::NotShuttingDown) => { - if self.pipeline_details.remove(&pipeline_id).is_none() { - panic!("Saw PaintTaskExited message from an unknown pipeline!"); - } + self.remove_pipeline_root_layer(pipeline_id); } (Msg::ViewportConstrained(pipeline_id, constraints), ShutdownState::NotShuttingDown) => { @@ -591,6 +589,16 @@ impl IOCompositor { 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) => { diff --git a/components/compositing/compositor_layer.rs b/components/compositing/compositor_layer.rs index b13295e496c..36e78aef207 100644 --- a/components/compositing/compositor_layer.rs +++ b/components/compositing/compositor_layer.rs @@ -89,6 +89,14 @@ pub trait CompositorLayer { /// painter to be destroyed or reused. fn clear_all_tiles(&self, compositor: &IOCompositor) 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(&self, + compositor: &IOCompositor, + 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 { } } + fn remove_root_layer_with_pipeline_id(&self, + compositor: &IOCompositor, + 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. diff --git a/components/compositing/compositor_task.rs b/components/compositing/compositor_task.rs index 69cc33ad8de..793e1ecc9cf 100644 --- a/components/compositing/compositor_task.rs +++ b/components/compositing/compositor_task.rs @@ -114,6 +114,10 @@ impl PaintListener for Box { // `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. diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index 4ad7d699073..0c344f1c758 100644 --- a/components/compositing/constellation.rs +++ b/components/compositing/constellation.rs @@ -443,6 +443,10 @@ impl Constellation { 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 Constellation { rect: TypedRect) { // 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 Constellation { 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 Constellation { // 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 { - self.close_frame(*child, exit_mode); + for child_frame in &frames_to_close { + 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 Constellation { // 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 diff --git a/components/compositing/lib.rs b/components/compositing/lib.rs index de013523827..4bfdf0fe2a9 100644 --- a/components/compositing/lib.rs +++ b/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)] diff --git a/components/compositing/pipeline.rs b/components/compositing/pipeline.rs index 2402ed23f1f..6ed96704be4 100644 --- a/components/compositing/pipeline.rs +++ b/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) { diff --git a/components/gfx/paint_task.rs b/components/gfx/paint_task.rs index d02782473d2..8b11614971f 100644 --- a/components/gfx/paint_task.rs +++ b/components/gfx/paint_task.rs @@ -208,6 +208,11 @@ impl PaintTask 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 PaintTask 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 PaintTask 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 PaintTask 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 PaintTask 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; } diff --git a/components/msg/compositor_msg.rs b/components/msg/compositor_msg.rs index efb84a0402d..0ecd8f293ae 100644 --- a/components/msg/compositor_msg.rs +++ b/components/msg/compositor_msg.rs @@ -93,6 +93,8 @@ pub trait PaintListener { replies: Vec<(LayerId, Box)>, 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, diff --git a/components/msg/constellation_msg.rs b/components/msg/constellation_msg.rs index ed8de8c3a97..a147842db25 100644 --- a/components/msg/constellation_msg.rs +++ b/components/msg/constellation_msg.rs @@ -244,6 +244,8 @@ pub enum Msg { CompositePng(Sender>), /// Query the constellation to see if the current compositor output is stable IsReadyToSaveImage(HashMap), + /// Notification that this iframe should be removed. + RemoveIFrame(PipelineId, SubpageId), } #[derive(Clone, Eq, PartialEq)] diff --git a/components/script/dom/htmliframeelement.rs b/components/script/dom/htmliframeelement.rs index 58096b18bea..685f46db48e 100644 --- a/components/script/dom/htmliframeelement.rs +++ b/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); + } + _ => {} + } + } } diff --git a/components/script/script_task.rs b/components/script/script_task.rs index c88dbb8537a..c9d92497a04 100644 --- a/components/script/script_task.rs +++ b/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);