From 329c8f2fc14ebfe773f2283694087ecd691cb9f3 Mon Sep 17 00:00:00 2001
From: Andrew Farkas <6060305+HactarCE@users.noreply.github.com>
Date: Tue, 7 Jan 2025 02:37:23 -0500
Subject: [PATCH] Fix panic due to non-total ordering in
 `Area::compare_order()` (#5569)

[Area::compare_order()](https://github.com/emilk/egui/blob/ee4ab08c8a208f4044a8c571326c9414e7a1c8a6/crates/egui/src/memory/mod.rs#L1174-L1183)
is not a total ordering. If three layers A, B, and C have the same
`order` field but only A and B are present in `order_map`, then `A==C`
and `B==C` but `A!=C`. This can cause a panic in the stdlib sort
function, and does in [my
app](https://github.com/HactarCE/Hyperspeedcube/tree/v2.0) although it's
very difficult to reproduce.

* [x] I have self-reviewed this PR and run `./scripts/check.sh`
* [x] I have followed the instructions in the PR template
---
 crates/egui/src/memory/mod.rs | 56 +++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 5 deletions(-)

diff --git a/crates/egui/src/memory/mod.rs b/crates/egui/src/memory/mod.rs
index 976ad2d95ef..2c669f0ffd8 100644
--- a/crates/egui/src/memory/mod.rs
+++ b/crates/egui/src/memory/mod.rs
@@ -1175,17 +1175,19 @@ impl Areas {
     ///
     /// May return [`std::cmp::Ordering::Equal`] if the layers are not in the order list.
     pub(crate) fn compare_order(&self, a: LayerId, b: LayerId) -> std::cmp::Ordering {
-        if let (Some(a), Some(b)) = (self.order_map.get(&a), self.order_map.get(&b)) {
-            a.cmp(b)
-        } else {
-            a.order.cmp(&b.order)
+        // Sort by layer `order` first and use `order_map` to resolve disputes.
+        // If `order_map` only contains one layer ID, then the other one will be
+        // lower because `None < Some(x)`.
+        match a.order.cmp(&b.order) {
+            std::cmp::Ordering::Equal => self.order_map.get(&a).cmp(&self.order_map.get(&b)),
+            cmp => cmp,
         }
     }
 
     pub(crate) fn set_state(&mut self, layer_id: LayerId, state: area::AreaState) {
         self.visible_areas_current_frame.insert(layer_id);
         self.areas.insert(layer_id.id, state);
-        if !self.order.iter().any(|x| *x == layer_id) {
+        if !self.order.contains(&layer_id) {
             self.order.push(layer_id);
         }
     }
@@ -1351,3 +1353,47 @@ fn memory_impl_send_sync() {
     fn assert_send_sync<T: Send + Sync>() {}
     assert_send_sync::<Memory>();
 }
+
+#[test]
+fn order_map_total_ordering() {
+    let mut layers = [
+        LayerId::new(Order::Tooltip, Id::new("a")),
+        LayerId::new(Order::Background, Id::new("b")),
+        LayerId::new(Order::Background, Id::new("c")),
+        LayerId::new(Order::Tooltip, Id::new("d")),
+        LayerId::new(Order::Background, Id::new("e")),
+        LayerId::new(Order::Background, Id::new("f")),
+        LayerId::new(Order::Tooltip, Id::new("g")),
+    ];
+    let mut areas = Areas::default();
+
+    // skip some of the layers
+    for &layer in &layers[3..] {
+        areas.set_state(layer, crate::AreaState::default());
+    }
+    areas.end_pass(); // sort layers
+
+    // Sort layers
+    layers.sort_by(|&a, &b| areas.compare_order(a, b));
+
+    // Assert that `areas.compare_order()` forms a total ordering
+    let mut equivalence_classes = vec![0];
+    let mut i = 0;
+    for l in layers.windows(2) {
+        assert!(l[0].order <= l[1].order, "does not follow LayerId.order");
+        if areas.compare_order(l[0], l[1]) != std::cmp::Ordering::Equal {
+            i += 1;
+        }
+        equivalence_classes.push(i);
+    }
+    assert_eq!(layers.len(), equivalence_classes.len());
+    for (&l1, c1) in std::iter::zip(&layers, &equivalence_classes) {
+        for (&l2, c2) in std::iter::zip(&layers, &equivalence_classes) {
+            assert_eq!(
+                c1.cmp(c2),
+                areas.compare_order(l1, l2),
+                "not a total ordering",
+            );
+        }
+    }
+}