Skip to content

fix(value): handle transfer-full, in arguments properly #303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(value): handle transfer-full, in arguments properly #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48c744f
tests/function_call: add a failing test for transfer-full args
peat-psuwit Jun 17, 2021
77a0fb3
fix(value): handle transfer-full, in arguments properly
peat-psuwit Jul 18, 2021
cdb1bcb
lint: de-duplicate boxed value copying logic
peat-psuwit Aug 28, 2021
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
32 changes: 22 additions & 10 deletions src/boxed.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,17 +179,9 @@ static void BoxedConstructor(const Nan::FunctionCallbackInfo<Value> &info) {
boxed = External::Cast(*info[0])->Value();

if (mustCopy) {
if (gtype != G_TYPE_NONE) {
boxed = g_boxed_copy (gtype, boxed);
}
else if ((size = Boxed::GetSize(gi_info)) != 0) {
void *boxedCopy = malloc(size);
memcpy(boxedCopy, boxed, size);
boxed = boxedCopy;
}
else {
boxed = CopyBoxed(gi_info, boxed, &size);
if (!boxed)
ERROR("Could not copy boxed object.");
}
}
else {
owns_memory = false;
Expand Down Expand Up @@ -445,4 +437,24 @@ void* PointerFromWrapper(Local<Value> value) {
return boxed;
}

void* CopyBoxed(GIBaseInfo *gi_info, void *boxed, size_t *size_out)
{
GType gtype = g_registered_type_info_get_g_type (gi_info);
size_t size;

if (gtype != G_TYPE_NONE) {
return g_boxed_copy (gtype, boxed);
} else if ((size = Boxed::GetSize(gi_info)) != 0) {
void *boxedCopy = malloc(size);
memcpy(boxedCopy, boxed, size);
if (size_out)
*size_out = size;
return boxedCopy;
} else {
WARN("Cannot copy a boxed object of type '%s'.",
g_base_info_get_name(gi_info));
return NULL;
}
}

};
1 change: 1 addition & 0 deletions src/boxed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,6 @@ Local<Function> MakeBoxedClass (GIBaseInfo *info);
Local<FunctionTemplate> GetBoxedTemplate (GIBaseInfo *info, GType gtype);
Local<Value> WrapperFromBoxed (GIBaseInfo *info, void *data, bool mustCopy = false);
void * PointerFromWrapper (Local<Value>);
void * CopyBoxed (GIBaseInfo *gi_info, void *boxed, size_t *size_out = nullptr);

};
7 changes: 6 additions & 1 deletion src/callback.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,16 @@ void Callback::Execute (GIArgument *result, GIArgument **args, Callback *callbac
}
}

GITypeInfo ret_info;
g_callable_info_load_return_type(callback->info, &ret_info);
GITransfer transfer = g_arg_info_get_ownership_transfer (&ret_info);

success = V8ToGIArgument(
&return_type_info,
result,
jsRealReturnValue,
g_callable_info_may_return_null (callback->info));
g_callable_info_may_return_null (callback->info),
transfer);

