Add test to constellation to avoid writing reftest image if there are…

… pending frames.

Also change when pipelines become active.

This makes the constellation activate a pipeline as the current frame
when it is ready to do initial reflow, rather than when it is ready
to paint.

This fixes a number of intermittent failures that could previously occur
if an iframe was not visible - which would mean it was never moved from
a pending frame in the constellation to an active frame.

(It happens that webrender exposes these intermittents as permanent failures).

components/compositing/constellation.rs

@@ -29,7 +29,7 @@ use msg::compositor_msg::Epoch;
 use msg::constellation_msg::AnimationState;
 use msg::constellation_msg::PaintMsg as FromPaintMsg;
 use msg::constellation_msg::WebDriverCommandMsg;
-use msg::constellation_msg::{FrameId, PipelineId};
+use msg::constellation_msg::{DocumentState, FrameId, PipelineId};
 use msg::constellation_msg::{IframeLoadInfo, IFrameSandboxState, MozBrowserEvent, NavigationDirection};
 use msg::constellation_msg::{Key, KeyModifiers, KeyState, LoadData};
 use msg::constellation_msg::{PipelineNamespace, PipelineNamespaceId};
@@ -45,7 +45,7 @@ use profile_traits::mem;
 use profile_traits::time;
 use sandboxing;
 use script_traits::{CompositorEvent, ConstellationControlMsg, LayoutControlMsg};
-use script_traits::{ScriptMsg as FromScriptMsg, ScriptState, ScriptTaskFactory};
+use script_traits::{ScriptMsg as FromScriptMsg, ScriptTaskFactory};
 use script_traits::{TimerEventRequest};
 use std::borrow::ToOwned;
 use std::collections::HashMap;
@@ -66,6 +66,7 @@ use util::{opts, prefs};
 #[derive(Debug, PartialEq)]
 enum ReadyToSave {
     NoRootFrame,
+    PendingFrames,
     WebFontNotLoaded,
     DocumentLoading,
     EpochMismatch,
@@ -170,6 +171,9 @@ pub struct Constellation<LTF, STF> {
 
     /// A list of child content processes.
     child_processes: Vec<ChildProcess>,
+
+    /// Document states for loaded pipelines (used only when writing screenshots).
+    document_states: HashMap<PipelineId, DocumentState>,
 }
 
 /// State needed to construct a constellation.
@@ -225,7 +229,7 @@ impl Frame {
 struct FrameChange {
     old_pipeline_id: Option<PipelineId>,
     new_pipeline_id: PipelineId,
-    painter_ready: bool,
+    document_ready: bool,
 }
 
 /// An iterator over a frame tree, returning nodes in depth-first order.
@@ -330,6 +334,7 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
                 webgl_paint_tasks: Vec::new(),
                 scheduler_chan: TimerScheduler::start(),
                 child_processes: Vec::new(),
+                document_states: HashMap::new(),
             };
             let namespace_id = constellation.next_pipeline_namespace_id();
             PipelineNamespace::install(namespace_id);
@@ -428,7 +433,7 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
         self.pending_frames.push(FrameChange {
             old_pipeline_id: old_pipeline_id,
             new_pipeline_id: new_pipeline_id,
-            painter_ready: false,
+            document_ready: false,
         });
     }
 
@@ -600,6 +605,11 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
                 debug!("constellation got navigation message from script");
                 self.handle_navigate_msg(pipeline_info, direction);
             }
+            // Notification that the new document is ready to become active
+            Request::Script(FromScriptMsg::ActivateDocument(pipeline_id)) => {
+                debug!("constellation got activate document message");
+                self.handle_activate_document_msg(pipeline_id);
+            }
             Request::Script(FromScriptMsg::MozBrowserEvent(pipeline_id,
                                               subpage_id,
                                               event)) => {
@@ -670,16 +680,16 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
                 debug!("constellation got NodeStatus message");
                 self.compositor_proxy.send(ToCompositorMsg::Status(message));
             }
+            Request::Script(FromScriptMsg::SetDocumentState(pipeline_id, state)) => {
+                debug!("constellation got SetDocumentState message");
+                self.document_states.insert(pipeline_id, state);
+            }
 
 
             // Messages from paint task
 
 
             // Notification that painting has finished and is requesting permission to paint.
