quickjspp icon indicating copy to clipboard operation
quickjspp copied to clipboard

Binding a callback (std::function) causes runtime error

Open projectitis opened this issue 2 years ago • 11 comments

Hi again,

I'm trying to implement a callback in javascript that can be triggered from c++. This is what I am trying to do:

typedef std::function<void(void)> Callback;

class Listener {
private
    Callback _callback;
public:
    // Javascript can register a callback
    void listen( Callback callback ){
        _callback = callback;
    }

    // The callback can later be triggered by either javascript or by c++
    void trigger() {
        _callback();
    }
}

Binding like this compiles ok:

module.class_<Listener >( "Listener" )
    .fun<&Listener::listen>( "listen" );

But this javascript code causes a runtime exception:

function test() {
    console.log("test");
}
class MyListener extends Listener {
    constructor() {
        listen( test ); // Both of these calls to 'listen' (either of them) results in an exception 
        listen( methodTest );
    }
    methodTest() {
        console.log("method test");
    }
}

The exception occurs in the Value constructor:

Value(JSContext * ctx, T&& val) : ctx(ctx)
{
    v = js_traits<std::decay_t<T>>::wrap(ctx, std::forward<T>(val));
    if(JS_IsException(v))
        throw exception{}; // <-- this exception is thrown
}

Any ideas how to get (something like) this to work?

projectitis avatar Sep 04 '21 05:09 projectitis

I'm not a JS expert, but maybe Listener's constructor is not getting called?

ftk avatar Sep 04 '21 10:09 ftk

Thanks for your reply ftk. So theoretically quickjspp should be able to bind a method that takes a std::function as am argument?

// c++
void Listener::listen( std::function<void(void)> callback ){
    _callback = callback;
}

// quickjspp
module.class_<Listener>( "Listener" )
    .constructor<>()
    .fun<&Listener::listen>( "listen" );

If so, I am happy to continue to look for a solution to where I have gone wrong! As long as you don't see any reason the library won't support it? :)

projectitis avatar Sep 04 '21 10:09 projectitis

@projectitis are you showing your full code? need a super in this constructor, without can cause errors like yours

class MyListener extends Listener {
    constructor() {
        super(); // init listenere
        listen( test ); // Both of these calls to 'listen' (either of them) results in an exception 
        listen( methodTest );
    }
    methodTest() {
        console.log("method test");
    }
}

cykoder avatar Sep 04 '21 12:09 cykoder

No, sorry, it was just a snippet showing what my intentions are. Super is called in my actual code. BTW, it works perfectly in c++ (my listener system is working fine) it's just my quickjs implementation that is failing.

projectitis avatar Sep 04 '21 22:09 projectitis

Update: std::function works ok if the arguments and return type are non-custom types such as int, double, string etc. Where this fails is if the types are custom classes - even those that are bound via quickjspp and available in JS.

What might I be doing wrong? I'd like to get the second example to work.

Working example:

// c++
class Listener {
public:
    listen( std::function<bool(int)> callback ){ // Callback contains only non-custom types
        auto res = callback( 1234 );
        cout << "result " << res << endl; // This is fine. Prints "result 1"
    }
};

// quickjspp
module.class_<Listener>( "Listener" )
    .constructor<>()
    .fun<&Listener::listen>( "listen" );

// js
class MyClass extends Listener {
    constructor() {
        super();
        this.listen( this.exampleCallback );
    }
    exampleCallback( v ) {
        console.log( v ); // This is fine. Prints "1234"
        return true;
    }
}

Non-working example. This is what I would like to achieve. Compiles, but unexpected runtime result (empty object):

// c++
class Event {
public:
    int type = 0;
}
class Listener {
public:
    listen( std::function<bool(Event*)> callback ){ // Callback passes event by pointer
        Event* event = new Event();
        event->type = 1234;
        auto res = callback( event );
        delete event;
        cout << "result " << res << endl; // This is fine. Prints "result 1"
    }
};

// quickjspp
module.class_<Event>( "Event" )
    .constructor<>()
    .fun<&Event::type>( "type" );

module.class_<Listener>( "Listener" )
    .constructor<>()
    .fun<&Listener::listen>( "listen" );

