script: Add some workarounds for image cache task races

components/layout/layout_task.rs

@@ -590,6 +590,8 @@ impl LayoutTask {
                                        requested_node: TrustedNodeAddress,
                                        layout_root: &mut FlowRef,
                                        rw_data: &mut RWGuard<'a>) {
+        // FIXME(pcwalton): This has not been updated to handle the stacking context relative
+        // stuff. So the position is wrong in most cases.
         let requested_node: OpaqueNode = OpaqueNodeMethods::from_script_node(requested_node);
         let mut iterator = UnioningFragmentBoundsIterator::new(requested_node);
         sequential::iterate_through_flow_tree_fragment_bounds(layout_root, &mut iterator);
@@ -600,6 +602,8 @@ impl LayoutTask {
                                          requested_node: TrustedNodeAddress,
                                          layout_root: &mut FlowRef,
                                          rw_data: &mut RWGuard<'a>) {
+        // FIXME(pcwalton): This has not been updated to handle the stacking context relative
+        // stuff. So the position is wrong in most cases.
         let requested_node: OpaqueNode = OpaqueNodeMethods::from_script_node(requested_node);
         let mut iterator = CollectingFragmentBoundsIterator::new(requested_node);
         sequential::iterate_through_flow_tree_fragment_bounds(layout_root, &mut iterator);

components/net/image/holder.rs

@@ -7,7 +7,6 @@ use image_cache_task::{ImageReady, ImageNotReady, ImageFailed};
 use local_image_cache::LocalImageCache;
 
 use geom::size::Size2D;
-use std::mem;
 use sync::{Arc, Mutex};
 use url::Url;
 
@@ -87,24 +86,13 @@ impl<NodeAddress: Send> ImageHolder<NodeAddress> {
                 local_image_cache.get_image(node_address, &self.url)
             };
             match port.recv() {
-                ImageReady(image) => {
+                ImageReady(image) => self.image = Some(image),
-                    self.image = Some(image);
+                ImageNotReady => debug!("image not ready for {:s}", self.url.serialize()),
-                }
+                ImageFailed => debug!("image decoding failed for {:s}", self.url.serialize()),
-                ImageNotReady => {
-                    debug!("image not ready for {:s}", self.url.serialize());
-                }
-                ImageFailed => {
-                    debug!("image decoding failed for {:s}", self.url.serialize());
-                }
             }
         }
 
-        // Clone isn't pure so we have to swap out the mutable image option
+        return self.image.clone();
-        let image = mem::replace(&mut self.image, None);
-        let result = image.clone();
-        mem::replace(&mut self.image, image);
-
-        return result;
     }
 
     pub fn url(&self) -> &Url {

components/script/dom/htmlimageelement.rs

@@ -15,7 +15,7 @@ use dom::element::{Element, HTMLImageElementTypeId};
 use dom::element::AttributeHandlers;
 use dom::eventtarget::{EventTarget, NodeTargetTypeId};
 use dom::htmlelement::HTMLElement;
-use dom::node::{Node, ElementNodeTypeId, NodeHelpers, window_from_node};
+use dom::node::{Node, ElementNodeTypeId, NodeHelpers, OtherNodeDamage, window_from_node};
 use dom::virtualmethods::VirtualMethods;
 use servo_net::image_cache_task;
 use servo_util::geometry::to_px;
@@ -112,7 +112,14 @@ impl<'a> HTMLImageElementMethods for JSRef<'a, HTMLImageElement> {
     }
 
     fn Width(self) -> u32 {
+        // FIXME(pcwalton): This is a really nasty thing to do, but the interaction between the
+        // image cache task, the reflow messages that it sends to us via layout, and the image
+        // holders seem to just plain be racy, and this works around it by ensuring that we
+        // recreate the flow (picking up image changes on the way). The image cache task needs a
+        // rewrite to modern Rust.
         let node: JSRef<Node> = NodeCast::from_ref(self);
+        node.dirty(OtherNodeDamage);
+
         let rect = node.get_bounding_content_box();
         to_px(rect.size.width) as u32
     }
@@ -123,7 +130,14 @@ impl<'a> HTMLImageElementMethods for JSRef<'a, HTMLImageElement> {
     }
 
     fn Height(self) -> u32 {
+        // FIXME(pcwalton): This is a really nasty thing to do, but the interaction between the
+        // image cache task, the reflow messages that it sends to us via layout, and the image
+        // holders seem to just plain be racy, and this works around it by ensuring that we
+        // recreate the flow (picking up image changes on the way). The image cache task needs a
+        // rewrite to modern Rust.
         let node: JSRef<Node> = NodeCast::from_ref(self);
+        node.dirty(OtherNodeDamage);
+
         let rect = node.get_bounding_content_box();
         to_px(rect.size.height) as u32
     }

components/script/script_task.rs

@@ -55,7 +55,7 @@ use servo_net::resource_task::{ResourceTask, Load};
 use servo_net::resource_task::LoadData as NetLoadData;
 use servo_net::storage_task::StorageTask;
 use servo_util::geometry::to_frac_px;
-use servo_util::smallvec::{SmallVec1, SmallVec};
+use servo_util::smallvec::SmallVec;
 use servo_util::task::spawn_named_with_send_on_failure;
 use servo_util::task_state;
 
@@ -72,7 +72,6 @@ use url::Url;
 
 use libc::size_t;
 use std::any::{Any, AnyRefExt};
-use std::collections::HashSet;
 use std::comm::{channel, Sender, Receiver, Select};
 use std::fmt::{mod, Show};
 use std::mem::replace;
@@ -478,8 +477,6 @@ impl ScriptTask {
             }
         };
 
