Skip to content

Commit ec4c879

Browse files
committed
feat(gobject, closure): record created Closure to be traced by cppgc
Replace a Persistent in Closure with a TracedReference. Then, keep track of created Closure in GObjectWrapper. When GObjectWrapper is traced by cppgc, trace those reference to tell cppgc they're still alive. By not creating a Persistent, we prevent a reference loop where the Persistent holds function, holds JS wrapper, holds GObject, holds Persistent. When TracedReference, evetually the whole loop will not be traced by cppgc, allowing the whole loop to be dropped. This approach is inspired by PyGObject.
1 parent bb793a1 commit ec4c879

File tree

3 files changed

+73
-13
lines changed

3 files changed

+73
-13
lines changed

src/closure.cc

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,13 @@ using v8::Isolate;
1515
using v8::Local;
1616
using v8::Object;
1717
using v8::Value;
18-
using Nan::Persistent;
1918

2019
namespace GNodeJS {
2120

2221
GClosure *Closure::New (Local<Function> function, GICallableInfo* info, guint signalId) {
2322
Closure *closure = (Closure *) g_closure_new_simple (sizeof (*closure), GUINT_TO_POINTER(signalId));
24-
closure->persistent.Reset(function);
23+
closure->isolate = function->GetIsolate();
24+
closure->functionRef.Reset(closure->isolate, function);
2525
if (info) {
2626
closure->info = g_base_info_ref(info);
2727
} else {
@@ -34,11 +34,12 @@ GClosure *Closure::New (Local<Function> function, GICallableInfo* info, guint si
3434
}
3535

3636
void Closure::Execute(GICallableInfo *info, guint signal_id,
37-
const Nan::Persistent<v8::Function> &persFn,
37+
v8::Isolate *isolate,
38+
const v8::TracedReference<v8::Function> &fnRef,
3839
GValue *g_return_value, guint n_param_values,
3940
const GValue *param_values) {
4041
Nan::HandleScope scope;
41-
auto func = Nan::New<Function>(persFn);
42+
auto func = fnRef.Get(isolate);
4243

4344
GSignalQuery signal_query = { 0, };
4445

@@ -139,11 +140,25 @@ void Closure::Marshal(GClosure *base,
139140

140141
if (env->IsSameThread()) {
141142
/* Case 1: same thread */
142-
Closure::Execute(closure->info, signal_id, closure->persistent, g_return_value, n_param_values, param_values);
143+
Closure::Execute(
144+
closure->info,
145+
signal_id,
146+
closure->isolate,
147+
closure->functionRef,
148+
g_return_value,
149+
n_param_values,
150+
param_values);
143151
} else {
144152
/* Case 2: different thread */
145153
env->Call([&]() {
146-
Closure::Execute(closure->info, signal_id, closure->persistent, g_return_value, n_param_values, param_values);
154+
Closure::Execute(
155+
closure->info,
156+
signal_id,
157+
closure->isolate,
158+
closure->functionRef,
159+
g_return_value,
160+
n_param_values,
161+
param_values);
147162
});
148163
}
149164
}

src/closure.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,12 @@ namespace GNodeJS {
1616

1717
struct Closure {
1818
GClosure base;
19-
Nan::Persistent<v8::Function> persistent;
19+
v8::Isolate *isolate;
20+
v8::TracedReference<v8::Function> functionRef;
2021
GICallableInfo* info;
2122

2223
~Closure() {
23-
persistent.Reset();
24+
functionRef.Reset();
2425
if (info) g_base_info_unref (info);
2526
}
2627

@@ -29,7 +30,8 @@ struct Closure {
2930
guint signalId);
3031

3132
static void Execute(GICallableInfo *info, guint signal_id,
32-
const Nan::Persistent<v8::Function> &persFn,
33+
v8::Isolate *isolate,
34+
const v8::TracedReference<v8::Function> &persFn,
3335
GValue *g_return_value, guint n_param_values,
3436
const GValue *param_values);
3537

src/gobject.cc

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11

22
#include <string.h>
3+
#include <algorithm>
4+
#include <mutex>
35

46
#include "cppgc/allocation.h"
57
#include "cppgc/garbage-collected.h"
@@ -70,6 +72,11 @@ class GObjectWrapper : public V8Wrappable {
7072
}
7173

7274
~GObjectWrapper() {
75+
for (auto closure : watchedClosures) {
76+
g_closure_remove_invalidate_notifier(
77+
closure, this, &closureInvalidated);
78+
}
79+
7380
g_object_remove_toggle_ref (gobject, &ToggleNotify, NULL);
7481
}
7582

@@ -79,14 +86,40 @@ class GObjectWrapper : public V8Wrappable {
7986
}
8087

8188
void Trace(cppgc::Visitor* visitor) const override {
82-
// TODO
89+
std::lock_guard l(mutex);
90+
91+
for (auto closure : watchedClosures) {
92+
visitor->Trace(reinterpret_cast<Closure *>(closure)->functionRef);
93+
}
8394
}
8495

8596
GObject * getGObject() {
8697
return gobject;
8798
}
99+
100+
void watchClosure(GClosure * closure) {
101+
std::lock_guard l(mutex);
102+
103+
watchedClosures.push_back(closure);
104+
g_closure_add_invalidate_notifier(closure, this, &closureInvalidated);
105+
}
106+
107+
static void closureInvalidated(gpointer data, GClosure * closure) {
108+
GObjectWrapper * self = static_cast<GObjectWrapper *>(data);
109+
std::lock_guard l(self->mutex);
110+
111+
// https://stackoverflow.com/a/3385251
112+
self->watchedClosures.erase(std::remove(
113+
self->watchedClosures.begin(),
114+
self->watchedClosures.end(),
115+
closure
116+
), self->watchedClosures.end());
117+
}
88118
private:
89119
GObject * gobject;
120+
121+
mutable std::mutex mutex;
122+
std::vector<GClosure *> watchedClosures;
90123
};
91124

92125
// Our base template for all GObjects
@@ -399,10 +432,15 @@ static void DestroyVFuncs(GType gtype) {
399432
g_type_set_qdata (gtype, GNodeJS::vfuncs_quark(), NULL);
400433
}
401434

435+
// FIXME: reorganize so that we don't have this out-of-place declaration.
436+
// We probably want to declare GObjectWrapper class in gobject.h now.
437+
GObjectWrapper * GObjectCppWrapperFromWrapper(Local<Value> value);
438+
402439
NAN_METHOD(SignalConnect) {
403440
bool after = false;
404441

405-
GObject *gobject = GObjectFromWrapper (info.This ());
442+
GObjectWrapper *cppWrapper = GObjectCppWrapperFromWrapper(info.This ());
443+
GObject *gobject = cppWrapper->getGObject();
406444

407445
if (!gobject) {
408446
Nan::ThrowTypeError("Object is not a GObject");
@@ -448,6 +486,7 @@ NAN_METHOD(SignalConnect) {
448486
}
449487

450488
gclosure = Closure::New (callback, signal_info, signalId);
489+
cppWrapper->watchClosure(gclosure);
451490
handler_id = g_signal_connect_closure (gobject, signalName, gclosure, after);
452491

453492
info.GetReturnValue().Set((double)handler_id);
@@ -737,7 +776,7 @@ Local<Value> WrapperFromGObject(GObject *gobject) {
737776
return obj;
738777
}
739778

740-
GObject * GObjectFromWrapper(Local<Value> value) {
779+
GObjectWrapper * GObjectCppWrapperFromWrapper(Local<Value> value) {
741780
if (!ValueHasInternalField(value))
742781
return nullptr;
743782

@@ -755,7 +794,11 @@ GObject * GObjectFromWrapper(Local<Value> value) {
755794
descriptor.wrappable_instance_index));
756795
#endif
757796

758-
return cppWrapper->getGObject();
797+
return cppWrapper;
798+
}
799+
800+
GObject * GObjectFromWrapper(Local<Value> value) {
801+
return GObjectCppWrapperFromWrapper(value)->getGObject();
759802
}
760803

761804
MaybeLocal<Value> GetGObjectProperty(GObject * gobject, const char *prop_name) {

0 commit comments

Comments
 (0)