-            Request::Paint(FromPaintMsg::Ready(pipeline_id)) => {
-                debug!("constellation got painter ready message");
-                self.handle_painter_ready_msg(pipeline_id);
-            }
             Request::Paint(FromPaintMsg::Failure(Failure { pipeline_id, parent_info })) => {
                 debug!("handling paint failure message from pipeline {:?}, {:?}", pipeline_id, parent_info);
                 self.handle_failure_msg(pipeline_id, parent_info);
@@ -1257,11 +1267,8 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
         }
     }
 
-    fn handle_painter_ready_msg(&mut self, pipeline_id: PipelineId) {
-        debug!("Painter {:?} ready to send paint msg", pipeline_id);
-        // This message could originate from a pipeline in the navigation context or
-        // from a pending frame. The only time that we will grant paint permission is
-        // when the message originates from a pending frame or the current frame.
+    fn handle_activate_document_msg(&mut self, pipeline_id: PipelineId) {
+        debug!("Document ready to activate {:?}", pipeline_id);
 
         // If this pipeline is already part of the current frame tree,
         // we don't need to do anything.
@@ -1275,12 +1282,12 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
             frame_change.new_pipeline_id == pipeline_id
         });
         if let Some(pending_index) = pending_index {
-            self.pending_frames[pending_index].painter_ready = true;
+            self.pending_frames[pending_index].document_ready = true;
         }
 
         // This is a bit complex. We need to loop through pending frames and find
         // ones that can be swapped. A frame can be swapped (enabled) once it is
-        // ready to paint (has painter_ready set), and also has no dependencies
+        // ready to layout (has document_ready set), and also has no dependencies
         // (i.e. the pipeline it is replacing has been enabled and now has a frame).
         // The outer loop is required because any time a pipeline is enabled, that
         // may affect whether other pending frames are now able to be enabled. On the
@@ -1291,7 +1298,7 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
             let waiting_on_dependency = frame_change.old_pipeline_id.map_or(false, |old_pipeline_id| {
                 self.pipeline_to_frame_map.get(&old_pipeline_id).is_none()
             });
-            frame_change.painter_ready && !waiting_on_dependency
+            frame_change.document_ready && !waiting_on_dependency
         }) {
             let frame_change = self.pending_frames.swap_remove(valid_frame_change);
             self.add_or_replace_pipeline_in_frame_tree(frame_change);
@@ -1348,6 +1355,11 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
             return ReadyToSave::NoRootFrame;
         }
 
+        // If there are pending loads, wait for those to complete.
+        if self.pending_frames.len() > 0 {
+            return ReadyToSave::PendingFrames;
+        }
+
         // Step through the current frame tree, checking that the script
         // task is idle, and that the current epoch of the layout task
         // matches what the compositor has painted. If all these conditions
@@ -1371,14 +1383,12 @@ impl<LTF: LayoutTaskFactory, STF: ScriptTaskFactory> Constellation<LTF, STF> {
                 return ReadyToSave::WebFontNotLoaded;
             }
 
