Skip to content

Commit 693b9a4

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 a77d1c9 commit 693b9a4

3 files changed

Lines changed: 55 additions & 13 deletions

File tree

src/closure.cc

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,12 @@ 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->functionRef.Reset(function->GetIsolate(), function);
2524
if (info) {
2625
closure->info = g_base_info_ref(info);
2726
} else {
@@ -34,11 +33,11 @@ GClosure *Closure::New (Local<Function> function, GICallableInfo* info, guint si
3433
}
3534

3635
void Closure::Execute(GICallableInfo *info, guint signal_id,
37-
const Nan::Persistent<v8::Function> &persFn,
36+
const v8::TracedReference<v8::Function> &fnRef,
3837
GValue *g_return_value, guint n_param_values,
3938
const GValue *param_values) {
4039
Nan::HandleScope scope;
41-
auto func = Nan::New<Function>(persFn);
40+
auto func = Local<v8::Function>::New(fnRef->GetIsolate(), fnRef);
4241

4342
GSignalQuery signal_query = { 0, };
4443

@@ -135,11 +134,11 @@ void Closure::Marshal(GClosure *base,
135134

136135
if (env->IsSameThread()) {
137136
/* Case 1: same thread */
138-
Closure::Execute(closure->info, signal_id, closure->persistent, g_return_value, n_param_values, param_values);
137+
Closure::Execute(closure->info, signal_id, closure->functionRef, g_return_value, n_param_values, param_values);
139138
} else {
140139
/* Case 2: different thread */
141140
env->Call([&]() {
142-
Closure::Execute(closure->info, signal_id, closure->persistent, g_return_value, n_param_values, param_values);
141+
Closure::Execute(closure->info, signal_id, closure->functionRef, g_return_value, n_param_values, param_values);
143142
});
144143
}
145144
}

src/closure.h

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

1717
struct Closure {
1818
GClosure base;
19-
Nan::Persistent<v8::Function> persistent;
19+
v8::TracedReference<v8::Function> functionRef;
2020
GICallableInfo* info;
2121

2222
~Closure() {
23-
persistent.Reset();
23+
functionRef.Reset();
2424
if (info) g_base_info_unref (info);
2525
}
2626

@@ -29,7 +29,7 @@ struct Closure {
2929
guint signalId);
3030

3131
static void Execute(GICallableInfo *info, guint signal_id,
32-
const Nan::Persistent<v8::Function> &persFn,
32+
const v8::TracedReference<v8::Function> &persFn,
3333
GValue *g_return_value, guint n_param_values,
3434
const GValue *param_values);
3535

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"
@@ -73,6 +75,11 @@ class GObjectWrapper : public V8Wrappable {
7375
}
7476

7577
~GObjectWrapper() {
78+
for (auto closure : watchedClosures) {
79+
g_closure_remove_invalidate_notifier(
80+
closure, this, &closureInvalidated);
81+
}
82+
7683
g_object_remove_toggle_ref (gobject, &ToggleNotify, NULL);
7784
}
7885

@@ -82,14 +89,40 @@ class GObjectWrapper : public V8Wrappable {
8289
}
8390

8491
void Trace(cppgc::Visitor* visitor) const override {
85-
// TODO
92+
std::lock_guard l(mutex);
93+
94+
for (auto closure : watchedClosures) {
95+
visitor->Trace(reinterpret_cast<Closure *>(closure)->functionRef);
96+
}
8697
}
8798

8899
GObject * getGObject() {
89100
return gobject;
90101
}
102+
103+
void watchClosure(GClosure * closure) {
104+
std::lock_guard l(mutex);
105+
106+
watchedClosures.push_back(closure);
107+
g_closure_add_invalidate_notifier(closure, this, &closureInvalidated);
108+
}
109+
110+
static void closureInvalidated(gpointer data, GClosure * closure) {
111+
GObjectWrapper * self = static_cast<GObjectWrapper *>(data);
112+
std::lock_guard l(self->mutex);
113+
114+
// https://stackoverflow.com/a/3385251
115+
self->watchedClosures.erase(std::remove(
116+
self->watchedClosures.begin(),
117+
self->watchedClosures.end(),
118+
closure
119+
), self->watchedClosures.end());
120+
}
91121
private:
92122
GObject * gobject;
123+
124+
mutable std::mutex mutex;
125+
std::vector<GClosure *> watchedClosures;
93126
};
94127

95128
// Our base template for all GObjects
@@ -384,10 +417,15 @@ static void DestroyVFuncs(GType gtype) {
384417
g_type_set_qdata (gtype, GNodeJS::vfuncs_quark(), NULL);
385418
}
386419

420+
// FIXME: reorganize so that we don't have this out-of-place declaration.
421+
// We probably want to declare GObjectWrapper class in gobject.h now.
422+
GObjectWrapper * GObjectCppWrapperFromWrapper(Local<Value> value);
423+
387424
NAN_METHOD(SignalConnect) {
388425
bool after = false;
389426

390-
GObject *gobject = GObjectFromWrapper (info.This ());
427+
GObjectWrapper *cppWrapper = GObjectCppWrapperFromWrapper(info.This ());
428+
GObject *gobject = cppWrapper->getGObject();
391429

392430
if (!gobject) {
393431
Nan::ThrowTypeError("Object is not a GObject");
@@ -433,6 +471,7 @@ NAN_METHOD(SignalConnect) {
433471
}
434472

435473
gclosure = Closure::New (callback, signal_info, signalId);
474+
cppWrapper->watchClosure(gclosure);
436475
handler_id = g_signal_connect_closure (gobject, signalName, gclosure, after);
437476

438477
info.GetReturnValue().Set((double)handler_id);
@@ -723,7 +762,7 @@ Local<Value> WrapperFromGObject(GObject *gobject) {
723762
return obj;
724763
}
725764

726-
GObject * GObjectFromWrapper(Local<Value> value) {
765+
GObjectWrapper * GObjectCppWrapperFromWrapper(Local<Value> value) {
727766
if (!ValueHasInternalField(value))
728767
return nullptr;
729768

@@ -739,7 +778,11 @@ GObject * GObjectFromWrapper(Local<Value> value) {
739778
descriptor.wrappable_instance_index));
740779
#endif
741780

742-
return cppWrapper->getGObject();
781+
return cppWrapper;
782+
}
783+
784+
GObject * GObjectFromWrapper(Local<Value> value) {
785+
return GObjectCppWrapperFromWrapper(value)->getGObject();
743786
}
744787

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

0 commit comments

Comments
 (0)