Many C APIs allow passing C functions as callbacks. The functions are called to communicate when certain actions happen. js-ctypes does not support passing a JS function to such a C API

(Note that while I marked this blocking bug 513783, bsmedberg indicated that this isn't necessary for 1.9.2 - but certainly nice to have.)

This feature also allow to rewrite maemo-geolocation provider with JS-ctypes API.

Got a prototype patch here. Waiting for bug 513788 to land to get this finished up.

Quick braindump: Initial implementation should be single-threaded (i.e. assert if C tries to call the closure off main thread). We can do better in a followup. We'll want to maintain a map of thread -> cx. When the closure is called, we PR_GetCurrentThread(), hash the pointer, and use it to look up a thread id. If none exists, we PR_NewThreadPrivateIndex() one, create a new cx, and use PR_GetThreadPrivate() to stash it. Otherwise, we use PR_GetThreadPrivate() to get at a previously-created cx. (The dtor function we register with PR_NewThreadPrivateIndex() will take care of destroying the cx, and removing the thread pointer from our hash, if the thread is destroyed.) The "fast path" here, for C code on the main thread, would be a call to PR_GetCurrentThread() and a hash lookup. If that's slow, we could split this off into a separate closure API that says "I need thread support". All this will let us maintain one cx per thread, and lazily create new ones whenever C code calls into a closure. I *think*, once that part is done, that all should be happy on the JSAPI side. But there may be more trickiness to work out. The job of keeping the JS function itself alive is entirely up to the JS code using this API.

The PR_GetCurrentThread() bit was wrong - no need to do that; we just do PR_NewThreadPrivateIndex() once on ctypes startup and PR_SetThreadPrivate() to stash per-thread private data.

OK. Here's my outline proposal. There are several parts to this: 1) Introduce ctypes.FunctionType; this is a new CType, a data instance of which holds a function pointer. It is distinct from ctypes.PointerType, and has various useful properties - 'argTypes', 'returnType', 'abi'. We want data instances of this to be callable, so that one can get arbitrary function pointers from C and call them. (More fundamental than the closure stuff here.) let fn_t = ctypes.FunctionType(ctypes.default_abi, ctypes.int32_t, ctypes.int8_t); fn_t.name; ===> int32_t (*)(int8_t) let fn = fn_t("0xdeadbeef"); // pretend this is a legit C function pointer let i = fn(-7); I think we want to add a JSClass.call hook onto CData, instead of making fn_t() a JSFunction like we did with library.declare(). This makes casting back and forth between ctypes.PointerType seamless ("I've got a ctypes.voidptr_t, give me a ctypes.FunctionType"). I also think we want to have library.declare() return a CData instance of a FunctionType, rather than a JSFunction. This means moving all the code in Function.{h,cpp} into FunctionType / CData. It also means, sadly, that function instances won't be 'instanceof Function' anymore, since that's not in the CData prototype chain. (Maybe we could add it? Would that be confusing for non-FunctionType CDatas? The call hook will be present for all CData's, anyway.) let fn = library.declare("myfn", ctypes.default_abi, ctypes.int32_t, ctypes.int8_t); fn.toSource(); ===> ctypes.FunctionType(ctypes.default_abi, ctypes.int32_t, ctypes.int8_t)(ctypes.UInt64("0xdeadbeef")) let fn = fn_t(library, "myfn"); // alternate form of the above? Constructing 'fn' from a library, as in the above two examples, would add a reference to the library into an additional reserved slot on the CData, present only for FunctionTypes. 2) For closures: function js_fn() { return 42; } let fn_t = ctypes.FunctionType(ctypes.default_abi, ctypes.int32_t); let closure_fn = fn_t(js_fn); // create all the stuff required for a closure some_C_fn(closure_fn); // pass to a C function that wants a fnptr closure_fn(); // you can also call it yourself - it's just a fnptr ===> 42 To create 'closure_fn', we need to get an appropriate JSContext, create a closure object using libffi, and stash 'js_fn'. This can be done with the same additional reserved slot on the CData. The CData finalizer would look at this slot and free the closure when it's done. The reference system will magically work here: let fn2 = fn_t(closure_fn); // fn2 holds closure_fn, and transitively all its stuff, alive via the 'referent' slot; fn2's additional slot remains JSVAL_VOID. My earlier point that FunctionTypes should create CDatas rather than JSFunctions is supported by the fact that we need a finalizer for the closure object. How does this sound?

We probably want to give FunctionTypes a name, as well, c.f. structs and pointers - kinda like a typedef name. And perhaps change the syntax of the arguments, to use an array instead. And allow library.declare to take a FunctionType directly? ctypes.FunctionType("fn_t", ctypes.default_abi, ctypes.void_t, [ ctypes.voidptr_t, ...]);

Comment 6 sounds good. It would be nice to be able to use .call, .apply, etc on CData objects of function types, so if you have a bunch of spare time consider copying them from Function.prototype onto ctypes.FunctionType.prototype (which I think is a CData object whose properties will be inherited by all ctypes functions...?). Not a big deal either way. C and C++ don't give function types names, so offhand I'd say don't bother. OTOH it would be nice to be able to do C-like typedefs and have the typedefs show up in ctypes error messages (as they do in GCC error messages). library.declare taking a FunctionType sounds good, but again, not a huge priority just because it doesn't seem like a very discoverable feature for the few cases where it's useful.

(In reply to comment #8) > Comment 6 sounds good. It would be nice to be able to use .call, .apply, etc on > CData objects of function types, so if you have a bunch of spare time consider > copying them from Function.prototype onto ctypes.FunctionType.prototype (which > I think is a CData object whose properties will be inherited by all ctypes > functions...?). Not a big deal either way. Ah! Could do that. I was thinking about having Function.prototype in the proto chain of ctypes.FunctionType.prototype, but that's hard because its __proto__ is ctypes.CType.prototype. Which means ctypes.CType.prototype.__proto__ would have to be Function.prototype, and all CDatas get it. Which kinda sucks. So yeah, either we do that, or copy the right things over. Minor detail, either way. > C and C++ don't give function types names, so offhand I'd say don't bother. > OTOH it would be nice to be able to do C-like typedefs and have the typedefs > show up in ctypes error messages (as they do in GCC error messages). OK. Depends on whether you want your .name to have the short name, or the full function pointer syntax. I kinda like the latter :) > library.declare taking a FunctionType sounds good, but again, not a huge > priority just because it doesn't seem like a very discoverable feature for the > few cases where it's useful. OK. Tabled.

Zero substantive changes here; this just moves everything in Function.{cpp,h} into CTypes.{cpp,h}, in preparation for renaming Function to FunctionType. (To make the following patches easier to read.) rs= is sufficient here -- no need for actual review.

This gives us a ctypes.FunctionType() constructor, and reimplements library.declare() using it. Comments in the patch should be pretty self-explanatory!

Actually, I'm going to make this depend on bug 550982, so I don't need to add the extra SLOT_LIBOBJ here.

Same as before, but stuffs the Library object into SLOT_REFERENT now that its meaning is not overloaded with buffer ownership.

Comment on attachment 432933 [details] [diff] [review] part 1 - move Function impl This one is code movement, no substantive change -- just looking for rs= here.

Comment on attachment 433678 [details] [diff] [review] part 2.1 - implement FunctionType About half of this is tests -- not that big of a code change. (See above comments for details.)

Wait, I lied. Much less than half of this patch is tests. :)

This implements C callbacks. I haven't updated the wiki (https://wiki.mozilla.org/Jsctypes/api) with the spec yet, but briefly: function c() {}; let f_t = ctypes.FunctionType(ctypes.default_abi, ctypes.void_t); let f = f_t(c); // create a FunctionType CData that points to 'c' f(); // call it, just like a regular fnptr where the arg and return values of 'c' have to be convertable to the types declared in 'f_t'. One may also specify a 'this' object to use for the call: let f2 = f_t(c, { a: 5 }); f2(); // calls 'c' with 'this = { a: 5 }' All of this is main-thread-only, for now. See https://wiki.mozilla.org/Jsctypes/api#Callbacks for notes on why we roll our own JSContext.

I should note that these patches depend on the proto patch in bug 513788, which hasn't landed yet -- if you want to read baseline code for these patches, you probably want to apply that locally first.

Comment on attachment 432933 [details] [diff] [review] part 1 - move Function impl Applies and compiles, which seems like an adequate test of code movement.

P1, need this for 1.9.3.

Comment on attachment 432933 [details] [diff] [review] part 1 - move Function impl http://hg.mozilla.org/mozilla-central/rev/77834c13d000

Comment on attachment 433678 [details] [diff] [review] part 2.1 - implement FunctionType This patch isn't applying cleanly for me, so unfortunately (1) you may have some bit-unrotting to do, and (2) I've only reviewed it by inspection. Fortunately, it looks great! Only a few comments: > case TYPE_struct: > // Require exact type object equality. >- return t1 == t2; >+ return false; What was the motivation for this change? Is "exact type object equality" between struct types impossible now? > AutoValue& value = *values.AppendElement(); I realize this patch doesn't introduce this code, but I find AutoValue* value = values.AppendElement() a teeny bit more natural. >+ // Prepare a new array. >+ JSObject* argTypes = JS_NewArrayObject(cx, 0, NULL); >+ if (!argTypes) >+ return JS_FALSE; >+ JSAutoTempValueRooter argsroot(cx, argTypes); >+ >+ FunctionInfo* fninfo = GetFunctionInfo(cx, obj); >+ for (PRUint32 i = 0; i < fninfo->mArgTypes.Length(); ++i) { >+ if (!JS_DefineElement(cx, argTypes, i, OBJECT_TO_JSVAL(fninfo->mArgTypes[i]), >+ NULL, NULL, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT)) >+ return JS_FALSE; >+ } Micro-optimization. Since you're sealing the array object later, and since JS_DefineElement can't fail in this case, you could use js_NewArrayObjectWithCapacity and avoid JS_DefineElement, if you like: FunctionInfo* fninfo = GetFunctionInfo(cx, obj); PRUint32 argc = fninfo->mArgTypes.Length(); jsval* vector; JSObject* argTypes = js_NewArrayObjectWithCapacity(cx, argc, &vector); if (!argTypes) return JS_FALSE; JSAutoTempValueRooter argsroot(cx, argTypes); for (PRUint32 i = 0; i < argc; ++i) vector[i] = OBJECT_TO_JSVAL(fninfo->mArgTypes[i]); >+bool >+Library::IsLibrary(JSContext* cx, JSObject* obj) >+{ >+ return JS_GET_CLASS(cx, obj) == &sLibraryClass; >+} >+ > PRLibrary* > Library::GetLibrary(JSContext* cx, JSObject* obj) > { >- JS_ASSERT(JS_GET_CLASS(cx, obj) == &sLibraryClass); >+ JS_ASSERT(IsLibrary(cx, obj)); I realize that you're just decomposing a useful method here, but I'm curious: can library objects ever be used as a prototypes? Would there be any value in walking the prototype chain of the object to see if any JS_GET_CLASS(cx, proto) == &sLibraryClass?

(In reply to comment #22) > This patch isn't applying cleanly for me Yeah, it rotted from changes to patches I've since checked in, and I didn't post the new rev here. :( I'll make sure I post trivial updates from now on, since it seems you like to apply them. :) > > case TYPE_struct: > > // Require exact type object equality. > >- return t1 == t2; > >+ return false; > > What was the motivation for this change? Is "exact type object equality" > between struct types impossible now? Nope, same as before, the t1 == t2 case is covered by a 'fast path' check earlier in the function. > I realize this patch doesn't introduce this code, but I find AutoValue* value = > values.AppendElement() a teeny bit more natural. Mmm, fair enough. > Micro-optimization. Since you're sealing the array object later, and since > JS_DefineElement can't fail in this case, you could use > js_NewArrayObjectWithCapacity and avoid JS_DefineElement, if you like: Aha. I don't think we can use js_NewArrayObjectWithCapacity just yet (til we move into js/src), because we can't link against spidermonkey private functions. But wait, this is JS_FRIEND_API -- I'll poke. Nice find on the stealth API. I like! > I realize that you're just decomposing a useful method here, but I'm curious: > can library objects ever be used as a prototypes? Would there be any value in > walking the prototype chain of the object to see if any JS_GET_CLASS(cx, proto) > == &sLibraryClass? I *suppose* you could, but by that token, wouldn't we also want to walk the proto chains for CTypes and CDatas? Not sure what house rules are for that kind of thing. For instance, do Array objects do that?

Updated for rot.

Just the changes requested during review. This also includes a bonus change: using the js_NewArrayThingie in StructType::Create also.

Updated for rot.

Comment on attachment 435217 [details] [diff] [review] part 3.1 - implement closures > static JSBool > AttachProtos(JSContext* cx, JSObject* proto, JSObject** protos) > { > // For a given 'proto' of [[Class]] "CTypeProto", attach each of the 'protos' > // to the appropriate CTypeProtoSlot. >- for (PRUint32 i = 0; i < CTYPEPROTO_SLOTS; ++i) { >+ for (PRUint32 i = 0; i < SLOT_CLOSURECX; ++i) { So that people reading this code in the future don't have to go find the definition of SLOT_CLOSURECX in order to understand its weird relationship to the other prototype slot enum values, please change this loop condition either to i <= SLOT_UINT64PROTO (the last of the actual prototype slot enums) or back to i < CTYPEPROTO_SLOTS (and just skip SLOT_CLOSURECX). If you don't mind the extra iteration, the latter option seems the most readable. >+ // Construct from a JS function, and allow an optional 'this' argument. >+ JSObject* thisObj = NULL; >+ if (argc == 2) { >+ if (!JSVAL_IS_OBJECT(argv[1])) { >+ JS_ReportError(cx, "second argument must be an object"); >+ return JS_FALSE; >+ } >+ >+ thisObj = JSVAL_TO_OBJECT(argv[1]); >+ } Why so picky? What if I wanted 'this' to be a string? Can you use JS_ValueToObject to handle those cases? >+ // Convert each argument, and have any CData objects created depend on >+ // the existing buffers. >+ if (!ConvertToJS(cx, fninfo->mArgTypes[i], NULL, *(args++), false, false, >+ &argv[i])) This can be simpler: if (!ConvertToJS(cx, fninfo->mArgTypes[i], NULL, args[i], false, false, argv + i)) >+function run_single_closure_tests(library, abi, suffix) >+{ >+ function closure_fn(i) >+ { >+ return this.a ? this.a + i : i; >+ } In principle, I think you want return "a" in this ? this.a + i : i; for closure_fn, although your tests avoid cases like { a: 0 }. r=me with that stuff addressed.

(In reply to comment #23) > I'll make sure I post trivial updates from now on, since it seems you like to > apply them. :) I do like to apply patches, yes. I'm totally willing to repair minor bit-rot, but this case was a little more involved than usual. > I *suppose* you could, but by that token, wouldn't we also want to walk the > proto chains for CTypes and CDatas? Not sure what house rules are for that kind > of thing. For instance, do Array objects do that? I can imagine clients adding convenience methods to a subinstance of the library object. The over-engineer in me would like that to work, at least ;)

Comment on attachment 435216 [details] [diff] [review] part 2.3 - array tweaks Looks good. > // Duplicate the object for the fields property. >- if (!AddFieldToArray(cx, fieldsProp, i, info->mName, info->mType)) >+ if (!AddFieldToArray(cx, fieldsProp, &fieldsVec[i], >+ info->mName, info->mType)) > return JS_FALSE; Just personal preference, but I would write fieldsVec + i instead of &fieldsVec[i].

Comment on attachment 432933 [details] [diff] [review] part 1 - move Function impl http://hg.mozilla.org/mozilla-central/rev/145e2a27787a

Comment on attachment 435215 [details] [diff] [review] part 2.2 - implement FunctionType http://hg.mozilla.org/mozilla-central/rev/f2f30c49a9e8

Comment on attachment 435216 [details] [diff] [review] part 2.3 - array tweaks This got rolled into http://hg.mozilla.org/mozilla-central/rev/f2f30c49a9e8.

Comment on attachment 435217 [details] [diff] [review] part 3.1 - implement closures http://hg.mozilla.org/mozilla-central/rev/6db0eded43e5