[glib/g_main_context_check_skipping_pollrec_updates] gmain: g_main_context_check() can skip updating polled FD sources

[Claudio Saavedra <csaavedra src gnome org>]

commit 0d4d8fc048184d4dd9b7fb155d60f77e32f038f3
Author: Claudio Saavedra <csaavedra igalia com>
Date:   Wed Oct 21 13:19:42 2020 +0300

    gmain: g_main_context_check() can skip updating polled FD sources
    
    If there is a file descriptor source that has a lower priority
    than the one for sources that are going to be dispatched,
    all subsequent file descriptor sources (internally sorted by
    file descriptor identifier) do not get an update in their GPollRec
    and later on wrong sources can be dispatched.
    
    Fix this by first finding the first GPollRec that matches the current
    GPollFD, instead of relying on it to be the current one. At
    the same time, document the assumptions about the ordering of the
    file descriptor records and array and make explicit in the documentation
    that the array needs to be passed to g_main_context_check() as it was
    received from g_main_context_query().
    
    Added a new test that reproduces the bug by creating two file
    descriptor sources and an idle one. Since the first
    file descriptor created has a lower identifier and a low priority,
    the second one is not dispatched even when it has the same, higher,
    priority as the idle source. After fixing this bug, both
    higher priority sources are dispatched as expected.
    
    While this patch was written independently, a similar fix for this
    bug was first submitted by Eugene M in GNOME/glib!562. Having a
    second fix that basically does the same is a reassurance that we
    are in the right here.
    
    Fixes #1592

 glib/gmain.c          | 29 ++++++++++++++++++++++++--
 glib/tests/mainloop.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 2 deletions(-)
