Skip to content

Upstream: Apply closure patches #55

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

Merged
clefebvre merged 2 commits into from
Oct 21, 2017
Merged

Upstream: Apply closure patches #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff0bccc
Revert freeing closures in idle handler
ptomato Aug 21, 2017
cedab61
closure: Prevent collection of invalidated closure
ptomato Aug 23, 2017
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
40 changes: 8 additions & 32 deletions gi/closure.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
struct Closure {
JSContext *context;
GjsMaybeOwned<JSObject *> obj;
unsigned idle_clear_id;
};

struct GjsClosure {
Expand Down Expand Up @@ -117,25 +116,6 @@ global_context_finalized(JS::HandleObject obj,
}
}

/* Closures have to drop their references to their JS functions in an idle
* handler, because otherwise the closure might stop tracing the function object
* in the middle of garbage collection. That is not allowed with incremental GC.
*/
static gboolean
closure_clear_idle(void *data)
{
auto closure = static_cast<GjsClosure *>(data);
gjs_debug_closure("Clearing closure %p which calls object %p",
&closure->priv, closure->priv.obj.get());

closure->priv.obj.reset();
closure->priv.context = nullptr;
closure->priv.idle_clear_id = 0;

g_closure_unref(static_cast<GClosure *>(data));
return G_SOURCE_REMOVE;
}

/* Invalidation is like "dispose" - it is guaranteed to happen at
* finalize, but may happen before finalize. Normally, g_closure_invalidate()
* is called when the "target" of the closure becomes invalid, so that the
Expand Down Expand Up @@ -176,18 +156,21 @@ closure_invalidated(gpointer data,
"removing our destroy notifier on global object)",
closure);

c->idle_clear_id = g_idle_add(closure_clear_idle, closure);
g_closure_ref(closure);
c->obj.reset();
c->context = nullptr;
}

static void
closure_set_invalid(gpointer data,
GClosure *closure)
{
Closure *self = &((GjsClosure*) closure)->priv;

JS::ExposeObjectToActiveJS(self->obj);
self->obj.reset();
self->context = nullptr;

GJS_DEC_COUNTER(closure);
Closure *c = &(reinterpret_cast<GjsClosure *>(closure))->priv;
c->idle_clear_id = g_idle_add(closure_clear_idle, closure);
g_closure_ref(closure);
}

static void
Expand All @@ -196,13 +179,6 @@ closure_finalize(gpointer data,
{
Closure *self = &((GjsClosure*) closure)->priv;

if (self->idle_clear_id > 0) {
/* Remove any pending closure_clear_idle(), we are doing it
* immediately here. */
g_source_remove(self->idle_clear_id);
closure_clear_idle(closure);
}

self->~Closure();
}

Expand Down
64 changes: 4 additions & 60 deletions gi/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#include <config.h>

#include <deque>
#include <map>
#include <memory>
#include <set>
#include <stack>
Expand Down Expand Up @@ -66,7 +65,6 @@ struct ObjectInstance {

/* a list of all signal connections, used when tracing */
std::set<ConnectData *> signals;
std::map<ConnectData *, unsigned> pending_invalidations;

/* the GObjectClass wrapped by this JS Object (only used for
prototypes) */
Expand Down Expand Up @@ -1408,61 +1406,14 @@ object_instance_trace(JSTracer *tracer,
vfunc->js_function.trace(tracer, "ObjectInstance::vfunc");
}

/* Removing the signal connection data from the list means that the object stops
* tracing the JS function objects belonging to the closures. Incremental GC
* does not allow that in the middle of a garbage collection. Therefore, we must
* do it in an idle handler.
*/
static gboolean
signal_connection_invalidate_idle(void *user_data)
{
auto cd = static_cast<ConnectData *>(user_data);
cd->obj->pending_invalidations.erase(cd);
cd->obj->signals.erase(cd);
g_slice_free(ConnectData, cd);
return G_SOURCE_REMOVE;
}

static void
signal_connection_invalidated(void *data,
GClosure *closure)
{
auto cd = static_cast<ConnectData *>(data);
std::map<ConnectData *, unsigned>& pending = cd->obj->pending_invalidations;
g_assert(pending.count(cd) == 0);
pending[cd] = g_idle_add(signal_connection_invalidate_idle, cd);
}

/* This is basically the same as invalidate_all_signals(), but does not defer
* the invalidation to an idle handler. */
static void
invalidate_all_signals_now(ObjectInstance *priv)
{
for (auto& iter : priv->pending_invalidations) {
ConnectData *cd = iter.first;
g_source_remove(iter.second);
g_slice_free(ConnectData, cd);
/* Erase element if not already erased */
priv->signals.erase(cd);
}
priv->pending_invalidations.clear();

/* Can't loop directly through the items, since invalidating an item's
* closure might have the effect of removing the item from the set in the
* invalidate notifier. */
while (!priv->signals.empty()) {
ConnectData *cd = *priv->signals.begin();

/* We have to remove the invalidate notifier, which would
* otherwise schedule a new pending invalidation. */
g_closure_remove_invalidate_notifier(cd->closure, cd,
signal_connection_invalidated);
g_closure_invalidate(cd->closure);

g_slice_free(ConnectData, cd);
/* Erase element if not already erased */
priv->signals.erase(cd);
}
cd->obj->signals.erase(cd);
g_slice_free(ConnectData, cd);
}

static void
Expand All @@ -1484,16 +1435,9 @@ object_instance_finalize(JSFreeOp *fop,
priv->info ? g_base_info_get_namespace((GIBaseInfo*) priv->info) : "_gjs_private",
priv->info ? g_base_info_get_name((GIBaseInfo*) priv->info) : g_type_name(priv->gtype)));

/* We must invalidate all signal connections now, instead of in an idle
* handler, because the object will not exist anymore when we get around to
* the idle function. We originally needed to defer these invalidations to
* an idle function since the object needs to continue tracing its signal
* connections while GC is going on. However, once the object is finalized,
* it will not be tracing them any longer anyway, so it's safe to do them
* now.
* This applies only to instances, not prototypes, but it's possible that
/* This applies only to instances, not prototypes, but it's possible that
* an instance's GObject is already freed at this point. */
invalidate_all_signals_now(priv);
invalidate_all_signals(priv);

/* Object is instance, not prototype, AND GObject is not already freed */
if (priv->gobj) {
Expand Down