auto merge of #966 : tikue/servo/master, r=metajack

Fixes #967 and #965 

This has been wrong for a long time. Previously, only the pipeline associated with the root frame evicted would be shut down. 1) It shouldn't necessarily be closed, because there could be references to it still in the navigation context, and 2) Presumably none of the children pipelines of the root frame were ever exiting.

It's hard to test this right now because #965 covers up other pipeline exiting issues, but when that's fixed, a pathological case in which things would have broken down would be:

1) Load a page with an iframe that contains a link
2) Click the link
3) Press backspace to navigate back
4) Navigate to any new page, at which point the forward page would be evicted from the navigation context, and the outer frame's pipeline would be shut down improperly.
5) Press backspace, at which point there is no longer a pipeline for the old page, because it was shut down prematurely. Presumably this would cause a crash.

I also changed the FrameTree function ```find_mut``` to ```find``` because find_mut implies it's doing something to cause mutability, but the mutability is caused by the type of object being iterated over, nothing else.

Additionally, script was exiting completely when receiving an exit message. Instead, it needs to handle exit messages according to who sent it. It should only close the subframes of the frame whose pipeline sent the exit message. This is now fixed.

Inexplicably, script was also closing the compositor upon receiving an exit message. This doesn't seem like it'd ever be the right thing to do. *Edit: this is _only_ the right thing to do when received from the window.* I've fixed that. I don't think anyone shuts down the compositor now. *Edit: the script shuts down the compositor only when receiving an exit from the window.*

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 {
         }
     }
 
-    /// 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);
-        }
+    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_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
+        }
+        // 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