Auto merge of #14052 - asajeffrey:constellation-manages-script-thread-lifetime, r=jdm

bors-servo · web-flow · commit 13009fe0254d · 2016-11-08T20:09:21.000-06:00
Constellation manages script thread lifetime  Yet another step towards fixing #633... At the moment, script threads are responsible for killing themselves when they have no more pipelines to manage. This is fine right now, because script threads have a root pipeline: once it is closed, everything is closed. It's not fine once we fix #633 because there's a race condition where the constellation could kill the last pipeline in a script thread, then open a new one in the same script thread. We can't have the constellation block on the script thread, waiting to make sure a pipeline is created, since this could introduce deadlock. The easiest solution is to have the constellation manage the script thread lifetime: when the constellation discovers that a script thread is not managing any live pipelines, it closes the script thread, and updates its own state. Most of the commits are from #14013, its just "Script thread lifetime is now managed by the constellation." (9ac6e4d) that's new. cc @jdm @ConnorGBrewster ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14052)
diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs
@@ -53,6 +53,7 @@ use std::iter::once;
 use std::marker::PhantomData;
 use std::mem::replace;
 use std::process;
+use std::rc::Rc;
 use std::sync::Arc;
 use std::sync::mpsc::{Receiver, Sender, channel};
 use std::thread;
@@ -365,6 +366,30 @@ enum ExitPipelineMode {
     Force,
 }
 