if (!success) {
Throw::InvalidReturnValue (&return_type_info, jsReturnValue);
Expand Down
5 changes: 3 additions & 2 deletions src/function.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ static void FillArgument(GIArgInfo *arg_info, GIArgument *argument, Local<Value>
GITypeInfo type_info;
bool may_be_null = g_arg_info_may_be_null (arg_info);
g_arg_info_load_type (arg_info, &type_info);
V8ToGIArgument(&type_info, argument, value, may_be_null);
GITransfer transfer = g_arg_info_get_ownership_transfer(arg_info);
V8ToGIArgument(&type_info, argument, value, may_be_null, transfer);
}

static int GetV8ArrayLength (Local<Value> value) {
Expand Down Expand Up @@ -331,7 +332,7 @@ Local<Value> FunctionCall (

if (func->is_method) {
GIBaseInfo *container = g_base_info_get_container (gi_info);
V8ToGIArgument(container, &total_arg_values[0], info.This());
V8ToGIArgument(container, &total_arg_values[0], info.This(), GI_TRANSFER_NOTHING);
callable_arg_values = &total_arg_values[1];
} else {
callable_arg_values = &total_arg_values[0];
Expand Down
2 changes: 1 addition & 1 deletion src/gi.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ NAN_METHOD(StructFieldSetter) {

GIArgument arg;

if (!GNodeJS::V8ToGIArgument(field_type, &arg, value, true)) {
if (!GNodeJS::V8ToGIArgument(field_type, &arg, value, true, GI_TRANSFER_NOTHING)) {
char *message = g_strdup_printf("Couldn't convert value for field '%s'",
g_base_info_get_name(field));
Nan::ThrowTypeError (message);
Expand Down
63 changes: 44 additions & 19 deletions src/value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ long GIArgumentToLength(GITypeInfo *type_info, GIArgument *arg, bool is_pointer)
}


GArray * V8ToGArray(GITypeInfo *type_info, Local<Value> value) {
GArray * V8ToGArray(GITypeInfo *type_info, Local<Value> value, GITransfer transfer) {
GArray* g_array = NULL;
bool zero_terminated = g_type_info_is_zero_terminated(type_info);

Expand Down Expand Up @@ -382,8 +382,10 @@ GArray * V8ToGArray(GITypeInfo *type_info, Local<Value> value) {
for (int i = 0; i < length; i++) {
auto value = Nan::Get(array, i).ToLocalChecked();
GIArgument arg;
GITransfer inner_transfer =
(transfer == GI_TRANSFER_CONTAINER ? GI_TRANSFER_NOTHING : transfer);

if (V8ToGIArgument(element_info, &arg, value, true)) {
if (V8ToGIArgument(element_info, &arg, value, true, inner_transfer)) {
g_array_append_val (g_array, arg);
} else {
g_warning("V8ToGArray: couldnt convert value: %s",
Expand All @@ -399,7 +401,7 @@ GArray * V8ToGArray(GITypeInfo *type_info, Local<Value> value) {
return g_array;
}

static void *V8ArrayToCArray(GITypeInfo *type_info, Local<Value> value) {
static void *V8ArrayToCArray(GITypeInfo *type_info, Local<Value> value, GITransfer transfer) {
auto array = Local<Array>::Cast (TO_OBJECT (value));
int length = array->Length();

Expand All @@ -413,8 +415,10 @@ static void *V8ArrayToCArray(GITypeInfo *type_info, Local<Value> value) {
auto value = Nan::Get(array, i).ToLocalChecked();

GIArgument arg;
GITransfer inner_transfer =
(transfer == GI_TRANSFER_CONTAINER ? GI_TRANSFER_NOTHING : transfer);

if (V8ToGIArgument(element_info, &arg, value, true)) {
if (V8ToGIArgument(element_info, &arg, value, true, inner_transfer)) {
void* pointer = (void*)((ulong)result + i * element_size);
memcpy(pointer, &arg, element_size);
} else {
Expand Down Expand Up @@ -453,15 +457,15 @@ static void *V8TypedArrayToCArray(GITypeInfo *type_info, Local<Value> value) {
return result;
}

void *V8ToCArray(GITypeInfo *type_info, Local<Value> value) {
void *V8ToCArray(GITypeInfo *type_info, Local<Value> value, GITransfer transfer) {
if (value->IsString()) {
Local<String> string = TO_STRING (value);
const char *utf8_data = *Nan::Utf8String(string);
return g_strdup(utf8_data);
}

if (value->IsArray()) {
return V8ArrayToCArray(type_info, value);
return V8ArrayToCArray(type_info, value, transfer);
}

if (value->IsTypedArray()) {
Expand All @@ -473,7 +477,7 @@ void *V8ToCArray(GITypeInfo *type_info, Local<Value> value) {
return NULL;
}

gpointer V8ToGList (GITypeInfo *type_info, Local<Value> value) {
gpointer V8ToGList (GITypeInfo *type_info, Local<Value> value, GITransfer transfer) {

// FIXME can @value be null?
if (!value->IsArray()) {
Expand Down Expand Up @@ -504,7 +508,10 @@ gpointer V8ToGList (GITypeInfo *type_info, Local<Value> value) {
GIArgument arg;
Local<Value> value = Nan::Get(array, i).ToLocalChecked();

if (!V8ToGIArgument(element_info, &arg, value, false)) {
GITransfer inner_transfer =
(transfer == GI_TRANSFER_CONTAINER ? GI_TRANSFER_NOTHING : transfer);

if (!V8ToGIArgument(element_info, &arg, value, false, inner_transfer)) {
g_warning("V8ToGList: couldnt convert value #%i to GIArgument", i);
continue;
}
Expand All @@ -526,7 +533,7 @@ gpointer V8ToGList (GITypeInfo *type_info, Local<Value> value) {
return list;
}

gpointer V8ToGHash (GITypeInfo *type_info, Local<Value> value) {
gpointer V8ToGHash (GITypeInfo *type_info, Local<Value> value, GITransfer transfer) {

if (!value->IsObject()) {
Nan::ThrowTypeError("Expected object");
Expand Down Expand Up @@ -587,21 +594,24 @@ gpointer V8ToGHash (GITypeInfo *type_info, Local<Value> value) {
auto object = TO_OBJECT (value);
auto keys = Nan::GetOwnPropertyNames(object).ToLocalChecked();

GITransfer inner_transfer =
(transfer == GI_TRANSFER_CONTAINER ? GI_TRANSFER_NOTHING : transfer);

for (uint32_t i = 0; i < keys->Length(); i++) {
auto key = Nan::Get(keys, i).ToLocalChecked();
auto value = Nan::Get(object, key).ToLocalChecked();

GIArgument key_arg;
GIArgument value_arg;

if (!V8ToGIArgument(key_type_info, &key_arg, key, false)) {
if (!V8ToGIArgument(key_type_info, &key_arg, key, false, inner_transfer)) {
char* message = g_strdup_printf("Couldn't convert key '%s'", *Nan::Utf8String(key));
Nan::ThrowError(message);
free(message);
goto item_error;
}

if (!V8ToGIArgument(value_type_info, &value_arg, value, false)) {
if (!V8ToGIArgument(value_type_info, &value_arg, value, false, inner_transfer)) {
char* message = g_strdup_printf("Couldn't convert value for key '%s'", *Nan::Utf8String(key));
Nan::ThrowError(message);
free(message);
Expand All @@ -613,7 +623,11 @@ gpointer V8ToGHash (GITypeInfo *type_info, Local<Value> value) {
continue;

item_error:
/* Free everything we have converted so far. */
/*
* Free everything we have converted so far.
* Uses TRANSFER_NOTHING, DIRECTION_IN, as if the function doesn't take
* ownership, instead of passed in `transfer`.
*/
FreeGIArgument(type_info, (GIArgument *) &hash_table, GI_TRANSFER_NOTHING, GI_DIRECTION_IN);
hash_table = NULL;
break;
Expand All @@ -625,14 +639,19 @@ gpointer V8ToGHash (GITypeInfo *type_info, Local<Value> value) {
return hash_table;
}

bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local<Value> value) {
bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local<Value> value, GITransfer transfer) {
GIInfoType type = g_base_info_get_type (gi_info);

switch (type) {
case GI_INFO_TYPE_BOXED:
case GI_INFO_TYPE_STRUCT:
case GI_INFO_TYPE_UNION:
arg->v_pointer = PointerFromWrapper(value);
if (transfer == GI_TRANSFER_EVERYTHING) {
arg->v_pointer = CopyBoxed(gi_info, arg->v_pointer);
if (!arg->v_pointer)
return false;
}
break;

case GI_INFO_TYPE_FLAGS:
Expand All @@ -652,6 +671,8 @@ bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local<Value> value) {
}
case GI_INFO_TYPE_INTERFACE:
arg->v_pointer = GObjectFromWrapper(value);
if (transfer == GI_TRANSFER_EVERYTHING)
g_object_ref(arg->v_pointer);
Copy link
Owner

Choose a reason for hiding this comment

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

And this one isn't going to leak anything? Is this case the one tested by the test down here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this one isn't going to leak anything?

I suppose no. The new reference should be taken by the callee, by the definition of TRANSFER_EVERYTHING. I don't see a failure case in this function.

Is this case the one tested by the test down here?

Similar, but of different type. The added test is for GBoxed [1], while this one is for GObject. That's why I said I want to test this.

[1] specifically GstMiniObject, but to node-gtk it's a boxed object.

Copy link
Owner

Choose a reason for hiding this comment

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

The new reference should be taken by the callee

But maybe here TRANSFER_EVERYTHING means to take ownership of the passed ref, no? Either way, I'd first try to find a failing case before adding a reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whatever the function's transfer say, we cannot transfer the JS wrapper to the C world. That means the JS wrapper will have to stay, and so does the wrapper's object reference. If we don't increase a reference here, the JS wrapper's reference will (metaphorically) be transferred to the function. And if such function un-reference this reference, it can leave the JS wrapper dangling.

Maybe you intend to leave the JS wrapper dangling after a call to transfer-full function. However, that doesn't work either. Every reference to this object in the JS world shares single JS wrapper, and thus have single GObject reference. If the JS wrapper becomes dangling, so does every refernce in the JS world.

break;

case GI_INFO_TYPE_CALLBACK:
Expand All @@ -662,7 +683,7 @@ bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local<Value> value) {
return true;
}

bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local<Value> value, bool may_be_null) {
bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local<Value> value, bool may_be_null, GITransfer transfer) {
GITypeTag type_tag = g_type_info_get_tag (type_info);

if (value->IsUndefined () || value->IsNull ()) {
Expand All @@ -680,6 +701,10 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local<Value> value,
case GI_TYPE_TAG_VOID:
if (g_type_info_is_pointer(type_info)) {
arg->v_pointer = PointerFromWrapper(value);
if (transfer == GI_TRANSFER_EVERYTHING) {
ERROR("Case should not be reached. Please report this to "
"https://github.com/romgrk/node-gtk/issues");
}
break;
}
arg->v_pointer = NULL;
Expand Down Expand Up @@ -744,11 +769,11 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local<Value> value,

switch (array_type) {
case GI_ARRAY_TYPE_C:
arg->v_pointer = V8ToCArray(type_info, value);
arg->v_pointer = V8ToCArray(type_info, value, transfer);
break;
case GI_ARRAY_TYPE_ARRAY:
case GI_ARRAY_TYPE_BYTE_ARRAY:
arg->v_pointer = V8ToGArray(type_info, value);
arg->v_pointer = V8ToGArray(type_info, value, transfer);
break;
case GI_ARRAY_TYPE_PTR_ARRAY:
default:
Expand All @@ -761,21 +786,21 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local<Value> value,
case GI_TYPE_TAG_INTERFACE:
{
GIBaseInfo *interface_info = g_type_info_get_interface (type_info);
V8ToGIArgument (interface_info, arg, value);
V8ToGIArgument (interface_info, arg, value, transfer);
g_base_info_unref(interface_info);
}
break;

case GI_TYPE_TAG_GLIST:
case GI_TYPE_TAG_GSLIST:
{
arg->v_pointer = V8ToGList(type_info, value);
arg->v_pointer = V8ToGList(type_info, value, transfer);
}
break;

case GI_TYPE_TAG_GHASH:
{
arg->v_pointer = V8ToGHash(type_info, value);
arg->v_pointer = V8ToGHash(type_info, value, transfer);
}
break;

Expand Down
4 changes: 2 additions & 2 deletions src/value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ Local<Value> GErrorToV8 (GITypeInfo *type_info, GError *err);
Local<Value> GIArgumentToV8 (GITypeInfo *type_info, GIArgument *argument, long length = -1, bool mustCopy = false);
long GIArgumentToLength(GITypeInfo *type_info, GIArgument *arg, bool is_pointer);

bool V8ToGIArgument (GITypeInfo *type_info, GIArgument *argument, Local<Value> value);
bool V8ToGIArgument (GITypeInfo *type_info, GIArgument *argument, Local<Value> value, bool may_be_null);
bool V8ToGIArgument (GITypeInfo *type_info, GIArgument *argument, Local<Value> value, GITransfer transfer);
bool V8ToGIArgument (GITypeInfo *type_info, GIArgument *argument, Local<Value> value, bool may_be_null, GITransfer transfer);
bool V8ToOutGIArgument(GITypeInfo *type_info, GIArgument *arg, Local<Value> value, bool may_be_null);
void FreeGIArgument (GITypeInfo *type_info, GIArgument *argument, GITransfer transfer = GI_TRANSFER_EVERYTHING, GIDirection direction = GI_DIRECTION_OUT);
void FreeGIArgumentArray (GITypeInfo *type_info, GIArgument *arg, GITransfer transfer = GI_TRANSFER_EVERYTHING, GIDirection direction = GI_DIRECTION_OUT, long length = -1);
Expand Down
27 changes: 27 additions & 0 deletions tests/function_call.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@

const gi = require('../lib/')
const GLib = gi.require('GLib')
const Gst = gi.require('Gst', '1.0')
const Gtk = gi.require('Gtk', '3.0')
const { describe, it, mustThrow, expect } = require('./__common__.js')

Gst.init()
Gtk.init()

describe('function arguments', () => {
Expand All @@ -24,3 +26,28 @@ describe('function call can throw',
const result = GLib.filenameFromUri('http://google.com')
console.log('Result:', result)
}))

describe('transfer-full function argument', () => {
it('does not invalidate js wrapper -- boxed', () => {
let gstPromise = new Gst.Promise();
let structure = Gst.Structure.newEmpty('test');

/*
* https://gstreamer.freedesktop.org/documentation/gstreamer/gstpromise.html?gi-language=c#gst_promise_reply
*
* Parameters:
* <...>
* s ( [transfer: full][nullable]) – a GstStructure with the the reply contents
*/
gstPromise.reply(structure);

// GstPromise will take the structure with it.
gstPromise = null;
// Required, to actually take gstPromise down.
global.gc();

// Gst will issue an assertion and return false if structure's
// backing object is destroyed.
expect(structure.isEqual(structure), true);
});
})