[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    RE: WIN_CritSection problem
From:       "Francois Boisvert" <francois () macadamian ! com>
Date:       1999-04-29 18:15:18
[Download RAW message or body]

Hi Andreas!

> The problem is that without this patch the window structure remains locked
> during the window proc call.
> As several InstallShield installers do a DispatchMessage16() which calls
> a huge part of code (in fact 10000 relay lines until a wnd crst deadlock),
> this huge code of course tries to use window locking, which fails.
> But the problem with the patch is that it exposes a "raw", i.e. unlocked,
> winproc variable which might get changed again by other threads
> even before the CallWindowProc16() is issued.
>
> So I'm not sure at all if this fix is valid or if I found The Right Thing
> at all.
> But I guess it is valid...
>

Your fix is not valid because the windows lock is always suspended before
any call to a window procedure. Check WINPROC_CallWndProc (in
windows/winproc.c) and CALLBACK_CallWndProc (in misc/callback.c) for further
explications. The problem probably happens a lot earlier in the program
execution and it's very hard to tell where/when.

That's why I've attached a patch that contains a couple of debugging
functions that are able to print out all threads WIN_CritSection activities.
Just apply the patch WinLockDebug.diff and run your test program without
any -debugmsg. Make sure to redirect the output in a file because it can get
really big and you wont be able to see a thing in your shell.

If you find nothing, send me the output file and I'll see what I can do.

Francois


Email : francois@macadamian.com
____________________________________________

Macadamian Technologies Inc.
700, Industrial Avenue
suite 220
Ottawa, ON, K1G 0Y9
http://www.macadamian.com
____________________________________________

["WinLockDebug.diff" (text/plain)]

Index: windows/win.c
===================================================================
RCS file: /home/wine/wine/windows/win.c,v
retrieving revision 1.50
diff -u -w -r1.50 win.c
--- windows/win.c	1999/04/19 14:56:45	1.50
+++ windows/win.c	1999/04/23 13:48:36
@@ -34,6 +34,8 @@
 #include "local.h"
 #include "desktop.h"
 
+#include "debugger.h"
+
 DECLARE_DEBUG_CHANNEL(msg)
 DECLARE_DEBUG_CHANNEL(win)
 
@@ -51,6 +53,92 @@
 
 /* thread safeness */
 static CRITICAL_SECTION WIN_CritSection;
+static DWORD ThreadArray[20];
+static int ThreadMarker = 0;
+typedef struct
+{
+    DWORD bp;
+    DWORD ip;
+}MYFRAME;
+
+#define GET_ESP(res) __asm__( "movl %%esp,%0" : "=r" (res))
+#define GET_EBP(res) __asm__( "movl %%ebp,%0" : "=r" (res))
+
+//#define WIN_DEBUG TRUE
+
+int WIN_GetRecCount()
+{
+    return WIN_CritSection.RecursionCount;
+}
+
+void PrintCaller(int back)
+{
+    DBG_ADDR addr;
+    DWORD mystack;
+    MYFRAME *caller;
+    int i=0;
+    
+    GET_EBP(mystack);
+    caller = (MYFRAME *)mystack;
+    for(;i<back;i++)
+    {
+        caller = (MYFRAME *)caller->bp;
+    }
+
+    
+    addr.seg = 0;
+    addr.off = caller->ip;
+
+    DEBUG_PrintAddress(&addr,32,TRUE);
+    
+}
+
+void CheckThread(DWORD ThreadId)
+{
+    int i = 0;
+    for(;i<ThreadMarker;i++)
+    {
+        if(ThreadId == ThreadArray[i])
+            return;
+    }
+    ThreadArray[ThreadMarker] = ThreadId;
+    ThreadMarker++;
+
+}
+void PrintThreadAction(int Action)
+{
+
+    DWORD ThreadId = GetCurrentThreadId();
+    
+    int i = 0;
+    for(;i<ThreadMarker;i++)
+    {
+        if(ThreadArray[i] == ThreadId)
+            break;
+        printf("--------");
+    }
+    printf(" %lx : ",ThreadId);
+    switch(Action)
+    {
+    case 1: printf("LOCKED   %lx   ",WIN_CritSection.RecursionCount);
+    for(i=0;i<WIN_CritSection.RecursionCount;i++)
+        printf("  ");
+    PrintCaller(3);
+    break;
+    case 2: printf("UNLOCKED   %lx ",WIN_CritSection.RecursionCount);
+    for(i=0;i<WIN_CritSection.RecursionCount;i++)
+        printf("  ");
+    PrintCaller(3);
+    break;
+    case 3: printf("SUSPEND      ");
+    break;
+    case 4: printf("RESTORE      ");
+    break;
+    }
+    
+    printf(" -> CS thread %lx\n",WIN_CritSection.OwningThread);
+
+}
 
 /***********************************************************************
  *           WIN_LockWnds
@@ -59,7 +147,13 @@
  */
 void WIN_LockWnds()
 {
+/*
+     This code will be released in the future
+     info : francois@macadamian.com
+*/
     EnterCriticalSection(&WIN_CritSection);
+    CheckThread(GetCurrentThreadId());
+    PrintThreadAction(1);
 }
 
 /***********************************************************************
@@ -69,7 +163,14 @@
  */
 void WIN_UnlockWnds()
     {
+/*
+     This code will be released in the future
+     info : francois@macadamian.com
+*/
+    CheckThread(GetCurrentThreadId());
+    PrintThreadAction(2);
         LeaveCriticalSection(&WIN_CritSection);
+
 }
 /***********************************************************************
  *           WIN_SuspendWndsLock
@@ -94,8 +195,10 @@
      so the owning thread will be able to leave it */
     WIN_CritSection.RecursionCount = 1;
     /* leave critical section*/
-    WIN_UnlockWnds();
-
+    //WIN_UnlockWnds();
+    CheckThread(GetCurrentThreadId());
+    PrintThreadAction(3);
+    LeaveCriticalSection(&WIN_CritSection);
     return isuspendedLocks;
 }
 
