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

Commit

Permalink
[LEAK FIX] Use Nan::ObjectWrap to handle memory management
Browse files Browse the repository at this point in the history
Nan::ObjectWrap Docs: https://github.com/nodejs/nan/blob/master/doc/object_wrappers.md#api_nan_object_wrap

Prior to this, all wrapper Objects would never be deleted, allowing memory to grow unbounded.

Example:
The following, would leak WrapperObjects, and never

```
while (true) {
  new sass.types.String(‘I LEAK’);
}
```
  • Loading branch information
stefanpenner committed Oct 31, 2017
1 parent ad15435 commit 41550fd
Show file tree
Hide file tree
Showing 12 changed files with 141 additions and 148 deletions.
13 changes: 8 additions & 5 deletions src/custom_function_bridge.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
#include "sass_types/factory.h"
#include "sass_types/value.h"

Sass_Value* CustomFunctionBridge::post_process_return_value(v8::Local<v8::Value> val) const {
SassTypes::Value *v_;
if ((v_ = SassTypes::Factory::unwrap(val))) {
return v_->get_sass_value();
Sass_Value* CustomFunctionBridge::post_process_return_value(v8::Local<v8::Value> _val) const {
SassTypes::Value *value = SassTypes::Factory::unwrap(_val);
if (value) {
return value->get_sass_value();
} else {
return sass_make_error("A SassValue object was expected.");
}
Expand All @@ -17,7 +17,10 @@ std::vector<v8::Local<v8::Value>> CustomFunctionBridge::pre_process_args(std::ve
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());
Sass_Value* x = static_cast<Sass_Value*>(value);
SassTypes::Value* y = SassTypes::Factory::create(x);

argv.push_back(y->get_js_object());
}

return argv;
Expand Down
20 changes: 9 additions & 11 deletions src/sass_types/boolean.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ namespace SassTypes
Nan::Persistent<v8::Function> Boolean::constructor;
bool Boolean::constructor_locked = false;

Boolean::Boolean(bool v) : value(v) {}
Boolean::Boolean(bool _value) {
value = sass_make_boolean(_value);
}

Boolean& Boolean::get_singleton(bool v) {
static Boolean instance_false(false), instance_true(true);
Expand All @@ -15,7 +17,7 @@ namespace SassTypes

v8::Local<v8::Function> Boolean::get_constructor() {
Nan::EscapableHandleScope scope;
v8::Local<v8::Function> conslocal;
v8::Local<v8::Function> conslocal;
if (constructor.IsEmpty()) {
v8::Local<v8::FunctionTemplate> tpl = Nan::New<v8::FunctionTemplate>(New);

Expand All @@ -42,16 +44,15 @@ namespace SassTypes
return scope.Escape(conslocal);
}

Sass_Value* Boolean::get_sass_value() {
return sass_make_boolean(value);
}

v8::Local<v8::Object> Boolean::get_js_object() {
return Nan::New(this->js_object);
}

NAN_METHOD(Boolean::New) {
v8::Local<v8::Boolean> Boolean::get_js_boolean() {
return sass_boolean_get_value(this->value) ? Nan::True() : Nan::False();
}

NAN_METHOD(Boolean::New) {
if (info.IsConstructCall()) {
if (constructor_locked) {
return Nan::ThrowTypeError("Cannot instantiate SassBoolean");
Expand All @@ -67,9 +68,6 @@ namespace SassTypes
}

NAN_METHOD(Boolean::GetValue) {
Boolean *out;
if ((out = static_cast<Boolean*>(Factory::unwrap(info.This())))) {
info.GetReturnValue().Set(Nan::New(out->value));
}
info.GetReturnValue().Set(Boolean::Unwrap<Boolean>(info.This())->get_js_boolean());
}
}
3 changes: 1 addition & 2 deletions src/sass_types/boolean.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ namespace SassTypes
static Boolean& get_singleton(bool);
static v8::Local<v8::Function> get_constructor();

Sass_Value* get_sass_value();
v8::Local<v8::Object> get_js_object();

static NAN_METHOD(New);
Expand All @@ -21,11 +20,11 @@ namespace SassTypes
private:
Boolean(bool);

bool value;
Nan::Persistent<v8::Object> js_object;

static Nan::Persistent<v8::Function> constructor;
static bool constructor_locked;
v8::Local<v8::Boolean> get_js_boolean();
};
}

Expand Down
16 changes: 8 additions & 8 deletions src/sass_types/color.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,19 @@ namespace SassTypes
}

NAN_METHOD(Color::GetR) {
info.GetReturnValue().Set(sass_color_get_r(unwrap(info.This())->value));
info.GetReturnValue().Set(sass_color_get_r(Color::Unwrap<Color>(info.This())->value));
}

NAN_METHOD(Color::GetG) {
info.GetReturnValue().Set(sass_color_get_g(unwrap(info.This())->value));
info.GetReturnValue().Set(sass_color_get_g(Color::Unwrap<Color>(info.This())->value));
}

NAN_METHOD(Color::GetB) {
info.GetReturnValue().Set(sass_color_get_b(unwrap(info.This())->value));
info.GetReturnValue().Set(sass_color_get_b(Color::Unwrap<Color>(info.This())->value));
}

NAN_METHOD(Color::GetA) {
info.GetReturnValue().Set(sass_color_get_a(unwrap(info.This())->value));
info.GetReturnValue().Set(sass_color_get_a(Color::Unwrap<Color>(info.This())->value));
}

NAN_METHOD(Color::SetR) {
Expand All @@ -86,7 +86,7 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied value should be a number");
}

sass_color_set_r(unwrap(info.This())->value, Nan::To<double>(info[0]).FromJust());
sass_color_set_r(Color::Unwrap<Color>(info.This())->value, Nan::To<double>(info[0]).FromJust());
}

NAN_METHOD(Color::SetG) {
Expand All @@ -98,7 +98,7 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied value should be a number");
}

sass_color_set_g(unwrap(info.This())->value, Nan::To<double>(info[0]).FromJust());
sass_color_set_g(Color::Unwrap<Color>(info.This())->value, Nan::To<double>(info[0]).FromJust());
}

NAN_METHOD(Color::SetB) {
Expand All @@ -110,7 +110,7 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied value should be a number");
}

sass_color_set_b(unwrap(info.This())->value, Nan::To<double>(info[0]).FromJust());
sass_color_set_b(Color::Unwrap<Color>(info.This())->value, Nan::To<double>(info[0]).FromJust());
}

