Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
projectitis opened this issue Sep 4, 2021 · 11 comments
Open

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

projectitis opened this issue Sep 4, 2021 · 11 comments

Comments

@projectitis
Copy link

projectitis commented Sep 4, 2021

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?

@ftk
Copy link
Owner

ftk commented Sep 4, 2021

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

@projectitis
Copy link
Author

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? :)

@cykoder
Copy link

cykoder commented Sep 4, 2021

@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");
    }
}

@projectitis
Copy link
Author

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
Copy link
Author

projectitis commented Sep 4, 2021

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)

@ftk
Copy link
Owner

ftk commented Sep 5, 2021

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

@projectitis
Copy link
Author

projectitis commented Sep 5, 2021

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
Copy link
Author

projectitis commented Sep 5, 2021

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
Copy link
Author

projectitis commented Sep 6, 2021

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
};

@ftk
Copy link
Owner

ftk commented Sep 6, 2021

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

@projectitis
Copy link
Author

projectitis commented Sep 6, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants