Bug 226920 - devel/glib20: pull the latest file monitor fix from upstream
Summary: devel/glib20: pull the latest file monitor fix from upstream
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Koop Mast
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-25 17:27 UTC by Ting-Wei Lan
Modified: 2018-05-07 18:46 UTC (History)
9 users (show)

See Also:
bugzilla: maintainer-feedback? (gnome)


Attachments
Use kqueue and polling file monitor fixes from upstream (72.47 KB, patch)
2018-03-25 17:27 UTC, Ting-Wei Lan
no flags Details | Diff
Use kqueue and polling file monitor fixes from upstream (commit hash updated) (72.47 KB, patch)
2018-03-26 13:19 UTC, Ting-Wei Lan
no flags Details | Diff
Proposed patch (since 460230 revision) (34.54 KB, patch)
2018-03-26 22:01 UTC, lightside
no flags Details | Diff
Proposed patch (since 460230 revision) (34.53 KB, patch)
2018-03-26 23:57 UTC, lightside
no flags Details | Diff
Proposed patch (since 460230 revision) (33.93 KB, patch)
2018-03-27 13:20 UTC, lightside
no flags Details | Diff
Proposed patch (since 460230 revision) (33.31 KB, patch)
2018-03-27 13:36 UTC, lightside
no flags Details | Diff
Proposed patch (since 460230 revision) (33.33 KB, patch)
2018-03-27 14:15 UTC, lightside
no flags Details | Diff
Proposed patch (since 466648 revision) (33.33 KB, patch)
2018-04-16 23:07 UTC, lightside
no flags Details | Diff
Backtrace for x11-fm/caja (v1.20.2) (4.15 KB, text/plain)
2018-04-16 23:10 UTC, lightside
no flags Details
Backtrace for x11-fm/caja (v1.20.2) (5.61 KB, text/plain)
2018-04-19 04:54 UTC, lightside
no flags Details
Backtrace for x11-fm/caja (v1.20.2) (12.67 KB, text/plain)
2018-04-21 03:47 UTC, lightside
no flags Details
Proposed patch (since 466648 revision) (33.49 KB, patch)
2018-04-24 03:23 UTC, lightside
no flags Details | Diff
Proposed patch (since 466648 revision) (33.55 KB, patch)
2018-04-24 03:31 UTC, lightside
lightside: maintainer-approval? (gnome)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ting-Wei Lan 2018-03-25 17:27:10 UTC
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
Comment 1 Ting-Wei Lan 2018-03-25 17:35:32 UTC
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.
Comment 2 rozhuk.im 2018-03-25 22:39:36 UTC
Mine https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214338 is much better.
Comment 3 Ting-Wei Lan 2018-03-26 13:19:29 UTC
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.
Comment 4 lightside 2018-03-26 22:01:27 UTC
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
Comment 5 lightside 2018-03-26 22:54:04 UTC
(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-
Comment 6 lightside 2018-03-26 23:57:57 UTC
Created attachment 191861 [details]
Proposed patch (since 460230 revision)

Simplified some sed patch.
Comment 7 lightside 2018-03-27 13:20:48 UTC
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.
Comment 8 lightside 2018-03-27 13:36:40 UTC
Created attachment 191869 [details]
Proposed patch (since 460230 revision)

Simplified gio/kqueue/Makefile.in patch.
Comment 9 lightside 2018-03-27 14:15:50 UTC
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.
Comment 10 lightside 2018-03-27 14:21:13 UTC
(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.
Comment 11 Ting-Wei Lan 2018-03-27 14:46:40 UTC
(In reply to lightside from comment #9)
Your patch has the same effect as mine, so yes, you can obsolete my patch.
Comment 12 Ting-Wei Lan 2018-03-27 15:32:30 UTC
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].
Comment 13 Koop Mast freebsd_committer 2018-04-09 21:45:45 UTC
Take, will give this a whirl for a few days.
Comment 14 lightside 2018-04-16 23:07:00 UTC
Created attachment 192573 [details]
Proposed patch (since 466648 revision)

Updated patch after ports r466648 changes, i.e. bumped PORTREVISION.
Comment 15 lightside 2018-04-16 23:10:11 UTC
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.
Comment 16 Ting-Wei Lan 2018-04-17 16:18:09 UTC
(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 17 lightside 2018-04-18 16:54:48 UTC
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).
Comment 18 lightside 2018-04-19 04:54:02 UTC
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.
Comment 19 lightside 2018-04-21 03:47:53 UTC
Created attachment 192694 [details]
Backtrace for x11-fm/caja (v1.20.2)

(In reply to comment #18)
Added available backtrace information from other threads.
Comment 20 lightside 2018-04-24 03:23:47 UTC
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.
Comment 21 lightside 2018-04-24 03:31:03 UTC
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.
Comment 22 commit-hook freebsd_committer 2018-05-07 18:42:58 UTC
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