NAN_METHOD(Color::SetA) {
Expand All @@ -122,6 +122,6 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied value should be a number");
}

sass_color_set_a(unwrap(info.This())->value, Nan::To<double>(info[0]).FromJust());
sass_color_set_a(Color::Unwrap<Color>(info.This())->value, Nan::To<double>(info[0]).FromJust());
}
}
11 changes: 6 additions & 5 deletions src/sass_types/factory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,12 @@ namespace SassTypes
}

Value* Factory::unwrap(v8::Local<v8::Value> obj) {
// Todo: non-SassValue objects could easily fall under that condition, need to be more specific.
if (!obj->IsObject() || obj.As<v8::Object>()->InternalFieldCount() != 1) {
if (obj->IsObject()) {
v8::Local<v8::Object> v8_obj = obj.As<v8::Object>();
if (v8_obj->InternalFieldCount() == 1) {
return SassTypes::Value::Unwrap<Value>(v8_obj);
}
}
return NULL;
}

return static_cast<Value*>(Nan::GetInternalFieldPointer(obj.As<v8::Object>(), 0));
}
}
10 changes: 5 additions & 5 deletions src/sass_types/list.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied index should be an integer");
}

Sass_Value* list = unwrap(info.This())->value;
Sass_Value* list = List::Unwrap<List>(info.This())->value;
size_t index = Nan::To<uint32_t>(info[0]).FromJust();


Expand All @@ -73,14 +73,14 @@ namespace SassTypes

Value* sass_value = Factory::unwrap(info[1]);
if (sass_value) {
sass_list_set_value(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
sass_list_set_value(List::Unwrap<List>(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
} else {
Nan::ThrowTypeError("A SassValue is expected as the list item");
}
}

NAN_METHOD(List::GetSeparator) {
info.GetReturnValue().Set(sass_list_get_separator(unwrap(info.This())->value) == SASS_COMMA);
info.GetReturnValue().Set(sass_list_get_separator(List::Unwrap<List>(info.This())->value) == SASS_COMMA);
}

NAN_METHOD(List::SetSeparator) {
Expand All @@ -92,10 +92,10 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied value should be a boolean");
}

sass_list_set_separator(unwrap(info.This())->value, Nan::To<bool>(info[0]).FromJust() ? SASS_COMMA : SASS_SPACE);
sass_list_set_separator(List::Unwrap<List>(info.This())->value, Nan::To<bool>(info[0]).FromJust() ? SASS_COMMA : SASS_SPACE);
}

NAN_METHOD(List::GetLength) {
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_list_get_length(unwrap(info.This())->value)));
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_list_get_length(List::Unwrap<List>(info.This())->value)));
}
}
15 changes: 8 additions & 7 deletions src/sass_types/map.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied index should be an integer");
}

