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

Commit 41550fd

Browse files
committed
[LEAK FIX] Use Nan::ObjectWrap to handle memory management
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’); } ```
1 parent ad15435 commit 41550fd

File tree

12 files changed

+141
-148
lines changed

12 files changed

+141
-148
lines changed

src/custom_function_bridge.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@
44
#include "sass_types/factory.h"
55
#include "sass_types/value.h"
66

7-
Sass_Value* CustomFunctionBridge::post_process_return_value(v8::Local<v8::Value> val) const {
8-
SassTypes::Value *v_;
9-
if ((v_ = SassTypes::Factory::unwrap(val))) {
10-
return v_->get_sass_value();
7+
Sass_Value* CustomFunctionBridge::post_process_return_value(v8::Local<v8::Value> _val) const {
8+
SassTypes::Value *value = SassTypes::Factory::unwrap(_val);
9+
if (value) {
10+
return value->get_sass_value();
1111
} else {
1212
return sass_make_error("A SassValue object was expected.");
1313
}
@@ -17,7 +17,10 @@ std::vector<v8::Local<v8::Value>> CustomFunctionBridge::pre_process_args(std::ve
1717
std::vector<v8::Local<v8::Value>> argv = std::vector<v8::Local<v8::Value>>();
1818

1919
for (void* value : in) {
20-
argv.push_back(SassTypes::Factory::create(static_cast<Sass_Value*>(value))->get_js_object());
20+
Sass_Value* x = static_cast<Sass_Value*>(value);
21+
SassTypes::Value* y = SassTypes::Factory::create(x);
22+
23+
argv.push_back(y->get_js_object());
2124
}
2225

2326
return argv;

src/sass_types/boolean.cpp

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ namespace SassTypes
66
Nan::Persistent<v8::Function> Boolean::constructor;
77
bool Boolean::constructor_locked = false;
88

9-
Boolean::Boolean(bool v) : value(v) {}
9+
Boolean::Boolean(bool _value) {
10+
value = sass_make_boolean(_value);
11+
}
1012

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

1618
v8::Local<v8::Function> Boolean::get_constructor() {
1719
Nan::EscapableHandleScope scope;
18-
v8::Local<v8::Function> conslocal;
20+
v8::Local<v8::Function> conslocal;
1921
if (constructor.IsEmpty()) {
2022
v8::Local<v8::FunctionTemplate> tpl = Nan::New<v8::FunctionTemplate>(New);
2123

@@ -42,16 +44,15 @@ namespace SassTypes
4244
return scope.Escape(conslocal);
4345
}
4446

45-
Sass_Value* Boolean::get_sass_value() {
46-
return sass_make_boolean(value);
47-
}
48-
4947
v8::Local<v8::Object> Boolean::get_js_object() {
5048
return Nan::New(this->js_object);
5149
}
5250

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

55+
NAN_METHOD(Boolean::New) {
5556
if (info.IsConstructCall()) {
5657
if (constructor_locked) {
5758
return Nan::ThrowTypeError("Cannot instantiate SassBoolean");
@@ -67,9 +68,6 @@ namespace SassTypes
6768
}
6869

6970
NAN_METHOD(Boolean::GetValue) {
70-
Boolean *out;
71-
if ((out = static_cast<Boolean*>(Factory::unwrap(info.This())))) {
72-
info.GetReturnValue().Set(Nan::New(out->value));
73-
}
71+
info.GetReturnValue().Set(Boolean::Unwrap<Boolean>(info.This())->get_js_boolean());
7472
}
7573
}

src/sass_types/boolean.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ namespace SassTypes
1212
static Boolean& get_singleton(bool);
1313
static v8::Local<v8::Function> get_constructor();
1414

15-
Sass_Value* get_sass_value();
1615
v8::Local<v8::Object> get_js_object();
1716

1817
static NAN_METHOD(New);
@@ -21,11 +20,11 @@ namespace SassTypes
2120
private:
2221
Boolean(bool);
2322

24-
bool value;
2523
Nan::Persistent<v8::Object> js_object;
2624

2725
static Nan::Persistent<v8::Function> constructor;
2826
static bool constructor_locked;
27+
v8::Local<v8::Boolean> get_js_boolean();
2928
};
3029
}
3130

src/sass_types/color.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -62,19 +62,19 @@ namespace SassTypes
6262
}
6363

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

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

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

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

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

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

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

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

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

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

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

125-
sass_color_set_a(unwrap(info.This())->value, Nan::To<double>(info[0]).FromJust());
125+
sass_color_set_a(Color::Unwrap<Color>(info.This())->value, Nan::To<double>(info[0]).FromJust());
126126
}
127127
}

src/sass_types/factory.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,12 @@ namespace SassTypes
6161
}
6262

6363
Value* Factory::unwrap(v8::Local<v8::Value> obj) {
64-
// Todo: non-SassValue objects could easily fall under that condition, need to be more specific.
65-
if (!obj->IsObject() || obj.As<v8::Object>()->InternalFieldCount() != 1) {
64+
if (obj->IsObject()) {
65+
v8::Local<v8::Object> v8_obj = obj.As<v8::Object>();
66+
if (v8_obj->InternalFieldCount() == 1) {
67+
return SassTypes::Value::Unwrap<Value>(v8_obj);
68+
}
69+
}
6670
return NULL;
67-
}
68-
69-
return static_cast<Value*>(Nan::GetInternalFieldPointer(obj.As<v8::Object>(), 0));
7071
}
7172
}

src/sass_types/list.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ namespace SassTypes
4747
return Nan::ThrowTypeError("Supplied index should be an integer");
4848
}
4949

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

5353

@@ -73,14 +73,14 @@ namespace SassTypes
7373

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

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

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

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

9898
NAN_METHOD(List::GetLength) {
99-
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_list_get_length(unwrap(info.This())->value)));
99+
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_list_get_length(List::Unwrap<List>(info.This())->value)));
100100
}
101101
}

src/sass_types/map.cpp

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ namespace SassTypes
3737
return Nan::ThrowTypeError("Supplied index should be an integer");
3838
}
3939

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

4343

@@ -63,14 +63,13 @@ namespace SassTypes
6363

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

7272
NAN_METHOD(Map::GetKey) {
73-
7473
if (info.Length() != 1) {
7574
return Nan::ThrowTypeError("Expected just one argument");
7675
}
@@ -79,15 +78,17 @@ namespace SassTypes
7978
return Nan::ThrowTypeError("Supplied index should be an integer");
8079
}
8180

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

8584

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

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

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

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

114115
NAN_METHOD(Map::GetLength) {
115-
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_map_get_length(unwrap(info.This())->value)));
116+
info.GetReturnValue().Set(Nan::New<v8::Number>(sass_map_get_length(Map::Unwrap<Map>(info.This())->value)));
116117
}
117118
}

src/sass_types/null.cpp

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ namespace SassTypes
66
Nan::Persistent<v8::Function> Null::constructor;
77
bool Null::constructor_locked = false;
88

9-
Null::Null() {}
9+
Null::Null() {
10+
value = sass_make_null();
11+
}
1012

1113
Null& Null::get_singleton() {
1214
static Null singleton_instance;
@@ -37,10 +39,6 @@ namespace SassTypes
3739
return scope.Escape(conslocal);
3840
}
3941

40-
Sass_Value* Null::get_sass_value() {
41-
return sass_make_null();
42-
}
43-
4442
v8::Local<v8::Object> Null::get_js_object() {
4543
return Nan::New(this->js_object);
4644
}

src/sass_types/number.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,11 @@ namespace SassTypes
4141
}
4242

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

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

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

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

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

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

0 commit comments

Comments
 (0)