Now using v8::MaybeLocal::ToLocal when getting object and function va…

[jacobsologub]

…lues inside detail::object_registry::wrap_object()

[pmed]

Hi @jacobsologub

Could you please elaborate the reason for ToLocal() instead of ToLocalChecked() usage? Currently the checked version throws an exception, for empty MaybeLocal instance (that shouldn't be empty in the wrap_object() ).

As I understand, this is another way to handle such a hypothetical empty MaybeLocal on the wrapping C++ object. The wrap_object() function would return an empty handle, instead of the exception throwing. It seems also a reasonable behavior, but different to the previous version.

[jacobsologub]

Hi @pmed

This fixes a v8:: Fatal error in v8::ToLocalChecked Empty/MaybeLocal that occurs when throwing an exception from a constructor of a wrapped object. The error only happens in specific cases.

I've put together a little test harness so you can have a go. There are three JavaScript string literals: JavaScript1 (ok), JavaScript2 (crash), and JavaScript3 (crash). I haven't wrapped my head around on why this happens but the fix seems to resolve it.

//
//  main.cpp
//  v8harness
//  Created on 10/28/19.
//

#include <iostream>
#include <libplatform/libplatform.h>
#include <v8pp/module.hpp>
#include <v8pp/class.hpp>
#include <v8pp/convert.hpp>
#include <v8pp/object.hpp>
#include <v8pp/call_v8.hpp>

/**
 * Interface to access C++ classes bound to V8
 */
namespace v8pp {
template<typename T>
using raw_class = class_<T, raw_ptr_traits>;
}

/**
 * Bind Helper
 */
template <class T>
struct Object {
    static void bind (v8pp::module& lib, std::string_view name) {
        v8pp::raw_class<T> _class (lib.isolate());
        T::bind (_class);
        lib.class_ (name, _class);
    }

private:
    Object() = delete;
    ~Object() = delete;
};

/**
 * Represents a Filter
*/
class Filter {
public:
    Filter (const v8::FunctionCallbackInfo<v8::Value>& args) {
        auto isolate = args.GetIsolate();
        auto arg = args [0];
        if (arg->IsString()) {
            // xx
        }
        else {
            v8pp::throw_ex (isolate, "Invalid Filter", v8::Exception::Error);
        }
    }

    ~Filter() = default;

    static void bind (v8pp::raw_class<Filter>& class_) {
        class_.ctor<const v8::FunctionCallbackInfo<v8::Value>&>()
            .auto_wrap_objects()
        ;
    }

private:
    // xx
};

/**
 * JavaScript1 OK
 */
const char* JavaScript1 = R"(

// ok

(function main() {
    let f1 = new lib.Filter ("blur");
    let f2 = new lib.Filter();
});

)";

/**
 * JavaScript2 CRASH
 */
const char* JavaScript2 = R"(

// crash

(function main() {
    let f1 = new lib.Filter();
    let f2 = new lib.Filter ("blur");
});

)";

/**
 * JavaScript3 CRASH
 */
const char* JavaScript3 = R"(

// crash

(function main() {
    let f1 = new lib.Filter();
});

)";

int main (int argc, char* argv[]) {
    auto v8Platform = v8::platform::NewDefaultPlatform();
    v8::V8::InitializePlatform (v8Platform.get());
    v8::V8::Initialize();

    v8::Isolate::CreateParams v8Create_params{};
    v8Create_params.array_buffer_allocator = v8::ArrayBuffer::Allocator::NewDefaultAllocator();
    auto isolate = v8::Isolate::New (v8Create_params);

    {
        v8::Isolate::Scope isolate_scope (isolate);
        v8::HandleScope handle_scope (isolate);
        v8::Local<v8::Context> context = v8::Context::New (isolate);
        v8::Context::Scope context_scope (context);

        v8pp::module lib (isolate);
        Object<Filter>::bind (lib, "Filter");

        isolate->GetCurrentContext()->Global()->Set (context, v8pp::to_v8 (isolate, "lib"), lib.new_instance()).FromJust();

        v8::TryCatch try_catch (isolate);
        v8::Local<v8::Script> scriptResult{};
        if (! v8::Script::Compile (context, v8pp::to_v8 (isolate, JavaScript1)).ToLocal (&scriptResult)) {
            v8::String::Utf8Value utf8 (isolate, try_catch.Exception());
            std::cout << *utf8 << "\n";
        }
        else {
            v8::TryCatch try_catch (isolate);
            v8::Local<v8::Value> result{};
            if (! scriptResult->Run (context).ToLocal (&result)) {
                v8::String::Utf8Value utf8 (isolate, try_catch.Exception());
                std::cout << *utf8 << "\n";
            }
            else {
                if (result->IsFunction()) {
                    v8::Local<v8::Function> mainFunc = v8::Local<v8::Function>::Cast (result);

                    v8::TryCatch try_catch (isolate);
                    v8::Local<v8::Value> argv [0] = {};
                    v8::Local<v8::Value> result{};

                    if (! mainFunc->Call (context, context->Global(), 0, argv).ToLocal (&result)) {
                        v8::String::Utf8Value utf8 (isolate, try_catch.Exception());
                        std::cout << *utf8 << "\n";

                        // Exception Handled!
                    }
                    else {
                        // xx
                    }
                }
            }
        }
    }

    v8pp::detail::classes::remove_all (isolate);

    isolate->Dispose();
    v8::V8::Dispose();
    v8::V8::ShutdownPlatform();
    delete v8Create_params.array_buffer_allocator;
    return 0;
}

Let me know if you need any additional info.

Thanks, Jacob

[pmed]

Hi @jacobsologub

that's great, thanks a lot! I will try to add a test case and merge these changes.

[jcmonnin]

ToLocalChecked terminates the program if the value happens to be empty. I can't comment much on this specific case, but I'm not certain this patch to silently ignore the error is the best way to handle it. On a different but related topic, another case when local values may be empty is when a script is aborted. Supporting aborting user script would be a feature I would like to support, but the numerous calls to ToLocalChecked makes it impossible as there are many cases where the application would be terminated. To support that, I think that v8pp would need a helper function that throws a C++ exception when converting to local fails.