QuickJS: mark nginx wrapper references during GC

The HTTP request and stream session wrappers hold JavaScript references
that were invisible to the QuickJS cycle collector.  For example, this
creates a cycle through the native request wrapper:

    function content(r) {
        r.args.request = r;
        r.return(200);
    }

When the context was destroyed, the collector could leave this graph in
the runtime, causing a worker abort in an assertion-enabled build or a
leak otherwise.  Cached request and response bodies had the same
ownership problem.  A stream callback capturing its session formed an
equivalent native cycle.

Add gc_mark callbacks symmetric with the class finalizers.  Stream
callbacks can now rely on normal cycle collection, so remove the special
teardown path that released them before the common exit-hook and pending
job processing.

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index cfb250773b7813ab2dd597b7efe3dec4eed9ef02..8fc3a154c045d5ac344e1631e5e6c5dea69d4571 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -494,6 +494,8 @@ static int ngx_http_qjs_headers_out_delete_property(JSContext *cx,
 static ngx_http_request_t *ngx_http_qjs_request(JSValueConst val);
 static JSValue ngx_http_qjs_request_make(JSContext *cx, ngx_int_t proto_id,
     ngx_http_request_t *r);
+static void ngx_http_qjs_request_mark(JSRuntime *rt, JSValueConst val,
+    JS_MarkFunc *mark_func);
 static void ngx_http_qjs_request_finalizer(JSRuntime *rt, JSValue val);
 static void ngx_http_qjs_periodic_finalizer(JSRuntime *rt, JSValue val);
 #endif
@@ -1469,6 +1471,7 @@ static const JSCFunctionListEntry ngx_http_qjs_ext_request_form[] = {
 static JSClassDef ngx_http_qjs_request_class = {
     "Request",
     .finalizer = ngx_http_qjs_request_finalizer,
+    .gc_mark = ngx_http_qjs_request_mark,
 };
 
 
@@ -9418,6 +9421,21 @@ ngx_http_qjs_request_make(JSContext *cx, ngx_int_t proto_id,
 }
 
 
+static void
+ngx_http_qjs_request_mark(JSRuntime *rt, JSValueConst val,
+    JS_MarkFunc *mark_func)
+{
+    ngx_http_qjs_request_t  *req;
+
+    req = JS_GetOpaque(val, NGX_QJS_CLASS_ID_HTTP_REQUEST);
+    if (req != NULL) {
+        JS_MarkValue(rt, req->args, mark_func);
+        JS_MarkValue(rt, req->request_body, mark_func);
+        JS_MarkValue(rt, req->response_body, mark_func);
+    }
+}
+
+
 static void
 ngx_http_qjs_request_finalizer(JSRuntime *rt, JSValue val)
 {

diff --git a/nginx/ngx_stream_js_module.c b/nginx/ngx_stream_js_module.c
index 2332b42e8d456e3c134c954587f55b3a1fcfe647..8bed4cab0001437e39f90a7a116fe9fbcd1348e8 100644 (file)
--- a/nginx/ngx_stream_js_module.c
+++ b/nginx/ngx_stream_js_module.c
@@ -194,6 +194,8 @@ static ngx_int_t ngx_stream_qjs_body_filter(ngx_stream_session_t *s,
 static ngx_stream_session_t *ngx_stream_qjs_session(JSValueConst val);
 static JSValue ngx_stream_qjs_session_make(JSContext *cx, ngx_int_t proto_id,
     ngx_stream_session_t *s);
+static void ngx_stream_qjs_session_mark(JSRuntime *rt, JSValueConst val,
+    JS_MarkFunc *mark_func);
 static void ngx_stream_qjs_session_finalizer(JSRuntime *rt, JSValue val);
 static void ngx_stream_qjs_periodic_finalizer(JSRuntime *rt, JSValue val);
 
@@ -882,6 +884,7 @@ static const JSCFunctionListEntry ngx_stream_qjs_ext_flags[] = {
 static JSClassDef ngx_stream_qjs_session_class = {
     "Session",
     .finalizer = ngx_stream_qjs_session_finalizer,
+    .gc_mark = ngx_stream_qjs_session_mark,
 };
 
 
@@ -3042,6 +3045,22 @@ ngx_stream_qjs_session_make(JSContext *cx, ngx_int_t proto_id,
 }
 
 
+static void
+ngx_stream_qjs_session_mark(JSRuntime *rt, JSValueConst val,
+    JS_MarkFunc *mark_func)
+{
+    ngx_uint_t                 i;
+    ngx_stream_qjs_session_t  *ses;
+
+    ses = JS_GetOpaque(val, NGX_QJS_CLASS_ID_STREAM_SESSION);
+    if (ses != NULL) {
+        for (i = 0; i < NGX_JS_EVENT_MAX; i++) {
+            JS_MarkValue(rt, ses->callbacks[i], mark_func);
+        }
+    }
+}
+
+
 static void
 ngx_stream_qjs_session_finalizer(JSRuntime *rt, JSValue val)
 {
@@ -3170,33 +3189,6 @@ ngx_engine_qjs_clone(ngx_js_ctx_t *ctx, ngx_js_loc_conf_t *cf,
 }
 
 
-static void
-ngx_stream_qjs_destroy(ngx_engine_t *e, ngx_js_ctx_t *ctx,
-    ngx_js_loc_conf_t *conf)
-{
-    ngx_uint_t                 i;
-    JSValue                    cb;
-    ngx_stream_qjs_session_t  *ses;
-
-    if (ctx != NULL) {
-        /*
-         * explicitly freeing the callback functions
-         * to avoid circular references with the session object.
-         */
-        ses = JS_GetOpaque(ngx_qjs_arg(ctx->args[0]),
-                           NGX_QJS_CLASS_ID_STREAM_SESSION);
-        if (ses != NULL) {
-            for (i = 0; i < NGX_JS_EVENT_MAX; i++) {
-                cb = ses->callbacks[i];
-                ses->callbacks[i] = JS_UNDEFINED;
-                JS_FreeValue(e->u.qjs.ctx, cb);
-            }
-        }
-    }
-
-    ngx_engine_qjs_destroy(e, ctx, conf);
-}
-
 #endif
 
 
@@ -3224,7 +3216,6 @@ ngx_stream_js_init_conf_vm(ngx_conf_t *cf, ngx_js_loc_conf_t *conf)
         options.u.qjs.metas = ngx_stream_js_uptr;
         options.u.qjs.addons = njs_stream_qjs_addon_modules;
         options.clone = ngx_engine_qjs_clone;
-        options.destroy = ngx_stream_qjs_destroy;
 
         options.core_conf = (ngx_js_core_conf_t *)
                  ngx_get_conf(cf->cycle->conf_ctx, ngx_stream_js_core_module);

diff --git a/nginx/t/js_qjs_gc_mark.t b/nginx/t/js_qjs_gc_mark.t
new file mode 100644 (file)
index 0000000..c1af9af
--- /dev/null
+++ b/nginx/t/js_qjs_gc_mark.t
@@ -0,0 +1,140 @@
+#!/usr/bin/perl
+
+# (C) Dmitry Volyntsev
+# (C) F5, Inc.
+
+# Tests for JavaScript references held by QuickJS nginx wrapper classes.
+
+###############################################################################
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+use Test::Nginx::Stream qw/ stream /;
+
+###############################################################################
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+my $t = Test::Nginx->new()->has(qw/http stream stream_return/)
+    ->write_file_expand('nginx.conf', <<'EOF');
+
+%%TEST_GLOBALS%%
+
+daemon off;
+
+events {
+}
+
+http {
+    %%TEST_GLOBALS_HTTP%%
+
+    js_engine qjs;
+    js_context_reuse 0;
+    js_import test.js;
+
+    server {
+        listen       127.0.0.1:8080;
+        server_name  localhost;
+
+        location /cycle {
+            js_content test.http_cycle;
+        }
+
+        location /request_body_cycle {
+            js_content test.request_body_cycle;
+        }
+
+        location /response_body_cycle {
+            js_content test.response_body_cycle;
+        }
+
+        location /body {
+            return 200 body;
+        }
+    }
+}
+
+stream {
+    %%TEST_GLOBALS_STREAM%%
+
+    js_engine qjs;
+    js_context_reuse 0;
+    js_import test.js;
+
+    server {
+        listen      127.0.0.1:8081;
+        js_preread  test.stream_cycle;
+        return      OK;
+    }
+}
+
+EOF
+
+$t->write_file('test.js', <<'EOF');
+    function http_cycle(r) {
+        r.args.request = r;
+        r.return(200, 'ok');
+    }
+
+    function request_body_cycle(r) {
+        var body = r.requestBuffer;
+
+        body.request = r;
+        r.return(200, body.length);
+    }
+
+    async function response_body_cycle(r) {
+        var reply = await r.subrequest('/body');
+        var body = reply.responseBuffer;
+
+        body.reply = reply;
+        r.return(200, body.length);
+    }
+
+    function stream_cycle(s) {
+        s.on('upload', function(data) {
+            if (data.length != 0) {
+                s.done();
+            }
+        });
+    }
+
+    export default {http_cycle, request_body_cycle, response_body_cycle,
+                    stream_cycle};
+EOF
+
+$t->try_run('no QuickJS support')->plan(4);
+
+###############################################################################
+
+# Disabling reuse forces JS_FreeRuntime() after each request or session.
+# A missing gc_mark leaves the cycle in the runtime and the harness catches
+# the resulting worker abort through its implicit no-alerts check.
+
+like(http_get('/cycle'), qr/200 OK/, 'HTTP request cycle');
+like(http_post('/request_body_cycle'), qr/200 OK/, 'request body cycle');
+like(http_get('/response_body_cycle'), qr/200 OK/, 'response body cycle');
+is(stream('127.0.0.1:' . port(8081))->io('x'), 'OK',
+    'stream session callback cycle');
+
+###############################################################################
+
+sub http_post {
+    my ($uri) = @_;
+    my $crlf = "\x0d\x0a";
+
+    return http("POST $uri HTTP/1.0" . $crlf
+        . "Host: localhost" . $crlf
+        . "Content-Length: 4" . $crlf . $crlf
+        . "body");
+}
+
+###############################################################################