DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
434 stars 59 forks source link

Memory Leak in rust allocator #251

Closed richarddd closed 5 months ago

richarddd commented 5 months ago

Hi, I've noticed that rquickjs leaks memory when using the rust_alloc feature.

It happens for recursive or self referencing functions, even though they are not invoked:

while (true) {
  const a = () => {
      if(a){
        return true
      }
      return false
    };
}

It does not matter which allocator is being used. I tested with system, mimalloc, snmalloc, jemalloc and they all produce the same result. Release or debug doesn't matter, nor does async, parallel or regular runtime.

Using disabling the rust_alloc feature fixes it.

Minimum reproducible:

use std::error::Error;

use rquickjs::{
    loader::{BuiltinResolver, ModuleLoader},
    AsyncContext, AsyncRuntime, CatchResultExt,
};

#[tokio::main]
async fn main() -> Result<(), Box<dyn Error>> {
    let resolver = BuiltinResolver::default();
    let loader = ModuleLoader::default();

    let rt = AsyncRuntime::new().unwrap();
    rt.set_max_stack_size(512 * 1024).await;
    rt.set_loader(resolver, loader).await;

    let ctx: AsyncContext = AsyncContext::full(&rt).await.unwrap();

    let res = ctx
        .with(|ctx| -> rquickjs::Result<()> {
            let _ = ctx.compile(
                "index",
                r#"

while (true) {
  const a = () => {
      if(a){
        return true
      }
      return false
    };
}

            "#,
            )?;

            Ok(())
        })
        .await;

    ctx.with(|ctx| {
        if let Err(err) = res.catch(&ctx) {
            eprintln!("{}", err);
        }
    })
    .await;
    rt.idle().await;

    drop(ctx);
    drop(rt);

    Ok::<_, Box<dyn Error>>(())
}
richarddd commented 5 months ago

@DelSkayn I have located the issue.

When moving allocation to rust we no longer keep track of GC malloc_state as quickjs want's it. This causes the GC threshold to be ignored and js_trigger_gc that is invoked from JS_NewObjectFromShape no longer runs GC: https://github.com/bellard/quickjs/blob/c6cc6a9a5e420fa2707e828da23d131d2bf170f7/quickjs.c#L1286.

The fix would be to keep track of the GC malloc_state in the rust allocator as well.

DelSkayn commented 5 months ago

Right! That should be fixed! Will fix it in #257