+/// A script channel, that closes the script thread down when it is dropped
+pub struct ScriptChan {
+    chan: IpcSender<ConstellationControlMsg>,
+    dont_send_or_sync: PhantomData<Rc<()>>,
+}
+
+impl Drop for ScriptChan {
+    fn drop(&mut self) {
+        let _ = self.chan.send(ConstellationControlMsg::ExitScriptThread);
+    }
+}
+
+impl ScriptChan {
+    pub fn send(&self, msg: ConstellationControlMsg) -> Result<(), IOError> {
+        self.chan.send(msg)
+    }
+    pub fn new(chan: IpcSender<ConstellationControlMsg>) -> Rc<ScriptChan> {
+        Rc::new(ScriptChan { chan: chan, dont_send_or_sync: PhantomData })
+    }
+    pub fn sender(&self) -> IpcSender<ConstellationControlMsg> {
+        self.chan.clone()
+    }
+}
+
 /// A logger directed at the constellation from content processes
 #[derive(Clone)]
 pub struct FromScriptLogger {
@@ -561,7 +586,7 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
                     parent_info: Option<(PipelineId, FrameType)>,
                     old_pipeline_id: Option<PipelineId>,
                     initial_window_size: Option<TypedSize2D<f32, PagePx>>,
-                    script_channel: Option<IpcSender<ConstellationControlMsg>>,
+                    script_channel: Option<Rc<ScriptChan>>,
                     load_data: LoadData,
                     is_private: bool) {
         if self.shutting_down { return; }
diff --git a/components/constellation/pipeline.rs b/components/constellation/pipeline.rs
@@ -6,6 +6,7 @@ use bluetooth_traits::BluetoothRequest;
 use compositing::CompositionPipeline;
 use compositing::CompositorProxy;
 use compositing::compositor_thread::Msg as CompositorMsg;
+use constellation::ScriptChan;
 use devtools_traits::{DevtoolsControlMsg, ScriptToDevtoolsControlMsg};
 use euclid::scale_factor::ScaleFactor;
 use euclid::size::TypedSize2D;
@@ -30,6 +31,7 @@ use std::env;
 use std::ffi::OsStr;
 use std::io::Error as IOError;
 use std::process;
+use std::rc::Rc;
 use std::sync::mpsc::Sender;
 use style_traits::{PagePx, ViewportPx};
 use url::Url;
@@ -50,7 +52,7 @@ pub struct Pipeline {
     /// The ID of the frame that contains this Pipeline.
     pub frame_id: FrameId,
     pub parent_info: Option<(PipelineId, FrameType)>,
-    pub script_chan: IpcSender<ConstellationControlMsg>,
+    pub script_chan: Rc<ScriptChan>,
     /// A channel to layout, for performing reflows and shutdown.
     pub layout_chan: IpcSender<LayoutControlMsg>,
     /// A channel to the compositor.
@@ -113,7 +115,7 @@ pub struct InitialPipelineState {
     pub device_pixel_ratio: ScaleFactor<f32, ViewportPx, DevicePixel>,
     /// A channel to the script thread, if applicable. If this is `Some`,
     /// then `parent_info` must also be `Some`.
-    pub script_chan: Option<IpcSender<ConstellationControlMsg>>,
+    pub script_chan: Option<Rc<ScriptChan>>,
     /// Information about the page to load.
     pub load_data: LoadData,
     /// The ID of the pipeline namespace for this script thread.
@@ -165,7 +167,7 @@ impl Pipeline {
             }
             None => {
                 let (script_chan, script_port) = ipc::channel().expect("Pipeline script chan");
-                (script_chan, Some((script_port, pipeline_port)))
+                (ScriptChan::new(script_chan), Some((script_port, pipeline_port)))
             }
         };
 
@@ -215,7 +217,7 @@ impl Pipeline {
                 mem_profiler_chan: state.mem_profiler_chan,
                 window_size: window_size,
                 layout_to_constellation_chan: state.layout_to_constellation_chan,
-                script_chan: script_chan.clone(),
+                script_chan: script_chan.sender(),
                 load_data: state.load_data.clone(),
                 script_port: script_port,
                 opts: (*opts::get()).clone(),
@@ -258,7 +260,7 @@ impl Pipeline {
     fn new(id: PipelineId,
            frame_id: FrameId,
            parent_info: Option<(PipelineId, FrameType)>,
-           script_chan: IpcSender<ConstellationControlMsg>,
+           script_chan: Rc<ScriptChan>,
            layout_chan: IpcSender<LayoutControlMsg>,
            compositor_proxy: Box<CompositorProxy + 'static + Send>,
            is_private: bool,
@@ -329,7 +331,7 @@ impl Pipeline {
     pub fn to_sendable(&self) -> CompositionPipeline {
         CompositionPipeline {
             id: self.id.clone(),
-            script_chan: self.script_chan.clone(),
+            script_chan: self.script_chan.sender(),
             layout_chan: self.layout_chan.clone(),
         }
     }
diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs
@@ -840,10 +840,9 @@ impl ScriptThread {
 
             let result = self.profile_event(category, move || {
                 match msg {
-                    FromConstellation(ConstellationControlMsg::ExitPipeline(id)) => {
-                        if self.handle_exit_pipeline_msg(id) {
-                            return Some(false)
-                        }
+                    FromConstellation(ConstellationControlMsg::ExitScriptThread) => {
+                        self.handle_exit_script_thread_msg();
+                        return Some(false);
                     },
                     FromConstellation(inner_msg) => self.handle_msg_from_constellation(inner_msg),
                     FromScript(inner_msg) => self.handle_msg_from_script(inner_msg),
@@ -990,11 +989,13 @@ impl ScriptThread {
                 self.handle_css_error_reporting(pipeline_id, filename, line, column, msg),
             ConstellationControlMsg::Reload(pipeline_id) =>
                 self.handle_reload(pipeline_id),
+            ConstellationControlMsg::ExitPipeline(pipeline_id) =>
+                self.handle_exit_pipeline_msg(pipeline_id),
             msg @ ConstellationControlMsg::AttachLayout(..) |
             msg @ ConstellationControlMsg::Viewport(..) |
             msg @ ConstellationControlMsg::SetScrollState(..) |
             msg @ ConstellationControlMsg::Resize(..) |
-            msg @ ConstellationControlMsg::ExitPipeline(..) =>
+            msg @ ConstellationControlMsg::ExitScriptThread =>
                       panic!("should have handled {:?} already", msg),
         }
     }
@@ -1487,10 +1488,9 @@ impl ScriptThread {
         document.send_title_to_compositor();
     }
 
-    /// Handles a request to exit the script thread and shut down layout.
-    /// Returns true if the script thread should shut down and false otherwise.
-    fn handle_exit_pipeline_msg(&self, id: PipelineId) -> bool {
-        debug!("Exiting pipeline {:?}.", id);
+    /// Handles a request to exit a pipeline and shut down layout.
+    fn handle_exit_pipeline_msg(&self, id: PipelineId) {
+        debug!("Exiting pipeline {}.", id);
 
         self.closed_pipelines.borrow_mut().insert(id);
 
@@ -1507,7 +1507,7 @@ impl ScriptThread {
             let (response_chan, response_port) = channel();
             let chan = &load.layout_chan;
             if chan.send(message::Msg::PrepareToExit(response_chan)).is_ok() {
-                debug!("shutting down layout for page {:?}", id);
+                debug!("shutting down layout for page {}", id);
                 response_port.recv().unwrap();
                 chan.send(message::Msg::ExitNow).ok();
             }
@@ -1518,13 +1518,22 @@ impl ScriptThread {
             let _ = self.constellation_chan.send(ConstellationMsg::PipelineExited(id));
         }
 
-        let no_pending_loads = self.incomplete_loads.borrow().is_empty();
-        let no_remaining_contexts = self.documents.borrow().is_empty();
+        debug!("Exited pipeline {}.", id);
+    }
 
-        debug!("Exited pipeline {:?} ({}&{}).", id, no_pending_loads, no_remaining_contexts);
+    /// Handles a request to exit the script thread and shut down layout.
+    fn handle_exit_script_thread_msg(&self) {
+        debug!("Exiting script thread.");
+
+        let mut pipeline_ids = Vec::new();
+        pipeline_ids.extend(self.incomplete_loads.borrow().iter().next().map(|load| load.pipeline_id));
+        pipeline_ids.extend(self.documents.borrow().iter().next().map(|(pipeline_id, _)| pipeline_id));
+
+        for pipeline_id in pipeline_ids {
+            self.handle_exit_pipeline_msg(pipeline_id);
+        }
 
-        // Exit if no pending loads and no remaining contexts
-        no_pending_loads && no_remaining_contexts
+        debug!("Exited script thread.");
     }
 
     /// Handles when layout thread finishes all animation in one tick
diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs
@@ -195,6 +195,8 @@ pub enum ConstellationControlMsg {
     ResizeInactive(PipelineId, WindowSizeData),
     /// Notifies the script that a pipeline should be closed.
     ExitPipeline(PipelineId),
+    /// Notifies the script that the whole thread should be closed.
+    ExitScriptThread,
     /// Sends a DOM event.
     SendEvent(PipelineId, CompositorEvent),
     /// Notifies script of the viewport.
@@ -259,6 +261,7 @@ impl fmt::Debug for ConstellationControlMsg {
             Resize(..) => "Resize",
             ResizeInactive(..) => "ResizeInactive",
             ExitPipeline(..) => "ExitPipeline",
+            ExitScriptThread => "ExitScriptThread",
             SendEvent(..) => "SendEvent",
             Viewport(..) => "Viewport",
             SetScrollState(..) => "SetScrollState",
