Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-30663

QQuickRenderThreadSingleContextWindowManager race condition

    XMLWordPrintable

Details

    • Bug
    • Resolution: Done
    • P1: Critical
    • 5.1.0
    • 5.0.0, 5.0.1
    • None
    • Gentoo i686. Reproduced on both Qt 5.0.0 and 5.0.1
    • f56829662308e84a578a3816cc054538f7d91e44

    Description

      We have a fairly complex Qt application where render thread occasionally goes to sleep and never wakes up (in Qt 5.0.0, this is in qquickthreadedwindowmanager.cpp:525). Inspecting it in gdb reveals that isRenderBlocked == false, despite the render thread having just set it to true right before it calls wait().

      I believe this is the result of using a number of bit field variables without thread synchronization.

      QQuickRenderThreadContextWindow declares a number of true/false variables as bitfields instead of plain old integers. e.g., this:

      uint animationRunning: 1;
      uint isPostingSyncEvent : 1;
      uint isRenderBlocked : 1;
      uint isExternalUpdatePending : 1;
      uint syncAlreadyHappened : 1;
      uint inSync : 1;
      uint shouldExit : 1;
      uint hasExited : 1;
      uint isDeferredUpdatePosted : 1;
      

      instead of this:

      uint animationRunning;
      uint isPostingSyncEvent;
      uint isRenderBlocked;
      uint isExternalUpdatePending;
      uint syncAlreadyHappened;
      uint inSync;
      uint shouldExit;
      uint hasExited;
      uint isDeferredUpdatePosted;
      

      Additionally, the class itself is accessed from two threads: the GUI thread and the internally managed render thread. A number of the bit field variables are read and written from both threads without being locked. For normal word-sized and word-aligned aligned variables, that's fine and thread safe. However, because the variables are declared as bit fields, writes are no longer atomic operations since a single word can be used to store up to 32 of those bit field variables (i.e., all of them). Setting a single bit variable requires setting all of them at once. That means if one thread does this:

      isRenderBlocked = true;
      

      and then another thread almost simultaneously sets any of the others, e.g.:

      isDeferredUpdatePosted = true;
      isExternalUpdatePending = true;
      syncAlreadyHappened = false;
      

      then it is possible that the first writing of isRenderBlocked gets wiped out by the second because the threads are not synchronized. I believe this is happening, and is causing the render thread to block forever on a condition variable and not wake up.

      I've attached a simple C++ program that demonstrates how using bit fields messes with thread safety in an unintuitive way.

      The fix that works for us is to change a bunch of the instance variables to be normal uint and not bit fields. Patch is attached.

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          People

            sletta Gunnar Sletta
            ashuang Albert Huang
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews

                There are no open Gerrit changes