v8pp::cleanup deletes instances exported with reference_external

[jcmonnin]

I have a stack allocated object. I wan't to access to that particular instance from JavaScript. I don't want v8 to manage the lifetime of that object. I'm using v8pp::class_<X>::reference_external to achieve that. However, when calling v8pp::cleanup(isolate) v8pp tries to delete the instance. I think this is a bug (unless I'm misunderstanding reference_external).

Please check the code below that illustrates the problem. There is also the AddressSanitizer output below. Sample is a bit long because there is the whole boilerplate code to init/destroy v8, which is relevant for that issue (based on https://chromium.googlesource.com/v8/v8/+/master/samples/hello-world.cc):

#include <iostream>
#include "v8pp/module.hpp"
#include "v8pp/class.hpp"
#include <libplatform/libplatform.h>

struct X
{
    bool b;
    X(bool b) : b(b) {}
};

int main(int argc, const char * argv[])
{
    using namespace v8;

    // Instance of X that is going to be accessible from JavaScript
    X x(false);

    // Initialize V8.
    V8::InitializeICU();
    V8::InitializeExternalStartupData(argv[0]);
    Platform* platform = platform::CreateDefaultPlatform();
    V8::InitializePlatform(platform);
    V8::Initialize();

    // Create a new Isolate and make it the current one.
    Isolate::CreateParams create_params;
    create_params.array_buffer_allocator = v8::ArrayBuffer::Allocator::NewDefaultAllocator();
    Isolate* isolate = Isolate::New(create_params);
    {
        Isolate::Scope isolate_scope(isolate);

        // Create a stack-allocated handle scope.
        HandleScope handle_scope(isolate);

        // Create a new context.
        Local<Context> context = Context::New(isolate);

        // Enter the context for compiling and running the hello world script.
        Context::Scope context_scope(context);

        // Bind custom C++ functions and objects
        v8pp::class_<X> X_class(isolate);
        X_class.set("b", &X::b);

        v8pp::module module(isolate);
        module.set("X", X_class);

        isolate->GetCurrentContext()->Global()->Set(v8::String::NewFromUtf8(isolate, "module"),
                                                    module.new_instance());

        isolate->GetCurrentContext()->Global()->Set(v8::String::NewFromUtf8(isolate, "x"),
                                                    v8pp::class_<X>::reference_external(isolate, &x));

        // Create a string containing the JavaScript source code.
        Local<String> source = String::NewFromUtf8(isolate, "x.b", NewStringType::kNormal).ToLocalChecked();

        // Compile the source code.
        Local<Script> script = Script::Compile(context, source).ToLocalChecked();

        // Run the script to get the result.
        TryCatch trycatch(isolate);
        Local<Value> result = script->Run();
        if (result.IsEmpty()) {
            Local<Value> exception = trycatch.Exception();
            String::Utf8Value exception_str(exception);
            printf("Exception: %s\n", *exception_str);
        } else {
            // Convert the result to an UTF8 string and print it.
            String::Utf8Value utf8(result);
            printf("%s\n", *utf8);
        }
    }

    // Dispose the isolate and tear down V8.
    v8pp::cleanup(isolate);
    isolate->Dispose();
    V8::Dispose();
    V8::ShutdownPlatform();
    delete platform;
    delete create_params.array_buffer_allocator;
    return 0;
}

=================================================================
==59589==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x7fff5fbfeb60 in thread T0
    #0 0x100d36bbb in wrap__ZdlPv (libclang_rt.asan_osx_dynamic.dylib+0x57bbb)
    #1 0x10002ca2d in v8pp::factory<X>::destroy(v8::Isolate*, X*) factory.hpp:35
    #2 0x10002c9f3 in v8pp::detail::class_singleton<X>::class_singleton(v8::Isolate*, v8pp::detail::type_info const&)::'lambda'(v8::Isolate*, void*)::operator()(v8::Isolate*, void*) const class.hpp:356
    #3 0x10002c9bf in v8pp::detail::class_singleton<X>::class_singleton(v8::Isolate*, v8pp::detail::type_info const&)::'lambda'(v8::Isolate*, void*)::__invoke(v8::Isolate*, void*) class.hpp:354
    #4 0x10002eb42 in v8pp::detail::class_info::remove_objects(v8::Isolate*, void (*)(v8::Isolate*, void*)) class.hpp:153
    #5 0x10002e07f in v8pp::detail::class_singleton<X>::destroy_objects() class.hpp:505
    #6 0x10002db13 in v8pp::detail::class_singleton<X>::~class_singleton() class.hpp:365
    #7 0x10001c594 in v8pp::detail::class_singleton<X>::~class_singleton() class.hpp:364
    #8 0x10001c5b8 in v8pp::detail::class_singleton<X>::~class_singleton() class.hpp:364
    #9 0x10000980f in std::__1::__vector_base<std::__1::unique_ptr<v8pp::detail::class_info, std::__1::default_delete<v8pp::detail::class_info> >, std::__1::allocator<std::__1::unique_ptr<v8pp::detail::class_info, std::__1::default_delete<v8pp::detail::class_info> > > >::~__vector_base() memory:2525
    #10 0x100009374 in std::__1::vector<std::__1::unique_ptr<v8pp::detail::class_info, std::__1::default_delete<v8pp::detail::class_info> >, std::__1::allocator<std::__1::unique_ptr<v8pp::detail::class_info, std::__1::default_delete<v8pp::detail::class_info> > > >::~vector() iterator:1244
    #11 0x100009354 in std::__1::vector<std::__1::unique_ptr<v8pp::detail::class_info, std::__1::default_delete<v8pp::detail::class_info> >, std::__1::allocator<std::__1::unique_ptr<v8pp::detail::class_info, std::__1::default_delete<v8pp::detail::class_info> > > >::~vector() iterator:1244
    #12 0x100009334 in v8pp::detail::class_singletons::~class_singletons() class.hpp:199
    #13 0x100009314 in v8pp::detail::class_singletons::~class_singletons() class.hpp:199
    #14 0x1000092f8 in std::__1::pair<v8::Isolate* const, v8pp::detail::class_singletons>::~pair() utility:253
    #15 0x1000092d4 in std::__1::pair<v8::Isolate* const, v8pp::detail::class_singletons>::~pair() utility:253
    #16 0x100011992 in std::__1::__hash_table<std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons>, std::__1::__unordered_map_hasher<v8::Isolate*, std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons>, std::__1::hash<v8::Isolate*>, true>, std::__1::__unordered_map_equal<v8::Isolate*, std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons>, std::__1::equal_to<v8::Isolate*>, true>, std::__1::allocator<std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons> > >::erase(std::__1::__hash_const_iterator<std::__1::__hash_node<std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons>, void*>*>) memory:1673
    #17 0x1000112f9 in unsigned long std::__1::__hash_table<std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons>, std::__1::__unordered_map_hasher<v8::Isolate*, std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons>, std::__1::hash<v8::Isolate*>, true>, std::__1::__unordered_map_equal<v8::Isolate*, std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons>, std::__1::equal_to<v8::Isolate*>, true>, std::__1::allocator<std::__1::__hash_value_type<v8::Isolate*, v8pp::detail::class_singletons> > >::__erase_unique<v8::Isolate*>(v8::Isolate* const&) __hash_table:2370
    #18 0x1000064b5 in v8pp::detail::class_singletons::instance(v8pp::detail::class_singletons::operation, v8::Isolate*) unordered_map:1099
    #19 0x10000516b in v8pp::detail::class_singletons::remove_all(v8::Isolate*) class.hpp:257
    #20 0x100004b64 in v8pp::cleanup(v8::Isolate*) class.hpp:749
    #21 0x100003693 in main testExternalReference.cpp:78
    #22 0x7fff8d59b5ac in start (libdyld.dylib+0x35ac)

AddressSanitizer can not describe address in more detail (wild memory access suspected).
SUMMARY: AddressSanitizer: bad-free (libclang_rt.asan_osx_dynamic.dylib+0x57bbb) in wrap__ZdlPv
==59589==ABORTING

[pmed]

Thanks, this is definitely a bug, I'll fix it.

[pmed]

I'm trying to merge this fix with a shared_ptr branch. This branch allows to use store wrapped objects either as std::shared_ptr, or as raw pointers.

Currently there is a workaround to use class_<T>::unreference_external(isolate, ptr) function for all variables referenced in v8pp with v8pp::class_<T>::reference_external() function:

#include "v8.h"
#include "libplatform/libplatform.h"

#include "v8pp/context.hpp"
#include "v8pp/class.hpp"
#include "v8pp/object.hpp"

struct X
{
    bool b;
    X(bool b) : b(b) {}
};

int main()
{
    X x(true);

    v8::V8::InitializeICU();
    //v8::V8::InitializeExternalStartupData(argv[0]);
    std::unique_ptr<v8::Platform> platform(v8::platform::CreateDefaultPlatform());
    v8::V8::InitializePlatform(platform.get());
    v8::V8::Initialize();

    {
        v8pp::context context; // a wrapper for v8::Context
        v8::Isolate* isolate = context.isolate();
        v8::HandleScope scope(isolate);

        v8pp::class_<X> X_class(isolate);
        X_class.set("b", &X::b);

        context.set("X", X_class);
        v8pp::set_option(isolate, isolate->GetCurrentContext()->Global(),
            "x", X_class.reference_external(isolate, &x));

        bool const b = v8pp::from_v8<bool>(isolate, context.run_script("x.b"));
        assert(b == x.b);

        // unreference x object before context cleanup
        X_class.unreference_external(isolate, &x);

    // v8pp::context::~context() calls v8pp::cleanup()
    }

    v8::V8::Dispose();
    v8::V8::ShutdownPlatform();
}

[jcmonnin]

Thanks. The workaround is ok for now. Good to hear you plan to merge the shared_ptr branch to master. I'm currently on that branch. I need std::shared_ptr.

[pmed]

Fixed in #60