From 21e7c979751942f6e89edda18f4c61806dc8af12 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sa=C3=BAl=20Cabrera?= <saulecabrera@gmail.com>
Date: Thu, 20 Jun 2024 07:14:25 -0400
Subject: [PATCH] Fix issues between reference counting and the custom
 `JSON.parse` implementation (#673)

* Introduce a minimal failing test

This commit introduces a minimal failing test case where SIMD JSON
parsing and reference couting causes a panic.

Analysis:

Even though our fork of rquickjs disallows GC, it doesn't prevent
reference counting. This, combined with the usage of
`JS_DefinePropertyValue` which decrements the reference count of each
String value inside a map, introduced the bug showcased in this commit.

* Fix issues with reference counting and SIMD JSON (parsing)

This commit is follow-up to the previous commit, in which the
relationship between reference counting and String Object value creation
is fixed by:

- Avoiding the usage of `unsafe` APIs
- Ensuring that everytime a string value gets created it gets correctly
  taken into acount.

Using rquickjs' safe APIs ensures that new values get cloned, which
means that `JS_DupValue` is called, which correctly increases the
reference count.

* Gate the misc test with #[cfg(feature = 'json')]

* Clippy fixes
---
 crates/javy/src/serde/ser.rs                  | 34 +++++--------------
 crates/javy/tests/misc.rs                     | 21 ++++++++++++
 .../tests/string_keys_and_ref_counting.js     |  8 +++++
 3 files changed, 38 insertions(+), 25 deletions(-)
 create mode 100644 crates/javy/tests/misc.rs
 create mode 100644 crates/javy/tests/string_keys_and_ref_counting.js

diff --git a/crates/javy/src/serde/ser.rs b/crates/javy/src/serde/ser.rs
index ba78cece..3b6e06fb 100644
--- a/crates/javy/src/serde/ser.rs
+++ b/crates/javy/src/serde/ser.rs
@@ -1,7 +1,4 @@
-use crate::quickjs::{
-    qjs::{JS_DefinePropertyValue, JS_ValueToAtom, JS_PROP_C_W_E},
-    Array, Ctx, Object, String as JSString, Value,
-};
+use crate::quickjs::{object::Property, Array, Ctx, Object, String as JSString, Value};
 use crate::serde::err::{Error, Result};
 use anyhow::anyhow;
 
@@ -340,29 +337,16 @@ impl<'a> ser::SerializeMap for &'a mut Serializer<'_> {
     {
         let mut map_serializer = Serializer::from_context(self.context.clone())?;
         value.serialize(&mut map_serializer)?;
-        let atom = unsafe { JS_ValueToAtom(self.context.as_raw().as_ptr(), self.key.as_raw()) };
-
         if let Some(o) = self.value.as_object() {
-            // Use `JS_DefinePropertyValue` to keep the semantics of the object
-            // unchanged.
-            let result = unsafe {
-                JS_DefinePropertyValue(
-                    self.context.as_raw().as_ptr(),
-                    o.as_raw(),
-                    atom,
-                    map_serializer.value.as_raw(),
-                    JS_PROP_C_W_E as i32,
-                )
-            };
-
-            return if result != 0 {
-                Ok(())
-            } else {
-                Err(Error::custom("Error while serializing object"))
-            };
+            let prop = Property::from(map_serializer.value.clone())
+                .writable()
+                .configurable()
+                .enumerable();
+            o.prop::<_, _, _>(self.key.clone(), prop)
+                .map_err(|e| Error::custom(e.to_string()))
+        } else {
+            Err(Error::custom("Expected to be an object"))
         }
-
-        Err(Error::custom("Expected to be an object"))
     }
 
     fn end(self) -> Result<()> {
diff --git a/crates/javy/tests/misc.rs b/crates/javy/tests/misc.rs
new file mode 100644
index 00000000..435a484b
--- /dev/null
+++ b/crates/javy/tests/misc.rs
@@ -0,0 +1,21 @@
+use anyhow::Result;
+use javy::{quickjs::context::EvalOptions, Config, Runtime};
+
+#[cfg(feature = "json")]
+#[test]
+fn string_keys_and_ref_counting() -> Result<()> {
+    let mut config = Config::default();
+    config.override_json_parse_and_stringify(true);
+
+    let source = include_bytes!("string_keys_and_ref_counting.js");
+    let rt = Runtime::new(config)?;
+
+    rt.context().with(|this| {
+        let _: () = this
+            .eval_with_options(*source, EvalOptions::default())
+            .inspect_err(|e| println!("{e}"))
+            .expect("source evaluation to succeed");
+    });
+
+    Ok(())
+}
diff --git a/crates/javy/tests/string_keys_and_ref_counting.js b/crates/javy/tests/string_keys_and_ref_counting.js
new file mode 100644
index 00000000..80e9c946
--- /dev/null
+++ b/crates/javy/tests/string_keys_and_ref_counting.js
@@ -0,0 +1,8 @@
+const input = JSON.parse('{"elements":[{"id":"zza"},{"id":"zzb"},{"id":"zzc"},{"id":"zzd"},{"id":"zze"}]}');
+
+const acc = {};
+input.elements.forEach(e => {
+  if (!acc[e.id]) {
+    acc[e.id] = 1;
+  }
+});