-            // Synchronously query the script task for this pipeline
-            // to see if it is idle.
-            let (sender, receiver) = ipc::channel().unwrap();
-            let msg = ConstellationControlMsg::GetCurrentState(sender, frame.current);
-            pipeline.script_chan.send(msg).unwrap();
-            let result = receiver.recv().unwrap();
-            if result == ScriptState::DocumentLoading {
-                return ReadyToSave::DocumentLoading;
+            // See if this pipeline has reached idle script state yet.
+            match self.document_states.get(&frame.current) {
+                Some(&DocumentState::Idle) => {}
+                Some(&DocumentState::Pending) | None => {
+                    return ReadyToSave::DocumentLoading;
+                }
             }
 
             // Check the visible rectangle for this pipeline. If the constellation has received a

components/gfx/paint_task.rs

@@ -212,7 +212,6 @@ pub struct PaintTask<C> {
     layout_to_paint_port: Receiver<LayoutToPaintMsg>,
     chrome_to_paint_port: Receiver<ChromeToPaintMsg>,
     compositor: C,
-    constellation_chan: ConstellationChan<ConstellationMsg>,
 
     /// A channel to the time profiler.
     time_profiler_chan: time::ProfilerChan,
@@ -274,7 +273,6 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                     layout_to_paint_port: layout_to_paint_port,
                     chrome_to_paint_port: chrome_to_paint_port,
                     compositor: compositor,
-                    constellation_chan: constellation_chan,
                     time_profiler_chan: time_profiler_chan,
                     root_paint_layer: None,
                     paint_permission: false,
@@ -326,46 +324,36 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                     self.current_epoch = Some(epoch);
                     self.root_paint_layer = Some(Arc::new(paint_layer));
 
-                    if !self.paint_permission {
-                        debug!("PaintTask: paint ready msg");
-                        let ConstellationChan(ref mut c) = self.constellation_chan;
-                        c.send(ConstellationMsg::Ready(self.id)).unwrap();
-                        continue;
+                    if self.paint_permission {
+                        self.initialize_layers();
                     }
-
-                    self.initialize_layers();
                 }
                 // Inserts a new canvas renderer to the layer map
                 Msg::FromLayout(LayoutToPaintMsg::CanvasLayer(layer_id, canvas_renderer)) => {
                     debug!("Renderer received for canvas with layer {:?}", layer_id);
                     self.canvas_map.insert(layer_id, canvas_renderer);
                 }
                 Msg::FromChrome(ChromeToPaintMsg::Paint(requests, frame_tree_id)) => {
-                    if !self.paint_permission {
-                        debug!("PaintTask: paint ready msg");
-                        let ConstellationChan(ref mut c) = self.constellation_chan;
-                        c.send(ConstellationMsg::Ready(self.id)).unwrap();
-                        continue;
-                    }
-
-                    let mut replies = Vec::new();
-                    for PaintRequest { buffer_requests, scale, layer_id, epoch, layer_kind }
-                          in requests {
-                        if self.current_epoch == Some(epoch) {
-                            self.paint(&mut replies, buffer_requests, scale, layer_id, layer_kind);
-                        } else {
-                            debug!("PaintTask: Ignoring requests with epoch mismatch: {:?} != {:?}",
-                                   self.current_epoch,
-                                   epoch);
-                            self.compositor.ignore_buffer_requests(buffer_requests);
+                    if self.paint_permission && self.root_paint_layer.is_some() {
+                        let mut replies = Vec::new();
+                        for PaintRequest { buffer_requests, scale, layer_id, epoch, layer_kind }
+                              in requests {
+                            if self.current_epoch == Some(epoch) {
+                                self.paint(&mut replies, buffer_requests, scale, layer_id, layer_kind);
+                            } else {
+                                debug!("PaintTask: Ignoring requests with epoch mismatch: {:?} != {:?}",
+                                       self.current_epoch,
+                                       epoch);
+                                self.compositor.ignore_buffer_requests(buffer_requests);
+                            }
                         }
-                    }
 
-                    debug!("PaintTask: returning surfaces");
-                    self.compositor.assign_painted_buffers(self.id,
-                                                           self.current_epoch.unwrap(),
-                                                           replies,
-                                                           frame_tree_id);
+                        debug!("PaintTask: returning surfaces");
+                        self.compositor.assign_painted_buffers(self.id,
+                                                               self.current_epoch.unwrap(),
+                                                               replies,
+                                                               frame_tree_id);
+                    }
                 }
                 Msg::FromChrome(ChromeToPaintMsg::PaintPermissionGranted) => {
                     self.paint_permission = true;

components/msg/constellation_msg.rs

@@ -244,7 +244,6 @@ pub enum MouseButton {
 /// Messages from the paint task to the constellation.
 #[derive(Deserialize, Serialize)]
 pub enum PaintMsg {
-    Ready(PipelineId),
     Failure(Failure),
 }
 
@@ -256,6 +255,15 @@ pub enum AnimationState {
     NoAnimationCallbacksPresent,
 }
 
+/// Used to determine if a script has any pending asynchronous activity.
+#[derive(Copy, Clone, Debug, PartialEq, Deserialize, Serialize)]
+pub enum DocumentState {
+    /// The document has been loaded and is idle.
+    Idle,
+    /// The document is either loading or waiting on an event.
+    Pending,
+}
+
 // https://developer.mozilla.org/en-US/docs/Web/API/Using_the_Browser_API#Events
 #[derive(Deserialize, Serialize)]
 pub enum MozBrowserEvent {

components/script/dom/window.rs

@@ -6,7 +6,7 @@ use app_units::Au;
 use devtools_traits::{ScriptToDevtoolsControlMsg, TimelineMarker, TimelineMarkerType, WorkerId};
 use dom::bindings::callback::ExceptionHandling;
 use dom::bindings::cell::DOMRefCell;
-use dom::bindings::codegen::Bindings::DocumentBinding::DocumentMethods;
+use dom::bindings::codegen::Bindings::DocumentBinding::{DocumentMethods, DocumentReadyState};
 use dom::bindings::codegen::Bindings::EventHandlerBinding::{EventHandlerNonNull, OnErrorEventHandlerNonNull};
 use dom::bindings::codegen::Bindings::FunctionBinding::Function;
 use dom::bindings::codegen::Bindings::WindowBinding::{ScrollBehavior, ScrollToOptions};
@@ -44,7 +44,8 @@ use layout_interface::{LayoutChan, LayoutRPC, Msg, Reflow, ReflowGoal, ReflowQue
 use libc;
 use msg::ParseErrorReporter;
 use msg::compositor_msg::{LayerId, ScriptToCompositorMsg};
-use msg::constellation_msg::{ConstellationChan, LoadData, PipelineId, SubpageId, WindowSizeData};
+use msg::constellation_msg::{ConstellationChan, DocumentState, LoadData};
+use msg::constellation_msg::{PipelineId, SubpageId, WindowSizeData};
 use msg::webdriver_msg::{WebDriverJSError, WebDriverJSResult};
 use net_traits::ResourceTask;
 use net_traits::image_cache_task::{ImageCacheChan, ImageCacheTask};
@@ -990,16 +991,43 @@ impl Window {
     ///
     /// TODO(pcwalton): Only wait for style recalc, since we have off-main-thread layout.
     pub fn reflow(&self, goal: ReflowGoal, query_type: ReflowQueryType, reason: ReflowReason) {
-        if query_type == ReflowQueryType::NoQuery && !self.Document().needs_reflow() {
+        let for_display = query_type == ReflowQueryType::NoQuery;
+
+        if !for_display || self.Document().needs_reflow() {
+            self.force_reflow(goal, query_type, reason);
+
+            // If window_size is `None`, we don't reflow, so the document stays dirty.
+            // Otherwise, we shouldn't need a reflow immediately after a reflow.
+            assert!(!self.Document().needs_reflow() || self.window_size.get().is_none());
+        } else {
             debug!("Document doesn't need reflow - skipping it (reason {:?})", reason);
-            return
         }
 
-        self.force_reflow(goal, query_type, reason);
-
-        // If window_size is `None`, we don't reflow, so the document stays dirty.
-        // Otherwise, we shouldn't need a reflow immediately after a reflow.
-        assert!(!self.Document().needs_reflow() || self.window_size.get().is_none());
+        // If writing a screenshot, check if the script has reached a state
+        // where it's safe to write the image. This means that:
+        // 1) The reflow is for display (otherwise it could be a query)
+        // 2) The html element doesn't contain the 'reftest-wait' class
+        // 3) The load event has fired.
+        // When all these conditions are met, notify the constellation
+        // that this pipeline is ready to write the image (from the script task
+        // perspective at least).
+        if opts::get().output_file.is_some() && for_display {
+            let document = self.Document();
+
+            // Checks if the html element has reftest-wait attribute present.
+            // See http://testthewebforward.org/docs/reftests.html
+            let html_element = document.GetDocumentElement();
+            let reftest_wait = html_element.map_or(false, |elem| {
+                elem.has_class(&Atom::from("reftest-wait"))
+            });
+
+            let ready_state = document.ReadyState();
+
+            if ready_state == DocumentReadyState::Complete && !reftest_wait {
+                let event = ConstellationMsg::SetDocumentState(self.id, DocumentState::Idle);
+                self.constellation_chan().0.send(event).unwrap();
+            }
+        }
     }
 
     pub fn layout(&self) -> &LayoutRPC {