Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Update bindings for io.js 3.0.0 #1054

Merged
merged 15 commits into from
Aug 19, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
"glob": "^5.0.13",
"meow": "^3.3.0",
"mkdirp": "^0.5.1",
"nan": "^1.8.4",
"nan": "^2.0.1",
"npmconf": "^2.1.2",
"pangyp": "^2.2.1",
"request": "^2.58.0",
Expand Down
209 changes: 119 additions & 90 deletions src/binding.cpp

Large diffs are not rendered by default.

140 changes: 89 additions & 51 deletions src/callback_bridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@

#define COMMA ,

using namespace v8;

template <typename T, typename L = void*>
class CallbackBridge {
public:
CallbackBridge(NanCallback*, bool);
CallbackBridge(Nan::Callback*, bool);
virtual ~CallbackBridge();

// Executes the callback
Expand All @@ -22,12 +20,12 @@ class CallbackBridge {
protected:
// We will expose a bridge object to the JS callback that wraps this instance so we don't loose context.
// This is the V8 constructor for such objects.
static Handle<Function> get_wrapper_constructor();
static Nan::MaybeLocal<v8::Function> get_wrapper_constructor();
static void async_gone(uv_handle_t *handle);
static NAN_METHOD(New);
static NAN_METHOD(ReturnCallback);
static Persistent<Function> wrapper_constructor;
Persistent<Object> wrapper;
static Nan::Persistent<v8::Function> wrapper_constructor;
Nan::Persistent<v8::Object> wrapper;

// The callback that will get called in the main thread after the worker thread used for the sass
// compilation step makes a call to uv_async_send()
Expand All @@ -36,12 +34,12 @@ class CallbackBridge {
// The V8 values sent to our ReturnCallback must be read on the main thread not the sass worker thread.
// This gives a chance to specialized subclasses to transform those values into whatever makes sense to
// sass before we resume the worker thread.
virtual T post_process_return_value(Handle<Value>) const =0;
virtual T post_process_return_value(v8::Local<v8::Value>) const =0;


virtual std::vector<Handle<Value>> pre_process_args(std::vector<L>) const =0;
virtual std::vector<v8::Local<v8::Value>> pre_process_args(std::vector<L>) const =0;

NanCallback* callback;
Nan::Callback* callback;
bool is_sync;

std::mutex cv_mutex;
Expand All @@ -53,25 +51,30 @@ class CallbackBridge {
};

template <typename T, typename L>
Persistent<Function> CallbackBridge<T, L>::wrapper_constructor;
Nan::Persistent<v8::Function> CallbackBridge<T, L>::wrapper_constructor;

template <typename T, typename L>
CallbackBridge<T, L>::CallbackBridge(NanCallback* callback, bool is_sync) : callback(callback), is_sync(is_sync) {
// This assumes the main thread will be the one instantiating the bridge
CallbackBridge<T, L>::CallbackBridge(Nan::Callback* callback, bool is_sync) : callback(callback), is_sync(is_sync) {
/*
* This is invoked from the main JavaScript thread.
* V8 context is available.
*/
Nan::HandleScope scope;
if (!is_sync) {
this->async = new uv_async_t;
this->async->data = (void*) this;
uv_async_init(uv_default_loop(), this->async, (uv_async_cb) dispatched_async_uv_callback);
}

NanAssignPersistent(wrapper, NanNew(CallbackBridge<T, L>::get_wrapper_constructor())->NewInstance());
NanSetInternalFieldPointer(NanNew(wrapper), 0, this);
v8::Local<v8::Function> func = CallbackBridge<T, L>::get_wrapper_constructor().ToLocalChecked();
wrapper.Reset(Nan::NewInstance(func).ToLocalChecked());
Nan::SetInternalFieldPointer(Nan::New(wrapper), 0, this);
}

template <typename T, typename L>
CallbackBridge<T, L>::~CallbackBridge() {
delete this->callback;
NanDisposePersistent(this->wrapper);
this->wrapper.Reset();

if (!is_sync) {
uv_close((uv_handle_t*)this->async, &async_gone);
Expand All @@ -81,51 +84,88 @@ CallbackBridge<T, L>::~CallbackBridge() {
template <typename T, typename L>
T CallbackBridge<T, L>::operator()(std::vector<void*> argv) {
// argv.push_back(wrapper);

if (this->is_sync) {
std::vector<Handle<Value>> argv_v8 = pre_process_args(argv);
argv_v8.push_back(NanNew(wrapper));
/*
* This is invoked from the main JavaScript thread.
* V8 context is available.
*
* Establish Local<> scope for all functions
* from types invoked by pre_process_args() and
* post_process_args().
*/
Nan::HandleScope scope;
std::vector<v8::Local<v8::Value>> argv_v8 = pre_process_args(argv);
argv_v8.push_back(Nan::New(wrapper));

return this->post_process_return_value(
NanNew<Value>(this->callback->Call(argv_v8.size(), &argv_v8[0]))
this->callback->Call(argv_v8.size(), &argv_v8[0])
);
} else {
/*
* This is invoked from the worker thread.
* No V8 context and functions available.
* Just wait for response from asynchronously
* scheduled JavaScript code
*
* XXX Issue #1048: We block here even if the
* event loop stops and the callback
* would never be executed.
* XXX Issue #857: By waiting here we occupy
* one of the threads taken from the
* uv threadpool. Might deadlock if
* async I/O executed from JavaScript callbacks.
*/
this->argv = argv;

std::unique_lock<std::mutex> lock(this->cv_mutex);
this->has_returned = false;
uv_async_send(this->async);
this->condition_variable.wait(lock, [this] { return this->has_returned; });

return this->return_value;
}

this->argv = argv;

std::unique_lock<std::mutex> lock(this->cv_mutex);
this->has_returned = false;
uv_async_send(this->async);
this->condition_variable.wait(lock, [this] { return this->has_returned; });

return this->return_value;
}

template <typename T, typename L>
void CallbackBridge<T, L>::dispatched_async_uv_callback(uv_async_t *req) {
CallbackBridge* bridge = static_cast<CallbackBridge*>(req->data);

NanScope();
TryCatch try_catch;
/*
* Function scheduled via uv_async mechanism, therefore
* it is invoked from the main JavaScript thread.
* V8 context is available.
*
* Establish Local<> scope for all functions
* from types invoked by pre_process_args() and
* post_process_args().
*/
Nan::HandleScope scope;
Nan::TryCatch try_catch;

std::vector<Handle<Value>> argv_v8 = bridge->pre_process_args(bridge->argv);
argv_v8.push_back(NanNew(bridge->wrapper));
std::vector<v8::Local<v8::Value>> argv_v8 = bridge->pre_process_args(bridge->argv);
argv_v8.push_back(Nan::New(bridge->wrapper));

NanNew<Value>(bridge->callback->Call(argv_v8.size(), &argv_v8[0]));
bridge->callback->Call(argv_v8.size(), &argv_v8[0]);

if (try_catch.HasCaught()) {
node::FatalException(try_catch);
Nan::FatalException(try_catch);
}
}

template <typename T, typename L>
NAN_METHOD(CallbackBridge<T COMMA L>::ReturnCallback) {
NanScope();

CallbackBridge<T, L>* bridge = static_cast<CallbackBridge<T, L>*>(NanGetInternalFieldPointer(args.This(), 0));
TryCatch try_catch;
/*
* Callback function invoked by the user code.
* It is invoked from the main JavaScript thread.
* V8 context is available.
*
* Implicit Local<> handle scope created by NAN_METHOD(.)
*/
CallbackBridge<T, L>* bridge = static_cast<CallbackBridge<T, L>*>(Nan::GetInternalFieldPointer(info.This(), 0));
Nan::TryCatch try_catch;

bridge->return_value = bridge->post_process_return_value(args[0]);
bridge->return_value = bridge->post_process_return_value(info[0]);

{
std::lock_guard<std::mutex> lock(bridge->cv_mutex);
Expand All @@ -135,33 +175,31 @@ NAN_METHOD(CallbackBridge<T COMMA L>::ReturnCallback) {
bridge->condition_variable.notify_all();

if (try_catch.HasCaught()) {
node::FatalException(try_catch);
Nan::FatalException(try_catch);
}

NanReturnUndefined();
}

template <typename T, typename L>
Handle<Function> CallbackBridge<T, L>::get_wrapper_constructor() {
Nan::MaybeLocal<v8::Function> CallbackBridge<T, L>::get_wrapper_constructor() {
/* Uses handle scope created in the CallbackBridge<T, L> constructor */
if (wrapper_constructor.IsEmpty()) {
Local<FunctionTemplate> tpl = NanNew<FunctionTemplate>(New);
tpl->SetClassName(NanNew("CallbackBridge"));
v8::Local<v8::FunctionTemplate> tpl = Nan::New<v8::FunctionTemplate>(New);
tpl->SetClassName(Nan::New("CallbackBridge").ToLocalChecked());
tpl->InstanceTemplate()->SetInternalFieldCount(1);
tpl->PrototypeTemplate()->Set(
NanNew("success"),
NanNew<FunctionTemplate>(ReturnCallback)->GetFunction()

Nan::SetPrototypeTemplate(tpl, "success",
Nan::GetFunction(Nan::New<v8::FunctionTemplate>(ReturnCallback)).ToLocalChecked()
);

NanAssignPersistent(wrapper_constructor, tpl->GetFunction());
wrapper_constructor.Reset(Nan::GetFunction(tpl).ToLocalChecked());
}

return NanNew(wrapper_constructor);
return Nan::New(wrapper_constructor);
}

template <typename T, typename L>
NAN_METHOD(CallbackBridge<T COMMA L>::New) {
NanScope();
NanReturnValue(args.This());
info.GetReturnValue().Set(info.This());
}

template <typename T, typename L>
Expand Down
12 changes: 9 additions & 3 deletions src/create_string.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,18 @@
#include <string.h>
#include "create_string.h"

char* create_string(Local<Value> value) {
if (value->IsNull() || !value->IsString()) {
char* create_string(Nan::MaybeLocal<v8::Value> maybevalue) {
v8::Local<v8::Value> value;

if (maybevalue.ToLocal(&value)) {
if (value->IsNull() || !value->IsString()) {
return 0;
}
} else {
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use the Maybe API instead of dangerous null pointers. This would be return Nan::Nothing();, the other case return Nan::Just(str);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-char* create_string(Local value) {

  • if (value->IsNull() || !value->IsString()) {
    +char* create_string(Nan::MaybeLocalv8::Value maybevalue) {
  • v8::Localv8::Value value;
  • if (maybevalue.ToLocal(&value)) {
  • if (value->IsNull() || !value->IsString()) {
  •  return 0;
    
  • }
  • } else {
    return 0;

Could use the Maybe API instead of dangerous null pointers. This
would be return Nan::Nothing();, the other case return Nan::Just(str);

Unfortunately we are leaving a nice V8 world here:

create_string() pointers are being directly fed into C structures.
In fact this function exists only because the libsass C API insists
that almost all (char *) pointers should be heap allocated, because it
says it frees them (I think this API is incorrect, but that's another story).

But ~Utf8String() may crash if someone free()'s that underlying string, am I correct?
And if the string fits n str_st_ it can't be free()d.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revisit it later - we might need to review create_string() anyway to better comply with libsass API.

For now I have more issues to worry than V8 sending me wrong UTF-8 :)

}

String::Utf8Value string(value);
v8::String::Utf8Value string(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nan::Utf8String is better, since it handles invalid UTF-8 in a sensible way.

char *str = (char *)malloc(string.length() + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it would be better if you made it return a pointer to a heap allocated Nan::Utf8String, which can be dereferenced for access to the underlying char * and allows you to do delete when appropriate. This way you avoid the second memory allocation and string copy.

strcpy(str, *string);
return str;
Expand Down
6 changes: 2 additions & 4 deletions src/create_string.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@

#include <nan.h>

using namespace v8;
char* create_string(Nan::MaybeLocal<v8::Value>);

char* create_string(Local<Value>);

#endif
#endif
6 changes: 3 additions & 3 deletions src/custom_function_bridge.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#include "custom_function_bridge.h"
#include "sass_types/factory.h"

Sass_Value* CustomFunctionBridge::post_process_return_value(Handle<Value> val) const {
Sass_Value* CustomFunctionBridge::post_process_return_value(v8::Local<v8::Value> val) const {
try {
return SassTypes::Factory::unwrap(val)->get_sass_value();
}
Expand All @@ -11,8 +11,8 @@ Sass_Value* CustomFunctionBridge::post_process_return_value(Handle<Value> val) c
}
}

std::vector<Handle<Value>> CustomFunctionBridge::pre_process_args(std::vector<void*> in) const {
std::vector<Handle<Value>> argv = std::vector<Handle<Value>>();
std::vector<v8::Local<v8::Value>> CustomFunctionBridge::pre_process_args(std::vector<void*> in) const {
std::vector<v8::Local<v8::Value>> argv = std::vector<v8::Local<v8::Value>>();

for (void* value : in) {
argv.push_back(SassTypes::Factory::create(static_cast<Sass_Value*>(value))->get_js_object());
Expand Down
8 changes: 3 additions & 5 deletions src/custom_function_bridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@
#include <sass_context.h>
#include "callback_bridge.h"

using namespace v8;

class CustomFunctionBridge : public CallbackBridge<Sass_Value*> {
public:
CustomFunctionBridge(NanCallback* cb, bool is_sync) : CallbackBridge<Sass_Value*>(cb, is_sync) {}
CustomFunctionBridge(Nan::Callback* cb, bool is_sync) : CallbackBridge<Sass_Value*>(cb, is_sync) {}

private:
Sass_Value* post_process_return_value(Handle<Value>) const;
std::vector<Handle<Value>> pre_process_args(std::vector<void*>) const;
Sass_Value* post_process_return_value(v8::Local<v8::Value>) const;
std::vector<v8::Local<v8::Value>> pre_process_args(std::vector<void*>) const;
};

#endif
Loading