Close the correct pipelines when evicted from navigation context.

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<Size2D<uint>>) {
         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 {
@@ -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,15 +792,7 @@ impl Constellation {
         match navigation_type {
             constellation_msg::Load => {
                 let evicted = self.navigation_context.load(frame_tree);
-                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);
-                        }
-                    }
-                }
+                self.handle_evicted_frames(evicted);
             }
             _ => {}
         }

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
             }
         }

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))
         }
     );

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);
-
-        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));
     }
 }
 

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),
+                            TimerMessage_TriggerExit => script_chan.send(ExitWindowMsg(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;

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<uint>),
-    /// Exits the constellation.
-    ExitMsg,
+    /// 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 {
@@ -171,6 +173,28 @@ impl PageTree {
             stack: ~[self],
         }
     }
+
+    // must handle root case separately
+    pub fn remove(&mut self, id: PipelineId) -> Option<PageTree> {
+        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,10 +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 => {
-                    self.handle_exit_msg();
-                    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"),
             }
         }
@@ -602,13 +624,40 @@ 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.
-    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_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() {
+                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);
+                }
+                false
+            }
+            // 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,
+        }
+
     }
 
     /// The entry point to document loading. Defines bindings, sets up the window and document