From 11a97e9de78304a6eae6d94682ae5a07f69d8eb3 Mon Sep 17 00:00:00 2001 From: Jeff Charles Date: Tue, 19 Nov 2024 15:46:17 -0500 Subject: [PATCH 1/5] Address memory leak in JSON parse and stringify --- crates/javy/CHANGELOG.md | 5 +++++ crates/javy/src/apis/json.rs | 17 ++++++++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/crates/javy/CHANGELOG.md b/crates/javy/CHANGELOG.md index cd643474..954525da 100644 --- a/crates/javy/CHANGELOG.md +++ b/crates/javy/CHANGELOG.md @@ -8,6 +8,11 @@ Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + +- Addressed memory leak when registering `JSON.parse` and `JSON.stringify` + functions. + ## [3.0.2] - 2024-11-12 ### Changed diff --git a/crates/javy/src/apis/json.rs b/crates/javy/src/apis/json.rs index 10de0ef3..17c4a584 100644 --- a/crates/javy/src/apis/json.rs +++ b/crates/javy/src/apis/json.rs @@ -58,15 +58,21 @@ impl Intrinsic for Json { fn register<'js>(this: Ctx<'js>) -> Result<()> { let global = this.globals(); + + const DEFAULT_PARSE_KEY: &str = "__javy_json_parse"; + const DEFAULT_STRINGIFY_KEY: &str = "__javy_json_stringify"; + let json: Object = global.get("JSON")?; let default_parse: Function = json.get("parse")?; + global.set(DEFAULT_PARSE_KEY, default_parse)?; let default_stringify: Function = json.get("stringify")?; + global.set(DEFAULT_STRINGIFY_KEY, default_stringify)?; let parse = Function::new( this.clone(), - MutFn::new(move |cx: Ctx<'js>, args: Rest>| { - call_json_parse(hold!(cx.clone(), args), default_parse.clone()) - .map_err(|e| to_js_error(cx, e)) + MutFn::new(|cx: Ctx<'js>, args: Rest>| { + let default_parse: Function = cx.globals().get(DEFAULT_PARSE_KEY)?; + call_json_parse(hold!(cx.clone(), args), default_parse).map_err(|e| to_js_error(cx, e)) }), )?; @@ -78,8 +84,9 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { let stringify = Function::new( this.clone(), - MutFn::new(move |cx: Ctx<'js>, args: Rest>| { - call_json_stringify(hold!(cx.clone(), args), default_stringify.clone()) + MutFn::new(|cx: Ctx<'js>, args: Rest>| { + let default_stringify: Function = cx.globals().get(DEFAULT_STRINGIFY_KEY)?; + call_json_stringify(hold!(cx.clone(), args), default_stringify) .map_err(|e| to_js_error(cx, e)) }), )?; From f803afca05f0ece97f6e36acfb6fcb7b2921ecc8 Mon Sep 17 00:00:00 2001 From: Jeff Charles Date: Wed, 20 Nov 2024 09:53:48 -0500 Subject: [PATCH 2/5] Only perform function lookups if we need to call the fallback --- crates/javy/src/apis/json.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/crates/javy/src/apis/json.rs b/crates/javy/src/apis/json.rs index 17c4a584..d5223ad5 100644 --- a/crates/javy/src/apis/json.rs +++ b/crates/javy/src/apis/json.rs @@ -38,6 +38,9 @@ use std::{ ptr::NonNull, }; +const DEFAULT_PARSE_KEY: &str = "__javy_json_parse"; +const DEFAULT_STRINGIFY_KEY: &str = "__javy_json_stringify"; + /// Intrinsic to attach faster JSON.{parse/stringify} functions. pub struct Json; @@ -59,9 +62,6 @@ impl Intrinsic for Json { fn register<'js>(this: Ctx<'js>) -> Result<()> { let global = this.globals(); - const DEFAULT_PARSE_KEY: &str = "__javy_json_parse"; - const DEFAULT_STRINGIFY_KEY: &str = "__javy_json_stringify"; - let json: Object = global.get("JSON")?; let default_parse: Function = json.get("parse")?; global.set(DEFAULT_PARSE_KEY, default_parse)?; @@ -71,8 +71,7 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { let parse = Function::new( this.clone(), MutFn::new(|cx: Ctx<'js>, args: Rest>| { - let default_parse: Function = cx.globals().get(DEFAULT_PARSE_KEY)?; - call_json_parse(hold!(cx.clone(), args), default_parse).map_err(|e| to_js_error(cx, e)) + call_json_parse(hold!(cx.clone(), args)).map_err(|e| to_js_error(cx, e)) }), )?; @@ -85,9 +84,7 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { let stringify = Function::new( this.clone(), MutFn::new(|cx: Ctx<'js>, args: Rest>| { - let default_stringify: Function = cx.globals().get(DEFAULT_STRINGIFY_KEY)?; - call_json_stringify(hold!(cx.clone(), args), default_stringify) - .map_err(|e| to_js_error(cx, e)) + call_json_stringify(hold!(cx.clone(), args)).map_err(|e| to_js_error(cx, e)) }), )?; @@ -102,7 +99,7 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { Ok(()) } -fn call_json_parse<'a>(args: Args<'a>, default: Function<'a>) -> Result> { +fn call_json_parse(args: Args<'_>) -> Result> { let (this, args) = args.release(); match args.len() { @@ -141,6 +138,7 @@ fn call_json_parse<'a>(args: Args<'a>, default: Function<'a>) -> Result(args: Args<'a>, default: Function<'a>) -> Result(args: Args<'a>, default: Function<'a>) -> Result> { +fn call_json_stringify(args: Args<'_>) -> Result> { let (this, args) = args.release(); match args.len() { @@ -181,6 +179,7 @@ fn call_json_stringify<'a>(args: Args<'a>, default: Function<'a>) -> Result Date: Wed, 20 Nov 2024 17:24:14 -0500 Subject: [PATCH 3/5] Replace fallback for stringify with rquickjs builtin --- crates/javy/src/apis/json.rs | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/crates/javy/src/apis/json.rs b/crates/javy/src/apis/json.rs index d5223ad5..0f42d31e 100644 --- a/crates/javy/src/apis/json.rs +++ b/crates/javy/src/apis/json.rs @@ -39,7 +39,6 @@ use std::{ }; const DEFAULT_PARSE_KEY: &str = "__javy_json_parse"; -const DEFAULT_STRINGIFY_KEY: &str = "__javy_json_stringify"; /// Intrinsic to attach faster JSON.{parse/stringify} functions. pub struct Json; @@ -65,8 +64,6 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { let json: Object = global.get("JSON")?; let default_parse: Function = json.get("parse")?; global.set(DEFAULT_PARSE_KEY, default_parse)?; - let default_stringify: Function = json.get("stringify")?; - global.set(DEFAULT_STRINGIFY_KEY, default_stringify)?; let parse = Function::new( this.clone(), @@ -174,22 +171,18 @@ fn call_json_stringify(args: Args<'_>) -> Result> { let str = JSString::from_str(this, &str)?; Ok(str.into_value()) } - _ => { - // Similar to the parse case, if there's more than one argument, - // defer to the built-in JSON.stringify, which will take care of - // validating invoking the replacer function and/or applying the - // space argument. - let default: Function = this.globals().get(DEFAULT_STRINGIFY_KEY)?; - if args.len() == 2 { - default - .call((args[0].clone(), args[1].clone())) - .map_err(anyhow::Error::new) - } else { - default - .call((args[0].clone(), args[1].clone(), args[2].clone())) - .map_err(anyhow::Error::new) - } - } + 2 => Ok(this + .json_stringify_replacer(args[0].clone(), args[1].clone())? + .map_or_else( + || Value::new_undefined(this.clone()), + |str| str.into_value(), + )), + _ => Ok(this + .json_stringify_replacer_space(args[0].clone(), args[1].clone(), args[2].clone())? + .map_or_else( + || Value::new_undefined(this.clone()), + |str| str.into_value(), + )), } } From 71cf16f9ab671a85e9e4dbbf530b39ed39ed5268 Mon Sep 17 00:00:00 2001 From: Jeff Charles Date: Thu, 21 Nov 2024 10:58:09 -0500 Subject: [PATCH 4/5] Use unstable key for default JSON.parse function --- crates/javy/src/apis/json.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/crates/javy/src/apis/json.rs b/crates/javy/src/apis/json.rs index 0f42d31e..f418eb20 100644 --- a/crates/javy/src/apis/json.rs +++ b/crates/javy/src/apis/json.rs @@ -36,9 +36,11 @@ use anyhow::{anyhow, bail, Result}; use std::{ io::{Read, Write}, ptr::NonNull, + sync::OnceLock, + time::SystemTime, }; -const DEFAULT_PARSE_KEY: &str = "__javy_json_parse"; +static DEFAULT_PARSE_KEY: OnceLock = OnceLock::new(); /// Intrinsic to attach faster JSON.{parse/stringify} functions. pub struct Json; @@ -63,11 +65,16 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { let json: Object = global.get("JSON")?; let default_parse: Function = json.get("parse")?; - global.set(DEFAULT_PARSE_KEY, default_parse)?; + let millis = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH)? + .subsec_millis(); + // Make the global key unstable so users can't rely on it being stable. + let default_parse_key = DEFAULT_PARSE_KEY.get_or_init(|| format!("__javy_{millis}_json_parse")); + global.set(default_parse_key, default_parse)?; let parse = Function::new( this.clone(), - MutFn::new(|cx: Ctx<'js>, args: Rest>| { + MutFn::new(move |cx: Ctx<'js>, args: Rest>| { call_json_parse(hold!(cx.clone(), args)).map_err(|e| to_js_error(cx, e)) }), )?; @@ -96,7 +103,7 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { Ok(()) } -fn call_json_parse(args: Args<'_>) -> Result> { +fn call_json_parse<'a>(args: Args<'a>) -> Result> { let (this, args) = args.release(); match args.len() { @@ -135,7 +142,7 @@ fn call_json_parse(args: Args<'_>) -> Result> { // reviver argument. // // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#reviver. - let default: Function = this.globals().get(DEFAULT_PARSE_KEY)?; + let default: Function = this.globals().get(DEFAULT_PARSE_KEY.get().unwrap())?; default .call((args[0].clone(), args[1].clone())) .map_err(|e| anyhow!(e)) From fe8ec83e86203b915d72f5c39170a00c96e49c13 Mon Sep 17 00:00:00 2001 From: Jeff Charles Date: Thu, 21 Nov 2024 11:03:50 -0500 Subject: [PATCH 5/5] Linting --- crates/javy/src/apis/json.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/javy/src/apis/json.rs b/crates/javy/src/apis/json.rs index f418eb20..5a1d2c2c 100644 --- a/crates/javy/src/apis/json.rs +++ b/crates/javy/src/apis/json.rs @@ -103,7 +103,7 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> { Ok(()) } -fn call_json_parse<'a>(args: Args<'a>) -> Result> { +fn call_json_parse(args: Args<'_>) -> Result> { let (this, args) = args.release(); match args.len() {