pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
890 stars 120 forks source link

v8pp::from_v8<std::string>() crash #76

Closed VitaminCpp closed 6 years ago

VitaminCpp commented 6 years ago

Hi again,

I've encountered another problem when using V8 version 6.4.388. If you try the following code, your program will crash: auto str = v8pp::from_v8<std::string>(args.GetIsolate(), v8Value);

This happens because the new V8 version requires an isolate for each v8::String::Utf8Value() conversion. So this is not valid anymore: v8::String::Utf8Value const str(value);

Instead you have to write: v8::String::Utf8Value const str(isolate, value);

I think there are more places in v8pp where this issue must be fixed?

Thanks!

pmed commented 6 years ago

Hi,

This is a strange error. I've added several V8 versions (from 5.9 to 6.6) in Travis CI, and all the tests have been passed there. I need to look what is going on in my development machine.

pmed commented 6 years ago

May the v8Value be an empty value or not a String instance? In this case a std::invalid_argument("expected String") exception would be thrown: https://github.com/pmed/v8pp/blob/master/v8pp/convert.hpp#L64-L67

VitaminCpp commented 6 years ago

Hi,

I've found the problem... It was my fault. I forgot to "Enter" my own created isolate. So if you never enter an isolate, v8::Isolate::GetCurrent() will return null.

Anyway, the default constructor of Utf8Value without the isolate is deprecated and the new overload (with the isolate) should be used as stated here:

/**
   * Converts an object to a UTF-8-encoded character array.  Useful if
   * you want to print the object.  If conversion to a string fails
   * (e.g. due to an exception in the toString() method of the object)
   * then the length() method returns 0 and the * operator returns
   * NULL.
   */
  class V8_EXPORT Utf8Value {
   public:
    V8_DEPRECATED("Use Isolate version",
                  explicit Utf8Value(Local<v8::Value> obj));
    Utf8Value(Isolate* isolate, Local<v8::Value> obj);
    ~Utf8Value();
    char* operator*() { return str_; }
    const char* operator*() const { return str_; }
    int length() const { return length_; }

    // Disallow copying and assigning.
    Utf8Value(const Utf8Value&) = delete;
    void operator=(const Utf8Value&) = delete;

   private:
    char* str_;
    int length_;
  };

Sorry for any circumstances! :)

pmed commented 6 years ago

You are welcome!

Yes, deprecated parts of V8 need to be updated, but it's a matter of supporting cutting edge vs. stable V8 versions. Most of package managers contain not so new V8 versions, Homebrew in macOS for example contains 5.1.