rpms/gjs
5
0
Fork 0
Code Issues Pull Requests Releases Activity

Replace MR #585 reversion with MR #588, hopefully correct fix

Browse Source
This commit is contained in:
Adam Williamson 2021-03-19 17:54:34 -07:00
parent 87d72083e6
commit 540d19c6d0
4 changed files with 77 additions and 166 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
0001-Revert-tests-Gtk3-Functions-that-leads-running-dispo.patch
View File

@ -1,40 +0,0 @@
From c67ca084d7160f836e7dd7f38ee9941718a1fa82 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Thu, 18 Mar 2021 18:17:54 -0700
Subject: [PATCH 1/2] Revert "tests/Gtk3: Functions that leads running dispose
vfuncs can be safely called"
This reverts commit ffac59100c24ac180065e145b7d04c6af5daf927.
---
installed-tests/js/testGtk3.js | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/installed-tests/js/testGtk3.js b/installed-tests/js/testGtk3.js
index fd0608d1..15187b67 100644
--- a/installed-tests/js/testGtk3.js
+++ b/installed-tests/js/testGtk3.js
@@ -191,21 +191,6 @@ describe('Gtk overrides', function () {
'Gtk overrides avoid crashing and print a stack trace');
});
- it('GTK vfuncs can be explicitly called during disposition', function () {
- let called;
- const GoodLabel = GObject.registerClass(class GoodLabel extends Gtk.Label {
- vfunc_destroy() {
- called = true;
- }
- });
-
- let label = new GoodLabel();
- label.destroy();
- expect(called).toBeTruthy();
- label = null;
- System.gc();
- });
-
it('accepts string in place of GdkAtom', function () {
expect(() => Gtk.Clipboard.get(1)).toThrow();
expect(() => Gtk.Clipboard.get(true)).toThrow();
--
2.30.2

122
0002-Revert-object-Do-not-call-any-function-on-disposed-G.patch
View File

@ -1,122 +0,0 @@
From 457de7f08a2ab07ac7e65a6b27e5c9e09403df16 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Thu, 18 Mar 2021 18:18:00 -0700
Subject: [PATCH 2/2] Revert "object: Do not call any function on disposed
GObject pointers"
This reverts commit d37d6604ca0d50b9be408acd637e8073e30500c7.
---
gi/object.cpp | 15 ++++++---------
gi/toggle.cpp | 16 +++++++++-------
gi/toggle.h | 2 +-
installed-tests/js/testGtk3.js | 7 +++++--
4 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/gi/object.cpp b/gi/object.cpp
index 68410514..c811a065 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1297,9 +1297,7 @@ void
ObjectInstance::release_native_object(void)
{
discard_wrapper();
- if (m_gobj_disposed)
- m_ptr.release();
- else if (m_uses_toggle_ref)
+ if (m_uses_toggle_ref)
g_object_remove_toggle_ref(m_ptr.release(), wrapped_gobj_toggle_notify,
nullptr);
else
@@ -1481,6 +1479,9 @@ ObjectInstance::disassociate_js_gobject(void)
{
bool had_toggle_down, had_toggle_up;
+ if (!m_gobj_disposed)
+ g_object_weak_unref(m_ptr.get(), wrapped_gobj_dispose_notify, this);
+
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) {
@@ -1490,12 +1491,8 @@ ObjectInstance::disassociate_js_gobject(void)
m_ptr.get(), type_name());
}
- if (!m_gobj_disposed) {
- g_object_weak_unref(m_ptr.get(), wrapped_gobj_dispose_notify, this);
-
- /* Fist, remove the wrapper pointer from the wrapped GObject */
- unset_object_qdata();
- }
+ /* Fist, remove the wrapper pointer from the wrapped GObject */
+ unset_object_qdata();
/* Now release all the resources the current wrapper has */
invalidate_closure_list(&m_closures);
diff --git a/gi/toggle.cpp b/gi/toggle.cpp
index 4f7db32f..5f3d4386 100644
--- a/gi/toggle.cpp
+++ b/gi/toggle.cpp
@@ -71,17 +71,19 @@ ToggleQueue::is_queued(GObject *gobj) const
return {has_toggle_down, has_toggle_up};
}
-std::pair<bool, bool> ToggleQueue::cancel(GObject* gobj) {
+std::pair<bool, bool>
+ToggleQueue::cancel(GObject *gobj)
+{
debug("cancel", gobj);
std::lock_guard<std::mutex> hold(lock);
bool had_toggle_down = find_and_erase_operation_locked(gobj, DOWN);
bool had_toggle_up = find_and_erase_operation_locked(gobj, UP);
- gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "ToggleQueue: %p was %s", gobj,
- had_toggle_down && had_toggle_up
- ? "queued to toggle BOTH"
- : had_toggle_down ? "queued to toggle DOWN"
- : had_toggle_up ? "queued to toggle UP"
- : "not queued");
+ gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "ToggleQueue: %p (%s) was %s", gobj,
+ G_OBJECT_TYPE_NAME(gobj),
+ had_toggle_down && had_toggle_up ? "queued to toggle BOTH"
+ : had_toggle_down ? "queued to toggle DOWN"
+ : had_toggle_up ? "queued to toggle UP"
+ : "not queued");
return {had_toggle_down, had_toggle_up};
}
diff --git a/gi/toggle.h b/gi/toggle.h
index e77c5419..f7f8eac8 100644
--- a/gi/toggle.h
+++ b/gi/toggle.h
@@ -66,7 +66,7 @@ private:
* are / were queued. is_queued() just checks and does not modify. */
[[nodiscard]] std::pair<bool, bool> is_queued(GObject* gobj) const;
/* Cancels pending toggles and returns whether any were queued. */
- std::pair<bool, bool> cancel(GObject* gobj);
+ std::pair<bool, bool> cancel(GObject *gobj);
/* Pops a toggle from the queue and processes it. Call this if you don't
* want to wait for it to be processed in idle time. Returns false if queue
diff --git a/installed-tests/js/testGtk3.js b/installed-tests/js/testGtk3.js
index 15187b67..f3f6e724 100644
--- a/installed-tests/js/testGtk3.js
+++ b/installed-tests/js/testGtk3.js
@@ -180,11 +180,14 @@ describe('Gtk overrides', function () {
GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
'*destroy*');
- const BadLabel = GObject.registerClass(class BadLabel extends Gtk.Label {
+ let BadLabel = GObject.registerClass(class BadLabel extends Gtk.Label {
vfunc_destroy() {}
});
- new BadLabel();
+ let w = new Gtk.Window();
+ w.add(new BadLabel());
+
+ w.destroy();
System.gc();
GLib.test_assert_expected_messages_internal('Gjs', 'testGtk3.js', 0,
--
2.30.2

70
588.patch Normal file
View File

@ -0,0 +1,70 @@
From c0003eb5ad4c4b0421d723da3d1ff11991f70fb5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= <mail@3v1n0.net>
Date: Fri, 19 Mar 2021 16:46:42 +0100
Subject: [PATCH] object: Ignore toggle notifications after disposition
As per commit d37d6604 we are not removing a toggle reference on
disposed objects, however it may happen that a disposed object (but not
yet finalized) is still using the toggle references while we're releasing
it, and in such case we must always remove the toggle ref, otherwise
GObject (that doesn't remove toggle notifications on disposition) will
notify us after that the wrapper has been finalized, causing a crash
because at that point the GObject is still very well alive (and so
its qdata) and so when we'll try to get the gjs private data from it,
(namely the JS object wrapper instance) we'll end up accessing freed
memory.
So, on weak notify callback (that we get during disposition, when the
object memory is still valid like its toggle notifications) remove the
toggle reference that we set and consequently toggle down the JSObject
wrapper, unrooting it so that the garbage collector can pick it (this may
also make it a bit more reactive, without waiting for the last reference
being removed if disposition happens as consequence of a run_dispose()
call).
We keep the m_uses_toggle_ref set though to avoid adding another one,
but at this point we also have to check whether the object is also
disposed before considering the toggle ref active, and per this in such
case we've to only release (steal) the m_ptr when releasing the native
object not to access to potentially finalized data.
Fixes: #387
---
gi/object.cpp | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/gi/object.cpp b/gi/object.cpp
index 68410514..598e6bb0 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1082,10 +1082,18 @@ static void wrapped_gobj_dispose_notify(
where_the_object_was);
}
+static void wrapped_gobj_toggle_notify(void*, GObject* gobj,
+ gboolean is_last_ref);
+
void
ObjectInstance::gobj_dispose_notify(void)
{
m_gobj_disposed = true;
+
+ if (m_uses_toggle_ref) {
+ g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, nullptr);
+ wrapped_gobj_toggle_notify(nullptr, m_ptr, TRUE);
+ }
}
void ObjectInstance::iterate_wrapped_gobjects(
@@ -1297,7 +1305,7 @@ void
ObjectInstance::release_native_object(void)
{
discard_wrapper();
- if (m_gobj_disposed)
+ if (m_uses_toggle_ref && m_gobj_disposed)
m_ptr.release();
else if (m_uses_toggle_ref)
g_object_remove_toggle_ref(m_ptr.release(), wrapped_gobj_toggle_notify,
--
GitLab

11
gjs.spec
View File

@ -5,7 +5,7 @@
Name: gjs
Version: 1.67.3
Release: 2%{?dist}
Release: 3%{?dist}
Summary: Javascript Bindings for GNOME
# The following files contain code from Mozilla which
@ -16,11 +16,11 @@ License: MIT and (MPLv1.1 or GPLv2+ or LGPLv2+)
URL: https://wiki.gnome.org/Projects/Gjs
Source0: https://download.gnome.org/sources/%{name}/1.67/%{name}-%{version}.tar.xz
# Revert MR #585 to work around frequent crash on unlock:
# Fix a frequent crash on unlock:
# https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/588
# https://bugzilla.redhat.com/show_bug.cgi?id=1940171
# https://gitlab.gnome.org/GNOME/gjs/-/issues/387
Patch0001: 0001-Revert-tests-Gtk3-Functions-that-leads-running-dispo.patch
Patch0002: 0002-Revert-object-Do-not-call-any-function-on-disposed-G.patch
Patch0: 588.patch
BuildRequires: cairo-gobject-devel
BuildRequires: dbus-daemon
@ -103,6 +103,9 @@ the functionality of the installed gjs package.
%{_datadir}/installed-tests/
%changelog
* Fri Mar 19 2021 Adam Williamson <awilliam@redhat.com> - 1.67.3-3
- Replace MR #585 reversion with MR #588, hopefully correct fix
* Thu Mar 18 2021 Adam Williamson <awilliam@redhat.com> - 1.67.3-2
- Patches to revert MR #585 to work around frequent crash on unlock