fix(organize): preserve numbered names, indirect resource dicts, and comment threads (#5612)

crates/organize/src/comments.rs

@@ -54,9 +54,13 @@ pub fn import_comments(dst: &mut Document, src: &Document) -> Result<usize, Orga
     let to = walk(dst)?;
     let pages: Vec<(ObjRef, ObjRef)> = from.iter().zip(&to).map(|((s, _), (d, _))| (*s, *d)).collect();
     let mut incoming: Vec<Vec<Incoming>> = pages.iter().map(|(s, _)| page_comments(src, *s)).collect();
-    let kept = threads_kept(&incoming);
+    let kept = threads_kept(src, &incoming);
     for list in &mut incoming {
-        list.retain(|c| c.dict.get(b"IRT").is_none_or(|irt| irt.as_ref().is_some_and(|p| kept.contains(&p))));
+        list.retain(|c| match irt_parent(src, &c.dict) {
+            None => true,
+            Some(Some(p)) => kept.contains(&p),
+            Some(None) => false,
+        });
     }
     let mut copier = Copier::new(src, HashMap::new(), pages.iter().copied().collect());
     // Every comment's object is chosen before any is copied, so a comment that another one reaches
@@ -155,13 +159,34 @@ fn reserve(dst: &mut Document, src: &Document, annots: &mut PageAnnots, mut dict
     (target, dict)
 }
 
+/// Resolve `dict`'s `/IRT` entry: `None` when `/IRT` is absent or resolves to `null`,
+/// `Some(Some(r))` when it references annotation object `r` (directly or through indirect
+/// reference objects), or `Some(None)` for any other non-reference value.
+fn irt_parent(src: &Document, dict: &Dict) -> Option<Option<ObjRef>> {
+    let v = dict.get(b"IRT")?;
+    if matches!(v, Object::Null) {
+        return None;
+    }
+    let Some(mut r) = v.as_ref() else {
+        return Some(None);
+    };
+    for _ in 0..8 {
+        match *src.get(r) {
+            Object::Null => return None,
+            Object::Ref(next) => r = next,
+            _ => break,
+        }
+    }
+    Some(Some(r))
+}
+
 /// The comments of `incoming` that go: a reply goes only with the comment it answers, and that one
 /// only with the comment it answers, up to the first comment of the thread. A thread whose replies
 /// answer each other in a loop goes, as all of its comments are there. Each comment is visited once.
-fn threads_kept(incoming: &[Vec<Incoming>]) -> HashSet<ObjRef> {
+fn threads_kept(src: &Document, incoming: &[Vec<Incoming>]) -> HashSet<ObjRef> {
     // For each comment: no parent, a parent comment, or a parent that is no comment of the source.
     let parents: HashMap<ObjRef, Option<Option<ObjRef>>> =
-        incoming.iter().flatten().filter_map(|c| Some((c.entry.as_ref()?, c.dict.get(b"IRT").map(Object::as_ref)))).collect();
+        incoming.iter().flatten().filter_map(|c| Some((c.entry.as_ref()?, irt_parent(src, &c.dict)))).collect();
     let mut known: HashMap<ObjRef, bool> = HashMap::new();
     for &start in parents.keys() {
         let (mut path, mut on_path) = (Vec::new(), HashSet::new());
@@ -202,8 +227,11 @@ fn page_comments(src: &Document, page: ObjRef) -> Vec<Incoming> {
     list.unwrap_or_default()
         .into_iter()
         .filter_map(|entry| {
-            let dict = src.resolve(&entry).as_dict().cloned()?;
+            let mut dict = src.resolve(&entry).as_dict().cloned()?;
             let subtype = dict.get(b"Subtype").map(|s| src.resolve(s)).and_then(|s| s.as_name().map(<[u8]>::to_vec))?;
+            if let Some(Some(r)) = irt_parent(src, &dict) {
+                dict.set(b"IRT".to_vec(), Object::Ref(r));
+            }
             COMMENTS.contains(&subtype.as_slice()).then_some(Incoming { entry, dict })
         })
         .collect()
@@ -217,7 +245,8 @@ fn page_popups(src: &Document, page: ObjRef) -> Vec<(ObjRef, ObjRef)> {
         .filter_map(|entry| {
             let r = entry.as_ref()?;
             let d = src.get(r).as_dict().cloned()?;
-            (d.name(b"Subtype") == Some(b"Popup".as_slice())).then(|| d.reference(b"Parent").map(|p| (r, p)))?
+            let is_popup = d.get(b"Subtype").map(|s| src.resolve(s)).as_deref().and_then(Object::as_name) == Some(b"Popup".as_slice());
+            is_popup.then(|| d.reference(b"Parent").map(|p| (r, p)))?
         })
         .collect()
 }
@@ -265,7 +294,7 @@ impl PageAnnots {
             if let Some(name) = d.get(b"NM").and_then(|n| dst.resolve(n).as_string().map(|s| s.to_text())) {
                 names.entry(name).or_default().push(r);
             }
-            if d.name(b"Subtype") == Some(b"Popup".as_slice())
+            if d.get(b"Subtype").map(|s| dst.resolve(s)).as_deref().and_then(Object::as_name) == Some(b"Popup".as_slice())
                 && let Some(parent) = d.reference(b"Parent")
             {
                 popups.entry(parent).or_default().push(r);
@@ -654,4 +683,42 @@ mod tests {
         assert_eq!(import_comments(&mut doc, &src).unwrap(), 0);
         assert_eq!(page_annots(&doc, 0).len(), before);
     }
+
+    #[test]
+    fn comment_with_null_irt_and_popup_with_indirect_subtype_are_imported() {
+        let src = build(&[
+            "<< /Type /Catalog /Pages 2 0 R >>",
+            "<< /Type /Pages /Kids [3 0 R 8 0 R] /Count 2 /MediaBox [0 0 300 400] >>",
+            "<< /Type /Page /Parent 2 0 R /Annots [4 0 R 5 0 R 6 0 R 9 0 R 10 0 R 11 0 R 12 0 R 13 0 R 16 0 R] >>",
+            "<< /Type /Annot /Subtype /Text /NM (root-null-irt) /IRT null /Contents (root comment) /Rect [10 10 30 30] /P 3 0 R >>",
+            "<< /Type /Annot /Subtype /Text /IRT 14 0 R /Contents (reply to root) /Rect [10 10 30 30] /P 3 0 R >>",
+            "<< /Type /Annot /Subtype 7 0 R /Parent 4 0 R /Rect [40 40 200 120] /P 3 0 R >>",
+            "/Popup",
+            "<< /Type /Page /Parent 2 0 R /Annots [15 0 R] >>",
+            // Three-reply chain on page 1 whose root comment (15 0 R) is on dropped page 2:
+            "<< /Type /Annot /Subtype /Text /IRT 15 0 R /Contents (dropped-1) /Rect [10 10 30 30] /P 3 0 R >>",
+            "<< /Type /Annot /Subtype /Text /IRT 9 0 R /Contents (dropped-2) /Rect [10 10 30 30] /P 3 0 R >>",
+            "<< /Type /Annot /Subtype /Text /IRT 10 0 R /Contents (dropped-3) /Rect [10 10 30 30] /P 3 0 R >>",
+            // Two-reply cycle on page 1:
+            "<< /Type /Annot /Subtype /Text /IRT 13 0 R /Contents (cycle-a) /Rect [10 10 30 30] /P 3 0 R >>",
+            "<< /Type /Annot /Subtype /Text /IRT 12 0 R /Contents (cycle-b) /Rect [10 10 30 30] /P 3 0 R >>",
+            "4 0 R",
+            "<< /Type /Annot /Subtype /Text /Contents (dropped root) /Rect [10 10 30 30] /P 8 0 R >>",
+            // Comment whose /IRT chains through an intermediate reference (17 0 R -> 18 0 R -> null):
+            "<< /Type /Annot /Subtype /Text /IRT 17 0 R /Contents (indirect null irt) /Rect [10 10 30 30] /P 3 0 R >>",
+            "18 0 R",
+            "null",
+        ]);
+        let mut dst = build(&[
+            "<< /Type /Catalog /Pages 2 0 R >>",
+            "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 300 400] >>",
+            "<< /Type /Page /Parent 2 0 R >>",
+        ]);
+        assert_eq!(import_comments(&mut dst, &src).unwrap(), 5);
+        let annots = page_annots(&dst, 0);
+        assert_eq!(annots.len(), 6, "two root comments, reply via indirect IRT, two cycling replies, and the popup");
+        let (root_ref, _) = by_contents(&annots, "root comment");
+        assert_eq!(by_contents(&annots, "reply to root").1.reference(b"IRT"), Some(*root_ref));
+        assert!(by_contents(&annots, "indirect null irt").1.get(b"IRT").is_some());
+    }
 }

crates/organize/src/dedupe.rs

@@ -45,6 +45,39 @@ fn is_resource(o: &Object) -> bool {
 /// out on purpose: it names optional content groups, whose identity matters.
 const RESOURCE_CATEGORIES: &[&[u8]] = &[b"ExtGState", b"ColorSpace", b"Pattern", b"Shading", b"XObject", b"Font"];
 
+/// Whether `d` carries annotation, page, field, or structure identity rather than resource data.
+/// Annotations often omit `/Type /Annot` (ISO 32000-2 Table 166 requires `/Subtype` and `/Rect`),
+/// whereas resource dictionaries and category maps never carry `/Rect`, `/Popup`, `/IRT`, or `/FT`.
+fn is_identity_dict(doc: &Document, d: &pdfcraft_cos::Dict) -> bool {
+    if let Some(t) = d.get(b"Type").map(|v| doc.resolve(v)).as_deref().and_then(Object::as_name)
+        && !RESOURCE_TYPES.contains(&t)
+    {
+        return true;
+    }
+    d.contains(b"Rect") || d.contains(b"Popup") || d.contains(b"IRT") || d.contains(b"FT") || d.contains(b"Parent")
+}
+
+/// Whether a category member has the expected resource shape and is not an identity-bearing
+/// dictionary such as an annotation or form field.
+fn category_entry(doc: &Document, category: &[u8], value: &Object) -> bool {
+    let value = doc.resolve(value);
+    match category {
+        b"Font" | b"ExtGState" => matches!(&*value, Object::Dict(d) if !is_identity_dict(doc, d)),
+        b"XObject" => matches!(&*value, Object::Stream(s) if !is_identity_dict(doc, &s.dict)),
+        b"Pattern" | b"Shading" => match &*value {
+            Object::Dict(d) => !is_identity_dict(doc, d),
+            Object::Stream(s) => !is_identity_dict(doc, &s.dict),
+            _ => false,
+        },
+        b"ColorSpace" => match &*value {
+            Object::Name(name) => matches!(name.as_slice(), b"DeviceGray" | b"DeviceRGB" | b"DeviceCMYK" | b"Pattern"),
+            Object::Array(items) => items.first().and_then(Object::as_name).is_some_and(|name| COLOR_SPACE_FAMILIES.contains(&name)),
+            _ => false,
+        },
+        _ => false,
+    }
+}
+
 /// References listed in any `/Resources` dictionary reachable inside `o` (pages, forms, Type 3
 /// fonts, annotation appearances), including the `/Resources` dictionary itself when indirect.
 fn collect_resource_refs(doc: &Document, o: &Object, out: &mut HashSet<ObjRef>) {
@@ -69,6 +102,15 @@ fn collect_resource_refs(doc: &Document, o: &Object, out: &mut HashSet<ObjRef>)
                 for cat in RESOURCE_CATEGORIES {
                     let entries = res.get(cat).map(|c| doc.resolve(c));
                     if let Some(entries) = entries.as_ref().and_then(|e| e.as_dict()) {
+                        // A malformed category can point directly at an annotation or list
+                        // annotations inside a category map. Only resource-name maps whose
+                        // members are non-identity resources are containers.
+                        if is_identity_dict(doc, entries) || !entries.iter().all(|(_, value)| category_entry(doc, cat, value)) {
+                            continue;
+                        }
+                        if let Some(Object::Ref(cat_ref)) = res.get(cat) {
+                            out.insert(*cat_ref);
+                        }
                         out.extend(entries.iter().filter_map(|(_, v)| if let Object::Ref(r) = v { Some(*r) } else { None }));
                     }
                 }
@@ -189,11 +231,15 @@ fn contains_ref(o: &Object, map: &HashMap<ObjRef, ObjRef>) -> bool {
 /// Merge duplicate resources among `candidates` (into each other or into equal objects already
 /// in `doc`), rewrite references inside `candidates`, and free the merged-away objects.
 /// When `index_existing` is false, only candidates are compared (a brand-new document).
+/// Repeated candidates are processed once, by complete object reference.
 /// Returns how many objects were removed.
 pub fn dedupe_resources(doc: &mut Document, candidates: &[ObjRef], index_existing: bool) -> usize {
     let cand: HashSet<ObjRef> = candidates.iter().copied().collect();
     let mut sorted: Vec<ObjRef> = candidates.to_vec();
-    sorted.sort_unstable_by_key(|r| r.num);
+    sorted.sort_unstable_by_key(|r| (r.num, r.generation));
+    // A repeated candidate must not merge into itself: that would cycle `canon` and free
+    // the canonical object. Keep generations distinct when normalizing references.
+    sorted.dedup();
     // Existing objects first, so candidates merge into them rather than the reverse.
     let mut all: Vec<(ObjRef, std::sync::Arc<Object>)> = Vec::new();
     if index_existing {
@@ -273,6 +319,37 @@ mod tests {
         doc.add(Object::Dict(d))
     }
 
+    #[test]
+    fn repeated_candidate_keeps_the_canonical_object() {
+        for index_existing in [false, true] {
+            let mut doc = Document::new_empty();
+            let r = doc.add(stream(b"glyphs"));
+            assert_eq!(dedupe_resources(&mut doc, &[r, r], index_existing), 0);
+            assert_eq!(*doc.get(r), stream(b"glyphs"));
+        }
+    }
+
+    #[test]
+    fn repeated_candidate_with_referrer_terminates_and_keeps_generations() {
+        for index_existing in [false, true] {
+            let mut doc = Document::new_empty();
+            let r = ObjRef::new(20, 7);
+            doc.set(r, stream(b"glyphs"));
+            let s = font(&mut doc, r, "A");
+            assert_eq!(dedupe_resources(&mut doc, &[r, r, s], index_existing), 0);
+            assert_eq!(*doc.get(r), stream(b"glyphs"));
+            assert_eq!(doc.generation(r.num), 7);
+            assert_eq!(doc.get(s).as_dict().unwrap().get(b"FontFile"), Some(&Object::Ref(r)));
+
+            let duplicate = ObjRef::new(30, 9);
+            doc.set(duplicate, stream(b"glyphs"));
+            let t = font(&mut doc, duplicate, "B");
+            assert_eq!(dedupe_resources(&mut doc, &[r, r, s, duplicate, duplicate, t], index_existing), 1);
+            assert_eq!(*doc.get(r), stream(b"glyphs"));
+            assert_eq!(doc.get(t).as_dict().unwrap().get(b"FontFile"), Some(&Object::Ref(r)));
+        }
+    }
+
     #[test]
     fn merges_bottom_up_and_keeps_identity_objects() {
         let mut doc = Document::new_empty();
@@ -367,4 +444,53 @@ mod tests {
         let cs = doc.get(h).as_dict().unwrap().get(b"CS").unwrap().clone();
         assert_eq!(cs, Object::Array(vec![Object::name("ICCBased"), Object::Ref(old)]));
     }
+
+    #[test]
+    fn indirect_resource_category_and_resources_dictionaries_merge() {
+        let mut doc = Document::new_empty();
+        let mut gs = Dict::new();
+        gs.set(b"CA".to_vec(), Object::Real(0.5));
+        let g1 = doc.add(Object::Dict(gs.clone()));
+        let g2 = doc.add(Object::Dict(gs));
+
+        let page_with_indirect_res = |doc: &mut Document, g: ObjRef| {
+            let mut ext = Dict::new();
+            ext.set(b"G0".to_vec(), Object::Ref(g));
+            let ext_ref = doc.add(Object::Dict(ext));
+            let mut res = Dict::new();
+            res.set(b"ExtGState".to_vec(), Object::Ref(ext_ref));
+            let res_ref = doc.add(Object::Dict(res));
+            let mut p = Dict::new();
+            p.set(b"Type".to_vec(), Object::name("Page"));
+            p.set(b"Resources".to_vec(), Object::Ref(res_ref));
+            let p_ref = doc.add(Object::Dict(p));
+            (ext_ref, res_ref, p_ref)
+        };
+        let (ext1, res1, p1) = page_with_indirect_res(&mut doc, g1);
+        let (ext2, res2, p2) = page_with_indirect_res(&mut doc, g2);
+        assert_eq!(dedupe_resources(&mut doc, &[g1, g2, ext1, res1, p1, ext2, res2, p2], false), 3);
+        assert_eq!(doc.get(p1).as_dict().unwrap().get(b"Resources"), Some(&Object::Ref(res1)));
+        assert_eq!(doc.get(p2).as_dict().unwrap().get(b"Resources"), Some(&Object::Ref(res1)));
+        assert_eq!(doc.get(res1).as_dict().unwrap().get(b"ExtGState"), Some(&Object::Ref(ext1)));
+
+        // Distinct font maps (`F1` and `F10` with different fonts on one page vs the same on another)
+        // deduplicate identical fonts while preserving distinct `F1`/`F10` entries.
+        let ff1 = doc.add(stream(b"font-f1"));
+        let ff10 = doc.add(stream(b"font-f10"));
+        let f1_a = font(&mut doc, ff1, "Helvetica");
+        let f10_a = font(&mut doc, ff10, "Courier");
+        let f1_b = font(&mut doc, ff1, "Helvetica");
+        let f10_b = font(&mut doc, ff10, "Times-Roman");
+        let mut fmap_a = Dict::new();
+        fmap_a.set(b"F1".to_vec(), Object::Ref(f1_a));
+        fmap_a.set(b"F10".to_vec(), Object::Ref(f10_a));
+        let fmap_a_ref = doc.add(Object::Dict(fmap_a));
+        let mut fmap_b = Dict::new();
+        fmap_b.set(b"F1".to_vec(), Object::Ref(f1_b));
+        fmap_b.set(b"F10".to_vec(), Object::Ref(f10_b));
+        let fmap_b_ref = doc.add(Object::Dict(fmap_b));
+        assert_eq!(dedupe_resources(&mut doc, &[f1_a, f10_a, f1_b, f10_b, fmap_a_ref, fmap_b_ref], false), 1);
+        assert_eq!(doc.get(fmap_b_ref).as_dict().unwrap().get(b"F1"), Some(&Object::Ref(f1_a)));
+        assert_eq!(doc.get(fmap_b_ref).as_dict().unwrap().get(b"F10"), Some(&Object::Ref(f10_b)));
+    }
 }

crates/organize/src/import/carry.rs

@@ -28,14 +28,20 @@ pub(crate) fn set_attachments(dst: &mut Document, mut entries: Vec<(Vec<u8>, Obj
     }
     // Names are compared and numbered as text: a key written as UTF-16 (a name with an accent
     // or Chinese letters) must not get ASCII bytes appended to it.
+    let reserved: std::collections::HashSet<String> = entries.iter().map(|(k, _)| pdfcraft_cos::PdfString::literal(k.clone()).to_text()).collect();
     let mut seen = std::collections::HashSet::new();
     for (k, _) in entries.iter_mut() {
         let base = pdfcraft_cos::PdfString::literal(k.clone()).to_text();
         let mut text = base.clone();
         let mut n = 2;
-        while !seen.insert(text.clone()) {
-            text = format!("{base} ({n})");
-            n += 1;
+        if !seen.insert(text.clone()) {
+            loop {
+                text = format!("{base} ({n})");
+                n += 1;
+                if !reserved.contains(&text) && seen.insert(text.clone()) {
+                    break;
+                }
+            }
         }
         if text != base {
             *k = pdfcraft_cos::PdfString::text(&text).bytes;
@@ -141,10 +147,11 @@ fn outline_level(m: &Copier<'_>, first: Option<ObjRef>, depth: u8, budget: &mut
         let target = item.get(b"Dest").cloned().or_else(|| {
             let a = m.src.resolve(item.get(b"A")?);
             let a = a.as_dict()?;
-            (a.name(b"S") == Some(b"GoTo")).then(|| a.get(b"D").cloned()).flatten()
+            let is_goto = a.get(b"S").map(|s| m.src.resolve(s)).as_deref().and_then(Object::as_name) == Some(b"GoTo");
+            is_goto.then(|| a.get(b"D").cloned()).flatten()
         });
         let dest = target.and_then(|t| m.map_dest(&t));
-        let open = item.int(b"Count").is_some_and(|c| c > 0);
+        let open = item.get(b"Count").and_then(|c| m.src.resolve(c).as_int()).is_some_and(|c| c > 0);
         let children = outline_level(m, item.reference(b"First"), depth + 1, budget);
         made.push(OutlineNode { title, colour, flags, dest, open, children });
     }
@@ -235,3 +242,58 @@ pub(super) fn add_outline(doc: &mut Document, marks: &[Mark]) -> Result<(), Orga
     })?;
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use std::sync::Arc;
+
+    use super::*;
+    use crate::attachment_keys;
+
+    fn build(bodies: &[&str]) -> Document {
+        let mut out = b"%PDF-1.7\n".to_vec();
+        let mut offsets = Vec::new();
+        for (i, b) in bodies.iter().enumerate() {
+            offsets.push(out.len());
+            out.extend_from_slice(format!("{} 0 obj\n{b}\nendobj\n", i + 1).as_bytes());
+        }
+        let xref = out.len();
+        out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", bodies.len() + 1).as_bytes());
+        for o in offsets {
+            out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes());
+        }
+        out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", bodies.len() + 1).as_bytes());
+        Document::open(Arc::new(out)).expect("opens")
+    }
+
+    #[test]
+    fn set_attachments_does_not_clobber_later_numbered_attachment_name() {
+        let mut doc = build(&[
+            "<< /Type /Catalog /Pages 2 0 R >>",
+            "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>",
+            "<< /Type /Page /Parent 2 0 R >>",
+        ]);
+        let entries =
+            vec![(b"report.pdf".to_vec(), Object::Null), (b"report.pdf".to_vec(), Object::Null), (b"report.pdf (2)".to_vec(), Object::Null)];
+        set_attachments(&mut doc, entries).unwrap();
+        assert_eq!(attachment_keys(&doc), ["report.pdf", "report.pdf (2)", "report.pdf (3)"]);
+    }
+
+    #[test]
+    fn source_outline_resolves_indirect_goto_action_type_and_count() {
+        let src = build(&[
+            "<< /Type /Catalog /Pages 2 0 R /Outlines 4 0 R >>",
+            "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>",
+            "<< /Type /Page /Parent 2 0 R >>",
+            "<< /Type /Outlines /First 5 0 R /Last 5 0 R /Count 1 >>",
+            "<< /Title (Parent) /Parent 4 0 R /First 6 0 R /Last 6 0 R /Count 8 0 R /A << /S 7 0 R /D [3 0 R /Fit] >> >>",
+            "<< /Title (Child) /Parent 5 0 R /Dest [3 0 R /Fit] >>",
+            "/GoTo",
+            "1",
+        ]);
+        let nodes = source_outline(&src, HashMap::from([(ObjRef::new(3, 0), ObjRef::new(3, 0))]));
+        assert_eq!(nodes.len(), 1);
+        assert!(nodes[0].dest.is_some());
+        assert!(nodes[0].open);
+    }
+}

crates/organize/src/import/fields.rs

@@ -50,19 +50,26 @@ pub fn combine_field_conflicts(sources: &[&Document]) -> Vec<Vec<(String, String
 /// bytes, so one name written in two encodings (PDFDocEncoding, UTF-16BE) is one name.
 /// Read-only: returns (node, new `/T`, old name, new name). Cycle- and depth-safe.
 fn plan_renames(doc: &Document, roots: &[ObjRef], taken: &mut std::collections::HashSet<String>) -> Vec<(ObjRef, String, String, String)> {
+    let mut reserved = std::collections::HashSet::new();
+    let mut seen = std::collections::HashSet::new();
+    for &root in roots {
+        collect_names(doc, root, "", &mut seen, 0, &mut reserved);
+    }
     let mut planned = Vec::new();
-    let mut walk = RenameWalk::default();
+    let mut walk = RenameWalk { reserved, ..RenameWalk::default() };
     for &root in roots {
         plan_renames_under(doc, root, "", taken, &mut walk, 0, &mut planned);
     }
     planned
 }
 
-/// What one [`plan_renames`] call remembers: the nodes it visited, and for each colliding name
-/// the suffix to try next.
+/// What one [`plan_renames`] call remembers: the nodes it visited, the incoming tree's own
+/// names (so an earlier rename does not steal a later sibling's name), and for each colliding
+/// name the suffix to try next.
 #[derive(Default)]
 struct RenameWalk {
     seen: HashSet<ObjRef>,
+    reserved: HashSet<String>,
     next: HashMap<String, u32>,
 }
 
@@ -88,18 +95,19 @@ fn plan_renames_under(
             // still taken: resume there instead of at 1.
             let mut n = walk.next.get(&fqn).copied().unwrap_or(1);
             let mut variant;
+            let mut candidate;
             while {
                 #[cfg(test)]
                 crate::work(1);
                 variant = format!("{name}-{n}");
-                taken.contains(&qualified(prefix, &variant))
+                candidate = qualified(prefix, &variant);
+                taken.contains(&candidate) || walk.reserved.contains(&candidate)
             } {
                 n = n.saturating_add(1);
             }
             walk.next.insert(fqn.clone(), n.saturating_add(1));
-            let new_fqn = qualified(prefix, &variant);
-            planned.push((node, variant.clone(), fqn, new_fqn.clone()));
-            taken.insert(new_fqn);
+            planned.push((node, variant.clone(), fqn, candidate.clone()));
+            taken.insert(candidate);
             name = variant;
         } else {
             taken.insert(fqn);
@@ -143,15 +151,16 @@ fn field_roots(doc: &Document) -> Result<Vec<ObjRef>, OrganizeError> {
         let annots = d.get(b"Annots").map(|a| doc.resolve(a)).and_then(|a| a.as_array().cloned()).unwrap_or_default();
         for a in annots.iter() {
             let Some(w) = doc.resolve(a).as_dict().cloned() else { continue };
-            if w.name(b"Subtype") != Some(b"Widget") {
+            if w.get(b"Subtype").map(|s| doc.resolve(s)).as_deref().and_then(Object::as_name) != Some(b"Widget") {
                 continue;
             }
             // Walk up to the field the widget belongs to, however deep it sits; a widget that
             // is its own field is its own root.
             let Some(mut root) = a.as_ref() else { continue };
             let mut up = 0;
+            let mut visited = HashSet::from([root]);
             while let Some(parent) = doc.get(root).as_dict().and_then(|p| p.reference(b"Parent")) {
-                if up >= 32 || root == parent {
+                if up >= 32 || !visited.insert(parent) || doc.get(parent).as_dict().is_none() {
                     break;
                 }
                 root = parent;
@@ -295,3 +304,63 @@ pub(super) fn register_fields(dst: &mut Document, fields: &[ObjRef]) -> Result<(
     }
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use std::sync::Arc;
+
+    use super::*;
+
+    fn build(bodies: &[&str]) -> Document {
+        let mut out = b"%PDF-1.7\n".to_vec();
+        let mut offsets = Vec::new();
+        for (i, b) in bodies.iter().enumerate() {
+            offsets.push(out.len());
+            out.extend_from_slice(format!("{} 0 obj\n{b}\nendobj\n", i + 1).as_bytes());
+        }
+        let xref = out.len();
+        out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", bodies.len() + 1).as_bytes());
+        for o in offsets {
+            out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes());
+        }
+        out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", bodies.len() + 1).as_bytes());
+        Document::open(Arc::new(out)).expect("opens")
+    }
+
+    #[test]
+    fn renaming_colliding_field_does_not_steal_later_sibling_name() {
+        let dst = build(&[
+            "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] >> >>",
+            "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>",
+            "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>",
+            "<< /Type /Annot /Subtype /Widget /FT /Tx /T (item) /Rect [0 0 10 10] >>",
+        ]);
+        let src = build(&[
+            "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R 5 0 R] >> >>",
+            "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>",
+            "<< /Type /Page /Parent 2 0 R /Annots [4 0 R 5 0 R] >>",
+            "<< /Type /Annot /Subtype /Widget /FT /Tx /T (item) /Rect [0 0 10 10] >>",
+            "<< /Type /Annot /Subtype /Widget /FT /Tx /T (item-1) /Rect [0 20 10 30] >>",
+        ]);
+        // Only "item" collides with dst; "item-1" in src is already unique and must not be stolen.
+        assert_eq!(field_name_conflicts(&dst, &src), [("item".to_string(), "item-2".to_string())]);
+    }
+
+    #[test]
+    fn field_roots_keeps_widget_with_dangling_parent_or_indirect_subtype() {
+        let dst = build(&[
+            "<< /Type /Catalog /Pages 2 0 R >>",
+            "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>",
+            "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>",
+            "<< /Type /Annot /Subtype 5 0 R /FT /Tx /T (orphan) /Parent 99 0 R /Rect [0 0 10 10] >>",
+            "/Widget",
+        ]);
+        let src = build(&[
+            "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] >> >>",
+            "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>",
+            "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>",
+            "<< /Type /Annot /Subtype /Widget /FT /Tx /T (orphan) /Rect [0 0 10 10] >>",
+        ]);
+        assert_eq!(field_name_conflicts(&dst, &src), [("orphan".to_string(), "orphan-1".to_string())]);
+    }
+}

crates/organize/src/tests.rs

@@ -1747,6 +1747,63 @@ fn bookmarks_report_the_view_their_destination_asks_for() {
     );
 }
 
+#[test]
+fn malformed_resource_categories_preserve_annotation_identity_when_combining_and_inserting() {
+    for typed in [true, false] {
+        for with_popups in [false, true] {
+            let mut source = doc_a();
+            let page = pages(&source).unwrap()[0].obj;
+            let mut annot = Dict::new();
+            if typed {
+                annot.set(b"Type".to_vec(), Object::name("Annot"));
+            }
+            annot.set(b"Subtype".to_vec(), Object::name("Text"));
+            annot.set(b"Rect".to_vec(), Object::Array([0, 0, 10, 10].map(Object::Int).to_vec()));
+            let first = source.add(Object::Dict(annot.clone()));
+            let second = source.add(Object::Dict(annot.clone()));
+            if with_popups {
+                annot.set(b"Subtype".to_vec(), Object::name("Popup"));
+                let popup1 = source.add(Object::Dict(annot.clone()));
+                let popup2 = source.add(Object::Dict(annot));
+                source.update_dict(first, |d| d.set(b"Popup".to_vec(), Object::Ref(popup1))).unwrap();
+                source.update_dict(second, |d| d.set(b"Popup".to_vec(), Object::Ref(popup2))).unwrap();
+            }
+            source
+                .update_dict(page, |d| {
+                    let mut font_map = Dict::new();
+                    font_map.set(b"F1".to_vec(), Object::Ref(first));
+                    font_map.set(b"F2".to_vec(), Object::Ref(second));
+                    let mut resources = Dict::new();
+                    resources.set(b"Font".to_vec(), Object::Dict(font_map));
+                    resources.set(b"ExtGState".to_vec(), Object::Ref(second));
+                    d.set(b"Resources".to_vec(), Object::Dict(resources));
+                    d.set(b"Annots".to_vec(), Object::Array(vec![Object::Ref(first), Object::Ref(second)]));
+                })
+                .unwrap();
+            let combined = crate::combine(&[("source", &source)]).unwrap();
+            let mut inserted = doc_a();
+            crate::import_pages(&mut inserted, &source, &[0], 0).unwrap();
+            for doc in [combined, inserted] {
+                let doc = full_roundtrip(&doc);
+                let page = pages(&doc).unwrap()[0].obj;
+                let refs = doc.get(page).as_dict().unwrap().get(b"Annots").unwrap().as_array().unwrap().clone();
+                assert_eq!(refs.len(), 2);
+                assert_ne!(refs[0], refs[1], "equal annotations retain their separate identities");
+                if with_popups {
+                    let popups: Vec<_> = refs.iter().map(|r| doc.resolve(r).as_dict().unwrap().reference(b"Popup").unwrap()).collect();
+                    assert_ne!(popups[0], popups[1], "malformed category entries must not merge distinct popups");
+                    for popup in popups {
+                        assert_eq!(doc.get(popup).as_dict().unwrap().name(b"Subtype"), Some(b"Popup".as_slice()));
+                    }
+                }
+                for r in refs {
+                    assert_eq!(doc.resolve(&r).as_dict().unwrap().name(b"Subtype"), Some(b"Text".as_slice()));
+                }
+            }
+        }
+    }
+}
+
 #[test]
 fn combine_and_insert_share_identical_resources() {
     let a = doc_a();