Make save-restore foolproof

Author: Gankra

webrender_api/src/display_list.rs

@@ -51,14 +51,6 @@ impl<T> ItemRange<T> {
     }
 }
 
-#[repr(C)]
-#[derive(Debug)]
-pub struct DisplayListBuilderSaveState {
-    dl_len: usize,
-    clip_stack_len: usize,
-    next_clip_id: u64,
-}
-
 /// A display list.
 #[derive(Clone, Default)]
 pub struct BuiltDisplayList {
@@ -526,6 +518,13 @@ fn serialize_fast<T: Serialize>(vec: &mut Vec<u8>, e: &T) {
     bincode::serialize_into(&mut UnsafeVecWriter(vec), e, bincode::Infinite).unwrap();
 }
 
+#[derive(Clone, Debug)]
+pub struct SaveState {
+    dl_len: usize,
+    clip_stack_len: usize,
+    next_clip_id: u64,
+}
+
 #[derive(Clone)]
 pub struct DisplayListBuilder {
     pub data: Vec<u8>,
@@ -537,6 +536,7 @@ pub struct DisplayListBuilder {
     /// The size of the content of this display list. This is used to allow scrolling
     /// outside the bounds of the display list items themselves.
     content_size: LayoutSize,
+    save_state: Option<SaveState>,
 }
 
 impl DisplayListBuilder {
@@ -563,23 +563,41 @@ impl DisplayListBuilder {
             next_clip_id: FIRST_CLIP_ID,
             builder_start_time: start_time,
             content_size,
+            save_state: None,
         }
     }
 
-    pub fn save(&self) -> DisplayListBuilderSaveState {
-        DisplayListBuilderSaveState {
+    /// Saves the current display list state, so it may be `restore()`'d.
+    ///
+    /// # Conditions:
+    /// 
+    /// * Doesn't support popping clips that were pushed before the save.
+    /// * Doesn't support nested saves.
+    /// * Must call `clear_save()` if the restore becomes unnecessary.
+    pub fn save(&mut self) {
+        assert!(self.save_state.is_none(), "DisplayListBuilder doesn't support nested saves");
+
+        self.save_state = Some(SaveState {
             clip_stack_len: self.clip_stack.len(),
             dl_len: self.data.len(),
             next_clip_id: self.next_clip_id,
-        }
+        });
     }
 
-    pub fn restore(&mut self, state: DisplayListBuilderSaveState) {
+    /// Restores the state of the builder to when `save()` was last called.
+    pub fn restore(&mut self) {
+        let state = self.save_state.take().expect("No save to restore DisplayListBuilder from");
+
         self.clip_stack.truncate(state.clip_stack_len);
         self.data.truncate(state.dl_len);
         self.next_clip_id = state.next_clip_id;
     }
 
+    /// Discards the builder's save (indicating the attempted operation was sucessful).
+    pub fn clear_save(&mut self) {
+        self.save_state.take().expect("No save to clear in DisplayListBuilder");
+    }
+
     pub fn print_display_list(&mut self) {
         let mut temp = BuiltDisplayList::default();
         ::std::mem::swap(&mut temp.data, &mut self.data);
@@ -1136,6 +1154,10 @@ impl DisplayListBuilder {
 
     pub fn pop_clip_id(&mut self) {
         self.clip_stack.pop();
+        if let Some(save_state) = self.save_state.as_ref() {
+            assert!(self.clip_stack.len() >= save_state.clip_stack_len,
+                    "Cannot pop clips that were pushed before the DisplayListBuilder save.");
+        }
         assert!(self.clip_stack.len() > 0);
     }
 
@@ -1155,6 +1177,8 @@ impl DisplayListBuilder {
     }
 
     pub fn finalize(self) -> (PipelineId, LayoutSize, BuiltDisplayList) {
+        assert!(self.save_state.is_none(), "Finalized DisplayListBuilder with a pending save");
+
         let end_time = precise_time_ns();
 
 

wrench/src/rawtest.rs

@@ -216,7 +216,7 @@ impl<'a> RawtestHarness<'a> {
                               ColorF::new(0.0, 0.0, 1.0, 1.0));
 
             if should_try_and_fail {
-                let save_state = builder.save();
+                builder.save();
                 let clip = builder.define_clip(None, rect(80., 80., 90., 90.),
                                            None::<ComplexClipRegion>, None);
                 builder.push_clip_id(clip);
@@ -231,16 +231,21 @@ impl<'a> RawtestHarness<'a> {
                 builder.push_line(&PrimitiveInfo::new(rect(100., 100., 100., 100.)),
                                   110., 110., 160., LineOrientation::Horizontal, 2.0,
                                   ColorF::new(0.0, 0.0, 0.0, 1.0), LineStyle::Solid);
-                builder.restore(save_state);
+                builder.restore();
             }
 
-            let clip = builder.define_clip(None, rect(80., 80., 100., 100.),
-                                           None::<ComplexClipRegion>, None);
-            builder.push_clip_id(clip);
-            builder.push_rect(&PrimitiveInfo::new(rect(150., 150., 100., 100.)),
-                              ColorF::new(0.0, 0.0, 1.0, 1.0));
+            {
+                builder.save();
+                let clip = builder.define_clip(None, rect(80., 80., 100., 100.),
+                                               None::<ComplexClipRegion>, None);
+                builder.push_clip_id(clip);
+                builder.push_rect(&PrimitiveInfo::new(rect(150., 150., 100., 100.)),
+                                  ColorF::new(0.0, 0.0, 1.0, 1.0));
+
+                builder.pop_clip_id();
+                builder.clear_save();
+            }
 
-            builder.pop_clip_id();
             builder.pop_clip_id();
 
             self.wrench.api.set_display_list(