Deadlock in SCROLL_LoadBitmaps() when another thread exits

[Adam Gundy <arg@cyberscience.com>]

running WINE under valgrind causes it to deadlock in SCROLL_LoadBitmaps()
if another thread is exiting at the same time (valgrind tends to find deadlocks
because things happen much more slowly).

the problem is caused by:

1) SCROLL_GetScrollBarInfo() calling SCROLL_LoadBitmaps() while still holding
   a window pointer (and hence the USER_SysLevel lock).

2) The exiting thread already holds the loader_section lock since it is modifying
   loader details.

3) SCROLL_LoadBitmaps() indirectly attempts to get the loader_section lock so it can
   fetch the bitmaps, but waits because the other thread already has it.

4) thread_detach() indirectly tries to get the USER_SysLevel lock so it can clean
   up windows that the thread owns, but waits because the other thread already has it.

result: deadlock.

I have attached a patch which stops SCROLL_LoadBitmaps() from holding the USER_SysLevel
lock while loading the bitmaps. I have also attached the trace which shows the deadlock.


Seeya,
 Adam
--
Real Programmers don't comment their code. If it was hard to write,
it should be hard to read, and even harder to modify.
These are all my own opinions.

Index: controls/scroll.c
===================================================================
RCS file: /home/wine/wine/controls/scroll.c,v
retrieving revision 1.70
diff -u -r1.70 scroll.c
--- controls/scroll.c	17 Sep 2003 04:28:28 -0000	1.70
+++ controls/scroll.c	15 Oct 2003 12:54:51 -0000
@@ -42,6 +42,14 @@
     UINT  flags;    /* EnableScrollBar flags */
 } SCROLLBAR_INFO, *LPSCROLLBAR_INFO;
 
+static CRITICAL_SECTION scroll_section;
+static CRITICAL_SECTION_DEBUG critsect_debug =
+{
+    0, 0, &scroll_section,
+    { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList },
+      0, 0, { 0, (DWORD)(__FILE__ ": scroll_section") }
+};
+static CRITICAL_SECTION scroll_section = { &critsect_debug, -1, 0, 0, 0, 0 };
 
 static HBITMAP hUpArrow;
 static HBITMAP hDnArrow;
@@ -159,18 +167,23 @@
  */
 static void SCROLL_LoadBitmaps(void)
 {
-    hUpArrow  = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_UPARROW) );
-    hDnArrow  = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_DNARROW) );
-    hLfArrow  = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_LFARROW) );
-    hRgArrow  = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_RGARROW) );
-    hUpArrowD = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_UPARROWD) );
-    hDnArrowD = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_DNARROWD) );
-    hLfArrowD = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_LFARROWD) );
-    hRgArrowD = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_RGARROWD) );
-    hUpArrowI = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_UPARROWI) );
-    hDnArrowI = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_DNARROWI) );
-    hLfArrowI = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_LFARROWI) );
-    hRgArrowI = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_RGARROWI) );
+    EnterCriticalSection( &scroll_section );
+    if (!hUpArrow)
+    {
+       hUpArrow  = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_UPARROW) );
+       hDnArrow  = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_DNARROW) );
+       hLfArrow  = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_LFARROW) );
+       hRgArrow  = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_RGARROW) );
+       hUpArrowD = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_UPARROWD) );
+       hDnArrowD = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_DNARROWD) );
+       hLfArrowD = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_LFARROWD) );
+       hRgArrowD = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_RGARROWD) );
+       hUpArrowI = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_UPARROWI) );
+       hDnArrowI = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_DNARROWI) );
+       hLfArrowI = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_LFARROWI) );
+       hRgArrowI = LoadBitmapA( 0, MAKEINTRESOURCEA(OBM_RGARROWI) );
+    }
+    LeaveCriticalSection( &scroll_section );
 }
 
 
@@ -203,9 +216,13 @@
             if (nBar == SB_HORZ) wndPtr->pHScroll = infoPtr;
             else wndPtr->pVScroll = infoPtr;
         }
-        if (!hUpArrow) SCROLL_LoadBitmaps();
+        WIN_ReleaseWndPtr( wndPtr );
+        SCROLL_LoadBitmaps();
+    }
+    else
+    {
+       WIN_ReleaseWndPtr( wndPtr );
     }
-    WIN_ReleaseWndPtr( wndPtr );
     return infoPtr;
 }
 
@@ -1340,7 +1357,7 @@
     {
     case WM_CREATE:
         SCROLL_CreateScrollBar(hwnd, (LPCREATESTRUCTW)lParam);
-        if (!hUpArrow) SCROLL_LoadBitmaps();
+        SCROLL_LoadBitmaps();
         break;
 
     case WM_ENABLE:

Attachment: tt
Description: Binary data