animate control regression

[Krzysztof Foltman]

The current CVS version has a regression in the animate control, causing 
a deadlock, most probably in WM_DESTROY handler, in the app I'm testing 
Wine with.

trace:message:SPY_ExitMessage  (0x10038) L"{SysAnimate32}" message 
[0047] WM_WINDOWPOSCHANGED returned 00000000
trace:message:SPY_EnterMessage (0x10038) L"{SysAnimate32}" message 
[0002] WM_DESTROY sent from self wp=00000000 lp=00000000
err:ntdll:RtlpWaitForCriticalSection section 0x403a3cb8 "?" wait timed 
out in thread 0009, blocked by 000a, retrying (60 sec)

Reversing these two patches seems to fix the problem:
http://cvs.winehq.org/patch.py?id=16654
http://cvs.winehq.org/patch.py?id=16619

I don't know if patch 16654 has any effect on this problem - I don't 
know this code well enough to reverse patches in random order.

Hope it isn't difficult to fix...

Krzysztof

[Dimitrie O. Paun]

On Thu, Mar 17, 2005 at 10:31:24PM +0100, Krzysztof Foltman wrote:
> The current CVS version has a regression in the animate control, causing 
> a deadlock, most probably in WM_DESTROY handler, in the app I'm testing 
> Wine with.

We should just get rid of the thread and the critical section altogether,
comctl32 6.0 is documented not to support it anymore. But before we do
that, let's try to fix this, so we have good code in CVS :)

Here is a patch completely untested (just got home at 1am, and my tree
doesn't currently build due to other problems). Can you try it out?

Index: dlls/comctl32/animate.c
===================================================================
RCS file: /var/cvs/wine/dlls/comctl32/animate.c,v
retrieving revision 1.63
diff -u -r1.63 animate.c
--- dlls/comctl32/animate.c	16 Mar 2005 19:47:52 -0000	1.63
+++ dlls/comctl32/animate.c	18 Mar 2005 06:00:39 -0000
@@ -353,15 +353,12 @@

TRACE("Drawing frame %d (loop %d)\n", infoPtr->currFrame, infoPtr->nLoop);

-    EnterCriticalSection(&infoPtr->cs);
-
     mmioSeek(infoPtr->hMMio, infoPtr->lpIndex[infoPtr->currFrame], SEEK_SET);
     mmioRead(infoPtr->hMMio, infoPtr->indata, infoPtr->ash.dwSuggestedBufferSize);

if (infoPtr->hic &&
    fnIC.fnICDecompress(infoPtr->hic, 0, infoPtr->inbih, infoPtr->indata,
    	     infoPtr->outbih, infoPtr->outdata) != ICERR_OK) {
-	LeaveCriticalSection(&infoPtr->cs);
    WARN("Decompression error\n");
    return FALSE;
     }
@@ -373,13 +370,9 @@

if (infoPtr->currFrame++ >= infoPtr->nToFrame) {
    infoPtr->currFrame = infoPtr->nFromFrame;
-	if (infoPtr->nLoop != -1) {
-	    if (--infoPtr->nLoop == 0) {
-		ANIMATE_DoStop(infoPtr);
-	    }
-	}
+	if (infoPtr->nLoop != -1)
+	    infoPtr->nLoop--;
     }
-    LeaveCriticalSection(&infoPtr->cs);

return TRUE;
 }
@@ -400,6 +393,16 @@
     return 0;
 }

+static LRESULT ANIMATE_Timer(ANIMATE_INFO *infoPtr)
+{
+    ANIMATE_DrawFrame(infoPtr);
+
+    if (infoPtr->nLoop == 0)
+        ANIMATE_DoStop(infoPtr);
+
+    return 0;
+}
+
 static DWORD CALLBACK ANIMATE_AnimationThread(LPVOID ptr_)
 {
     ANIMATE_INFO *infoPtr = (ANIMATE_INFO *)ptr_;
@@ -414,6 +417,9 @@
         event = infoPtr->hStopEvent;
         LeaveCriticalSection(&infoPtr->cs);

+        if (infoPtr->nLoop == 0)
+            ANIMATE_DoStop(infoPtr);
+
         /* time is in microseconds, we should convert it to milliseconds */
         if ((event == 0) || WaitForSingleObject( event, (timeout+500)/1000) == WAIT_OBJECT_0)
             break;
@@ -863,7 +869,6 @@
     return 0;
 }

-
 static LRESULT WINAPI ANIMATE_WindowProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
 {
     ANIMATE_INFO *infoPtr = (ANIMATE_INFO *)GetWindowLongPtrW(hWnd, 0);
@@ -905,7 +910,7 @@
         return ANIMATE_StyleChanged(infoPtr, wParam, (LPSTYLESTRUCT)lParam);

case WM_TIMER:
-        return ANIMATE_DrawFrame(infoPtr);
+        return ANIMATE_Timer(infoPtr);

case WM_PAINT:
         /* the animation isn't playing, or has not decompressed

[Mike McCormack]

Dimitrie O. Paun wrote:
> We should just get rid of the thread and the critical section altogether,
> comctl32 6.0 is documented not to support it anymore. But before we do
> that, let's try to fix this, so we have good code in CVS :)

If you get rid of the thread, programs that don't run a message loop or 
that block for a time after receiving certain messages won't animate 
smoothly.

What we had there worked, and it took quite a while to get rid of the 
races and lockups.  Why can't we just revert to what we had?

Mike

[Dimitrie O. Paun]

On Fri, Mar 18, 2005 at 04:21:05PM +0900, Mike McCormack wrote:
> If you get rid of the thread, programs that don't run a message loop or 
> that block for a time after receiving certain messages won't animate 
> smoothly.

Yeah, I was half joking about removing it. comctl32 6.0 doesn't
run a thread anymore, do you know of many/any programs that depend
on that?
> What we had there worked, and it took quite a while to get rid of the 
> races and lockups.  Why can't we just revert to what we had?

I will, it's just that I can't test the code so it will take maybe
couple of iterations. I'll analize what changed in the locking
today.