Skip to content
Snippets Groups Projects

Fix a9e845 and 797755 Allow X*IfEvent() to reenter libX11

Merged GaryOderNichts requested to merge GaryOderNichts/libx11:fix/reentrant_events into master
2 unresolved threads

a9e84580 tried to fix 79775575 but introduced a condition where another thread could increase in_ifevent before waiting for the other thread to unlock the display and also prevented other threads from locking / unlocking the display. This commit tries to implement 79775575 with a simpler approach which hopefully fixes all issues mentioned in #168 (closed).
Note that I only tested this with firefox on my personal machine and it should be made sure that this still fixes the original problems described in !150 (merged).

Edited by GaryOderNichts

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
49 49 unsigned long qe_serial = 0;
50 50 int n; /* time through count */
51 51
52 dpy->in_ifevent++;
53 52 LockDisplay(dpy);
53 #ifdef XTHREADS
54 dpy->ifevent_thread = xthread_self();
55 #endif
56 dpy->in_ifevent++;
  • Comment on lines -52 to +56

    Moving the counter logic inside the display lock makes sense, that bit I understand and I approve. Is that all we need though? I don't understand what the xthread_self() stuff is trying to accomplish.

  • All of these commits make LockDisplay a no-op if a thread is inside of a X*IfEvent() functions.
    If another thread now calls another function which tries to lock the display it will simply move on without locking the mutex and waiting for the other thread to unlock the display.
    The xthread_self() tries to only make the lock functions no-op if it's actually the thread inside of X*IfEvent().

  • Hah, right, gotcha. Yeah, this makes sense then. I think we could reasonably use a recursive mutex here if we were willing to assume pthreads, but xthreads itself doesn't have that. Regardless:

    Reviewed-by: Adam Jackson <ajax@redhat.com>

  • Please register or sign in to reply
  • GaryOderNichts marked this merge request as ready

    marked this merge request as ready

  • 240 240 if (lock_hist_loc >= LOCK_HIST_SIZE)
    241 241 lock_hist_loc = 0;
    242 242 #endif /* XTHREADS_WARN */
    243 xmutex_unlock(dpy->lock->mutex);
    243
    244 if (dpy->in_ifevent == 0 || dpy->ifevent_thread != xthread_self())
  • GaryOderNichts added 1 commit

    added 1 commit

    • b3353c03 - Fix a9e845 and 797755 Allow X*IfEvent() to reenter libX11

    Compare with previous version

  • mentioned in issue #168 (closed)

  • Alan Coopersmith added 7 commits

    added 7 commits

    Compare with previous version

  • Merged, thanks!

  • Please register or sign in to reply
    Loading