-        let mut needs_reflow = HashSet::new();
-
         // Squash any pending resize and reflow events in the queue.
         loop {
             match event {
@@ -494,14 +491,12 @@ impl ScriptTask {
                     let page = page.find(id).expect("resize sent to nonexistent pipeline");
                     page.resize_event.set(Some(size));
                 }
-                FromConstellation(SendEventMsg(id, ReflowEvent(_))) => {
-                    needs_reflow.insert(id);
-                }
                 FromConstellation(ViewportMsg(id, rect)) => {
                     let page = self.page.borrow_mut();
                     let inner_page = page.find(id).expect("Page rect message sent to nonexistent pipeline");
                     if inner_page.set_page_clip_rect_with_new_viewport(rect) {
-                        needs_reflow.insert(id);
+                        let page = get_page(&*self.page.borrow(), id);
+                        self.force_reflow(&*page);
                     }
                 }
                 _ => {
@@ -535,11 +530,6 @@ impl ScriptTask {
             }
         }
 
-        // Now process any pending reflows.
-        for id in needs_reflow.into_iter() {
-            self.handle_event(id, ReflowEvent(SmallVec1::new()));
-        }
-
         true
     }
 
@@ -906,9 +896,27 @@ impl ScriptTask {
               self.handle_resize_event(pipeline_id, new_size);
             }
 
-            // FIXME(pcwalton): This reflows the entire document and is not incremental-y.
+            ReflowEvent(nodes) => {
-            ReflowEvent(to_dirty) => {
+                // FIXME(pcwalton): This event seems to only be used by the image cache task, and
-              self.handle_reflow_event(pipeline_id, to_dirty);
+                // the interaction between it and the image holder is really racy. I think that, in
+                // order to fix this race, we need to rewrite the image cache task to make the
+                // image holder responsible for the lifecycle of image loading instead of having
+                // the image holder and layout task both be observers. Then we can have the DOM
+                // image element observe the state of the image holder and have it send reflows
+                // via the normal dirtying mechanism, and ultimately remove this event.
+                //
+                // See the implementation of `Width()` and `Height()` in `HTMLImageElement` for
+                // fallout of this problem.
+                for node in nodes.iter() {
+                    let node_to_dirty = node::from_untrusted_node_address(self.js_runtime.ptr,
+                                                                          *node).root();
+                    let page = get_page(&*self.page.borrow(), pipeline_id);
+                    let frame = page.frame();
+                    let document = frame.as_ref().unwrap().document.root();
+                    document.content_changed(*node_to_dirty, OtherNodeDamage);
+                }
+
+                self.handle_reflow_event(pipeline_id);
             }
 
             ClickEvent(_button, point) => {
@@ -1068,9 +1076,8 @@ impl ScriptTask {
         }
     }
 
-    fn handle_reflow_event(&self, pipeline_id: PipelineId, to_dirty: SmallVec1<UntrustedNodeAddress>) {
+    fn handle_reflow_event(&self, pipeline_id: PipelineId) {
         debug!("script got reflow event");
-        assert_eq!(to_dirty.len(), 0);
         let page = get_page(&*self.page.borrow(), pipeline_id);
         let frame = page.frame();
         if frame.is_some() {

tests/content/test_getBoundingClientRect.html

@@ -3,8 +3,9 @@
 <script src="harness.js"></script>
 <style>
 div {
-    margin-top:  100px;
+    position: relative;
-    margin-left: 100px;
+    top:  100px;
+    left: 100px;
     width: 100px;
     height: 100px;
 }