diff options
| author | bptato <nincsnevem662@gmail.com> | 2024-06-03 20:28:29 +0200 |
|---|---|---|
| committer | bptato <nincsnevem662@gmail.com> | 2024-06-03 20:28:29 +0200 |
| commit | dfb03387691e72f8832734255765ddfdd38db372 (patch) | |
| tree | 31f2c489859aa8bd7d7bc2b3b1854f11708b45f2 | |
| parent | 34b0abd1a5811afa88b16442c0dc327881456d8f (diff) | |
| download | chawan-dfb03387691e72f8832734255765ddfdd38db372.tar.gz | |
js: fix runtime cleanup
This is a minefield. Intuitively, you would think that just clearing the opaque and manually freeing registered object should be enough. Unfortunately, this is not true; we do not store whether we are actually holding a reference to registered JS objects, so this approach leads to double frees. Instead, we add a QJS callback that is called right after the final GC cleanup, but before the list_free assertion. This way, we can be sure that any object still in our registry is referenced by us, and can therefore unreference them safely.
| -rw-r--r-- | lib/quickjs/quickjs.c | 8 | ||||
| -rw-r--r-- | lib/quickjs/quickjs.h | 3 | ||||
| -rw-r--r-- | src/bindings/quickjs.nim | 4 | ||||
| -rw-r--r-- | src/js/javascript.nim | 47 |
4 files changed, 44 insertions, 18 deletions
diff --git a/lib/quickjs/quickjs.c b/lib/quickjs/quickjs.c index af7ea852..427b36df 100644 --- a/lib/quickjs/quickjs.c +++ b/lib/quickjs/quickjs.c @@ -306,6 +306,7 @@ struct JSRuntime { uint32_t operator_count; #endif void *user_opaque; + JSRuntimeCleanUpFunc *user_cleanup; }; struct JSClass { @@ -1703,6 +1704,11 @@ void JS_SetRuntimeOpaque(JSRuntime *rt, void *opaque) rt->user_opaque = opaque; } +void JS_SetRuntimeCleanUpFunc(JSRuntime *rt, JSRuntimeCleanUpFunc cleanup_func) +{ + rt->user_cleanup = cleanup_func; +} + /* default memory allocation functions with memory limitation */ static size_t js_def_malloc_usable_size(const void *ptr) { @@ -1960,6 +1966,8 @@ void JS_FreeRuntime(JSRuntime *rt) JS_RunGC(rt); + rt->user_cleanup(rt); + #ifdef DUMP_LEAKS /* leaking objects */ { diff --git a/lib/quickjs/quickjs.h b/lib/quickjs/quickjs.h index 8c81be47..366f341b 100644 --- a/lib/quickjs/quickjs.h +++ b/lib/quickjs/quickjs.h @@ -331,6 +331,8 @@ typedef struct JSMallocFunctions { typedef struct JSGCObjectHeader JSGCObjectHeader; +typedef void JSRuntimeCleanUpFunc(JSRuntime *rt); + JSRuntime *JS_NewRuntime(void); /* info lifetime must exceed that of rt */ void JS_SetRuntimeInfo(JSRuntime *rt, const char *info); @@ -345,6 +347,7 @@ JSRuntime *JS_NewRuntime2(const JSMallocFunctions *mf, void *opaque); void JS_FreeRuntime(JSRuntime *rt); void *JS_GetRuntimeOpaque(JSRuntime *rt); void JS_SetRuntimeOpaque(JSRuntime *rt, void *opaque); +void JS_SetRuntimeCleanUpFunc(JSRuntime *rt, JSRuntimeCleanUpFunc cleanup_func); typedef void JS_MarkFunc(JSRuntime *rt, JSGCObjectHeader *gp); void JS_MarkValue(JSRuntime *rt, JSValueConst val, JS_MarkFunc *mark_func); void JS_RunGC(JSRuntime *rt); diff --git a/src/bindings/quickjs.nim b/src/bindings/quickjs.nim index 836bedac..20dd048b 100644 --- a/src/bindings/quickjs.nim +++ b/src/bindings/quickjs.nim @@ -132,6 +132,8 @@ type JSClassExoticMethodsConst* {.importc: "const JSClassExoticMethods *", header: qjsheader.} = ptr JSClassExoticMethods + JSRuntimeCleanUpFunc* {.importc.} = proc(rt: JSRuntime) {.cdecl.} + JSClassDef* {.importc, header: qjsheader.} = object class_name*: cstring finalizer*: JSClassFinalizer @@ -555,6 +557,8 @@ proc JS_ExecutePendingJob*(rt: JSRuntime; pctx: ptr JSContext): cint proc JS_GetRuntimeOpaque*(rt: JSRuntime): pointer proc JS_SetRuntimeOpaque*(rt: JSRuntime; p: pointer) +proc JS_SetRuntimeCleanUpFunc*(rt: JSRuntime; + cleanup_func: JSRuntimeCleanUpFunc) proc JS_SetContextOpaque*(ctx: JSContext; opaque: pointer) proc JS_GetContextOpaque*(ctx: JSContext): pointer diff --git a/src/js/javascript.nim b/src/js/javascript.nim index e2e91a4f..39330361 100644 --- a/src/js/javascript.nim +++ b/src/js/javascript.nim @@ -121,6 +121,27 @@ proc bindRealloc(s: ptr JSMallocState; p: pointer; size: csize_t): pointer {.cdecl.} = return realloc(p, size) +proc jsRuntimeCleanUp(rt: JSRuntime) {.cdecl.} = + let rtOpaque = rt.getOpaque() + GC_unref(rtOpaque) + assert rtOpaque.destroying == nil + var ps: seq[pointer] = @[] + for p in rtOpaque.plist.values: + ps.add(p) + rtOpaque.plist.clear() + var unrefs: seq[proc() {.closure.}] = @[] + for (_, unref) in rtOpaque.refmap.values: + unrefs.add(unref) + rtOpaque.refmap.clear() + for unref in unrefs: + unref() + for p in ps: + #TODO maybe finalize? + let val = JS_MKPTR(JS_TAG_OBJECT, p) + JS_SetOpaque(val, nil) + JS_FreeValueRT(rt, val) + JS_RunGC(rt) + proc newJSRuntime*(): JSRuntime = var mf {.global.} = JSMallocFunctions( js_malloc: bindMalloc, @@ -132,6 +153,7 @@ proc newJSRuntime*(): JSRuntime = let opaque = JSRuntimeOpaque() GC_ref(opaque) JS_SetRuntimeOpaque(rt, cast[pointer](opaque)) + JS_SetRuntimeCleanUpFunc(rt, jsRuntimeCleanUp) # Must be added after opaque is set, or there is a chance of # nim_finalize_for_js dereferencing it (at the new call). runtimes.add(rt) @@ -170,17 +192,6 @@ proc free*(ctx: JSContext) = JS_FreeContext(ctx) proc free*(rt: JSRuntime) = - let opaque = rt.getOpaque() - GC_unref(opaque) - var ps: seq[pointer] = @[] - for p in opaque.plist.values: - ps.add(p) - opaque.plist.clear() - for p in ps: - #TODO maybe finalize? - let val = JS_MKPTR(JS_TAG_OBJECT, p) - JS_SetOpaque(val, nil) - JS_FreeValueRT(rt, val) JS_FreeRuntime(rt) runtimes.del(runtimes.find(rt)) @@ -1503,8 +1514,8 @@ proc bindGetSet(stmts: NimNode; info: RegistryInfo) = JS_CGETSET_DEF_NOCONF(`k`, `get`, `set`)) proc bindExtraGetSet(stmts: NimNode; info: var RegistryInfo; - extra_getset: openArray[TabGetSet]) = - for x in extra_getset: + extraGetSet: openArray[TabGetSet]) = + for x in extraGetSet: let k = x.name let g = x.get let s = x.set @@ -1604,8 +1615,8 @@ proc bindEndStmts(endstmts: NimNode; info: RegistryInfo) = macro registerType*(ctx: JSContext; t: typed; parent: JSClassID = 0; asglobal: static bool = false; globalparent: static bool = false; nointerface = false; name: static string = ""; - has_extra_getset: static bool = false; - extra_getset: static openArray[TabGetSet] = []; namespace = JS_NULL; + hasExtraGetSet: static bool = false; + extraGetSet: static openArray[TabGetSet] = []; namespace = JS_NULL; errid = opt(JSErrorEnum); ishtmldda = false): JSClassID = var stmts = newStmtList() var info = newRegistryInfo(t, name) @@ -1622,11 +1633,11 @@ macro registerType*(ctx: JSContext; t: typed; parent: JSClassID = 0; stmts.registerSetters(info, pragmas.jsset) stmts.bindFunctions(info) stmts.bindGetSet(info) - if has_extra_getset: - #HACK: for some reason, extra_getset gets weird contents when nothing is + if hasExtraGetSet: + #HACK: for some reason, extraGetSet gets weird contents when nothing is # passed to it. So we need an extra flag to signal if anything has # been passed to it at all. - stmts.bindExtraGetSet(info, extra_getset) + stmts.bindExtraGetSet(info, extraGetSet) let sctr = stmts.bindConstructor(info) if not asglobal: stmts.bindCheckDestroy(info) |