Repository navigation
Conversation
…ue lock GObject calls a toggle notify after releasing its own locks, so another thread can deliver it after the toggle reference was removed and the wrapper freed, possibly with a freed GObject (see g_object_add_toggle_ref()). Passing the ObjectInstance as notify data let such a notification queue a dangling pointer, which the next queue drain at GC start dereferenced. Register each toggle reference in the ToggleQueue under a never-reused identity and pass that identity as notify data. The notify looks it up under the queue lock and drops the notification before touching the instance or the GObject once the identity is retired. Every teardown path retires the identity and cancels queued toggles before removing the toggle reference. The GC weak pointer sweep held the queue lock while releasing native objects. Releasing a GSettings runs its weak notify, which waits for the settings backend mutex, while the dconf worker holds that mutex and waits for the queue lock to deliver a toggle notify, deadlocking Cinnamon with one thread spinning in ToggleQueue::lock(). Disassociate under the lock and release only after unlocking. AI-Model: anthropic/claude-opus-5-5 AI-Variant: medium AI-Tool: opencode 1.18.34-vt-207-907b3bc518 AI-Platform: linux AI-Harness: Vibeterm e2a734d
A late toggle notification is matched to its instance by an identity that must never be reused, or a notification meant for a retired toggle reference would reach a newer instance. Abort instead of wrapping the counter, and test the boundary with the counter set just below its maximum. Describe the late notification tests accurately: the reused address test registers the same pointer again, and the finalized test proves the GObject was freed, not that the C++ instance was deleted separately. AI-Model: anthropic/claude-opus-5-5 AI-Variant: medium AI-Tool: opencode 1.18.34-vt-207-907b3bc518 AI-Platform: linux AI-Harness: Vibeterm 38f6148
|
I'm -1 for this PR, it looks like AI slop, adding this will make backports harder. |
|
I can point my AI slop at gjs if that's where it should go. I'm just fixing all random stuff that randomly causes 100% single-core CPU usage on my computer. |
##404 was closed 5 years ago as fixed. https://gitlab.gnome.org/GNOME/gjs/-/work_items/404 FTR I've never seen the issue with the default cinnamon applets, what triggers the issue? |
|
Opened the same fix against GJS, where both defects also exist on master: https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/1134. It is the remaining half of GNOME/gjs#558 (!895 fixed only the disassociate path). Unpatched gjs master deadlocked in 7 of 10 stress runs, patched passed 20 of 20, gjs tests 83/83. Leaving this PR open in case you want the fix in cjs before it comes back through a GJS rebase. |
3 participants
Cinnamon can lock up with one core at 100%: the main thread spins forever in
ToggleQueue::lock(). This fixes that deadlock, plus a use-after-free in late toggle notifications that came up while fixing it.The deadlock
update_heap_wrapper_weak_pointers()holds the toggle queue lock for the whole GC weak pointer sweep. The sweep callsdisassociate_js_gobject(), which releases the native object. When that object is aGSettings, its weak notify waits for the GIO settings backend mutex.At the same time the dconf worker thread holds that backend mutex and calls
g_weak_ref_get(), which takes a toggle ref and callswrapped_gobj_toggle_notify(), which waits for the toggle queue lock. Each thread waits for the other.I found this on Cinnamon 6.6.9 / cjs 128.1 / GLib 2.88.3 / dconf 0.49.0 (Arch). gdb and perf on the stuck process showed exactly these two stacks. The lock order is the same on master.
Fix: disassociate under the lock as before, but release the native objects (
invalidate_closures()+release_native_object(), nowrelease_disassociated_js_gobject()) after unlocking.The use-after-free
GLib calls a toggle notify after dropping its own locks, so another thread can deliver it after
g_object_remove_toggle_ref()returned and the wrapper was freed, possibly with a freed GObject too (documented ing_object_add_toggle_ref()). The notify data was the rawObjectInstance*, so such a late notify could queue a dangling pointer.Fix: each toggle ref gets a never-reused
ToggleQueue::ObjectId, registered in the queue under its lock and passed as the notify data. The notify looks the id up under the queue lock and returns before touching the instance or the GObject if it was retired. Every teardown path retires the id (and cancels queued toggles) before removing the toggle ref. The counter aborts rather than wrapping.Testing
The same code is in GJS (
gi/object.cpp,gi/toggle.cpp), see also GNOME/gjs#404.