Bug 1524427 - Fix panic when clip mask is supplied with non-existent image key. r=kvark
authorGlenn Watson <github@intuitionlibrary.com>
Thu, 09 May 2019 21:23:58 +0000
changeset 532146 d8fa440167f565c4553fc09e55caceb6544e2d89
parent 532145 dcf3a023d7d08907c6033a8c61c601a6b06e620a
child 532147 9cc74682a4300f035401fabbed830ebecf6e1db7
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskvark
bugs1524427
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1524427 - Fix panic when clip mask is supplied with non-existent image key. r=kvark In some cases, Gecko supplies a display item with an image clip mask where the image itself does not exist in the resource cache. In these cases, WR would skip requesting the image, but would still try to fetch the image info during batching, causing a panic. This patch skips adding clip items to the batching pass if the image mask does not exist. It also adds support to wrench for a crash test when the image mask is invalid. Differential Revision: https://phabricator.services.mozilla.com/D30560
gfx/wr/webrender/src/clip.rs
gfx/wr/wrench/reftests/mask/missing-mask-ref.yaml
gfx/wr/wrench/reftests/mask/missing-mask.yaml
gfx/wr/wrench/reftests/mask/reftest.list
gfx/wr/wrench/src/yaml_frame_reader.rs
--- a/gfx/wr/webrender/src/clip.rs
+++ b/gfx/wr/webrender/src/clip.rs
@@ -267,17 +267,17 @@ impl ClipNodeInfo {
     fn create_instance(
         &self,
         node: &ClipNode,
         clipped_rect: &LayoutRect,
         gpu_cache: &mut GpuCache,
         resource_cache: &mut ResourceCache,
         clip_scroll_tree: &ClipScrollTree,
         request_resources: bool,
-    ) -> ClipNodeInstance {
+    ) -> Option<ClipNodeInstance> {
         // Calculate some flags that are required for the segment
         // building logic.
         let mut flags = match self.conversion {
             ClipSpaceConversion::Local => {
                 ClipNodeFlags::SAME_SPATIAL_NODE | ClipNodeFlags::SAME_COORD_SYSTEM
             }
             ClipSpaceConversion::ScaleOffset(..) => {
                 ClipNodeFlags::SAME_COORD_SYSTEM
@@ -353,26 +353,31 @@ impl ClipNodeInfo {
                                 tile_rect: tile.rect,
                             });
                         }
                     }
                     visible_tiles = Some(mask_tiles);
                 } else if request_resources {
                     resource_cache.request_image(request, gpu_cache);
                 }
+            } else {
+                // If the supplied image key doesn't exist in the resource cache,
+                // skip the clip node since there is nothing to mask with.
+                warn!("Clip mask with missing image key {:?}", request.key);
+                return None;
             }
         }
 