@@ -111,10 +214,13 @@
         return;
     }
     /* restore the lock */
-    WIN_LockWnds();
+    //WIN_LockWnds();
+    EnterCriticalSection(&WIN_CritSection);
     /* set the recursion count of the critical section to the
      value of suspended locks (given by WIN_SuspendWndsLock())*/
     WIN_CritSection.RecursionCount = ipreviousLocks;
+    CheckThread(GetCurrentThreadId());
+    PrintThreadAction(4);
 
 }
 
@@ -313,7 +419,6 @@
 BOOL WIN_UnlinkWindow( HWND hwnd )
 {    
     WND *wndPtr, **ppWnd;
-    BOOL ret = FALSE;
 
     if (!(wndPtr = WIN_FindWndPtr( hwnd ))) return FALSE;
     else if(!wndPtr->parent)
@@ -321,16 +426,11 @@
         WIN_ReleaseWndPtr(wndPtr);
         return FALSE;
     }
-
     ppWnd = &wndPtr->parent->child;
-    while (*ppWnd && *ppWnd != wndPtr) ppWnd = &(*ppWnd)->next;
-    if (*ppWnd)
-    {
+    while (*ppWnd != wndPtr) ppWnd = &(*ppWnd)->next;
         *ppWnd = wndPtr->next;
-        ret = TRUE;
-    }
     WIN_ReleaseWndPtr(wndPtr);
-    return ret;
+    return TRUE;
 }
 
 
@@ -608,6 +708,7 @@
     /* Initialisation of the critical section for thread safeness */
     InitializeCriticalSection(&WIN_CritSection);
     MakeCriticalSectionGlobal(&WIN_CritSection);
+    DEBUG_ReadSymbolTable("wine.sym");
 
     if (!ICONTITLE_Init() ||
 	!WINPOS_CreateInternalPosAtom() ||

=========================================================================


[prev in list] [next in list] [prev in thread] [next in thread]