fix(value): handle transfer-full, in arguments properly #303
Conversation
peat-psuwit
changed the title
peat-psuwit
force-pushed
the
transfer-fully-in
branch
2 times, most recently
from
c63aedf
to
35de0ce
Compare
|
@peat-psuwit sorry for the delay, was traveling a bit this summer. Overall this looks good, one thing though. For all the changes it makes, the PR only has a single test. Is it really the only case affected by these changes? |
Well, the thing is that I can't find more APIs in GStreamer, GLib or GTK that would be affected by this. I would prefer to have at least an equivalent test but for GObject. I considered adding an introspectable library for testing, but I'm not sure if it can be done using node-gyp system |
|
Alright. There is #269 open to add libgimarshallingtest which does that kind of stuff, we can add more tests once the lib is used for testing. I'll merge as soon as you fix the last two nitpicks :] |
peat-psuwit
force-pushed
the
transfer-fully-in
branch
from
35de0ce
to
9821f44
Compare
|
How is this one related to #302? Do you want to include the memory ownership thing here? |
|
Got it. You can fix the last comment up here and it's ready for merging. |
peat-psuwit
force-pushed
the
transfer-fully-in
branch
from
9821f44
to
fb967ef
Compare
This is done by making V8ToGIArgument() accept the transfer status and react accordingly. I does not make the new argument optional; this forces me into thinking about the appropriate value to put in for various callsites of this function. The make the test added in the previous commit passes.
peat-psuwit
force-pushed
the
transfer-fully-in
branch
from
fb967ef
to
cdb1bcb
Compare
| @@ -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); | |||
There was a problem hiding this comment.
And this one isn't going to leak anything? Is this case the one tested by the test down here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This is done by making V8ToGIArgument() accept the transfer status and
react accordingly. I does not make the new argument optional; this
forces me into thinking about the appropriate value to put in for
various callsites of this function.
Closes #303