-        ClipNodeInstance {
+        Some(ClipNodeInstance {
             handle: self.handle,
             flags,
             spatial_node_index: self.spatial_node_index,
             local_pos: self.local_pos,
             visible_tiles,
-        }
+        })
     }
 }
 
 impl ClipNode {
     pub fn update(
         &mut self,
         gpu_cache: &mut GpuCache,
         device_pixel_scale: DevicePixelScale,
@@ -673,46 +678,46 @@ impl ClipStore {
 
                     // TODO(gw): Ensure this only runs once on each node per frame?
                     node.update(
                         gpu_cache,
                         device_pixel_scale,
                     );
 
                     // Create the clip node instance for this clip node
-                    let instance = node_info.create_instance(
+                    if let Some(instance) = node_info.create_instance(
                         node,
                         &local_bounding_rect,
                         gpu_cache,
                         resource_cache,
                         clip_scroll_tree,
                         request_resources,
-                    );
+                    ) {
+                        // As a special case, a partial accept of a clip rect that is
+                        // in the same coordinate system as the primitive doesn't need
+                        // a clip mask. Instead, it can be handled by the primitive
+                        // vertex shader as part of the local clip rect. This is an
+                        // important optimization for reducing the number of clip
+                        // masks that are allocated on common pages.
+                        needs_mask |= match node.item {
+                            ClipItem::Rectangle(_, ClipMode::ClipOut) |
+                            ClipItem::RoundedRectangle(..) |
+                            ClipItem::Image { .. } |
+                            ClipItem::BoxShadow(..) => {
+                                true
+                            }
 
-                    // As a special case, a partial accept of a clip rect that is
-                    // in the same coordinate system as the primitive doesn't need
-                    // a clip mask. Instead, it can be handled by the primitive
-                    // vertex shader as part of the local clip rect. This is an
-                    // important optimization for reducing the number of clip
-                    // masks that are allocated on common pages.
-                    needs_mask |= match node.item {
-                        ClipItem::Rectangle(_, ClipMode::ClipOut) |
-                        ClipItem::RoundedRectangle(..) |
-                        ClipItem::Image { .. } |
-                        ClipItem::BoxShadow(..) => {
-                            true
-                        }
+                            ClipItem::Rectangle(_, ClipMode::Clip) => {
+                                !instance.flags.contains(ClipNodeFlags::SAME_COORD_SYSTEM)
+                            }
+                        };
 
-                        ClipItem::Rectangle(_, ClipMode::Clip) => {
-                            !instance.flags.contains(ClipNodeFlags::SAME_COORD_SYSTEM)
-                        }
-                    };
-
-                    // Store this in the index buffer for this clip chain instance.
-                    self.clip_node_instances.push(instance);
+                        // Store this in the index buffer for this clip chain instance.
+                        self.clip_node_instances.push(instance);
+                    }
                 }
             }
         }
 
         // Get the range identifying the clip nodes in the index buffer.
         let clips_range = ClipNodeRange {
             first: first_clip_node_index,
             count: self.clip_node_instances.len() as u32 - first_clip_node_index,
new file mode 100644
--- /dev/null
+++ b/gfx/wr/wrench/reftests/mask/missing-mask-ref.yaml
@@ -0,0 +1,7 @@
+# Don't crash when supplied an invalid image key for the mask!
+---
+root:
+  items:
+    - type: rect
+      bounds: [0, 0, 35, 35]
+      color: blue
new file mode 100644
--- /dev/null
+++ b/gfx/wr/wrench/reftests/mask/missing-mask.yaml
@@ -0,0 +1,14 @@
+# Don't crash when supplied an invalid image key for the mask!
+---
+root:
+  items:
+    - type: clip
+      bounds: [0, 0, 95, 88]
+      image-mask:
+        image: invalid
+        rect: [0, 0, 35, 35]
+        repeat: false
+      items:
+      - type: rect
+        bounds: [0, 0, 95, 88]
+        color: blue
--- a/gfx/wr/wrench/reftests/mask/reftest.list
+++ b/gfx/wr/wrench/reftests/mask/reftest.list
@@ -8,8 +8,9 @@
 platform(linux,mac) == rounded-corners.yaml rounded-corners.png
 != mask.yaml out-of-bounds.yaml
 platform(linux,mac) fuzzy(1,8750) color_targets(2) alpha_targets(1) == mask-atomicity.yaml mask-atomicity-ref.yaml
 platform(linux,mac) fuzzy(1,8750) == mask-atomicity-tiling.yaml mask-atomicity-ref.yaml
 platform(linux,mac) == mask-perspective.yaml mask-perspective.png
 == fuzzy(1,6) mask-perspective-tiling.yaml mask-perspective.yaml
 platform(linux,mac) == checkerboard.yaml checkerboard.png
 == checkerboard.yaml checkerboard-tiling.yaml
+== missing-mask.yaml missing-mask-ref.yaml
--- a/gfx/wr/wrench/src/yaml_frame_reader.rs
+++ b/gfx/wr/wrench/src/yaml_frame_reader.rs
@@ -694,31 +694,34 @@ impl YamlFrameReader {
             })
     }
 
     fn to_image_mask(&mut self, item: &Yaml, wrench: &mut Wrench) -> Option<ImageMask> {
         if item.as_hash().is_none() {
             return None;
         }
 
-        let file = match item["image"].as_str() {
+        let tiling = item["tile-size"].as_i64();
+
+        let (image_key, image_dims) = match item["image"].as_str() {
             Some(filename) => {
-                let mut file = self.aux_dir.clone();
-                file.push(filename);
-                file
+                if filename == "invalid" {
+                    (ImageKey::DUMMY, LayoutSize::new(100.0, 100.0))
+                } else {
+                    let mut file = self.aux_dir.clone();
+                    file.push(filename);
+                    self.add_or_get_image(&file, tiling, wrench)
+                }
             }
             None => {
                 warn!("No image provided for the image-mask!");
                 return None;
             }
         };
 
-        let tiling = item["tile-size"].as_i64();
-        let (image_key, image_dims) =
-            self.add_or_get_image(&file, tiling, wrench);
         let image_rect = item["rect"]
             .as_rect()
             .unwrap_or(LayoutRect::new(LayoutPoint::zero(), image_dims));
         let image_repeat = item["repeat"].as_bool().expect("Expected boolean");
         Some(ImageMask {
             image: image_key,
             rect: image_rect,
             repeat: image_repeat,