fix: refuse handles whose owner is closed, and close every handle before destroying PDFium (ADR-028, ADR-029)

CLAUDE.md

@@ -136,13 +136,32 @@ for the rationale. The short version:
 ## Memory model — the rule that bites if you forget it
 
 - Every PDFium handle (`FPDF_DOCUMENT`, `FPDF_PAGE`, etc.) lives behind an
-  R `externalptr` with a C finalizer registered via `R_RegisterCFinalizerEx(..., TRUE)`.
-- The finalizer is the **only** path that calls `FPDF_*Close*`. After it
-  runs, it calls `R_ClearExternalPtr` so the pointer reads as NULL on
-  subsequent access. This makes `pdf_doc_close()` safely idempotent.
-- Children (pages, page objects) hold an R-level reference to their parent
-  (doc, page) so GC can't reclaim the parent before the child. Always set
-  the parent into the `prot` slot of the child's externalptr.
+  R `externalptr`. Every handle with a finalizer is in one registry,
+  `src/handle_registry.{h,cpp}` (ADR-024, ADR-025, ADR-028): annotation
+  contexts, pages, fonts and XObjects under their document; documents,
+  `pdf_clip_path_new()` clip paths, bitmaps and memory-document buffers
+  under the library.
+- One mint and one release path per kind: `make_*_handle()` alone creates
+  the externalptr (finalizer attached, handle registered), and
+  `release_*_handle()` alone closes and clears it — the finalizer, the
+  explicit `pdf_*_close()` / `pdf_annot_delete()` and the owner's release
+  all call it, so every close is idempotent. Never close a handle
+  `make_*_handle()` made, or call `R_RegisterCFinalizerEx`, anywhere
+  else.
+- Owners release their handles first: `pdf_doc_close()` closes the
+  document's annotations, pages, fonts and XObjects before
+  `FPDF_CloseDocument`, and `cpp_destroy_library()` closes every document,
+  clip path and bitmap before `FPDF_DestroyLibrary`. Those handles then
+  read as closed.
+- Registry entries are weak: the externalptr `SEXP`s sit in C++
+  containers R does not scan, and the finalizer removes each one from the
+  registry before R frees it, so an unreachable handle is still collected
+  promptly.
+- Children pin their immediate owner in `prot` (a page its document, an
+  annotation its page, a page-object its page, annotation or form, a clip
+  path its page-object), and both `validate_handle()` in C++ and
+  `is_open()` in R walk the whole chain (ADR-029): a handle is refused
+  once any owner is closed.
 - Automatic close on GC works — see `vignettes/architecture.Rmd`. But for
   large documents or platform-sensitive code (Windows file-handle blocking
   deletion), call `pdf_doc_close()` explicitly.

NEWS.md

@@ -151,6 +151,40 @@
   system-font provider once per library lifetime, and the package frees
   it when it shuts the library down. Each call used to allocate a new
   provider that was never completely freed.
+* Page-objects read with `pdf_annot_objects()`, and the objects of forms
+  among them, print as closed once their annotation is deleted with
+  `pdf_annot_delete()`, and page-object functions refuse them with
+  "Parent annotation has been closed: it was deleted with
+  pdf_annot_delete()." They used to print as open and fail with an
+  internal message. `pdf_annot_update_object()` and
+  `pdf_form_obj_remove_object()` now refuse a closed page-object with
+  the same messages as the other page-object functions.
+* A clip path from `pdf_obj_clip_path()` belongs to its page-object, not
+  to the page: `pdf_clip_path_count()` and `pdf_clip_path_segments()`
+  refuse it once the object is deleted with `pdf_obj_delete()` or
+  `pdf_form_obj_remove_object()`, moved into an annotation with
+  `pdf_annot_append_object()`, or once the annotation or form holding it
+  is deleted. They used to read the freed object, and for an
+  annotation's object after `pdf_annot_delete()` they crashed R
+  (ADR-029).
+* The objects `pdf_form_objects()` returns belong to the form handle
+  they were read through: they are refused once that form is deleted
+  with `pdf_obj_delete()`, removed with `pdf_form_obj_remove_object()`
+  or moved into an annotation with `pdf_annot_append_object()`. After a
+  move, read them again through `pdf_annot_objects()` and
+  `pdf_form_objects()`. They used to read the freed objects once the
+  form was destroyed (ADR-029).
+* Shutting PDFium down, as unloading the package does, first closes
+  every open document together with its pages, annotations, fonts and
+  XObjects, and every clip path and bitmap from `pdf_clip_path_new()`
+  and `pdf_bitmap_new()`; their handles read as closed afterwards.
+  Closing, using or collecting one of them after the shutdown, or after
+  PDFium was initialised again by opening a document or by
+  `pdf_system_fonts_install_default()`, crashed R (ADR-028).
+* `pdf_form_fields()`'s documentation says what it returns, a
+  `pdfium_form_field_list` of form-field handles, instead of describing
+  a tibble; the tibble's columns are now documented on
+  `as_tibble.pdfium_form_field_list()`, which builds it.
 
 # pdfium 0.1.0
 

