Auto merge of #8612 - glennw:pending-frames, r=jdm

Add test to constellation to avoid writing reftest image if there are pending frames.

This changes several tests that contain <iframe></iframe> from FAIL to TIMEOUT. This is correct
since there is a bug that prevents these iframes from ever rendering.

~~~There are also a few previous FAILs that changed to OK. These may be intermittents or they
may genuinely be fixed by this change.~~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8612)
<!-- Reviewable:end -->

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,15 +1383,13 @@ 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 {
+            // 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
             // size for the pipeline, then its painting should be up to date. If the constellation

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,28 +324,17 @@ 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();
                     }
+                }
                 // 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;
-                    }
-
+                    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 {
@@ -367,6 +354,7 @@ impl<C> PaintTask<C> where C: PaintListener + Send + 'static {
                                                                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() {
-            debug!("Document doesn't need reflow - skipping it (reason {:?})", reason);
-            return
-        }
+        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);
+        }
+
+        // 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 {

components/script/script_task.rs

@@ -80,7 +80,7 @@ use profile_traits::time::{self, ProfilerCategory, profile};
 use script_traits::CompositorEvent::{KeyEvent, MouseButtonEvent, MouseMoveEvent, ResizeEvent};
 use script_traits::CompositorEvent::{TouchEvent};
 use script_traits::{CompositorEvent, ConstellationControlMsg, InitialScriptState, NewLayoutInfo};
-use script_traits::{OpaqueScriptLayoutChannel, ScriptMsg as ConstellationMsg, ScriptState};
+use script_traits::{OpaqueScriptLayoutChannel, ScriptMsg as ConstellationMsg};
 use script_traits::{ScriptTaskFactory, TimerEvent, TimerEventRequest, TimerSource};
 use script_traits::{TouchEventType, TouchId};
 use std::any::Any;
@@ -97,7 +97,6 @@ use std::result::Result;
 use std::sync::atomic::{Ordering, AtomicBool};
 use std::sync::mpsc::{Receiver, Select, Sender, channel};
 use std::sync::{Arc, Mutex};
-use string_cache::Atom;
 use time::{Tm, now};
 use url::{Url, UrlParser};
 use util::opts;
@@ -1014,10 +1013,6 @@ impl ScriptTask {
             ConstellationControlMsg::DispatchFrameLoadEvent {
                 target: pipeline_id, parent: containing_id } =>
                 self.handle_frame_load_event(containing_id, pipeline_id),
-            ConstellationControlMsg::GetCurrentState(sender, pipeline_id) => {
-                let state = self.handle_get_current_state(pipeline_id);
-                sender.send(state).unwrap();
-            },
             ConstellationControlMsg::ReportCSSError(pipeline_id, filename, line, column, msg) =>
                 self.handle_css_error_reporting(pipeline_id, filename, line, column, msg),
         }
@@ -1173,40 +1168,6 @@ impl ScriptTask {
         panic!("Page rect message sent to nonexistent pipeline");
     }
 
-    /// Get the current state of a given pipeline.
-    fn handle_get_current_state(&self, pipeline_id: PipelineId) -> ScriptState {
-        // Check if the main page load is still pending
-        let loads = self.incomplete_loads.borrow();
-        if let Some(_) = loads.iter().find(|load| load.pipeline_id == pipeline_id) {
-            return ScriptState::DocumentLoading;
-        }
-
-        // If not in pending loads, the page should exist by now.
-        let page = self.root_page();
-        let page = page.find(pipeline_id).expect("GetCurrentState sent to nonexistent pipeline");
-        let doc = page.document();
-
-        // Check if document load event has fired. If the document load
-        // event has fired, this also guarantees that the first reflow
-        // has been kicked off. Since the script task does a join with
-        // layout, this ensures there are no race conditions that can occur
-        // between load completing and the first layout completing.
-        let load_pending = doc.ReadyState() != DocumentReadyState::Complete;
-        if load_pending {
-            return ScriptState::DocumentLoading;
-        }
-
-        // Checks if the html element has reftest-wait attribute present.
-        // See http://testthewebforward.org/docs/reftests.html
-        let html_element = doc.GetDocumentElement();
-        let reftest_wait = html_element.map_or(false, |elem| elem.has_class(&Atom::from("reftest-wait")));
-        if reftest_wait {
-            return ScriptState::DocumentLoading;
-        }
-
-        ScriptState::DocumentLoaded
-    }
-
     fn handle_new_layout(&self, new_layout_info: NewLayoutInfo) {
         let NewLayoutInfo {
             containing_pipeline_id,
@@ -1715,6 +1676,9 @@ impl ScriptTask {
             window: JS::from_rooted(&window),
         }));
 
+        let ConstellationChan(ref chan) = self.constellation_chan;
+        chan.send(ConstellationMsg::ActivateDocument(incomplete.pipeline_id)).unwrap();
+
         let is_javascript = incomplete.url.scheme == "javascript";
         let parse_input = if is_javascript {
             use url::percent_encoding::percent_decode_to;

components/script_traits/lib.rs

@@ -97,15 +97,6 @@ pub struct NewLayoutInfo {
     pub content_process_shutdown_chan: IpcSender<()>,
 }
 
-/// Used to determine if a script has any pending asynchronous activity.
-#[derive(Copy, Clone, Debug, PartialEq, Deserialize, Serialize)]
-pub enum ScriptState {
-    /// The document has been loaded.
-    DocumentLoaded,
-    /// The document is still loading.
-    DocumentLoading,
-}
-
 /// Messages sent from the constellation or layout to the script task.
 #[derive(Deserialize, Serialize)]
 pub enum ConstellationControlMsg {
@@ -142,8 +133,6 @@ pub enum ConstellationControlMsg {
     /// Notifies the script task that a new Web font has been loaded, and thus the page should be
     /// reflowed.
     WebFontLoaded(PipelineId),
-    /// Get the current state of the script task for a given pipeline.
-    GetCurrentState(IpcSender<ScriptState>, PipelineId),
     /// Cause a `load` event to be dispatched at the appropriate frame element.
     DispatchFrameLoadEvent {
         /// The pipeline that has been marked as loaded.

components/script_traits/script_msg.rs

@@ -6,7 +6,7 @@ use canvas_traits::CanvasMsg;
 use euclid::point::Point2D;
 use euclid::size::Size2D;
 use ipc_channel::ipc::IpcSender;
-use msg::constellation_msg::{AnimationState, IframeLoadInfo, NavigationDirection};
+use msg::constellation_msg::{AnimationState, DocumentState, IframeLoadInfo, NavigationDirection};
 use msg::constellation_msg::{Failure, MozBrowserEvent, PipelineId};
 use msg::constellation_msg::{LoadData, SubpageId};
 use msg::constellation_msg::{MouseButton, MouseEventType};
@@ -66,4 +66,8 @@ pub enum ScriptMsg {
     SetCursor(Cursor),
     /// Notifies the constellation that the viewport has been constrained in some manner
     ViewportConstrained(PipelineId, ViewportConstraints),
+    /// Mark a new document as active
+    ActivateDocument(PipelineId),
+    /// Set the document state for a pipeline (used by screenshot / reftests)
+    SetDocumentState(PipelineId, DocumentState),
 }

tests/wpt/metadata-css/css21_dev/html4/root-box-003.htm.ini

@@ -1,3 +1,3 @@
 [root-box-003.htm]
   type: reftest
-  expected: TIMEOUT
+  expected: FAIL