Issues with new SetOpaque API

[bptato]

Attempting to upgrade to the new version (?), I've noticed some oddities with SetOpaque:

• Since Prevent JS_SetOpaque from overriding internal class state #658 it can fail. But it returns JS_FALSE (0) on success and JS_TRUE (1) on failure, which I find somewhat counter-intuitive. Maybe it's not too late to change it?
• Previously, I could set an opaque on the global object, but now I can't because it's a JS_CLASS_OBJECT which is < JS_CLASS_INIT_COUNT.

The latter one is a bit problematic, because my type conversion functions expect the global object to have an opaque. I would change its class ID if I could, but at least when I was writing my wrapper for qjs I couldn't, and I don't see any changes in -ng that would enable this.

I don't know what the preferred fix is for this one; this patch works for me:

diff --git a/quickjs.c b/quickjs.c
index 9cac6de..e268ce3 100644
--- a/quickjs.c
+++ b/quickjs.c
@@ -10102,7 +10102,8 @@ JS_BOOL JS_SetOpaque(JSValue obj, void *opaque)
     if (JS_VALUE_GET_TAG(obj) == JS_TAG_OBJECT) {
         p = JS_VALUE_GET_OBJ(obj);
         // User code can't set the opaque of internal objects.
-        if (p->class_id >= JS_CLASS_INIT_COUNT) {
+        if (p->class_id >= JS_CLASS_INIT_COUNT ||
+            p->class_id == JS_CLASS_OBJECT) {
             p->u.opaque = opaque;
             return 0;
         }

[saghul]

Hey there!

Many APIs return 0 for success and -1 for error. I guess we could align it to that. JS_BOOL is indeed probably not the right return type here.

I took a quick look and JS_CLASS_OBJECT does not seem to use the union. That said, what is it that you are using the opaque for? If it's for hiding stuff perhaps a Symbol would do? Or the context / runtime opaque? Since there is only one global...

[bptato]

Many APIs return 0 for success and -1 for error. I guess we could align it to that. JS_BOOL is indeed probably not the right return type here.

Sounds good :)

I took a quick look and JS_CLASS_OBJECT does not seem to use the union. That said, what is it that you are using the opaque for? If it's for hiding stuff perhaps a Symbol would do? Or the context / runtime opaque? Since there is only one global...

It's for object conversion:

https://git.sr.ht/~bptato/monoucha/tree/faff9e175ae6e6d781c4fb770a49bfa30bd6f703/item/monoucha/fromjs.nim#L309

I could check if val is the global object and then override p with some pointer from the context opaque, but I'd prefer not to add special casing logic if possible. Less code = prettier code :P

[saghul]

I opened #696 to better align the API.

I gave it some thought and I think I'd like to keep it as is, that is, only supporting custom classes.

Arrays, Maps, Sets, are objects too but not quite JS_CLASS_OBJECT, so it wouldn't be obvious why it doesn't work there. Making it work with plain objects too would make things a bit more confusing IMHO. Right now it's easy to explain when it works and when it doesn't.

I could check if val is the global object and then override p with some pointer from the context opaque, but I'd prefer not to add special casing logic if possible. Less code = prettier code :P

Well, instead we would have to do the special casing ;-)

@bnoordhuis WDYT?

[bnoordhuis]

I'd prefer not to add special casing logic if possible

The flip side is we'd have to make JSObject bigger to support JS_SetOpaque on arbitrary objects. Then everyone is worse off.

[saghul]

Indeed. The corner case here is that plain objects don't use the union so the opaque is technically available, but I think it's best to not support it and have the simple rule of "just custom classes".

[bptato]

Arrays, Maps, Sets, are objects too but not quite JS_CLASS_OBJECT, so it wouldn't be obvious why it doesn't work there. Making it work with plain objects too would make things a bit more confusing IMHO. Right now it's easy to explain when it works and when it doesn't.

Well, now you have to explain that there is a breaking change lurking in there ;)

I can imagine code that also uses it for regular objects other than the global one, and that would also break. But maybe I'm the only one.

Well, instead we would have to do the special casing ;-)

Fair enough.
I'm still not a fan of adding a pointless check to every single object conversion, so I'll just patch my copy then.

The flip side is we'd have to make JSObject bigger to support JS_SetOpaque on arbitrary objects. Then everyone is worse off.

To clarify: SetOpaque had always worked with JS_CLASS_OBJECT, without the need for an extra pointer. QJS only uses the opaque for its own object classes.

[saghul]

When used with other types of objects it would cause memory corruption because it overwrote the memory as opaque is part of the union.

That was the reasoning for the initial change.

[bptato]

I know, I've read the linked PR & issue. By regular objects I mean objects with class_id JS_CLASS_OBJECT. That doesn't cause memory corruption, right?

[saghul]

It doesn't, but that is by chance really.

Let's wait a bit and see. The change only got released yesterday and we have a single data point so far :-)

Understanding how this impacts different users would go a long way here. From my side, txiki.js is not impacted because I actually didn't know we could set it on any object to begin with 😅

[bptato]

It doesn't, but that is by chance really.

Well, my code works because I had checked the source before using the API. No chance involved :p

Let's wait a bit and see. The change only got released yesterday and we have a single data point so far :-)

OK.

To note, my use case would also be satisfied if we had an API to change the class of the global object instead. I think I even implemented that at some point, but it didn't bring the benefits I'd expected (exotics) so I discarded the patch :/