Have renderPlot() error early if height/width of a plot aren't yet …

…defined (#3739)

* Close #3704. Close #3735. Close #1409. Throw informative error in renderPlot() early if height/width of a plot aren't yet defined

* `devtools::document()` (GitHub Actions)

* Add unit tests

* Use consistent filename; add intentional failure (to get artifact uploads)

* Make output id argument name more unique

* Update news

* plotPNG() test isn't worth it

* Don't try to provide a suggestion on how to fix the issue (it's no worse than what we currently have, and we probably should be defaulting to an 'arbitrary' size anyway

* update news

* minimize diff

Co-authored-by: cpsievert <cpsievert@users.noreply.github.com>

DESCRIPTION

@@ -202,7 +202,7 @@ Collate:
     'version_selectize.R'
     'version_strftime.R'
     'viewer.R'
-RoxygenNote: 7.2.1
+RoxygenNote: 7.2.2
 Encoding: UTF-8
 Roxygen: list(markdown = TRUE)
 RdMacros: lifecycle

NEWS.md

@@ -15,6 +15,8 @@ shiny 1.7.3.9000
 
 ### Bug fixes
 
+* Closed #3704 and #3735: `renderPlot()` no longer leads to a segfault when it executes before the output size is known. (#3739)
+
 * Closed #3687: Updated jQuery-UI to v1.13.2. (#3697)
 
 

R/imageutils.R

@@ -11,7 +11,27 @@ startPNG <- function(filename, width, height, res, ...) {
     grDevices::png
   }
 
-  args <- rlang::list2(filename=filename, width=width, height=height, res=res, ...)
+  # It's possible for width/height to be NULL (e.g., when using
+  # suspendWhenHidden=F w/ tabsetPanel()), which will lead to an error when
+  # attempting to open the device (rstudio/shiny#1409). In this case, ragg will
+  # actually segfault (instead of error), so explicitly throw an error before
+  # opening the device (r-lib/ragg#116, rstudio/shiny#3704)
+  check_empty_png_size <- function(x) {
+    if (length(x) > 0) return(x)
+
+    rlang::abort(
+      paste0("Invalid plot `", substitute(x), "`."),
+      call = rlang::caller_env()
+    )
+  }
+
+  args <- rlang::list2(
+    filename = filename,
+    width = check_empty_png_size(width),
+    height = check_empty_png_size(height),
+    res = res,
+    ...
+  )
 
   # Set a smarter default for the device's bg argument (based on thematic's global state).
   # Note that, technically, this is really only needed for CairoPNG, since the other

tests/testthat/test-plot-png.R

@@ -0,0 +1,23 @@
+test_that("startPNG() throws informative error", {
+  tmp <- tempfile(fileext = '.png')
+
+  expect_error(
+    startPNG(
+      filename = tmp,
+      width = NULL,
+      height = 100,
+      res = 72
+    ),
+    "Invalid plot `width`."
+  )
+
+  expect_error(
+    startPNG(
+      filename = tmp,
+      width = 100,
+      height = NULL,
+      res = 72
+    ),
+    "Invalid plot `height`."
+  )
+})