Sass_Value* map = unwrap(info.This())->value;
Sass_Value* map = Map::Unwrap<Map>(info.This())->value;
size_t index = Nan::To<uint32_t>(info[0]).FromJust();


Expand All @@ -63,14 +63,13 @@ namespace SassTypes

Value* sass_value = Factory::unwrap(info[1]);
if (sass_value) {
sass_map_set_value(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
sass_map_set_value(Map::Unwrap<Map>(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
} else {
Nan::ThrowTypeError("A SassValue is expected as a map value");
}
}

NAN_METHOD(Map::GetKey) {

if (info.Length() != 1) {
return Nan::ThrowTypeError("Expected just one argument");
}
Expand All @@ -79,15 +78,17 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied index should be an integer");
}

Sass_Value* map = unwrap(info.This())->value;
Sass_Value* map = Map::Unwrap<Map>(info.This())->value;
size_t index = Nan::To<uint32_t>(info[0]).FromJust();


if (index >= sass_map_get_length(map)) {
return Nan::ThrowRangeError(Nan::New("Out of bound index").ToLocalChecked());
}

info.GetReturnValue().Set(Factory::create(sass_map_get_key(map, Nan::To<uint32_t>(info[0]).FromJust()))->get_js_object());
SassTypes::Value* obj = Factory::create(sass_map_get_key(map, Nan::To<uint32_t>(info[0]).FromJust()));
v8::Local<v8::Object> js_obj = obj->get_js_object();
info.GetReturnValue().Set(js_obj);
}

NAN_METHOD(Map::SetKey) {
Expand All @@ -105,13 +106,13 @@ namespace SassTypes

Value* sass_value = Factory::unwrap(info[1]);
if (sass_value) {
sass_map_set_key(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
sass_map_set_key(Map::Unwrap<Map>(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
} else {
Nan::ThrowTypeError("A SassValue is expected as a map key");
}
}

NAN_METHOD(Map::GetLength) {
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_map_get_length(unwrap(info.This())->value)));
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_map_get_length(Map::Unwrap<Map>(info.This())->value)));
}
}
8 changes: 3 additions & 5 deletions src/sass_types/null.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ namespace SassTypes
Nan::Persistent<v8::Function> Null::constructor;
bool Null::constructor_locked = false;

Null::Null() {}
Null::Null() {
value = sass_make_null();
}

Null& Null::get_singleton() {
static Null singleton_instance;
Expand Down Expand Up @@ -37,10 +39,6 @@ namespace SassTypes
return scope.Escape(conslocal);
}

Sass_Value* Null::get_sass_value() {
return sass_make_null();
}

v8::Local<v8::Object> Null::get_js_object() {
return Nan::New(this->js_object);
}
Expand Down
8 changes: 4 additions & 4 deletions src/sass_types/number.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,11 @@ namespace SassTypes
}

NAN_METHOD(Number::GetValue) {
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_number_get_value(unwrap(info.This())->value)));
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_number_get_value(Number::Unwrap<Number>(info.This())->value)));
}

NAN_METHOD(Number::GetUnit) {
info.GetReturnValue().Set(Nan::New<v8::String>(sass_number_get_unit(unwrap(info.This())->value)).ToLocalChecked());
info.GetReturnValue().Set(Nan::New<v8::String>(sass_number_get_unit(Number::Unwrap<Number>(info.This())->value)).ToLocalChecked());
}

NAN_METHOD(Number::SetValue) {
Expand All @@ -58,7 +58,7 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied value should be a number");
}

sass_number_set_value(unwrap(info.This())->value, Nan::To<double>(info[0]).FromJust());
sass_number_set_value(Number::Unwrap<Number>(info.This())->value, Nan::To<double>(info[0]).FromJust());
}

NAN_METHOD(Number::SetUnit) {
Expand All @@ -70,6 +70,6 @@ namespace SassTypes
return Nan::ThrowTypeError("Supplied value should be a string");
}

sass_number_set_unit(unwrap(info.This())->value, create_string(info[0]));
sass_number_set_unit(Number::Unwrap<Number>(info.This())->value, create_string(info[0]));
}
}
Loading

0 comments on commit 41550fd

Please sign in to comment.