From 3e6ff80f18f3407eda6a445db1f4f56b592127b5 Mon Sep 17 00:00:00 2001 From: Lars Bergstrom Date: Wed, 15 Jan 2014 15:26:24 -0600 Subject: [PATCH 1/4] Add extra debugging information. --- src/components/gfx/render_task.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/gfx/render_task.rs b/src/components/gfx/render_task.rs index a17970e18d3..c692e901714 100644 --- a/src/components/gfx/render_task.rs +++ b/src/components/gfx/render_task.rs @@ -187,6 +187,7 @@ impl RenderTask { |ctx| render_task.buffer_map.clear(ctx)); } + debug!("render_task: shutdown_chan send"); shutdown_chan.send(()); }); } @@ -201,6 +202,7 @@ impl RenderTask { self.epoch.next(); self.compositor.set_layer_page_size_and_color(self.id, render_layer.size, self.epoch, render_layer.color); } else { + debug!("render_task: render ready msg"); self.constellation_chan.send(RendererReadyMsg(self.id)); } self.render_layer = Some(render_layer); @@ -232,6 +234,7 @@ impl RenderTask { self.paint_permission = false; } ExitMsg(response_ch) => { + debug!("render_task: exitmsg response send"); response_ch.send(()); break; } @@ -383,6 +386,7 @@ impl RenderTask { if self.paint_permission { self.compositor.paint(self.id, layer_buffer_set, self.epoch); } else { + debug!("render_task: RendererReadyMsg send"); self.constellation_chan.send(RendererReadyMsg(self.id)); } self.compositor.set_render_state(IdleRenderState); From 342844ed7b5684d67e16ca74ada6dcdb7bcf92fc Mon Sep 17 00:00:00 2001 From: Lars Bergstrom Date: Wed, 15 Jan 2014 15:26:39 -0600 Subject: [PATCH 2/4] When `window.close()` is called, we should just ask the compositor to exit normally. The old code made the mistake of attempting to shutdown the associated pipelines itself, which caused race conditions with the constellation and compositor, as they expect to be able to drain their message queues before exiting. --- src/components/script/script_task.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/components/script/script_task.rs b/src/components/script/script_task.rs index 9ca9af2f8a6..8034822e9d9 100644 --- a/src/components/script/script_task.rs +++ b/src/components/script/script_task.rs @@ -531,10 +531,7 @@ impl ScriptTask { ReflowCompleteMsg(id, reflow_id) => self.handle_reflow_complete_msg(id, reflow_id), ResizeInactiveMsg(id, new_size) => self.handle_resize_inactive_msg(id, new_size), ExitPipelineMsg(id) => if self.handle_exit_pipeline_msg(id) { return false }, - ExitWindowMsg(id) => { - self.handle_exit_window_msg(id); - return false - }, + ExitWindowMsg(id) => self.handle_exit_window_msg(id), ResizeMsg(..) => fail!("should have handled ResizeMsg already"), } } @@ -614,9 +611,13 @@ impl ScriptTask { } } - fn handle_exit_window_msg(&mut self, id: PipelineId) { + /// We have gotten a window.close from script, which we pass on to the compositor. + /// We do not shut down the script task now, because the compositor will ask the + /// constellation to shut down the pipeline, which will clean everything up + /// normally. If we do exit, we will tear down the DOM nodes, possibly at a point + /// where layout is still accessing them. + fn handle_exit_window_msg(&mut self, _: PipelineId) { debug!("script task handling exit window msg"); - self.handle_exit_pipeline_msg(id); // TODO(tkuehn): currently there is only one window, // so this can afford to be naive and just shut down the From 6d3429ad03728028fa4059dd192456de292f939d Mon Sep 17 00:00:00 2001 From: Lars Bergstrom Date: Wed, 15 Jan 2014 15:28:13 -0600 Subject: [PATCH 3/4] Changes the Constellation shutdown procedure to a message and response from the Compositor instead of a message with an immediate callback. The problem was that the old model introduced a potential deadlock. If the Constellation had a pending RenderReadyMsg in its queue when the ExitMsg arrived from the Compositor, the Compositor would exit its message loop and wait for the response from the Constellation. But, the Constellation would send a SetIds message to the Compositor, which also required a response - resulting in deadlock. --- src/components/main/compositing/compositor.rs | 18 +++++++++---- .../main/compositing/compositor_task.rs | 25 ++++++++----------- src/components/main/compositing/headless.rs | 12 ++++++--- src/components/main/constellation.rs | 12 ++++----- src/components/main/pipeline.rs | 1 + src/components/main/servo.rc | 11 +++----- src/components/msg/constellation_msg.rs | 4 +-- 7 files changed, 46 insertions(+), 37 deletions(-) diff --git a/src/components/main/compositing/compositor.rs b/src/components/main/compositing/compositor.rs index 3790664b73f..d65cab22c71 100644 --- a/src/components/main/compositing/compositor.rs +++ b/src/components/main/compositing/compositor.rs @@ -32,7 +32,7 @@ use layers::scene::Scene; use opengles::gl2; use png; use servo_msg::compositor_msg::{Epoch, IdleRenderState, LayerBufferSet, RenderState}; -use servo_msg::constellation_msg::{ConstellationChan, NavigateMsg, ResizedWindowMsg, LoadUrlMsg, PipelineId}; +use servo_msg::constellation_msg::{ConstellationChan, ExitMsg, NavigateMsg, ResizedWindowMsg, LoadUrlMsg, PipelineId}; use servo_msg::constellation_msg; use servo_util::time::{profile, ProfilerChan, Timer}; use servo_util::{time, url}; @@ -129,7 +129,7 @@ impl IOCompositor { zoom_action: false, zoom_time: 0f64, compositor_layer: None, - constellation_chan: constellation_chan.clone(), + constellation_chan: constellation_chan, profiler_chan: profiler_chan, fragment_point: None } @@ -202,10 +202,16 @@ impl IOCompositor { None => break, Some(Exit(chan)) => { - self.done = true; + debug!("shutting down the constellation"); + self.constellation_chan.send(ExitMsg); chan.send(()); } + Some(ShutdownComplete) => { + debug!("constellation completed shutdown"); + self.done = true; + } + Some(ChangeReadyState(ready_state)) => { self.window.set_ready_state(ready_state); } @@ -496,12 +502,14 @@ impl IOCompositor { FinishedWindowEvent => { let exit = self.opts.exit_after_load; if exit { - self.done = true; + debug!("shutting down the constellation for FinishedWindowEvent"); + self.constellation_chan.send(ExitMsg); } } QuitWindowEvent => { - self.done = true; + debug!("shutting down the constellation for QuitWindowEvent"); + self.constellation_chan.send(ExitMsg); } } } diff --git a/src/components/main/compositing/compositor_task.rs b/src/components/main/compositing/compositor_task.rs index 6e01b3d64e3..87f9a6b90ca 100644 --- a/src/components/main/compositing/compositor_task.rs +++ b/src/components/main/compositing/compositor_task.rs @@ -16,7 +16,7 @@ use gfx::opts::Opts; use layers::platform::surface::{NativeCompositingGraphicsContext, NativeGraphicsMetadata}; use servo_msg::compositor_msg::{Epoch, RenderListener, LayerBufferSet, RenderState, ReadyState}; use servo_msg::compositor_msg::{ScriptListener, Tile}; -use servo_msg::constellation_msg::{ConstellationChan, PipelineId, ExitMsg}; +use servo_msg::constellation_msg::{ConstellationChan, PipelineId}; use servo_util::time::ProfilerChan; use std::comm::{Chan, SharedChan, Port}; use std::num::Orderable; @@ -112,11 +112,16 @@ impl CompositorChan { self.chan.send(msg); } } - /// Messages from the painting task and the constellation task to the compositor task. pub enum Msg { /// Requests that the compositor shut down. Exit(Chan<()>), + + /// Informs the compositor that the constellation has completed shutdown. + /// Required because the constellation can have pending calls to make (e.g. SetIds) + /// at the time that we send it an ExitMsg. + ShutdownComplete, + /// Requests the compositor's graphics metadata. Graphics metadata is what the renderer needs /// to create surfaces that the compositor can see. On Linux this is the X display; on Mac this /// is the pixel format. @@ -185,9 +190,7 @@ impl CompositorTask { pub fn create(opts: Opts, port: Port, constellation_chan: ConstellationChan, - profiler_chan: ProfilerChan, - exit_chan: Chan<()>, - exit_response_from_constellation: Port<()>) { + profiler_chan: ProfilerChan) { let compositor = CompositorTask::new(opts.headless); @@ -197,18 +200,12 @@ impl CompositorTask { opts, port, constellation_chan.clone(), - profiler_chan); + profiler_chan) } Headless => { headless::NullCompositor::create(port, - constellation_chan.clone()); + constellation_chan.clone()) } - } - - // Constellation has to be shut down before the compositor goes out of - // scope, as the compositor manages setup/teardown of global subsystems - debug!("shutting down the constellation"); - constellation_chan.send(ExitMsg(exit_chan)); - exit_response_from_constellation.recv(); + }; } } diff --git a/src/components/main/compositing/headless.rs b/src/components/main/compositing/headless.rs index 07a2e973868..7cc299fd88d 100644 --- a/src/components/main/compositing/headless.rs +++ b/src/components/main/compositing/headless.rs @@ -5,7 +5,7 @@ use compositing::*; use geom::size::Size2D; -use servo_msg::constellation_msg::{ConstellationChan, ResizedWindowMsg}; +use servo_msg::constellation_msg::{ConstellationChan, ExitMsg, ResizedWindowMsg}; use std::comm::Port; @@ -32,14 +32,20 @@ impl NullCompositor { // Tell the constellation about the initial fake size. constellation_chan.send(ResizedWindowMsg(Size2D(640u, 480u))); - compositor.handle_message(); + compositor.handle_message(constellation_chan); } - fn handle_message(&self) { + fn handle_message(&self, constellation_chan: ConstellationChan) { loop { match self.port.recv() { Exit(chan) => { + debug!("shutting down the constellation"); + constellation_chan.send(ExitMsg); chan.send(()); + } + + ShutdownComplete => { + debug!("constellation completed shutdown"); break } diff --git a/src/components/main/constellation.rs b/src/components/main/constellation.rs index f2a4b16e2c4..81e9e8bb4aa 100644 --- a/src/components/main/constellation.rs +++ b/src/components/main/constellation.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use compositing::{CompositorChan, SetIds, SetLayerClipRect}; +use compositing::{CompositorChan, SetIds, SetLayerClipRect, ShutdownComplete}; use extra::url::Url; use geom::rect::Rect; @@ -315,9 +315,9 @@ impl Constellation { /// Handles loading pages, navigation, and granting access to the compositor fn handle_request(&mut self, request: Msg) -> bool { match request { - ExitMsg(sender) => { + ExitMsg => { debug!("constellation exiting"); - self.handle_exit(sender); + self.handle_exit(); return false; } FailureMsg(pipeline_id, subpage_id) => { @@ -363,14 +363,13 @@ impl Constellation { true } - fn handle_exit(&self, sender: Chan<()>) { + fn handle_exit(&self) { for (_id, ref pipeline) in self.pipelines.iter() { pipeline.exit(); } self.image_cache_task.exit(); self.resource_task.send(resource_task::Exit); - - sender.send(()); + self.compositor_chan.send(ShutdownComplete); } fn handle_failure_msg(&mut self, pipeline_id: PipelineId, subpage_id: Option) { @@ -804,6 +803,7 @@ impl Constellation { fn set_ids(&self, frame_tree: @mut FrameTree) { let (port, chan) = Chan::new(); + debug!("Constellation sending SetIds"); self.compositor_chan.send(SetIds(frame_tree.to_sendable(), chan, self.chan.clone())); match port.recv_opt() { Some(()) => { diff --git a/src/components/main/pipeline.rs b/src/components/main/pipeline.rs index 8f5aaef929d..0fee4bdcea3 100644 --- a/src/components/main/pipeline.rs +++ b/src/components/main/pipeline.rs @@ -181,6 +181,7 @@ impl Pipeline { } pub fn revoke_paint_permission(&self) { + debug!("pipeline revoking render channel paint permission"); self.render_chan.send(PaintPermissionRevoked); } diff --git a/src/components/main/servo.rc b/src/components/main/servo.rc index 6b1bbe37302..d46c77eabc6 100755 --- a/src/components/main/servo.rc +++ b/src/components/main/servo.rc @@ -135,7 +135,6 @@ pub extern "C" fn android_start(argc: int, argv: **u8) -> int { fn run(opts: Opts) { let mut pool = green::SchedPool::new(green::PoolConfig::new()); - let (exit_response_from_constellation, exit_chan) = Chan::new(); let (compositor_port, compositor_chan) = CompositorChan::new(); let profiler_chan = Profiler::create(opts.profiler_period); @@ -154,13 +153,13 @@ fn run(opts: Opts) { image_cache_task, profiler_chan_clone); - // Send the constallation Chan as the result - result_chan.send(constellation_chan.clone()); - // Send the URL command to the constellation. for filename in opts.urls.iter() { constellation_chan.send(InitLoadUrlMsg(make_url(filename.clone(), None))) } + + // Send the constallation Chan as the result + result_chan.send(constellation_chan); }); let constellation_chan = result_port.recv(); @@ -169,9 +168,7 @@ fn run(opts: Opts) { CompositorTask::create(opts, compositor_port, constellation_chan, - profiler_chan, - exit_chan, - exit_response_from_constellation); + profiler_chan); pool.shutdown(); } diff --git a/src/components/msg/constellation_msg.rs b/src/components/msg/constellation_msg.rs index ee8f2914785..0aea8963c92 100644 --- a/src/components/msg/constellation_msg.rs +++ b/src/components/msg/constellation_msg.rs @@ -8,7 +8,7 @@ use extra::url::Url; use geom::rect::Rect; use geom::size::Size2D; -use std::comm::{Chan, SharedChan}; +use std::comm::SharedChan; #[deriving(Clone)] pub struct ConstellationChan(SharedChan); @@ -28,7 +28,7 @@ pub enum IFrameSandboxState { /// Messages from the compositor to the constellation. pub enum Msg { - ExitMsg(Chan<()>), + ExitMsg, FailureMsg(PipelineId, Option), InitLoadUrlMsg(Url), FrameRectMsg(PipelineId, SubpageId, Rect), From 9ac2f5fd9b5664470b8e99f91332efb305b8550b Mon Sep 17 00:00:00 2001 From: Lars Bergstrom Date: Wed, 15 Jan 2014 15:54:47 -0600 Subject: [PATCH 4/4] Also handle the exit after load and output file cases for ref tests --- src/components/main/compositing/compositor.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/components/main/compositing/compositor.rs b/src/components/main/compositing/compositor.rs index d65cab22c71..1d3af84ca8d 100644 --- a/src/components/main/compositing/compositor.rs +++ b/src/components/main/compositing/compositor.rs @@ -663,14 +663,16 @@ impl IOCompositor { let res = png::store_png(&img, &path); assert!(res.is_ok()); - self.done = true; + debug!("shutting down the constellation after generating an output file"); + self.constellation_chan.send(ExitMsg); } self.window.present(); let exit = self.opts.exit_after_load; if exit { - self.done = true; + debug!("shutting down the constellation for exit_after_load"); + self.constellation_chan.send(ExitMsg); } }