From d77a07388f9acd1d45533d37544b09de98e4f36d Mon Sep 17 00:00:00 2001 From: Tim Kuehn Date: Fri, 20 Sep 2013 22:43:45 -0400 Subject: [PATCH 1/4] exit the correct evicted pipelines --- src/components/main/constellation.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/components/main/constellation.rs b/src/components/main/constellation.rs index 07b9601d641..bdb07af747a 100644 --- a/src/components/main/constellation.rs +++ b/src/components/main/constellation.rs @@ -770,12 +770,13 @@ impl Constellation { match navigation_type { constellation_msg::Load => { let evicted = self.navigation_context.load(frame_tree); + let mut exited = HashSet::new(); + // exit any pipelines that don't exist outside the evicted frame trees for frame_tree in evicted.iter() { - // exit any pipelines that don't exist outside the evicted frame trees - for frame in frame_tree.iter() { - if !self.navigation_context.contains(frame.pipeline.id) { - frame_tree.pipeline.exit(); - self.pipelines.remove(&frame_tree.pipeline.id); + for @FrameTree { pipeline, _ } in frame_tree.iter() { + if !self.navigation_context.contains(pipeline.id) { + pipeline.exit(); + self.pipelines.remove(&pipeline.id); } } } From c56b015623ee6f78c1603dfe789418add7618289 Mon Sep 17 00:00:00 2001 From: Tim Kuehn Date: Fri, 20 Sep 2013 23:56:53 -0400 Subject: [PATCH 2/4] find_mut ==> find. it's only mut because of the data type iterated over --- src/components/main/constellation.rs | 32 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/components/main/constellation.rs b/src/components/main/constellation.rs index bdb07af747a..8c1630d8e5a 100644 --- a/src/components/main/constellation.rs +++ b/src/components/main/constellation.rs @@ -107,7 +107,7 @@ impl FrameTree { } /// Returns the frame tree whose key is id - fn find_mut(@mut self, id: PipelineId) -> Option<@mut FrameTree> { + fn find(@mut self, id: PipelineId) -> Option<@mut FrameTree> { do self.iter().find |frame_tree| { id == frame_tree.pipeline.id } @@ -224,13 +224,13 @@ impl NavigationContext { /// Returns the frame trees whose keys are pipeline_id. pub fn find_all(&mut self, pipeline_id: PipelineId) -> ~[@mut FrameTree] { let from_current = do self.current.iter().filter_map |frame_tree| { - frame_tree.find_mut(pipeline_id) + frame_tree.find(pipeline_id) }; let from_next = do self.next.iter().filter_map |frame_tree| { - frame_tree.find_mut(pipeline_id) + frame_tree.find(pipeline_id) }; let from_prev = do self.previous.iter().filter_map |frame_tree| { - frame_tree.find_mut(pipeline_id) + frame_tree.find(pipeline_id) }; from_prev.chain(from_current).chain(from_next).collect() } @@ -306,7 +306,7 @@ impl Constellation { pub fn find_all(&mut self, pipeline_id: PipelineId) -> ~[@mut FrameTree] { let matching_navi_frames = self.navigation_context.find_all(pipeline_id); let matching_pending_frames = do self.pending_frames.iter().filter_map |frame_change| { - frame_change.after.find_mut(pipeline_id) + frame_change.after.find(pipeline_id) }; matching_navi_frames.move_iter().chain(matching_pending_frames).collect() } @@ -458,7 +458,7 @@ impl Constellation { // If the subframe is in the current frame tree, the compositor needs the new size for current_frame in self.current_frame().iter() { debug!("Constellation: Sending size for frame in current frame tree."); - let source_frame = current_frame.find_mut(pipeline_id); + let source_frame = current_frame.find(pipeline_id); for source_frame in source_frame.iter() { let found_child = source_frame.children.mut_iter() .find(|child| subpage_eq(child)); @@ -497,7 +497,7 @@ impl Constellation { let frame_trees: ~[@mut FrameTree] = { let matching_navi_frames = self.navigation_context.find_all(source_pipeline_id); let matching_pending_frames = do self.pending_frames.iter().filter_map |frame_change| { - frame_change.after.find_mut(source_pipeline_id) + frame_change.after.find(source_pipeline_id) }; matching_navi_frames.move_iter().chain(matching_pending_frames).collect() }; @@ -572,7 +572,7 @@ impl Constellation { fn handle_load_url_msg(&mut self, source_id: PipelineId, url: Url, size_future: Future>) { debug!("Constellation: received message to load %s", url.to_str()); // Make sure no pending page would be overridden. - let source_frame = self.current_frame().get_ref().find_mut(source_id).expect( + let source_frame = self.current_frame().get_ref().find(source_id).expect( "Constellation: received a LoadUrlMsg from a pipeline_id associated with a pipeline not in the active frame tree. This should be impossible."); @@ -581,7 +581,7 @@ impl Constellation { let old_id = frame_change.before.expect("Constellation: Received load msg from pipeline, but there is no currently active page. This should be impossible."); - let changing_frame = self.current_frame().get_ref().find_mut(old_id).expect("Constellation: + let changing_frame = self.current_frame().get_ref().find(old_id).expect("Constellation: Pending change has non-active source pipeline. This should be impossible."); if changing_frame.contains(source_id) || source_frame.contains(old_id) { @@ -706,7 +706,7 @@ impl Constellation { debug!("Constellation: revoking permission from %?", revoke_id); let current_frame = self.current_frame().unwrap(); - let to_revoke = current_frame.find_mut(revoke_id).expect( + let to_revoke = current_frame.find(revoke_id).expect( "Constellation: pending frame change refers to an old frame not contained in the current frame. This is a bug"); @@ -716,9 +716,9 @@ impl Constellation { // If to_add is not the root frame, then replace revoked_frame with it. // This conveniently keeps scissor rect size intact. - debug!("Constellation: replacing %? with %? in %?", - revoke_id, to_add.pipeline.id, next_frame_tree.pipeline.id); if to_add.parent.is_some() { + debug!("Constellation: replacing %? with %? in %?", + revoke_id, to_add.pipeline.id, next_frame_tree.pipeline.id); next_frame_tree.replace_child(revoke_id, to_add); } } @@ -730,7 +730,7 @@ impl Constellation { let subpage_id = to_add.pipeline.subpage_id.expect("Constellation: Child frame's subpage id is None. This should be impossible."); let rect = self.pending_sizes.pop(&(parent.id, subpage_id)); - let parent = next_frame_tree.find_mut(parent.id).expect( + let parent = next_frame_tree.find(parent.id).expect( "Constellation: pending frame has a parent frame that is not active. This is a bug."); parent.children.push(ChildFrameTree { @@ -774,9 +774,13 @@ impl Constellation { // exit any pipelines that don't exist outside the evicted frame trees for frame_tree in evicted.iter() { for @FrameTree { pipeline, _ } in frame_tree.iter() { - if !self.navigation_context.contains(pipeline.id) { + if !self.navigation_context.contains(pipeline.id) && + !exited.contains(&pipeline.id) { + debug!("Constellation: shutting down pipeline %?", pipeline.id); pipeline.exit(); self.pipelines.remove(&pipeline.id); + + exited.insert(pipeline.id); } } } From 99f125bb644db6eabef4ee282010db995be7d4eb Mon Sep 17 00:00:00 2001 From: Tim Kuehn Date: Sat, 21 Sep 2013 15:37:30 -0400 Subject: [PATCH 3/4] script task only exits when the root pipeline exits --- src/components/main/pipeline.rs | 2 +- src/components/script/dom/window.rs | 3 +- src/components/script/script_task.rs | 56 +++++++++++++++++++++++----- 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/components/main/pipeline.rs b/src/components/main/pipeline.rs index b90edc7e320..603f13a0d28 100644 --- a/src/components/main/pipeline.rs +++ b/src/components/main/pipeline.rs @@ -217,7 +217,7 @@ impl Pipeline { pub fn exit(&self) { // Script task handles shutting down layout, as well - self.script_chan.send(script_task::ExitMsg); + self.script_chan.send(script_task::ExitMsg(self.id)); let (response_port, response_chan) = comm::stream(); self.render_chan.send(render_task::ExitMsg(response_chan)); diff --git a/src/components/script/dom/window.rs b/src/components/script/dom/window.rs index 7c60da4d6c5..8b27b8862f1 100644 --- a/src/components/script/dom/window.rs +++ b/src/components/script/dom/window.rs @@ -212,7 +212,7 @@ impl Window { match timer_port.recv() { TimerMessage_Close => break, TimerMessage_Fire(td) => script_chan.send(FireTimerMsg(id, td)), - TimerMessage_TriggerExit => script_chan.send(ExitMsg), + TimerMessage_TriggerExit => script_chan.send(ExitMsg(id)), } } } @@ -225,7 +225,6 @@ impl Window { }; unsafe { - // TODO(tkuehn): This just grabs the top-level page. Need to handle subframes. let cache = ptr::to_unsafe_ptr(win.get_wrappercache()); win.wrap_object_shared(cx, ptr::null()); //XXXjdm proper scope let global = (*cache).wrapper; diff --git a/src/components/script/script_task.rs b/src/components/script/script_task.rs index 8c800727657..cb5f166daac 100644 --- a/src/components/script/script_task.rs +++ b/src/components/script/script_task.rs @@ -71,7 +71,7 @@ pub enum ScriptMsg { /// Notifies script that window has been resized but to not take immediate action. ResizeInactiveMsg(PipelineId, Size2D), /// Exits the constellation. - ExitMsg, + ExitMsg(PipelineId), } pub struct NewLayoutInfo { @@ -171,6 +171,28 @@ impl PageTree { stack: ~[self], } } + + // must handle root case separately + pub fn remove(&mut self, id: PipelineId) -> Option { + let remove_idx = { + self.inner.mut_iter() + .enumerate() + .find(|&(_idx, ref page_tree)| page_tree.page.id == id) + .map(|&(idx, _)| idx) + }; + match remove_idx { + Some(idx) => return Some(self.inner.remove(idx)), + None => { + for page_tree in self.inner.mut_iter() { + match page_tree.remove(id) { + found @ Some(_) => return found, + None => (), // keep going... + } + } + } + } + None + } } impl<'self> Iterator<@mut Page> for PageTreeIterator<'self> { @@ -494,9 +516,8 @@ impl ScriptTask { NavigateMsg(direction) => self.handle_navigate_msg(direction), 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(); - return false + ExitMsg(id) => { + if self.handle_exit_msg(id) { return false } }, ResizeMsg(*) => fail!("should have handled ResizeMsg already"), } @@ -603,12 +624,29 @@ impl ScriptTask { } /// Handles a request to exit the script task and shut down layout. - fn handle_exit_msg(&mut self) { - for page in self.page_tree.iter() { - page.join_layout(); - page.layout_chan.send(layout_interface::ExitMsg); + /// Returns true if the script task should shut down and false otherwise. + fn handle_exit_msg(&mut self, id: PipelineId) -> bool { + // If root is being exited, shut down all pages + if self.page_tree.page.id == id { + for page in self.page_tree.iter() { + page.join_layout(); + page.layout_chan.send(layout_interface::ExitMsg); + } + return true } - self.compositor.close(); + // otherwise find just the matching page and exit all sub-pages + match self.page_tree.remove(id) { + Some(ref mut page_tree) => { + for page in page_tree.iter() { + page.join_layout(); + page.layout_chan.send(layout_interface::ExitMsg); + } + return false + } + None => fail!("ScriptTask: Received exit message from + pipeline whose id is not in page tree"), + } + } /// The entry point to document loading. Defines bindings, sets up the window and document From 103cd6255d03f22fb8cc931a160d8601644cc35f Mon Sep 17 00:00:00 2001 From: Tim Kuehn Date: Tue, 24 Sep 2013 00:02:48 -0400 Subject: [PATCH 4/4] split script_task::ExitMsg into WindowExitMsg and PipelineExitMsg --- src/components/main/constellation.rs | 37 ++++++++++++++--------- src/components/main/layout/layout_task.rs | 4 +++ src/components/main/macros.rs | 2 +- src/components/main/pipeline.rs | 11 ++----- src/components/script/dom/window.rs | 4 +-- src/components/script/script_task.rs | 29 ++++++++++++------ 6 files changed, 53 insertions(+), 34 deletions(-) diff --git a/src/components/main/constellation.rs b/src/components/main/constellation.rs index 8c1630d8e5a..2b8c24c1da3 100644 --- a/src/components/main/constellation.rs +++ b/src/components/main/constellation.rs @@ -760,6 +760,28 @@ impl Constellation { } } + // Close all pipelines at and beneath a given frame + fn close_pipelines(&mut self, frame_tree: @mut FrameTree) { + // TODO(tkuehn): should only exit once per unique script task, + // and then that script task will handle sub-exits + for @FrameTree { pipeline, _ } in frame_tree.iter() { + pipeline.exit(); + self.pipelines.remove(&pipeline.id); + } + } + + fn handle_evicted_frames(&mut self, evicted: ~[@mut FrameTree]) { + for &frame_tree in evicted.iter() { + if !self.navigation_context.contains(frame_tree.pipeline.id) { + self.close_pipelines(frame_tree); + } else { + self.handle_evicted_frames(frame_tree.children.iter() + .map(|child| child.frame_tree) + .collect()); + } + } + } + // Grants a frame tree permission to paint; optionally updates navigation to reflect a new page fn grant_paint_permission(&mut self, frame_tree: @mut FrameTree, navigation_type: NavigationType) { // Give permission to paint to the new frame and all child frames @@ -770,20 +792,7 @@ impl Constellation { match navigation_type { constellation_msg::Load => { let evicted = self.navigation_context.load(frame_tree); - let mut exited = HashSet::new(); - // exit any pipelines that don't exist outside the evicted frame trees - for frame_tree in evicted.iter() { - for @FrameTree { pipeline, _ } in frame_tree.iter() { - if !self.navigation_context.contains(pipeline.id) && - !exited.contains(&pipeline.id) { - debug!("Constellation: shutting down pipeline %?", pipeline.id); - pipeline.exit(); - self.pipelines.remove(&pipeline.id); - - exited.insert(pipeline.id); - } - } - } + self.handle_evicted_frames(evicted); } _ => {} } diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs index c46c2080819..d6e0b86210b 100644 --- a/src/components/main/layout/layout_task.rs +++ b/src/components/main/layout/layout_task.rs @@ -26,6 +26,7 @@ use gfx::font_context::FontContext; use gfx::geometry::Au; use gfx::opts::Opts; use gfx::render_task::{RenderMsg, RenderChan, RenderLayer}; +use gfx::render_task; use newcss::select::SelectCtx; use newcss::stylesheet::Stylesheet; use newcss::types::OriginAuthor; @@ -162,6 +163,9 @@ impl LayoutTask { } ExitMsg => { debug!("layout: ExitMsg received"); + let (response_port, response_chan) = stream(); + self.render_chan.send(render_task::ExitMsg(response_chan)); + response_port.recv(); return false } } diff --git a/src/components/main/macros.rs b/src/components/main/macros.rs index ed7ec2d2f15..9f364c48f74 100644 --- a/src/components/main/macros.rs +++ b/src/components/main/macros.rs @@ -5,7 +5,7 @@ macro_rules! special_stream( ($Chan:ident) => ( { - let (port, chan) = comm::stream::(); + let (port, chan) = stream::(); (port, $Chan::new(chan)) } ); diff --git a/src/components/main/pipeline.rs b/src/components/main/pipeline.rs index 603f13a0d28..4e7f7c066cf 100644 --- a/src/components/main/pipeline.rs +++ b/src/components/main/pipeline.rs @@ -6,7 +6,6 @@ use extra::url::Url; use compositing::CompositorChan; use gfx::render_task::{RenderChan, RenderTask}; use gfx::render_task::{PaintPermissionGranted, PaintPermissionRevoked}; -use gfx::render_task; use gfx::opts::Opts; use layout::layout_task::LayoutTask; use script::layout_interface::LayoutChan; @@ -21,7 +20,6 @@ use servo_util::time::ProfilerChan; use geom::size::Size2D; use extra::future::Future; use std::cell::Cell; -use std::comm; use std::task; /// A uniquely-identifiable pipeline of script task, layout task, and render task. @@ -216,12 +214,9 @@ impl Pipeline { } pub fn exit(&self) { - // Script task handles shutting down layout, as well - self.script_chan.send(script_task::ExitMsg(self.id)); - - let (response_port, response_chan) = comm::stream(); - self.render_chan.send(render_task::ExitMsg(response_chan)); - response_port.recv(); + // Script task handles shutting down layout, + // and layout handles shutting down the renderer. + self.script_chan.try_send(script_task::ExitPipelineMsg(self.id)); } } diff --git a/src/components/script/dom/window.rs b/src/components/script/dom/window.rs index 8b27b8862f1..5c2bf7cda97 100644 --- a/src/components/script/dom/window.rs +++ b/src/components/script/dom/window.rs @@ -10,7 +10,7 @@ use dom::node::{AbstractNode, ScriptView}; use dom::navigator::Navigator; use layout_interface::ReflowForDisplay; -use script_task::{ExitMsg, FireTimerMsg, Page, ScriptChan}; +use script_task::{ExitWindowMsg, FireTimerMsg, Page, ScriptChan}; use servo_msg::compositor_msg::ScriptListener; use servo_net::image_cache_task::ImageCacheTask; @@ -212,7 +212,7 @@ impl Window { match timer_port.recv() { TimerMessage_Close => break, TimerMessage_Fire(td) => script_chan.send(FireTimerMsg(id, td)), - TimerMessage_TriggerExit => script_chan.send(ExitMsg(id)), + TimerMessage_TriggerExit => script_chan.send(ExitWindowMsg(id)), } } } diff --git a/src/components/script/script_task.rs b/src/components/script/script_task.rs index cb5f166daac..ab184fe67e1 100644 --- a/src/components/script/script_task.rs +++ b/src/components/script/script_task.rs @@ -70,8 +70,10 @@ pub enum ScriptMsg { ReflowCompleteMsg(PipelineId, uint), /// Notifies script that window has been resized but to not take immediate action. ResizeInactiveMsg(PipelineId, Size2D), - /// Exits the constellation. - ExitMsg(PipelineId), + /// Notifies the script that a pipeline should be closed. + ExitPipelineMsg(PipelineId), + /// Notifies the script that a window associated with a particular pipeline should be closed. + ExitWindowMsg(PipelineId), } pub struct NewLayoutInfo { @@ -516,9 +518,8 @@ impl ScriptTask { NavigateMsg(direction) => self.handle_navigate_msg(direction), 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(id) => { - if self.handle_exit_msg(id) { return false } - }, + ExitPipelineMsg(id) => if self.handle_exit_pipeline_msg(id) { return false }, + ExitWindowMsg(id) => if self.handle_exit_window_msg(id) { return false }, ResizeMsg(*) => fail!("should have handled ResizeMsg already"), } } @@ -623,9 +624,17 @@ impl ScriptTask { } } + fn handle_exit_window_msg(&mut self, _id: PipelineId) -> bool { + // TODO(tkuehn): currently there is only one window, + // so this can afford to be naive and just shut down the + // compositor. In the future it'll need to be smarter. + self.compositor.close(); + true + } + /// 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_msg(&mut self, id: PipelineId) -> bool { + fn handle_exit_pipeline_msg(&mut self, id: PipelineId) -> bool { // If root is being exited, shut down all pages if self.page_tree.page.id == id { for page in self.page_tree.iter() { @@ -641,10 +650,12 @@ impl ScriptTask { page.join_layout(); page.layout_chan.send(layout_interface::ExitMsg); } - return false + false } - None => fail!("ScriptTask: Received exit message from - pipeline whose id is not in page tree"), + // TODO(tkuehn): pipeline closing is currently duplicated across + // script and constellation, which can cause this to happen. Constellation + // needs to be smarter about exiting pipelines. + None => false, } }