javascript, jsopaque: call finalizers without corresponding JSValue

Previously, nim_finalize_for_js only called a finalizer if the opaque
Nim object already had a corresponding JSValue.  This was probably an
oversight (at least I had no idea this worked this way), so now we
always call the finalizer.

The upshot is that this means that finalizers can now receive a nil
runtime in this case, so finalizers not prepared for this might break.
The solution is to either explicitly nil-check the runtime, or to ensure
that such objects always get converted to a JSValue first.

4 files changed, 40 insertions, 25 deletions

diff --git a/lib/monoucha0/doc/manual.md b/lib/monoucha0/doc/manual.md
index a20b11c6..67fadc0b 100644
--- a/lib/monoucha0/doc/manual.md
+++ b/lib/monoucha0/doc/manual.md
@@ -583,14 +583,15 @@ The first parameter must be a reference to the object in question. Only one
 inherited. This means that you must set up separate `.jsfin` functions for each
 child object in the inheritance chain.
 
-To free up JS objects, you may precede your reference type parameter with a
-`JSRuntime` parameter.
-
-WARNING: like Nim `=destroy`, this pragma is very easy to misuse. In particular,
-you must make sure to **NEVER ALLOCATE** in a `.jsfin` finalizer, because this
-[breaks](https://github.com/nim-lang/Nim/issues/4851) Nim refc. (I don't know if
-this problem is still present in ORC, but at the moment Monoucha does not work
-with ORC anyway.)
+To free up JS objects, you may precede your reference type parameter
+with a `JSRuntime` parameter.  WARNING: this parameter is nil when
+an object that was not bound to a JS value is finalized.
+
+WARNING 2: like Nim `=destroy`, this pragma is very easy to misuse.  In
+particular, you must make sure to **NEVER ALLOCATE** in a `.jsfin`
+finalizer, because this [breaks](https://github.com/nim-lang/Nim/issues/4851)
+Nim refc.  (I don't know if this problem is still present in ORC, but at
+the moment Monoucha does not work with ORC anyway.)
 
 Example:
 
diff --git a/lib/monoucha0/monoucha/javascript.nim b/lib/monoucha0/monoucha/javascript.nim
index 23a6ed35..476595e3 100644
--- a/lib/monoucha0/monoucha/javascript.nim
+++ b/lib/monoucha0/monoucha/javascript.nim
@@ -151,11 +151,13 @@ proc jsRuntimeCleanUp(rt: JSRuntime) {.cdecl.} =
     GC_unref(cast[RootRef](rtOpaque.tmpunrefs[i]))
   for i in 0 ..< np:
     let p = rtOpaque.tmplist[i]
-    #TODO maybe finalize?
     let val = JS_MKPTR(JS_TAG_OBJECT, p)
     let classid = JS_GetClassID(val)
-    rtOpaque.fins.withValue(classid, fin):
-      fin[](rt, val)
+    let opaque = JS_GetOpaque(val, classid)
+    if opaque != nil:
+      let nimt = rtOpaque.inverseTypemap.getOrDefault(classid)
+      rtOpaque.fins.withValue(nimt, fin):
+        fin[](rt, opaque)
     JS_SetOpaque(val, nil)
     JS_FreeValueRT(rt, val)
   # GC will run again now
@@ -324,6 +326,7 @@ func newJSClass*(ctx: JSContext; cdef: JSClassDefConst; tname: cstring;
     raise newException(Defect, "Failed to allocate JS class: " &
       $cdef.class_name)
   ctxOpaque.typemap[nimt] = result
+  rtOpaque.inverseTypemap[result] = nimt
   ctxOpaque.creg[tname] = result
   if ctxOpaque.parents.len <= int(result):
     ctxOpaque.parents.setLen(int(result) + 1)
@@ -331,7 +334,7 @@ func newJSClass*(ctx: JSContext; cdef: JSClassDefConst; tname: cstring;
   if ishtmldda:
     ctxOpaque.htmldda = result
   if finalizer != nil:
-    rtOpaque.fins[result] = finalizer
+    rtOpaque.fins[nimt] = finalizer
   let proto = ctx.newProtoFromParentClass(parent)
   if funcs.len > 0:
     # We avoid funcs being GC'ed by putting the list in rtOpaque.
@@ -1102,10 +1105,8 @@ macro jsfin*(fun: typed) =
   else:
     error("Expected one or two parameters")
   let jsProc = quote do:
-    proc `finName`(rt {.inject.}: JSRuntime; val: JSValue) =
-      let opaque {.inject.} = JS_GetOpaque(val, JS_GetClassID(val))
-      if opaque != nil:
-        `finStmt`
+    proc `finName`(rt {.inject.}: JSRuntime; opaque {.inject.}: pointer) =
+      `finStmt`
   gen.registerFunction()
   return newStmtList(fun, jsProc)
 
@@ -1196,15 +1197,16 @@ proc findPragmas(t: NimNode): JSObjectPragmas =
               result.jsget.add(op)
               result.jsset.add(op)
 
-proc nim_finalize_for_js*(obj: pointer) =
+proc nim_finalize_for_js*(obj, typeptr: pointer) =
+  var fin: JSFinalizerFunction = nil
   for rt in runtimes:
     let rtOpaque = rt.getOpaque()
+    fin = rtOpaque.fins.getOrDefault(typeptr)
     rtOpaque.plist.withValue(obj, v):
       let p = v[].p
       let val = JS_MKPTR(JS_TAG_OBJECT, p)
-      let classid = JS_GetClassID(val)
-      rtOpaque.fins.withValue(classid, fin):
-        fin[](rt, val)
+      if fin != nil:
+        fin(rt, obj)
       JS_SetOpaque(val, nil)
       rtOpaque.plist.del(obj)
       if rtOpaque.destroying == obj:
@@ -1212,6 +1214,12 @@ proc nim_finalize_for_js*(obj: pointer) =
         rtOpaque.destroying = nil
       else:
         JS_FreeValueRT(rt, val)
+      return
+  # No JSValue exists for the object, but it likely still expects us to
+  # free it.
+  # We pass nil as the runtime, since that's the only sensible solution.
+  if fin != nil:
+    fin(nil, obj)
 
 type
   TabGetSet* = object
@@ -1229,14 +1237,14 @@ template jsDestructor*[U](T: typedesc[ref U]) =
     jsDtors.incl($T)
   {.warning[Deprecated]:off.}:
     proc `=destroy`(obj: var U) =
-      nim_finalize_for_js(addr obj)
+      nim_finalize_for_js(addr obj, getTypePtr(obj))
 
 template jsDestructor*(T: typedesc[object]) =
   static:
     jsDtors.incl($T)
   {.warning[Deprecated]:off.}:
     proc `=destroy`(obj: var T) =
-      nim_finalize_for_js(addr obj)
+      nim_finalize_for_js(addr obj, getTypePtr(obj))
 
 func tname(info: RegistryInfo): string =
   return info.t.strVal
diff --git a/lib/monoucha0/monoucha/jsopaque.nim b/lib/monoucha0/monoucha/jsopaque.nim
index 6d531a59..7b90329a 100644
--- a/lib/monoucha0/monoucha/jsopaque.nim
+++ b/lib/monoucha0/monoucha/jsopaque.nim
@@ -44,7 +44,7 @@ type
     htmldda*: JSClassID # only one of these exists: document.all.
     globalUnref*: JSEmptyOpaqueCallback
 
-  JSFinalizerFunction* = proc(rt: JSRuntime; val: JSValue) {.nimcall,
+  JSFinalizerFunction* = proc(rt: JSRuntime; opaque: pointer) {.nimcall,
     raises: [].}
 
   JSEmptyOpaqueCallback* = (proc() {.closure, raises: [].})
@@ -52,7 +52,9 @@ type
   JSRuntimeOpaque* = ref object
     plist*: Table[pointer, tuple[p: pointer; jsref: bool]] # Nim, JS
     flist*: seq[seq[JSCFunctionListEntry]]
-    fins*: Table[JSClassID, JSFinalizerFunction]
+    fins*: Table[pointer, JSFinalizerFunction]
+    #TODO maybe just extract this from typemap on JSContext free?
+    inverseTypemap*: Table[JSClassID, pointer]
     parentMap*: Table[pointer, pointer]
     destroying*: pointer
     # temp list for uninit
diff --git a/lib/monoucha0/monoucha/tojs.nim b/lib/monoucha0/monoucha/tojs.nim
index f92f0165..4284b3a0 100644
--- a/lib/monoucha0/monoucha/tojs.nim
+++ b/lib/monoucha0/monoucha/tojs.nim
@@ -291,10 +291,14 @@ proc getTypePtr*[T](x: T): pointer =
     # (This dereferences the object's first member, m_type. Probably.)
     return cast[ptr pointer](x)[]
   elif T is RootObj:
-    return cast[pointer](x)
+    static:
+      error("Please make it var")
   else:
     return getTypeInfo(x)
 
+proc getTypePtr*[T: RootObj](x: var T): pointer =
+  return cast[ptr pointer](addr x)[]
+
 func getTypePtr*(t: typedesc[ref object]): pointer =
   var x = t()
   return getTypePtr(x)