Created attachment 191808 [details] Use kqueue and polling file monitor fixes from upstream This is a follow-up of bug 199872. GLib recently landed a patch contributed by OpenBSD to fix crashes in the kqueue file monitor backend. Yes, I know we already committed a fix for the problem in bug 199872, but I saw no change in the frequency of gnome-shell crashes. The new patch accepted by upstream seems to change it greatly. It works very well for me and it has been used and tested in OpenBSD ports. I am unable to trigger any file monitor-related crash in gnome-shell, firefox, evolution. This new patch will be available in GLib 2.58 (GNOME 3.30). Given that we are slow on GNOME upgrades, can we replace the old patch committed in bug 199872 with the new one to fix things more completely and allow broader testing? The patch I uploaded here includes 4 commits. The first 3 commits are already committed to upstream repository, and the last one (written by me) is under review. Here is the list of referenced bugs in these commits: https://bugzilla.gnome.org/show_bug.cgi?id=778515 https://bugzilla.gnome.org/show_bug.cgi?id=776147 https://bugzilla.gnome.org/show_bug.cgi?id=739424 https://bugzilla.gnome.org/show_bug.cgi?id=794528
I just added people who may be willing to test the new patch from the CC list of bug 199872. If you don't want to receive notification from this bug, remove yourself from the CC list.
Mine https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214338 is much better.
Created attachment 191837 [details] Use kqueue and polling file monitor fixes from upstream (commit hash updated) The last patch is now committed to the upstream repository, so I updated the patch to include the correct commit hash. I am not going to reroll patches in patch-gio_filemonitor just to pass 'make checkpatch' unless it is strictly required. I think keeping different changes separated is easier for users to understand and validate them.
Created attachment 191855 [details] Proposed patch (since 460230 revision) Hello. (In reply to Ting-Wei Lan from comment #3) > I am not going to reroll patches in patch-gio_filemonitor just to pass > 'make checkpatch' unless it is strictly required. There is not only `make checkpatch` error: -8<-- % make checkpatch <..> ===> Patching for glib-2.50.3_2,1 ===> Applying FreeBSD patches for glib-2.50.3_2,1 2 out of 2 hunks failed while patching gio/kqueue/kqueue-helper.c => FreeBSD patch patch-gio_filemonitor failed to apply cleanly. *** Error code 1 Stop. <..> -->8- because of three patches for gio/kqueue/kqueue-helper.c: -8<-- cat files/patch-gio_filemonitor | grep "^+++ gio/kqueue/kqueue-helper.c" | wc -l | tr -d ' ' 3 -->8- But also a `make check-plist` errors: -8<-- % make check-plist <..> ====> Running Q/A tests (stage-qa) ====> Checking for pkg-plist issues (check-plist) ===> Parsing plist ===> Checking for items in STAGEDIR missing from pkg-plist Error: Orphaned: lib/charset.alias Error: Orphaned: share/doc/gio/GAction.html <..> Error: Orphaned: share/doc/gobject/up.png ===> Checking for items in pkg-plist which are not in STAGEDIR ===> Error: Plist issues found. *** Error code 1 Stop. <..> -->8- because of USES+=autoreconf usage, which requires to patch configure.ac instead of configure, Makefile.am instead of Makefile.in, etc. I fixed these issues in attached patch. The files/patch-gio_filemonitor patch was recreated with using known commits (based on attachment #191837 [details]): -8<-- % cat files/patch-gio_filemonitor | grep ^commit commit e305fe971e4647d971428a772b7290b9c308a96f commit 76072a2dde4a4acc8be8d3c47efbc6811ebe0c1e commit aa39a0557c679fc345b0ba72a87c33152eb8ebcd commit ba4a9538e14e8ba0ea037cab5f4b23aa47272a4c -->8- from https://github.com/GNOME/glib in some user's directory: -8<-- % git clone https://github.com/GNOME/glib.git <..> % git clone glib glib-devel <..> % cd glib-devel % git checkout -b devel 2.50.3 <..> % git cherry-pick e305fe971e4647d971428a772b7290b9c308a96f 76072a2dde4a4acc8be8d3c47efbc6811ebe0c1e <..> % git cherry-pick aa39a0557c679fc345b0ba72a87c33152eb8ebcd <..> % git diff * Unmerged path gio/kqueue/meson.build % git rm gio/kqueue/meson.build <..> % git commit --no-edit <..> % git cherry-pick ba4a9538e14e8ba0ea037cab5f4b23aa47272a4c <..> % git show -s --format="# %s%n# https://github.com/GNOME/glib/commit/%H" e305fe971e4647d971428a772b7290b9c308a96f 76072a2dde4a4acc8be8d3c47efbc6811ebe0c1e aa39a0557c679fc345b0ba72a87c33152eb8ebcd ba4a9538e14e8ba0ea037cab5f4b23aa47272a4c > ../patch-gio_filemonitor % echo >> ../patch-gio_filemonitor % git diff --no-color --no-prefix 2.50.3 HEAD | sed -e '/^index/d ; /^deleted/d' >> ../patch-gio_filemonitor -->8- Remove local glib-devel repository (including glib directory, if needed): % cd .. && rm -rf glib-devel I think, there is no much point to include removal of gio/kqueue/gkqueuefilemonitor.h, gio/kqueue/kqueue-thread.c, gio/kqueue/kqueue-thread.h, gio/kqueue/kqueue-sub.c, gio/kqueue/kqueue-sub.h, etc. files to patch-gio_filemonitor patch, because changes for gio/kqueue/Makefile.am removes them from build. So, I removed such files with "+++ /dev/null" from patch-gio_filemonitor patch manually. Next step was to move patch-gio_filemonitor to <..>/glib20/files/patch-gio_filemonitor of the port and recreate patch with `make makepatch` command. I created a backup of files directory and removed all patches from files directory, except patch-gio_filemonitor, then: -8<-- % make extract <..> % find work -type f -iname "*.orig" -delete % make patch <..> % make makepatch <..> % make clean <..> -->8- and copy/move generated files/patch-gio_filemonitor to backup files directory, then replace files directory in the port with backup files directory. The attached patch was created with using following command: % svn diff --ignore-properties > ../glib20.diff
(In reply to comment #4) Checksums of changed files, patched by files/patch-gio_filemonitor, are identical between attachment #191837 [details] and attachment #191855 [details]: -8<-- % make patch <..> % cd work/glib-2.50.3 % sha256 gio/gpollfilemonitor.c gio/kqueue/Makefile.am gio/kqueue/gkqueuefilemonitor.c gio/kqueue/kqueue-helper.c gio/kqueue/kqueue-helper.h gio/kqueue/kqueue-missing.c SHA256 (gio/gpollfilemonitor.c) = 84d9eea109c086ff0c0d6b6a9d9b3c019c03af697a2ba50d79427069c396781e SHA256 (gio/kqueue/Makefile.am) = 87bf0e9c963b59f46af9a0323223df97984764775f4b532ada41d23ca676bfce SHA256 (gio/kqueue/gkqueuefilemonitor.c) = 2da228bf2122eb86cedf1def6742ff9b9959c810af158045851e943a8b2737f9 SHA256 (gio/kqueue/kqueue-helper.c) = 0eecab3b43f61684c29367430364d0fd7bf66e13ad938a89094a4b49eaa4999f SHA256 (gio/kqueue/kqueue-helper.h) = 78fd033352ebd0798efa723c5a3ad4daf648cc2689fd7bbae6c8298c3482e26f SHA256 (gio/kqueue/kqueue-missing.c) = 5418419f2a7e690c9a36b1ec3184d3660a6009d045029e74d04daecfe22d41b2 % cd ../../ && make clean <..> -->8-
Created attachment 191861 [details] Proposed patch (since 460230 revision) Simplified some sed patch.
Created attachment 191868 [details] Proposed patch (since 460230 revision) (In reply to comment #4) Also possible to convert gio/kqueue/Makefile.am to gio/kqueue/Makefile.in patch. Attached this variant instead.
Created attachment 191869 [details] Proposed patch (since 460230 revision) Simplified gio/kqueue/Makefile.in patch.
Created attachment 191870 [details] Proposed patch (since 460230 revision) Changed links from "https://github.com/GNOME/glib/commit/" to "https://gitlab.gnome.org/GNOME/glib/commit/" for files/patch-gio_filemonitor. Requested by PR's submitter.
(In reply to comment #9) > Changed links from "https://github.com/GNOME/glib/commit/" to > "https://gitlab.gnome.org/GNOME/glib/commit/" from "https://github.com/GNOME/glib" to "https://gitlab.gnome.org/GNOME/glib", which is official repository.
(In reply to lightside from comment #9) Your patch has the same effect as mine, so yes, you can obsolete my patch.
Comment on attachment 191837 [details] Use kqueue and polling file monitor fixes from upstream (commit hash updated) It is now obsoleted by attachment 191870 [details].
Take, will give this a whirl for a few days.
Created attachment 192573 [details] Proposed patch (since 466648 revision) Updated patch after ports r466648 changes, i.e. bumped PORTREVISION.
Created attachment 192575 [details] Backtrace for x11-fm/caja (v1.20.2) Attached some backtrace for x11-fm/caja (v1.20.2; with WITH_DEBUG=yes) on FreeBSD 10.3 amd64. Testcase was many opened caja windows for some port in user directory, including for some directories in related work directory after `make check-plist` command (which was launched from mate-terminal (v1.20.0; x11/mate-terminal) window). The caja crashed (and caja.core file created inside ${HOME} directory) after `make clean` command. Also there was opened some text editor (editors/scite) for Makefile (and probably some file(s) inside work directory) before caja crash. Possible, that mate-terminal also was opened for some work directory, but probably this is not related. Previously I reproduced mentioned testcase (for attachment #191870 [details]) without debug symbols. Then I tested with debug symbols (WITH_DEBUG=yes) for devel/glib20 and x11-fm/caja (v1.18.4 and v1.20.2). But mentioned caja crash was once after this (for which backtrace was attached). So, I accidentally reproduced this testcase once more (i.e. it was rare). Not sure, if my explanations about testcase and some programs before caja crash was useful, but as is.
(In reply to lightside from comment #15) THere was an assertion failure in _kqsub_free on line 364: 360 static void 361 _kqsub_free (kqueue_sub *sub) 362 { 363 g_assert (sub->deps == NULL); 364 g_assert (sub->fd == -1); 365 366 g_source_unref ((GSource *) sub->source); 367 g_free (sub->filename); 368 g_slice_free (kqueue_sub, sub); 369 } _kqsub_free was called by g_kqueue_file_monitor_cancel on line 326. 323 if (kqueue_monitor->sub) 324 { 325 _kqsub_cancel (kqueue_monitor->sub); 326 _kqsub_free (kqueue_monitor->sub); 327 kqueue_monitor->sub = NULL; 328 } The previous function call, _kqsub_cancel, set sub->fd to -1 when it succeeded. 371 static gboolean 372 _kqsub_cancel (kqueue_sub *sub) 373 { 374 struct kevent ev; 375 376 if (sub->deps) 377 { 378 dl_free (sub->deps); 379 sub->deps = NULL; 380 } 381 382 _km_remove (sub); 383 384 /* Only in the missing list? We're done! */ 385 if (sub->fd == -1) 386 return TRUE; 387 388 EV_SET (&ev, sub->fd, EVFILT_VNODE, EV_DELETE, NOTE_ALL, 0, sub); 389 if (kevent (kq_queue, &ev, 1, NULL, 0, NULL) == -1) 390 { 391 g_warning ("Unable to remove event for %s: %s", sub->filename, g_strerror (errno)); 392 return FALSE; 393 } 394 395 close (sub->fd); 396 sub->fd = -1; 397 398 return TRUE; 399 } For _kqsub_cancel to return without setting sub->fd to -1, it must return on line 392, which means the kevent call failed.
Comment on attachment 192575 [details] Backtrace for x11-fm/caja (v1.20.2) (In reply to Ting-Wei Lan from comment #16) Thanks for clarification. Since there are no exact steps to reproduce testcase in comment #15 (e.g. the caja crash was rare (twice between a long period of time), which may suggests about "race condition" or other issue(s) somewhere), but some backtrace only, I think, possible to ignore it before confirmation from other people. So, please do not consider this as an "obstacle" in the resolution of this PR, if it solves other issue(s) (mentioned in comment #0).
Created attachment 192641 [details] Backtrace for x11-fm/caja (v1.20.2) (In reply to comment #15) Added attachment with "bt full" output for the same caja.core file, just in case.
Created attachment 192694 [details] Backtrace for x11-fm/caja (v1.20.2) (In reply to comment #18) Added available backtrace information from other threads.
Created attachment 192768 [details] Proposed patch (since 466648 revision) I checked https://gitlab.gnome.org/GNOME/glib/tree/master/gio/kqueue for changes and found new commit for gio/kqueue/gkqueuefilemonitor.c file: https://gitlab.gnome.org/GNOME/glib/commit/ab179184b883ad378a420223f378071821f0c8b9 https://bugzilla.gnome.org/show_bug.cgi?id=795193 Added new changes to the files/patch-gio_filemonitor patch. -8<-- % git clone https://gitlab.gnome.org/GNOME/glib.git <..> % git clone glib glib-devel <..> % cd glib-devel % git checkout -b devel 2.50.3 <..> % git cherry-pick e305fe971e4647d971428a772b7290b9c308a96f 76072a2dde4a4acc8be8d3c47efbc6811ebe0c1e <..> % git cherry-pick aa39a0557c679fc345b0ba72a87c33152eb8ebcd <..> % git diff * Unmerged path gio/kqueue/meson.build % git rm gio/kqueue/meson.build <..> % git commit --no-edit <..> % git cherry-pick ba4a9538e14e8ba0ea037cab5f4b23aa47272a4c ab179184b883ad378a420223f378071821f0c8b9 <..> % git show -s --format="# %s%n# https://gitlab.gnome.org/GNOME/glib/commit/%H" e305fe971e4647d971428a772b7290b9c308a96f 76072a2dde4a4acc8be8d3c47efbc6811ebe0c1e aa39a0557c679fc345b0ba72a87c33152eb8ebcd ba4a9538e14e8ba0ea037cab5f4b23aa47272a4c ab179184b883ad378a420223f378071821f0c8b9 > ../patch-gio_filemonitor % echo >> ../patch-gio_filemonitor % git diff --no-color --no-prefix 2.50.3 HEAD | sed -e '/^index/d ; /^deleted/d' >> ../patch-gio_filemonitor -->8- The attachment #192694 [details] was obsoleted. Please check new patch.
Created attachment 192769 [details] Proposed patch (since 466648 revision) (In reply to comment #20) Added description to files/patch-gio_filemonitor about "Convert gio/kqueue/Makefile.am to gio/kqueue/Makefile.in patch", similar to comment #8.
A commit references this bug: Author: kwm Date: Mon May 7 18:42:54 UTC 2018 New revision: 469308 URL: https://svnweb.freebsd.org/changeset/ports/469308 Log: Update the kqueue file monitering backend. This update is a backport of work done by Martin Pieuchot (mpi@openbsd). And will be available in the next major Glib released in September. This update simplifies the backend and fixes a number of races and other issues in the old backend. Many thanks to Martin Pieuchot mpi@openbsd for the rewrite! PR: 226920 Submitted by: Ting-Wei Lan <lantw44@gmail.com>, lightside@gmx.com Obtained from: glib upstream Changes: head/devel/glib20/Makefile head/devel/glib20/files/patch-gio_filemonitor head/devel/glib20/files/patch-gio_kqueue_gkqueuefilemonitor.c head/devel/glib20/files/patch-gio_kqueue_kqueue-helper.c