R/RcppExports.R

@@ -421,8 +421,8 @@ cpp_bookmark_action_handle <- function(bm_ptr, doc_ptr) {
     .Call(`_pdfium_cpp_bookmark_action_handle`, bm_ptr, doc_ptr)
 }
 
-cpp_obj_get_clip_path <- function(obj_ptr, page_ptr) {
-    .Call(`_pdfium_cpp_obj_get_clip_path`, obj_ptr, page_ptr)
+cpp_obj_get_clip_path <- function(obj_ptr) {
+    .Call(`_pdfium_cpp_obj_get_clip_path`, obj_ptr)
 }
 
 cpp_clip_path_count_paths <- function(clip_ptr) {
@@ -589,8 +589,8 @@ cpp_form_object_count <- function(form_ptr) {
     .Call(`_pdfium_cpp_form_object_count`, form_ptr)
 }
 
-cpp_form_get_object <- function(form_ptr, owner_ptr, index_zero_based) {
-    .Call(`_pdfium_cpp_form_get_object`, form_ptr, owner_ptr, index_zero_based)
+cpp_form_get_object <- function(form_ptr, index_zero_based) {
+    .Call(`_pdfium_cpp_form_get_object`, form_ptr, index_zero_based)
 }
 
 cpp_text_obj_glyph_path <- function(obj_ptr, glyph_code, font_size) {
@@ -613,6 +613,10 @@ cpp_doc_handle_counts <- function(doc_ptr) {
     .Call(`_pdfium_cpp_doc_handle_counts`, doc_ptr)
 }
 
+cpp_library_handle_counts <- function() {
+    .Call(`_pdfium_cpp_library_handle_counts`)
+}
+
 cpp_image_new_from_jpeg <- function(doc_ptr, page_ptr, jpeg_bytes) {
     .Call(`_pdfium_cpp_image_new_from_jpeg`, doc_ptr, page_ptr, jpeg_bytes)
 }

R/annot_authoring.R

@@ -101,8 +101,9 @@ pdf_annot_new <- function(page, subtype, bounds = NULL) {
 #' the page's `/Annots` array, PDFium's annotation context is
 #' released together with any page-objects of its appearance stream,
 #' and the handle is closed: further `pdf_annot_*` calls on it, and
-#' on page-objects read from it with [pdf_annot_objects()], error
-#' cleanly.
+#' on page-objects read from it with [pdf_annot_objects()], on their
+#' clip paths ([pdf_obj_clip_path()]) and on the objects of forms among
+#' them ([pdf_form_objects()]), error cleanly.
 #'
 #' Later annotations on the page move down one position. Other handles
 #' still print the position they had when they were made;

R/api_completion.R

@@ -651,9 +651,12 @@ pdf_annot_objects <- function(annot) {
 #' `annot`, so exactly one of them owns the object at any time.
 #'
 #' After the call `obj` is no longer on the page and its handle is
-#' closed; reach the object through [pdf_annot_objects()] instead.
-#' Other handles to the same object (for example from an earlier
-#' [pdf_page_objects()] call) are stale and must not be used, and the
+#' closed, as are the clip paths read from it with
+#' [pdf_obj_clip_path()] and, for a form object, the objects read from
+#' it with [pdf_form_objects()]; reach the object through
+#' [pdf_annot_objects()] instead. Other handles to the same object
+#' (for example from an earlier [pdf_page_objects()] call) are stale
+#' and must not be used, and the
 #' page-scoped indices of the remaining page-objects shift down by
 #' one. The move only succeeds as a whole: when `annot` cannot hold
 #' page-objects, or `obj` is not a top-level object of the
@@ -698,8 +701,9 @@ pdf_annot_append_object <- function(annot, obj) {
 #' position within the annotation's embedded content (one-based,
 #' matching [pdf_annot_objects()]). PDFium destroys the object and
 #' regenerates the annotation's appearance stream, so handles to it
-#' from an earlier [pdf_annot_objects()] call are stale and must not
-#' be used; call [pdf_annot_objects()] again for the remaining ones.
+#' from an earlier [pdf_annot_objects()] call, and the clip paths and
+#' nested objects read from them, are stale and must not be used; call
+#' [pdf_annot_objects()] again for the remaining ones.
 #'
 #' @param annot A `pdfium_annot` of subtype `"ink"` or `"stamp"`, the
 #'   only subtypes PDFium edits embedded objects of. Parent doc must
@@ -732,7 +736,7 @@ pdf_annot_remove_object <- function(annot, index) {
 #' @seealso [pdf_annot_append_object()], [pdf_annot_objects()].
 #' @export
 pdf_annot_update_object <- function(annot, obj) {
-  checkmate::assert_class(obj, "pdfium_obj")
+  check_pdfium_obj(obj)
   ctx <- assert_annot_writable(annot)
   expect_setter_ok(cpp_annot_update_object(annot$ptr, obj$ptr),
                     "FPDFAnnot_UpdateObject")
@@ -1175,9 +1179,11 @@ pdf_obj_form_from_xobject <- function(page, xobject) {
 #' Wraps `FPDFFormObj_RemoveObject` + `FPDFPageObj_Destroy`. The child
 #' must currently belong to the form-xobject. PDFium hands the removed
 #' child back to the caller, so it is destroyed straight away and the
-#' `child` handle is closed: further calls on it error cleanly. Other
-#' handles to the child (and, when the child is itself a form, to its
-#' own children) are stale and must not be used; call
+#' `child` handle is closed: further calls on it, on the clip paths
+#' read from it with [pdf_obj_clip_path()] and, when it is itself a
+#' form, on the objects read from it with [pdf_form_objects()], error
+#' cleanly. Other handles to the child, from separate
+#' [pdf_form_objects()] calls, are stale and must not be used; call
 #' [pdf_form_objects()] again for the remaining children.
 #'
 #' The removal changes the page in memory, so rendering and
@@ -1193,7 +1199,7 @@ pdf_obj_form_from_xobject <- function(page, xobject) {
 #' @return Invisibly returns the parent `pdfium_doc`.
 #' @export
 pdf_form_obj_remove_object <- function(form_obj, child) {
-  checkmate::assert_class(child, "pdfium_obj")
+  check_pdfium_obj(child, arg = "child")
   ctx <- assert_obj_writable(form_obj, allowed_types = "form",
                               arg = "form_obj")
   expect_setter_ok(

R/classes.R

@@ -38,22 +38,31 @@ new_pdfium_doc <- function(ptr, path, readwrite = FALSE) {
 #'
 #' Document and page handles check the underlying externalptr for
 #' non-NULL. Page-object handles do not own their lifetime - they
-#' live as long as their parent page - so for a `pdfium_obj` this
-#' delegates to the parent page's open state.
+#' live as long as what owns them (see `obj_owner()`) - so for a
+#' `pdfium_obj` this also requires the owner to be open, and for a
+#' `pdfium_clip_path` the page-object it was read from.
 #'
 #' @param x A `pdfium_handle`.
 #' @return `TRUE` if the underlying PDFium handle is still live,
-#'   `FALSE` if the parent has been closed.
+#'   `FALSE` if it or an owner has been closed.
 #' @keywords internal
 #' @noRd
 is_open <- function(x) {
-  # Page-children: obj + annot lifetimes both pivot on their
-  # parent page. Object normally has no finalizer (page-borrowed)
-  # but `pdf_obj_delete()` clears its externalptr explicitly, so
-  # the own-ptr check is necessary alongside the page check.
-  # Annot has its own finalizer (FPDFPage_CloseAnnot); same
-  # combined-check semantics apply.
-  if (inherits(x, c("pdfium_obj", "pdfium_annot"))) {
+  # Page-children: an object lives as long as its owner, an annotation
+  # as long as its page. Object normally has no finalizer (borrowed)
+  # but `pdf_obj_delete()` and the other mutators clear its
+  # externalptr explicitly, so the own-ptr check is necessary alongside
+  # the owner check. Annot has its own finalizer
+  # (FPDFPage_CloseAnnot); same combined-check semantics apply. The C++
+  # layer walks the same chain through the externalptrs' prot slots
+  # (ADR-029).
+  if (inherits(x, "pdfium_obj")) {
+    return(cpp_handle_is_valid(x$ptr) && is_open(obj_owner(x)))
+  }
+  if (inherits(x, "pdfium_clip_path")) {
+    return(cpp_handle_is_valid(x$ptr) && is_open(x$obj))
+  }
+  if (inherits(x, "pdfium_annot")) {
     return(cpp_handle_is_valid(x$ptr) && is_open(x$page))
   }
   # Doc-children: attachment / signature / bookmark carry no
@@ -145,25 +154,25 @@ print.pdfium_page <- function(x, ...) {
 #' Construct a `pdfium_obj` from an external pointer
 #'
 #' Internal helper. Page objects do not own their own lifetime - they
-#' point into the parent `pdfium_page`'s internal storage and become
-#' dangling when the page closes. The externalptr's `prot` slot holds
-#' the parent page's externalptr so R's GC cannot reclaim the page
-#' while any object reference is live, but there is no finalizer on
-#' the object itself.
+#' point into their owner's internal storage and become dangling when
+#' the owner goes. The owner of a top-level page object is its
+#' `pdfium_page`: the externalptr's `prot` slot holds the page's
+#' externalptr so R's GC cannot reclaim the page while any object
+#' reference is live, but there is no finalizer on the object itself.
 #'
 #' Nested objects (those inside a Form XObject, returned by
 #' [pdf_form_objects()]) additionally carry a `parent_form` field
-#' pointing back at the form's `pdfium_obj`. The form's own lifetime
-#' is still bound to the page externalptr, so the lifetime model is
-#' unchanged; `parent_form` is informational, used by
-#' [format.pdfium_obj()] to render the containment chain.
+#' pointing back at the form's `pdfium_obj`, which owns them: their
+#' `prot` slot pins the form's externalptr, and `is_open()` follows
+#' `parent_form`. `format.pdfium_obj()` also uses it to render the
+#' containment chain.
 #'
 #' Objects inside an annotation's appearance stream (returned by
 #' [pdf_annot_objects()]) carry a `parent_annot` field pointing at the
 #' `pdfium_annot` instead. Their externalptr's `prot` slot pins the
-#' annotation, which owns them, rather than the page, as does that of
-#' the children [pdf_form_objects()] returns for a form among them;
-#' none of them are part of the page's content.
+#' annotation, which owns them, rather than the page; none of them,
+#' nor the children [pdf_form_objects()] returns for a form among
+#' them, are part of the page's content.
 #'
 #' @param ptr An `externalptr` to a PDFium `FPDF_PAGEOBJECT`.
 #' @param page The parent `pdfium_page`.
@@ -197,6 +206,19 @@ new_pdfium_obj <- function(ptr, page, index, type, parent_form = NULL,
   )
 }
 
+# Internal: what owns `obj` and must stay open for it to be valid: the
+# form XObject it is nested in, else the annotation whose appearance
+# stream holds it, else its page.
+obj_owner <- function(obj) {
+  if (!is.null(obj$parent_form)) {
+    return(obj$parent_form)
+  }
+  if (!is.null(obj$parent_annot)) {
+    return(obj$parent_annot)
+  }
+  obj$page
+}
+
 # Internal: the annotation whose appearance stream holds `obj`, directly
 # or inside a Form XObject drawn there; NULL for page content.
 obj_parent_annot <- function(obj) {

R/clip.R

@@ -2,20 +2,19 @@
 # `FPDFClipPath_*` functions plus `FPDFPageObj_GetClipPath`, with
 # a small `pdfium_clip_path` S3 class for handle hygiene.
 
-# Internal constructor. The clip path's lifetime is owned by the
-# parent page; the externalptr already carries the page pointer in
-# its `prot` slot (see cpp_obj_get_clip_path), but we store the
-# pdfium_page on the R-side wrapper too so format/print can show
-# the containment chain.
-new_pdfium_clip_path <- function(ptr, page, source_obj_index, n_paths) {
+# Internal constructor. The clip path is part of the page-object it was
+# read from and lives as long as that object; the externalptr carries
+# the object's externalptr in its `prot` slot (see
+# cpp_obj_get_clip_path), and the R-side wrapper keeps the pdfium_obj,
+# which is_open() follows, and its page, which format/print show.
+new_pdfium_clip_path <- function(ptr, obj, n_paths) {
   checkmate::assert_class(ptr, "externalptr")
-  checkmate::assert_class(page, "pdfium_page")
-  checkmate::assert_number(source_obj_index)
+  checkmate::assert_class(obj, "pdfium_obj")
   checkmate::assert_number(n_paths)
   structure(
     list(
-      ptr = ptr, page = page,
-      source_obj_index = as.integer(source_obj_index),
+      ptr = ptr, obj = obj, page = obj$page,
+      source_obj_index = obj$index,
       n_paths = as.integer(n_paths)
     ),
     class = c("pdfium_clip_path", "pdfium_handle")
@@ -24,7 +23,7 @@ new_pdfium_clip_path <- function(ptr, page, source_obj_index, n_paths) {
 
 #' @export
 format.pdfium_clip_path <- function(x, ...) {
-  state <- if (is_open(x$page)) "open" else "closed"
+  state <- if (is_open(x)) "open" else "closed"
   sprintf(
     "<pdfium_clip_path [%s] %d sub-path(s) from obj %d on page %d>",
     state, x$n_paths, x$source_obj_index, x$page$index
@@ -50,8 +49,16 @@ print.pdfium_clip_path <- function(x, ...) {
 #' `NULL` so callers only see clip-path objects with at least one
 #' real sub-path.
 #'
-#' @param obj A `pdfium_obj` (from [pdf_page_objects()] or
-#'   [pdf_form_objects()]).
+#' The clip path is part of `obj` and is valid for as long as the
+#' `obj` handle is: [pdf_clip_path_count()] and
+#' [pdf_clip_path_segments()] refuse it once `obj` is closed, whether
+#' by closing its page or document, by deleting the annotation or
+#' form object that holds it, or by [pdf_obj_delete()],
+#' [pdf_form_obj_remove_object()] or [pdf_annot_append_object()] on
+#' `obj` itself.
+#'
+#' @param obj A `pdfium_obj` (from [pdf_page_objects()],
+#'   [pdf_form_objects()] or [pdf_annot_objects()]).
 #' @return A `pdfium_clip_path` object, or `NULL` when `obj` has no
 #'   clip path or only an empty one.
 #'
@@ -72,7 +79,7 @@ print.pdfium_clip_path <- function(x, ...) {
 #' @export
 pdf_obj_clip_path <- function(obj) {
   check_pdfium_obj(obj)
-  ptr <- cpp_obj_get_clip_path(obj$ptr, obj$page$ptr)
+  ptr <- cpp_obj_get_clip_path(obj$ptr)
   # FPDFPageObj_GetClipPath returns a handle for every page object
   # (it's a pointer to the obj's `m_ClipPath` member, which exists
   # even when empty), so this branch is defensive against future
@@ -86,7 +93,7 @@ pdf_obj_clip_path <- function(obj) {
   if (n == 0L) {
     return(NULL)
   }
-  new_pdfium_clip_path(ptr, obj$page, obj$index, n)
+  new_pdfium_clip_path(ptr, obj, n)
 }
 
 #' Count sub-paths in a clip path
@@ -100,10 +107,8 @@ pdf_obj_clip_path <- function(obj) {
 #' @export
 pdf_clip_path_count <- function(clip_path) {
   checkmate::assert_class(clip_path, "pdfium_clip_path")
-  if (!is_open(clip_path$page)) {
-    stop("Parent page has been closed; the clip path is no longer valid.",
-      call. = FALSE
-    )
+  if (!is_open(clip_path)) {
+    stop(clip_path_closed_message(clip_path), call. = FALSE)
   }
   cpp_clip_path_count_paths(clip_path$ptr)
 }
@@ -136,10 +141,8 @@ pdf_clip_path_count <- function(clip_path) {
 #' @export
 pdf_clip_path_segments <- function(clip_path) {
   checkmate::assert_class(clip_path, "pdfium_clip_path")
-  if (!is_open(clip_path$page)) {
-    stop("Parent page has been closed; the clip path is no longer valid.",
-      call. = FALSE
-    )
+  if (!is_open(clip_path)) {
+    stop(clip_path_closed_message(clip_path), call. = FALSE)
   }
   raw <- cpp_clip_path_segments_df(clip_path$ptr)
   # PDFium segment-type ints: 0 = LINETO, 1 = BEZIERTO, 2 = MOVETO.