Merge PR #3985: M6: a document script that throws no longer stops the form's other scripts (#3940)

crates/automation/src/tools.rs: js_run description: both sides' wording merged.

crates/automation/src/a11y.rs

@@ -669,7 +669,9 @@ impl Automation {
         let id = self.doc(a)?.id;
         let o = self.session.run_javascript(id, a.str("script")?, a.opt_str("field")?).map_err(failed)?;
         let reqs: Vec<Value> = o.requests.iter().map(request_json).collect();
-        Ok(json!({ "alerts": o.alerts, "console": o.console, "requests": reqs, "error": o.error, "result": o.result }))
+        Ok(
+            json!({ "alerts": o.alerts, "console": o.console, "requests": reqs, "error": o.error, "document_errors": o.doc_errors, "result": o.result }),
+        )
     }
 
     pub(crate) fn js_document_scripts(&self, a: &Args) -> Result<Value> {

crates/automation/src/tools.rs

@@ -1110,7 +1110,7 @@ pub fn tools() -> Vec<ToolDef> {
         t("form_merge_data", "Merge data files into spreadsheet", "Collect the field values of form data files (FDF, XFDF) or filled-in PDF forms into one CSV file at path: a column per field name, a row per file. Returns the row and column counts.")
             .cmd("form.merge_data")
             .with(schema(json!({ "paths": { "type": "array", "items": { "type": "string" }, "minItems": 1 }, "path": { "type": "string" } }), &["paths", "path"])),
-        t("js_run", "Run JavaScript", "Run Acrobat JavaScript in the document, as the JavaScript console does (or as push button `field`'s Mouse Up script when field is given; on a laid-out XFA form, `field` runs that button's XFA click script instead, JavaScript or FormCalc, which can add and remove rows and show or hide subforms). The form object model is available: this/getField, event, app, util, console, display, color, and the document-level scripts (except the PDF container scripts when XFA layout is active; they remain available through js_document_scripts). Field changes and resetForm are applied as one undoable step; returns the script's alerts, console output, requests (print, page, url, submit) and error.")
+        t("js_run", "Run JavaScript", "Run Acrobat JavaScript in the document, as the JavaScript console does (or as push button `field`'s Mouse Up script when field is given; on a laid-out XFA form, `field` runs that button's XFA click script instead, JavaScript or FormCalc, which can add and remove rows and show or hide subforms). The form object model is available: this/getField, event, app, util, console, display, color, and the document-level scripts (except the PDF container scripts when XFA layout is active; they remain available through js_document_scripts). Field changes and resetForm are applied as one undoable step; returns the script's alerts, console output, requests (print, page, url, submit), error, and the errors of document-level scripts that failed (the rest still ran).")
             .cmd("tools.js_console")
             .with(schema(
                 json!({ "doc": doc(), "script": { "type": "string" }, "field": { "type": "string", "description": "Run as this button's Mouse Up event." } }),

crates/automation/tests/it/automation.rs

@@ -5990,6 +5990,12 @@ fn javascript_through_tools() {
     assert_eq!(total["value"], "3750");
     let r = ok(&mut a, "js_run", json!({ "doc": doc, "script": "nope()" }));
     assert!(r["error"].as_str().unwrap().contains("nope"));
+    assert_eq!(r["document_errors"], json!([]));
+    // A document script that throws (#3940) is reported, and the others and the script still run.
+    ok(&mut a, "js_set_document_script", json!({ "doc": doc, "name": "zz", "script": "getField('Total').buttonSetIcon(null);" }));
+    let r = ok(&mut a, "js_run", json!({ "doc": doc, "script": "money(5)" }));
+    assert_eq!((r["error"].clone(), r["result"].clone()), (json!(null), json!("EUR 5,00")));
+    assert!(r["document_errors"][0].as_str().unwrap().starts_with("document script 2: TypeError"), "{r}");
     assert_eq!(ok(&mut a, "js_enabled", json!({ "enabled": false }))["enabled"], false);
     assert!(a.call("js_run", &json!({ "doc": doc, "script": "1" })).is_err());
 }

crates/engine/src/js.rs

@@ -19,6 +19,10 @@ pub struct JsOutput {
     pub requests: Vec<Request>,
     /// Script errors (as Acrobat's console shows them).
     pub errors: Vec<String>,
+    /// Which of `errors` are document scripts' (#3940), told apart by where they came from and
+    /// not by their text, which a field script's error can share: each is reported once, however
+    /// often the document scripts run again.
+    pub doc_errors: Vec<String>,
 }
 
 impl JsOutput {
@@ -31,13 +35,33 @@ impl JsOutput {
         self.console.extend(o.console.iter().cloned());
         self.requests.extend(o.requests.iter().cloned());
         self.errors.extend(o.error.iter().cloned());
+        // The document scripts run before every field script, so a failing one would repeat
+        // once per calculation; it is reported once.
+        for e in &o.doc_errors {
+            if !self.doc_errors.contains(e) {
+                self.doc_errors.push(e.clone());
+                self.errors.push(e.clone());
+            }
+        }
     }
 
     pub(crate) fn append(&mut self, other: JsOutput) {
         self.alerts.extend(other.alerts);
         self.console.extend(other.console);
         self.requests.extend(other.requests);
-        self.errors.extend(other.errors);
+        // As in `absorb`: a document script's error, already reported, is not repeated; a field
+        // script's errors are each their own, even one worded like a document script's.
+        let mut docs = other.doc_errors;
+        for e in other.errors {
+            if let Some(k) = docs.iter().position(|d| *d == e) {
+                docs.swap_remove(k);
+                if self.doc_errors.contains(&e) {
+                    continue;
+                }
+                self.doc_errors.push(e.clone());
+            }
+            self.errors.push(e);
+        }
     }
 }
 
@@ -454,6 +478,32 @@ mod review_tests {
         assert!(read <= 4 * n, "{read} characters read for {n} glyphs");
     }
 
+    /// Review on #3985: merging two outputs reports a failing document script once, as one run
+    /// does, while each field script error stays: told apart by where an error came from, so a
+    /// field script that throws a document script's very words is still reported.
+    #[test]
+    fn merged_output_reports_a_document_script_error_once() {
+        let doc = "document script 2: TypeError: not a callable function";
+        let field = "ReferenceError: x is not defined";
+        let run = |error: &str| {
+            let mut out = JsOutput::default();
+            out.absorb(&Outcome { error: Some(error.into()), doc_errors: vec![doc.into()], ..Outcome::default() });
+            out
+        };
+        let mut out = run(field);
+        out.append(run(field));
+        assert_eq!(out.errors, [field, doc, field]);
+        // CodeRabbit on #3985: a field script that throws the document script's text.
+        out.append(run(doc));
+        assert_eq!(out.errors, [field, doc, field, doc]);
+        let alone = run(doc);
+        assert_eq!(alone.errors, [doc, doc], "the field's error and the document script's");
+        let count = |o: &JsOutput| o.errors.iter().filter(|e| *e == doc).count();
+        let mut merged = run(field);
+        merged.append(alone);
+        assert_eq!(count(&merged), 2, "the field's copy stays; the document script's is listed once");
+    }
+
     #[test]
     fn duplicate_field_names_diff_against_first_original() {
         let first = pdfcraft_js::FieldState::new("duplicate", pdfcraft_js::FieldType::Text, vec!["first".into()]);

crates/engine/src/tests.rs

@@ -4110,6 +4110,11 @@ fn recognize_text_reads_a_picture_on_a_page_that_has_text() {
 /// A form whose scripts are custom JavaScript: total = price × qty (calculate, through a
 /// document-level function), shown with a custom format; qty is validated; a button script.
 fn scripted_form() -> Vec<u8> {
+    scripted_form_with("function times\\(a, b\\) { console.println('times'); return a * b; }")
+}
+
+/// [`scripted_form`] with `helpers` (PDF string syntax) as its document-level script.
+fn scripted_form_with(helpers: &str) -> Vec<u8> {
     let text = |name: &str, y: u32, aa: &str| {
         format!("<< /Type /Annot /Subtype /Widget /FT /Tx /T ({name}) /Rect [20 {y} 180 {}] /P 3 0 R /F 4 {aa} >>", y + 20)
     };
@@ -4123,7 +4128,7 @@ fn scripted_form() -> Vec<u8> {
         "<< /Type /Annot /Subtype /Widget /FT /Btn /Ff 65536 /T (go) /Rect [20 150 80 170] /P 3 0 R /F 4 >>".to_string(),
         "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".to_string(),
         "<< >>".to_string(),
-        "<< /S /JavaScript /JS (function times\\(a, b\\) { console.println('times'); return a * b; }) >>".to_string(),
+        format!("<< /S /JavaScript /JS ({helpers}) >>"),
     ];
     let mut out = b"%PDF-1.7\n".to_vec();
     let mut offsets = Vec::new();
@@ -4185,6 +4190,27 @@ fn scripts_in_an_xfa_form_see_the_xfa_plugin_installed() {
     assert_eq!((o.error, o.result.as_deref()), (None, Some("undefined number 3.5")));
 }
 
+/// #3940: a document-level script that defines the helper and then calls a method PdfCraft lacks
+/// (an Acrobat form copying a button icon at open) left every calculated field blank. The
+/// helper stays defined, the form calculates, and the error is reported once.
+#[test]
+fn a_document_script_that_throws_still_lets_the_form_calculate() {
+    let helpers =
+        "function times\\(a, b\\) { return a * b; } this.getField\\('go'\\).buttonSetIcon\\(this.getField\\('go'\\).buttonGetIcon\\(\\)\\);";
+    let mut s = Session::new().with_clock(|| 1_700_000_000);
+    let id = s.open("order.pdf", None, Arc::new(scripted_form_with(helpers)), None).unwrap();
+    let _ = s.take_js_output(id);
+    s.apply(id, Edit::SetFieldValue { name: "qty".into(), value: FieldValue::Text("4".into()) }).unwrap();
+    let total = s.get(id).unwrap().form.iter().find(|f| f.name == "total").unwrap().value.clone();
+    assert_eq!(total, ["10"], "calculated through the helper the failing script defined");
+    let out = s.take_js_output(id);
+    assert_eq!(out.errors.len(), 1, "reported once, not once per script: {out:?}");
+    assert!(out.errors[0].starts_with("document script 1: TypeError"), "{out:?}");
+    // The console runs too, and says which document script failed.
+    let o = s.run_javascript(id, "times(2, 3)", None).unwrap();
+    assert_eq!((o.error, o.result.as_deref(), o.doc_errors.len()), (None, Some("6"), 1));
+}
+
 #[test]
 fn active_xfa_does_not_run_pdf_container_scripts_but_keeps_requested_alerts() {
     let template = pdfcraft_xfa::fixtures::scripted_template().replace(

crates/js/src/lib.rs

@@ -181,6 +181,9 @@ pub struct Outcome {
     pub requests: Vec<Request>,
     /// The script's error (syntax error, exception, or a limit reached), if it failed.
     pub error: Option<String>,
+    /// Errors in the document-level scripts that ran first. Each is a script of its own: one
+    /// that throws stops where it failed, and the others and the script itself still run (#3940).
+    pub doc_errors: Vec<String>,
     /// The script's completion value (what the console prints), unless undefined.
     pub result: Option<String>,
 }
@@ -1204,16 +1207,31 @@ pub fn run(script: &str, event: &Event, doc: &DocInfo, fields: &[FieldState], do
 /// script can observe uncommitted writes. Read-only calculations avoid copying the whole form.
 pub fn run_shared(script: &str, event: &Event, doc: &DocInfo, fields: Arc<Vec<FieldState>>, doc_scripts: &[String], limits: Limits) -> Outcome {
     let failed = |why: String| Outcome { rc: true, value: event.value.clone(), change: event.change.clone(), error: Some(why), ..Default::default() };
-    if let Some(why) = std::iter::once(script).chain(doc_scripts.iter().map(String::as_str)).find_map(refuse) {
+    if let Some(why) = refuse(script) {
         return failed(why);
     }
+    // A document script too long or too deeply nested to run safely is skipped and reported,
+    // as one that throws is; the others and the script still run (#3940).
+    let mut skipped = Vec::new();
+    let kept: Vec<(usize, &str)> = doc_scripts
+        .iter()
+        .enumerate()
+        .filter_map(|(i, d)| match refuse(d) {
+            Some(why) => {
+                skipped.push((i, why));
+                None
+            }
+            None => Some((i, d.as_str())),
+        })
+        .collect();
+    let (kept, skipped) = (&kept, &skipped);
     #[cfg(not(target_arch = "wasm32"))]
     {
         std::thread::scope(|scope| {
             let spawned = std::thread::Builder::new()
                 .name("pdfcraft-js".into())
                 .stack_size(SCRIPT_STACK)
-                .spawn_scoped(scope, || run_here(script, event, doc, fields, doc_scripts, limits));
+                .spawn_scoped(scope, || run_here(script, event, doc, fields, kept, skipped, limits));
             match spawned {
                 Ok(thread) => thread.join().unwrap_or_else(|_| failed("the script stopped with an internal error".into())),
                 Err(e) => failed(format!("the script engine could not start: {e}")),
@@ -1221,7 +1239,7 @@ pub fn run_shared(script: &str, event: &Event, doc: &DocInfo, fields: Arc<Vec<Fi
         })
     }
     #[cfg(target_arch = "wasm32")]
-    run_here(script, event, doc, fields, doc_scripts, limits)
+    run_here(script, event, doc, fields, kept, skipped, limits)
 }
 
 /// Host hooks that refuse to compile code from strings (`eval`, `Function` and its generator
@@ -1256,7 +1274,22 @@ impl HostHooks for CheckedCodeFromStrings {
     }
 }
 
-fn run_here(script: &str, event: &Event, doc: &DocInfo, fields: Arc<Vec<FieldState>>, doc_scripts: &[String], limits: Limits) -> Outcome {
+/// How [`Outcome::doc_errors`] names a failing document script: by its 1-based place.
+fn doc_error(index: usize, error: &dyn std::fmt::Display) -> String {
+    format!("document script {}: {error}", index.saturating_add(1))
+}
+
+/// `doc_scripts` are the document scripts to run with their places among all of them;
+/// `skipped` the ones refused before running, with the reason.
+fn run_here(
+    script: &str,
+    event: &Event,
+    doc: &DocInfo,
+    fields: Arc<Vec<FieldState>>,
+    doc_scripts: &[(usize, &str)],
+    skipped: &[(usize, String)],
+    limits: Limits,
+) -> Outcome {
     let mut ctx = match Context::builder().host_hooks(std::rc::Rc::new(NoCodeFromStrings)).build() {
         Ok(ctx) => ctx,
         Err(e) => {
@@ -1277,12 +1310,18 @@ fn run_here(script: &str, event: &Event, doc: &DocInfo, fields: Arc<Vec<FieldSta
         dropped: 0,
     }));
     let mut out = Outcome { rc: true, value: event.value.clone(), change: event.change.clone(), ..Default::default() };
+    let mut doc_errors: Vec<(usize, String)> = skipped.iter().map(|(i, why)| (*i, doc_error(*i, why))).collect();
     let result = (|| -> JsResult<()> {
         install(&mut ctx)?;
         let ev = event_object(&mut ctx, event);
         ctx.register_global_property(js_string!("event"), ev.clone(), Attribute::all())?;
-        for d in doc_scripts {
-            ctx.eval(Source::from_bytes(d))?;
+        // One document script that throws (calling a method PdfCraft lacks, say) must not keep
+        // every field script in the form from running: what it defined before the error, and
+        // the other scripts, stay available (#3940).
+        for &(i, d) in doc_scripts {
+            if let Err(e) = ctx.eval(Source::from_bytes(d)) {
+                doc_errors.push((i, doc_error(i, &e)));
+            }
         }
         let r = ctx.eval(Source::from_bytes(script))?;
         if !r.is_undefined() {
@@ -1299,6 +1338,8 @@ fn run_here(script: &str, event: &Event, doc: &DocInfo, fields: Arc<Vec<FieldSta
     if let Err(e) = result {
         out.error = Some(e.to_string());
     }
+    doc_errors.sort_by_key(|(i, _)| *i);
+    out.doc_errors = doc_errors.into_iter().map(|(_, e)| e).collect();
     if let Some(h) = ctx.remove_data::<Shared>() {
         let h = h.into_inner();
         out.changed = h.changed.iter().map(|i| h.fields[*i].clone()).collect();

crates/js/src/tests.rs

@@ -297,6 +297,61 @@ fn document_level_functions_are_available() {
     assert_eq!(o.value, "8");
 }
 
+/// #3940: a document script that throws after defining a helper (here calling an Acrobat method
+/// PdfCraft lacks) still leaves the helper, and the later scripts, to the field script.
+#[test]
+fn a_failing_document_script_does_not_stop_the_field_script() {
+    let docs = [
+        "function double(x) { return x * 2; }\nthis.getField('qty').buttonSetIcon(null);\nvar after = 1;".to_string(),
+        "function plus(x) { return x + 1; }".to_string(),
+        "this is not javascript".to_string(),
+    ];
+    let o = run(
+        "event.value = plus(double(getField('qty').value));",
+        &Event::field("Calculate", "total", ""),
+        &doc(),
+        &fields(),
+        &docs,
+        Limits::default(),
+    );
+    assert_eq!((o.error.as_deref(), o.value.as_str()), (None, "9"));
+    assert_eq!(o.doc_errors.len(), 2, "{:?}", o.doc_errors);
+    assert!(o.doc_errors[0].starts_with("document script 1: TypeError"), "{:?}", o.doc_errors);
+    assert!(o.doc_errors[1].starts_with("document script 3: SyntaxError"), "{:?}", o.doc_errors);
+    // The failing script stopped where it failed.
+    let o = run("event.value = typeof after;", &Event::field("Calculate", "total", ""), &doc(), &fields(), &docs, Limits::default());
+    assert_eq!(o.value, "undefined");
+    // Working document scripts report nothing.
+    assert!(go("event.value = 1;", &Event::field("Calculate", "total", "")).doc_errors.is_empty());
+}
+
+/// A document script refused before running (too long, or nested too deeply to run safely) is
+/// skipped like one that throws: never run, reported in its place, the rest still run.
+#[test]
+fn a_document_script_too_big_to_run_is_skipped_and_reported() {
+    let docs = [
+        "this.getField('qty').buttonSetIcon(null);".to_string(),
+        format!("var big = 1{};", "+1".repeat(MAX_SCRIPT_BYTES / 2)),
+        "function double(x) { return x * 2; }".to_string(),
+        format!("var deep = {}1{};", "(".repeat(10_000), ")".repeat(10_000)),
+    ];
+    let o = run(
+        "event.value = double(getField('qty').value) + typeof big;",
+        &Event::field("Calculate", "total", ""),
+        &doc(),
+        &fields(),
+        &docs,
+        Limits::default(),
+    );
+    assert_eq!((o.error.as_deref(), o.value.as_str()), (None, "8undefined"));
+    let places: Vec<&str> = o.doc_errors.iter().map(|e| e.split(':').next().unwrap_or_default()).collect();
+    assert_eq!(places, ["document script 1", "document script 2", "document script 4"], "{:?}", o.doc_errors);
+    assert!(o.doc_errors[1].contains("too long") && o.doc_errors[2].contains("nested too deeply"), "{:?}", o.doc_errors);
+    // The field script's own limits still refuse it outright.
+    let o = run(&format!("1{}", "+1".repeat(MAX_SCRIPT_BYTES)), &Event::field("Calculate", "total", ""), &doc(), &fields(), &docs, Limits::default());
+    assert!(o.error.is_some_and(|e| e.contains("too long")) && o.doc_errors.is_empty());
+}
+
 #[test]
 fn errors_and_runaway_scripts_are_contained() {
     let o = go("event.value = nosuch.thing;", &Event::field("Calculate", "total", "keep"));
@@ -336,9 +391,11 @@ fn deeply_nested_scripts_fail_instead_of_crashing() {
         let o = go(s, &Event::doc("Open"));
         assert!(o.error.is_some(), "{}…", &s[..20]);
     }
-    // A hostile document-level script is refused too, before the field script runs.
+    // A hostile document-level script is refused too: it never runs, and is reported, while
+    // the field script still does (#3940).
     let o = run("event.value = 'ran';", &Event::field("Calculate", "a", ""), &doc(), &fields(), &[hostile[0].clone()], Limits::default());
-    assert!(o.error.is_some());
+    assert_eq!((o.error, o.value.as_str()), (None, "ran"));
+    assert!(o.doc_errors.len() == 1 && o.doc_errors[0].contains("nested too deeply"), "{:?}", o.doc_errors);
 }
 
 #[test]

crates/ui-egui/src/js_ui.rs

@@ -88,7 +88,13 @@ impl PdfCraftApp {
                 {
                     self.views[i].document_changed(&info);
                 }
-                let out = JsOutput { alerts: o.alerts, console: o.console, requests: o.requests, errors: o.error.into_iter().collect() };
+                let out = JsOutput {
+                    alerts: o.alerts,
+                    console: o.console,
+                    requests: o.requests,
+                    errors: o.error.into_iter().collect(),
+                    doc_errors: Vec::new(),
+                };
                 self.handle_js(id, out);
             }
             Err(e) => self.notify_fmt("{field}: {e}", &[("field", field), ("e", &e.to_string())]),
@@ -128,7 +134,7 @@ impl PdfCraftApp {
                     self.views[i].document_changed(&info);
                 }
                 let (error, result) = (o.error.clone(), o.result.clone());
-                let out = JsOutput { alerts: o.alerts, console: o.console, requests: o.requests, errors: Vec::new() };
+                let out = JsOutput { alerts: o.alerts, console: o.console, requests: o.requests, errors: Vec::new(), doc_errors: Vec::new() };
                 self.handle_js(id, out);
                 match error {
                     Some(e) => self.js_console.log.push(format!("Error: {e}")),

parity/acrobat-features.toml

@@ -5141,8 +5141,8 @@ milestone = "M6"
 status = "shipped"
 commands = ["tools.document_js"]
 tools = ["js_document_scripts", "js_set_document_script"]
-evidence = ["crates/ui-egui/tests/it/javascript.rs::document_scripts_and_preferences", "crates/automation/tests/it/automation.rs::javascript_through_tools", "crates/engine/src/tests.rs::form_javascript_validates_calculates_and_formats"]
-notes = "Names /JavaScript tree: list, add, edit, delete; functions are available to field scripts."
+evidence = ["crates/ui-egui/tests/it/javascript.rs::document_scripts_and_preferences", "crates/automation/tests/it/automation.rs::javascript_through_tools", "crates/engine/src/tests.rs::form_javascript_validates_calculates_and_formats", "crates/engine/src/tests.rs::a_document_script_that_throws_still_lets_the_form_calculate"]
+notes = "Names /JavaScript tree: list, add, edit, delete; functions are available to field scripts. A script that throws (calling an Acrobat method PdfCraft lacks, say) stops where it failed, but the others and the field scripts still run and the error is reported once (js_run's document_errors, the console) (#3940). Not like Acrobat: the scripts run again before every field script instead of once when the document opens, so what they do at top level repeats."
 
 [[feature]]
 id = "form.document-actions"