// js
class MyClass extends Listener {
    constructor() {
        super();
        this.listen( this.exampleCallback );
    }
    exampleCallback( evt ) {
        console.log( evt.type ); // This is wrong. Prints garbage.
                                 // Dumping properties shows evt is actually an empty object { }
        return true;
    }
}

Non-working example, just for kicks. Does not compile. The only code change is that the callback argument is of Event type, not Event*.

// c++
class Event {
public:
    int type = 0;
}
class Listener {
public:
    listen( std::function<bool(Event)> callback ){ // Callback passes event object
        Event event;
        event.type = 1234;
        auto res = callback( event );
        cout << "result " << res << endl; // This is fine. Prints "result 1"
    }
};

// quickjspp
module.class_<Event>( "Event" )
    .constructor<>()
    .fun<&Event::type>( "type" );

module.class_<Listener>( "Listener" )
    .constructor<>()
    .fun<&Listener::listen>( "listen" );

// Compile error
undefined symbol: public: static unsigned __int64 __cdecl qjs::js_traits<class Event, void>::wrap(struct JSContext *, class Event)

projectitis avatar Sep 04 '21 23:09 projectitis

The second example works for me. log: 1234 result 1 Full code: https://gist.github.com/ftk/3184bd9ab08d753efe6a3470163b2035

ftk avatar Sep 05 '21 08:09 ftk

Wow, so strange. It's obviously something my side. Really appreciate you verifying that, ftk - thank you.

Edit: It is my Event class, which is much more complicated than the example. If I swap my Event class out for the basic one in the example it works fine :(

projectitis avatar Sep 05 '21 08:09 projectitis

I have been able to reproduce it. Simply add an empty virtual destructor to event. This causes the issue. Change Event in your gist to:

class Event {
public:
    virtual ~Event(){}
    int type = 0;
};

And the log will give you strange results.

In my case I have many event types that all extend Event (MouseEvent, ResizeEvent etc) hence the virtual destructor (and other virtual methods).

projectitis avatar Sep 05 '21 09:09 projectitis

This just got weird. Digging a little deeper, it looks like the virtual destructor somehow causes int type to be wrapped/unwrapped to double type.

// c++
event->type = 105;

// Javascript
log( event.type ); // log: 5.2e-322
// 5.2e-322 is the double representation of the binary number 105 (taking the binary bits)

See this updated gist: https://gist.github.com/projectitis/b8ab7ee70b3d58fe82731cdd138fc001

Any idea where/why this is happening?

EDIT: I have a workaround, which is adding a getter and binding that instead, but this is not ideal:

// c++
class Event {
public:
    virtual ~Event(){}
    int type = 0;
    int getType() { return type; }
};

// quickjspp
module.class_<Event>( "Event" )
    .constructor<>()
    .property<&Event::getType>( "type" );

EDIT 2: It happens for the first property in Event after the destructor.

class Event {
public:
    virtual ~Event(){}
    bool cancelled = true;  // Now this is mangled
    int type = 0; // But this is ok
};

So another workaround I have is a throw-away property in my class. This works:

class Event {
public:
    virtual ~Event(){}
    bool _ignore; // Never use this
    bool cancelled = true;  // Now this is ok
    int type = 0; // And this is ok
};

projectitis avatar Sep 06 '21 07:09 projectitis

Still can't reproduce the bug from the gist: log: 105 result 1 Which compiler and quickjs version are you running?

ftk avatar Sep 06 '21 09:09 ftk

I'm compiling with visual studio 2019 using clang-cl (the one that ships with MSVC) on windows 10 (targeting x64).

I've built quickjs v2021-03-27 from https://github.com/c-smile/quickjspp (because it purported to support visual studio) though I still had to modify the code to compile using clang (mostly swapping out POSIX functions for their non-POSIX named equivalents).

If it works for you, that's a good sign for me - it means I can solve it :)

EDIT: One of my dependencies (Skia) relies on clang for optimum performance. so I've tried to use clang to compile everything to ensure compatibility. So far it's been ok, but it's a real pain on windows when everything is generally set up to compile with MSVC.

projectitis avatar Sep 06 '21 09:09 projectitis