---
diff --git a/glib/gmain.c b/glib/gmain.c
index 4d057badf..de529543d 100644
--- a/glib/gmain.c
+++ b/glib/gmain.c
@@ -3733,7 +3733,10 @@ g_main_context_prepare (GMainContext *context,
  *       store #GPollFD records that need to be polled.
  * @n_fds: (in): length of @fds.
  *
- * Determines information necessary to poll this main loop.
+ * Determines information necessary to poll this main loop. You should
+ * be careful to pass the resulting @fds array and its length @n_fds
+ * as is when calling g_main_context_check(), as this function relies
+ * on assumptions made when the array is filled.
  *
  * You must have successfully acquired the context with
  * g_main_context_acquire() before you may call this function.
@@ -3757,6 +3760,10 @@ g_main_context_query (GMainContext *context,
 
   TRACE (GLIB_MAIN_CONTEXT_BEFORE_QUERY (context, max_priority));
 
+  /* fds is filled sequentially from poll_records. Since poll_records
+   * are incrementally sorted by file descriptor identifier, fds will
+   * also be incrementally sorted.
+   */
   n_poll = 0;
   lastpollrec = NULL;
   for (pollrec = context->poll_records; pollrec; pollrec = pollrec->next)
@@ -3771,6 +3778,10 @@ g_main_context_query (GMainContext *context,
        */
       events = pollrec->fd->events & ~(G_IO_ERR|G_IO_HUP|G_IO_NVAL);
 
+      /* This optimization --using the same GPollFD to poll for more
+       * than one poll record-- relies on the poll records being
+       * incrementally sorted.
+       */
       if (lastpollrec && pollrec->fd->fd == lastpollrec->fd->fd)
         {
           if (n_poll - 1 < n_fds)
@@ -3816,7 +3827,10 @@ g_main_context_query (GMainContext *context,
  *       the last call to g_main_context_query()
  * @n_fds: return value of g_main_context_query()
  *
- * Passes the results of polling back to the main loop.
+ * Passes the results of polling back to the main loop. You should be
+ * careful to pass @fds and its length @n_fds as received from
+ * g_main_context_query(), as this functions relies on assumptions
+ * on how @fds is filled.
  *
  * You must have successfully acquired the context with
  * g_main_context_acquire() before you may call this function.
@@ -3871,10 +3885,19 @@ g_main_context_check (GMainContext *context,
       return FALSE;
     }
 
+  /* The linear iteration below relies on the assumption that both
+   * poll records and the fds array are incrementally sorted by file
+   * descriptor identifier.
+   */
   pollrec = context->poll_records;
   i = 0;
   while (pollrec && i < n_fds)
     {
+      /* Skip until finding the first GPollRec matching the current GPollFD. */
+      while (pollrec && pollrec->fd->fd != fds[i].fd)
+        pollrec = pollrec->next;
+
+      /* Update all consecutive GPollRecs that match. */
       while (pollrec && pollrec->fd->fd == fds[i].fd)
         {
           if (pollrec->priority <= max_priority)
@@ -3885,6 +3908,7 @@ g_main_context_check (GMainContext *context,
           pollrec = pollrec->next;
         }
 
+      /* Iterate to next GPollFD. */
       i++;
     }
 
@@ -4495,6 +4519,7 @@ g_main_context_add_poll_unlocked (GMainContext *context,
   newrec->fd = fd;
   newrec->priority = priority;
 
+  /* Poll records are incrementally sorted by file descriptor identifier. */
   prevrec = NULL;
   nextrec = context->poll_records;
   while (nextrec)
diff --git a/glib/tests/mainloop.c b/glib/tests/mainloop.c
index ec96bfa8a..efc896ccf 100644
--- a/glib/tests/mainloop.c
+++ b/glib/tests/mainloop.c
@@ -1542,6 +1542,62 @@ test_unix_file_poll (void)
   close (fd);
 }
 
+static void
+test_unix_fd_priority (void)
+{
+  gint fd1, fd2;
+  GMainLoop *loop;
+  GSource *source;
+
+  gint s1 = 0;
+  gboolean s2 = FALSE, s3 = FALSE;
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/1592";);
+
+  loop = g_main_loop_new (NULL, FALSE);
+
+  source = g_idle_source_new ();
+  g_source_set_callback (source, count_calls, &s1, NULL);
+  g_source_set_priority (source, 0);
+  g_source_attach (source, NULL);
+  g_source_unref (source);
+
+  fd1 = open ("/dev/random", O_RDONLY);
+  g_assert_cmpint (fd1, >=, 0);
+  source = g_unix_fd_source_new (fd1, G_IO_IN);
+  g_source_set_callback (source, G_SOURCE_FUNC (flag_bool), &s2, NULL);
+  g_source_set_priority (source, 10);
+  g_source_attach (source, NULL);
+  g_source_unref (source);
+
+  fd2 = open ("/dev/random", O_RDONLY);
+  g_assert_cmpint (fd2, >=, 0);
+  source = g_unix_fd_source_new (fd2, G_IO_IN);
+  g_source_set_callback (source, G_SOURCE_FUNC (flag_bool), &s3, NULL);
+  g_source_set_priority (source, 0);
+  g_source_attach (source, NULL);
+  g_source_unref (source);
+
+  /* This tests a bug that depends on the source with the lowest FD
+     identifier to have the lowest priority. Make sure that this is
+     the case. */
+  g_assert_cmpint (fd1, <, fd2);
+
+  g_assert_true (g_main_context_iteration (NULL, FALSE));
+
+  /* Idle source should have been dispatched. */
+  g_assert_cmpint (s1, ==, 1);
+  /* Low priority FD source shouldn't have been dispatched. */
+  g_assert_false (s2);
+  /* Default priority FD source should have been dispatched. */
+  g_assert_true (s3);
+
+  g_main_loop_unref (loop);
+
+  close (fd1);
+  close (fd2);
+}
+
 #endif
 
 #ifdef G_OS_UNIX
@@ -2035,6 +2091,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/mainloop/source-unix-fd-api", test_source_unix_fd_api);
   g_test_add_func ("/mainloop/wait", test_mainloop_wait);
   g_test_add_func ("/mainloop/unix-file-poll", test_unix_file_poll);
+  g_test_add_func ("/mainloop/unix-fd-priority", test_unix_fd_priority);
 #endif
   g_test_add_func ("/mainloop/nfds", test_nfds);