[gjs: 15/22] object: Do not disassociate a JS object if we have queued toggle up
- From: Philip Chimento <pchimento src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [gjs: 15/22] object: Do not disassociate a JS object if we have queued toggle up
- Date: Fri, 23 Apr 2021 04:56:07 +0000 (UTC)
commit 5adb51caa691943d0a516225b2cb5c14f71728ce
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date: Mon Mar 29 06:24:24 2021 +0200
object: Do not disassociate a JS object if we have queued toggle up
In case an object has toggle up events queued we need them to happen
in order to make the object to be rooted again, and there's no need to
disassociate it the object.
If instead the object has queued toggle downs, we can instead just
cancel them as the object is unrooted anyways, so there's no issue in
proceeding.
Add test to check whether this is happening, we can simulate such
scenario by queuing a further ref on an object from another thread and
wait for GC to pick the object.
Fixes: #294
gi/object.cpp | 44 ++++++---
gi/toggle.cpp | 76 +++++++++-----
gi/toggle.h | 8 +-
.../js/libgjstesttools/gjs-test-tools.cpp | 39 ++++++++
.../js/libgjstesttools/gjs-test-tools.h | 10 ++
installed-tests/js/testGObjectDestructionAccess.js | 109 +++++++++++++++++++++
6 files changed, 248 insertions(+), 38 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 072c46c2..a8ac6d86 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1329,6 +1329,9 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
is_main_thread = gjs->is_owner_thread();
auto& toggle_queue = ToggleQueue::get_default();
+ if (is_main_thread && toggle_queue.is_being_handled(gobj))
+ return;
+
std::tie(toggle_down_queued, toggle_up_queued) = toggle_queue.is_queued(gobj);
if (is_last_ref) {
@@ -1336,16 +1339,16 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
* The JSObject is rooted and we need to unroot it so it
* can be garbage collected
*/
- if (is_main_thread) {
- if (G_UNLIKELY (toggle_up_queued || toggle_down_queued)) {
- g_error("toggling down object %s that's already queued to toggle %s\n",
- G_OBJECT_TYPE_NAME(gobj),
- toggle_up_queued && toggle_down_queued? "up and down" :
- toggle_up_queued? "up" : "down");
+ if (is_main_thread && !toggle_up_queued) {
+ if (G_UNLIKELY(toggle_down_queued)) {
+ g_error(
+ "toggling down object %p (%s) that's already queued to "
+ "toggle down",
+ gobj, G_OBJECT_TYPE_NAME(gobj));
}
self->toggle_down();
- } else {
+ } else if (!toggle_down_queued) {
toggle_queue.enqueue(gobj, ToggleQueue::DOWN, toggle_handler);
}
} else {
@@ -1356,11 +1359,13 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
*/
if (is_main_thread && !toggle_down_queued) {
if (G_UNLIKELY (toggle_up_queued)) {
- g_error("toggling up object %s that's already queued to toggle up\n",
- G_OBJECT_TYPE_NAME(gobj));
+ g_error(
+ "toggling up object %p (%s) that's already queued to "
+ "toggle up",
+ gobj, G_OBJECT_TYPE_NAME(gobj));
}
self->toggle_up();
- } else {
+ } else if (!toggle_up_queued) {
toggle_queue.enqueue(gobj, ToggleQueue::UP, toggle_handler);
}
}
@@ -1467,7 +1472,22 @@ void ObjectInstance::update_heap_wrapper_weak_pointers(JSContext*,
bool
ObjectInstance::weak_pointer_was_finalized(void)
{
- if (has_wrapper() && !wrapper_is_rooted() && update_after_gc()) {
+ if (has_wrapper() && !wrapper_is_rooted()) {
+ bool toggle_down_queued, toggle_up_queued;
+
+ auto& toggle_queue = ToggleQueue::get_default();
+ std::tie(toggle_down_queued, toggle_up_queued) =
+ toggle_queue.is_queued(m_ptr);
+
+ if (!toggle_down_queued && toggle_up_queued)
+ return false;
+
+ if (!update_after_gc())
+ return false;
+
+ if (toggle_down_queued)
+ toggle_queue.cancel(m_ptr);
+
/* Ouch, the JS object is dead already. Disassociate the
* GObject and hope the GObject dies too. (Remove it from
* the weak pointer list first, since the disassociation
@@ -1576,7 +1596,7 @@ ObjectInstance::disassociate_js_gobject(void)
auto& toggle_queue = ToggleQueue::get_default();
std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(m_ptr.get());
- if (had_toggle_down != had_toggle_up) {
+ if (had_toggle_up && !had_toggle_down) {
g_error(
"JS object wrapper for GObject %p (%s) is being released while "
"toggle references are still pending.",
diff --git a/gi/toggle.cpp b/gi/toggle.cpp
index 4f7db32f..9714ebdb 100644
--- a/gi/toggle.cpp
+++ b/gi/toggle.cpp
@@ -13,6 +13,7 @@
#include <glib.h>
#include "gi/toggle.h"
+#include "gjs/jsapi-util.h"
std::deque<ToggleQueue::Item>::iterator
ToggleQueue::find_operation_locked(const GObject *gobj,
@@ -85,9 +86,13 @@ std::pair<bool, bool> ToggleQueue::cancel(GObject* gobj) {
return {had_toggle_down, had_toggle_up};
}
-bool
-ToggleQueue::handle_toggle(Handler handler)
-{
+bool ToggleQueue::is_being_handled(GObject* gobj) {
+ GObject* tmp_gobj = gobj;
+
+ return m_toggling_gobj.compare_exchange_strong(tmp_gobj, gobj);
+}
+
+bool ToggleQueue::handle_toggle(Handler handler) {
Item item;
{
std::lock_guard<std::mutex> hold(lock);
@@ -95,13 +100,37 @@ ToggleQueue::handle_toggle(Handler handler)
return false;
item = q.front();
- handler(item.gobj, item.direction);
q.pop_front();
}
- debug("handle", item.gobj);
- if (item.needs_unref)
- g_object_unref(item.gobj);
+ /* When getting the object from the weak reference we're implicitly
+ * adding a new reference to the object, this may cause the toggle
+ * notification to be triggered again and this may lead to enqueuing
+ * the object again, so let's save the toggling object in an atomic
+ * pointer so that we can check it quickly to ensure that we're not
+ * recursing into ourself.
+ */
+ GObject* null_gobj = nullptr;
+ m_toggling_gobj.compare_exchange_strong(null_gobj, item.gobj);
+
+ if (item.direction == Direction::DOWN) {
+ GjsSmartPointer<GObject> gobj(
+ static_cast<GObject*>(g_weak_ref_get(&item.weak_ref)));
+
+ if (gobj) {
+ debug("handle", gobj);
+ handler(gobj, item.direction);
+ } else {
+ debug("not handling finalized", item.gobj);
+ }
+
+ g_weak_ref_clear(&item.weak_ref);
+ } else {
+ debug("handle", item.gobj);
+ handler(item.gobj, item.direction);
+ }
+
+ m_toggling_gobj = nullptr;
return true;
}
@@ -128,30 +157,27 @@ ToggleQueue::enqueue(GObject *gobj,
return;
}
- Item item{gobj, direction};
- /* If we're toggling up we take a reference to the object now,
- * so it won't toggle down before we process it. This ensures we
- * only ever have at most two toggle notifications queued.
- * (either only up, or down-up)
+ if (is_being_handled(gobj))
+ return;
+
+ std::lock_guard<std::mutex> hold(lock);
+ /* Only keep a weak reference on the object here, as if we're here, the
+ * JSObject wrapper has already a reference and we don't want to cause
+ * any weak notify in case it has lost one already in the main thread.
+ * So let's use the weak reference to keep track of the object till we
+ * don't handle this toggle.
+ * Unfortunately due to GNOME/glib#2384 we can't be symmetric here and
+ * behave differently on toggle up and down events, however when toggling
+ * up we already have a strong reference, so there's no much need to do
*/
+ auto& item = q.emplace_back(gobj, direction);
+
if (direction == UP) {
debug("enqueue UP", gobj);
- g_object_ref(gobj);
- item.needs_unref = true;
} else {
+ g_weak_ref_init(&item.weak_ref, gobj);
debug("enqueue DOWN", gobj);
}
- /* If we're toggling down, we don't need to take a reference since
- * the associated JSObject already has one, and that JSObject won't
- * get finalized until we've completed toggling (since it's rooted,
- * until we unroot it when we dispatch the toggle down idle).
- *
- * Taking a reference now would be bad anyway, since it would force
- * the object to toggle back up again.
- */
-
- std::lock_guard<std::mutex> hold(lock);
- q.push_back(item);
if (m_idle_id) {
g_assert(((void) "Should always enqueue with the same handler",
diff --git a/gi/toggle.h b/gi/toggle.h
index e77c5419..38ad88f6 100644
--- a/gi/toggle.h
+++ b/gi/toggle.h
@@ -31,9 +31,11 @@ public:
private:
struct Item {
+ Item() {}
+ Item(GObject* o, ToggleQueue::Direction d) : gobj(o), direction(d) {}
GObject *gobj;
+ GWeakRef weak_ref;
ToggleQueue::Direction direction;
- unsigned needs_unref : 1;
};
mutable std::mutex lock;
@@ -42,6 +44,7 @@ private:
unsigned m_idle_id;
Handler m_toggle_handler;
+ std::atomic<GObject*> m_toggling_gobj = nullptr;
/* No-op unless GJS_VERBOSE_ENABLE_LIFECYCLE is defined to 1. */
inline void debug(const char* did GJS_USED_VERBOSE_LIFECYCLE,
@@ -73,6 +76,9 @@ private:
* is empty. */
bool handle_toggle(Handler handler);
+ /* Checks if the gobj is currently being handled, to avoid recursion */
+ bool is_being_handled(GObject* gobj);
+
/* After calling this, the toggle queue won't accept any more toggles. Only
* intended for use when destroying the JSContext and breaking the
* associations between C and JS objects. */
diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp
b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp
index 1e588c97..d5736ca0 100644
--- a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp
+++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp
@@ -10,6 +10,7 @@
#include "gjs/jsapi-util.h"
static GObject* m_tmp_object = NULL;
+static GWeakRef m_tmp_weak;
static std::unordered_set<GObject*> m_finalized_objects;
static std::mutex m_finalized_objects_lock;
@@ -28,6 +29,8 @@ void gjs_test_tools_reset() {
else
m_tmp_object = nullptr;
+ g_weak_ref_set(&m_tmp_weak, nullptr);
+
FinalizedObjectsLocked()->clear();
}
@@ -78,6 +81,12 @@ void gjs_test_tools_save_object(GObject* object) {
monitor_object_finalization(object);
}
+void gjs_test_tools_save_object_unreffed(GObject* object) {
+ g_assert(!m_tmp_object);
+ m_tmp_object = object;
+ monitor_object_finalization(object);
+}
+
void gjs_test_tools_ref_other_thread(GObject* object) {
// cppcheck-suppress leakNoVarFunctionCall
g_thread_join(g_thread_new("ref_object", g_object_ref, object));
@@ -184,6 +193,36 @@ GObject* gjs_test_tools_get_saved() {
return static_cast<GObject*>(g_steal_pointer(&m_tmp_object));
}
+/**
+ * gjs_test_tools_steal_saved:
+ * Returns: (transfer none)
+ */
+GObject* gjs_test_tools_steal_saved() { return gjs_test_tools_get_saved(); }
+
+void gjs_test_tools_save_weak(GObject* object) {
+ g_weak_ref_set(&m_tmp_weak, object);
+}
+
+/**
+ * gjs_test_tools_get_weak:
+ * Returns: (transfer full)
+ */
+GObject* gjs_test_tools_get_weak() {
+ return static_cast<GObject*>(g_weak_ref_get(&m_tmp_weak));
+}
+
+/**
+ * gjs_test_tools_get_weak_other_thread:
+ * Returns: (transfer full)
+ */
+GObject* gjs_test_tools_get_weak_other_thread() {
+ return static_cast<GObject*>(
+ // cppcheck-suppress leakNoVarFunctionCall
+ g_thread_join(g_thread_new(
+ "weak_get",
+ [](void*) -> void* { return gjs_test_tools_get_weak(); }, NULL)));
+}
+
/**
* gjs_test_tools_get_disposed:
* Returns: (transfer none)
diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.h
b/installed-tests/js/libgjstesttools/gjs-test-tools.h
index d56fa0f3..7f67413f 100644
--- a/installed-tests/js/libgjstesttools/gjs-test-tools.h
+++ b/installed-tests/js/libgjstesttools/gjs-test-tools.h
@@ -35,8 +35,18 @@ void gjs_test_tools_run_dispose_other_thread(GObject* object);
void gjs_test_tools_save_object(GObject* object);
+void gjs_test_tools_save_object_unreffed(GObject* object);
+
GObject* gjs_test_tools_get_saved();
+GObject* gjs_test_tools_steal_saved();
+
+void gjs_test_tools_save_weak(GObject* object);
+
+GObject* gjs_test_tools_get_weak();
+
+GObject* gjs_test_tools_get_weak_other_thread();
+
GObject* gjs_test_tools_get_disposed(GObject* object);
G_END_DECLS
diff --git a/installed-tests/js/testGObjectDestructionAccess.js
b/installed-tests/js/testGObjectDestructionAccess.js
index 4b6e9112..63819f5e 100644
--- a/installed-tests/js/testGObjectDestructionAccess.js
+++ b/installed-tests/js/testGObjectDestructionAccess.js
@@ -334,4 +334,113 @@ describe('Disposed or finalized GObject', function () {
file = null;
System.gc();
});
+
+ it('can be re-reffed from other thread delayed', function () {
+ let file = Gio.File.new_for_path('/');
+ file.expandMeWithToggleRef = true;
+ const objectAddress = System.addressOfGObject(file);
+ GjsTestTools.save_object_unreffed(file);
+ GjsTestTools.delayed_ref_other_thread(file, 10);
+ file = null;
+ System.gc();
+
+ const loop = new GLib.MainLoop(null, false);
+ GLib.timeout_add(GLib.PRIORITY_DEFAULT, 50, () => loop.quit());
+ loop.run();
+
+ // We need to cleanup the extra ref we added before now.
+ // However, depending on whether the thread ref happens the object
+ // may be already finalized, and in such case we need to throw
+ try {
+ file = GjsTestTools.steal_saved();
+ if (file) {
+ expect(System.addressOfGObject(file)).toBe(objectAddress);
+ expect(file instanceof Gio.File).toBeTruthy();
+ file.unref();
+ }
+ } catch (e) {
+ expect(() => {
+ throw e;
+ }).toThrowError(/.*Unhandled GType.*/);
+ }
+ });
+
+ it('can be re-reffed and unreffed again from other thread', function () {
+ let file = Gio.File.new_for_path('/');
+ const objectAddress = System.addressOfGObject(file);
+ file.expandMeWithToggleRef = true;
+ GjsTestTools.save_object(file);
+ GjsTestTools.delayed_unref_other_thread(file.ref(), 10);
+ file = null;
+ System.gc();
+
+ const loop = new GLib.MainLoop(null, false);
+ GLib.timeout_add(GLib.PRIORITY_DEFAULT, 50, () => loop.quit());
+ loop.run();
+
+ file = GjsTestTools.get_saved();
+ expect(System.addressOfGObject(file)).toBe(objectAddress);
+ expect(file instanceof Gio.File).toBeTruthy();
+ });
+
+ it('can be re-reffed and unreffed again from other thread with delay', function () {
+ let file = Gio.File.new_for_path('/');
+ file.expandMeWithToggleRef = true;
+ GjsTestTools.delayed_ref_unref_other_thread(file, 10);
+ file = null;
+ System.gc();
+
+ const loop = new GLib.MainLoop(null, false);
+ GLib.timeout_add(GLib.PRIORITY_DEFAULT, 50, () => loop.quit());
+ loop.run();
+ });
+
+ it('can be toggled up by getting a GWeakRef', function () {
+ let file = Gio.File.new_for_path('/');
+ file.expandMeWithToggleRef = true;
+ GjsTestTools.save_weak(file);
+ GjsTestTools.get_weak();
+ });
+
+ it('can be toggled up by getting a GWeakRef from another thread', function () {
+ let file = Gio.File.new_for_path('/');
+ file.expandMeWithToggleRef = true;
+ GjsTestTools.save_weak(file);
+ GjsTestTools.get_weak_other_thread();
+ });
+
+ it('can be toggled up by getting a GWeakRef from another thread and re-reffed in main thread', function
() {
+ let file = Gio.File.new_for_path('/');
+ file.expandMeWithToggleRef = true;
+ GjsTestTools.save_weak(file);
+ GjsTestTools.get_weak_other_thread();
+
+ // Ok, let's play more dirty now...
+ file.ref(); // toggle up
+ file.unref(); // toggle down
+
+ file.ref();
+ file.ref();
+ file.unref();
+ file.unref();
+ });
+
+ it('can be toggled up by getting a GWeakRef from another and re-reffed from various threads', function
() {
+ let file = Gio.File.new_for_path('/');
+ file.expandMeWithToggleRef = true;
+ GjsTestTools.save_weak(file);
+ GjsTestTools.get_weak_other_thread();
+
+ GjsTestTools.ref_other_thread(file);
+ GjsTestTools.unref_other_thread(file);
+
+ file.ref();
+ file.unref();
+
+ GjsTestTools.ref_other_thread(file);
+ file.unref();
+
+ file.ref();
+ GjsTestTools.unref_other_thread(file);
+ });
});
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
