diff --git a/crates/automation/src/a11y.rs b/crates/automation/src/a11y.rs index a7334f206..59dc985bd 100644 --- a/crates/automation/src/a11y.rs +++ b/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 = 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 { diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 725dde4bf..57e1c4ae4 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -1110,7 +1110,7 @@ pub fn tools() -> Vec { 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." } }), diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index f1637db20..e6bb3377a 100644 --- a/crates/automation/tests/it/automation.rs +++ b/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()); } diff --git a/crates/engine/src/js.rs b/crates/engine/src/js.rs index c3a6a8ef7..78e026e24 100644 --- a/crates/engine/src/js.rs +++ b/crates/engine/src/js.rs @@ -19,6 +19,10 @@ pub struct JsOutput { pub requests: Vec, /// Script errors (as Acrobat's console shows them). pub errors: Vec, + /// 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, } 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()]); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 3d91b0c69..e83dbe207 100644 --- a/crates/engine/src/tests.rs +++ b/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 { + 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 { 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 { "<< /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( diff --git a/crates/js/src/lib.rs b/crates/js/src/lib.rs index 13bdc4ba3..f6d5a076c 100644 --- a/crates/js/src/lib.rs +++ b/crates/js/src/lib.rs @@ -181,6 +181,9 @@ pub struct Outcome { pub requests: Vec, /// The script's error (syntax error, exception, or a limit reached), if it failed. pub error: Option, + /// 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, /// The script's completion value (what the console prints), unless undefined. pub result: Option, } @@ -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>, 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>, 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>, + 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 = 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() { let h = h.into_inner(); out.changed = h.changed.iter().map(|i| h.fields[*i].clone()).collect(); diff --git a/crates/js/src/tests.rs b/crates/js/src/tests.rs index b2e866fae..7124af144 100644 --- a/crates/js/src/tests.rs +++ b/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] diff --git a/crates/ui-egui/src/js_ui.rs b/crates/ui-egui/src/js_ui.rs index 7bc3dd044..f5ec01625 100644 --- a/crates/ui-egui/src/js_ui.rs +++ b/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}")), diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 96504bcb0..7d9e221b0 100644 --- a/parity/acrobat-features.toml +++ b/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"