June 2011 - Wine-Devel - list.winehq.org

Re: [1/7] server: Implement RegisterHotKey/UnregisterHotKey.
by Alexandre Julliard June 17, 2011

June 17, 2011

"Vincent Povirk" <vincent(a)codeweavers.com> writes: > @@ -2632,6 +2632,29 @@ enum coords_relative > #define SET_USER_OBJECT_FLAGS 1 > > > +/* Register a hotkey */ > +(a)REQ(register_hotkey) > + user_handle_t window; /* handle to the window */ > + int id; /* hotkey identifier */ > + int flags; /* modifier keys */ > + int vkey; /* virtual key code */ > +(a)REPLY > + int replaced; /* did we replace an existing hotkey? */ > + int flags; /* flags of replaced hotkey */ > + int vkey; /* virtual key code of replaced hotkey */ > +(a)END The docs say that hotkeys are no longer replaced in recent Windows versions. It would be a good idea to have a test for that. -- Alexandre Julliard julliard(a)winehq.org

2 1

dead-lock free design
by Joerg-Cyril.Hoehle＠t-systems.com June 17, 2011

June 17, 2011

Hi, there are two guidelines that I identified while trying to prevent dead-locks with critical sections and access to uninitialiased memory in Wine. My latest patch shows both applications. http://www.winehq.org/pipermail/wine-patches/2011-June/103247.html 1. In callbacks, detect when it should not run anymore and exit *early*. void CALLBACK mycallback (void* user) { struct mycbdata * mycb = user; EnterCriticalSection(mycb->lock); if(!mycb->started || mycb->shutdown) { LeaveCriticalSection; return; } /* else do actual work */ This is best done after EnterCS, because the critical section is the most likely place where the callback thread will have been preempted. Note that this doesn't prevent mycb from pointing to free'ed memory. You may only deallocate resources once you've made sure that the callback cannot be invoked any more. 2. Use two phases to end cleanly. Delegate part of the work to a later stage. Pause is not Stop. Stop is not Close. Close is not Shutdown. Shutdown is not DLL Unload. My current patch applies this idea partially. DeleteTimerQueueTimer() is called within a CS, whereas Wait() is called afterwards (which is safe because the mmdevapi object is not used any more). The original deadlock happened because DeleteTimerQueue and Wait were combined in one call, within the CS. Waiting for completion is *essential* with the DeleteTimerQueueTimer API because that's the only means to ensure that callbacks are done. Then only can you free the memory that mycb above points to. A variation is to partition the work among AudioClient_Stop and AudioClient_Release. In Stop() call DeleteTimerQueueTimer() but Wait for completion of the callbacks in Release(). Alternatively, in Stop() use ChangeTimerQueueTimer(DueTime=INFINITE) to prevent firing new callbacks and use DeleteTimerQueue in Release(). I didn't write my patch that way yet because it requires some attention in AudioClient_Start() when restarting the timer callbacks. Why does the rule work? Functions likes Play/Pause/Stop typically prevent concurrent access (either from the app or from internal callback threads) using critical sections. At Close/Release time, the typical assumption is that there's no more concurrency -- at least from the side of the application. Partitioning the work allows for plenty of new possibilities. E.g. one known bug in MS-Windows is that MCI/winmm's Stop() may take a lot of time, freezing the application. However, if Stop() simply sends a stop signal to the player thread, without waiting for the thread to actually stop, it can be plenty fast. Likewise, if mmdevapi's AudioClient_Stop() simply sets This->started to FALSE, the callback will still be ticking, but not disturb. Fast too if it calls DeleteTimerQueueTimer() but delegates the Wait to AudioClient_Release(). One drawback is that resources will be allocated for longer than with the naïve approach. Sometimes, the partitioning decision depends on what state you want to guarantee after Pause, Stop or Close. This is rarely documented. E.g. for MCI's audio player you want to make sure that an immediately following Open won't fail when Wine's winmm ALSA or OSS drivers only support opening one device at a time. Regards, Jörg Höhle

1 0

dead link on directdraw wiki to directdrawrenderer
by Jarek Czekalski June 17, 2011

June 17, 2011

There's a dead link on the page http://wiki.winehq.org/DirectDraw pointing to http://wiki.winehq.org/DirectDrawRenderer as DirectDrawRenderer. Is there any better place for such reports?

2 1

Re: urlmon: Fix incorrect pointer arithmetic (too conversative) in map_url_to_zone. (RESEND^2)
by Gerald Pfeifer June 17, 2011

June 17, 2011

A friendly soul pointed out to me that I had missed three(!) responses to my patch which went to wine-devel(a). Sorry about that, I'll see to set up some mail filters to prevent that from happening. For the record, somehow I got mislead re the semantics of sizeof, and that after some 25 years of programming C. :-( Sorry! Gerald On Wed, 15 Jun 2011, Gerald Pfeifer wrote: > Anything wrong with this patch? The original code surely looks wrong? > > The difference between two pointers (of the same type) is the number > of elements, not the number of bytes. Thus the code below was way > incorrect, luckily only too conversative. > > Gerald > > --- > dlls/urlmon/sec_mgr.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/dlls/urlmon/sec_mgr.c b/dlls/urlmon/sec_mgr.c > index 7b4bb35..75850ee 100644 > --- a/dlls/urlmon/sec_mgr.c > +++ b/dlls/urlmon/sec_mgr.c > @@ -529,7 +529,7 @@ static HRESULT map_url_to_zone(LPCWSTR url, DWORD *zone, LPWSTR *ret_url) > hres = CoInternetParseUrl(secur_url, PARSE_PATH_FROM_URL, 0, path, > sizeof(path)/sizeof(WCHAR), &size, 0); > > - if(SUCCEEDED(hres) && (ptr = strchrW(path, '\\')) && ptr-path < sizeof(root)/sizeof(WCHAR)) { > + if(SUCCEEDED(hres) && (ptr = strchrW(path, '\\')) && ptr-path < sizeof(root)) { > UINT type; > > memcpy(root, path, (ptr-path)*sizeof(WCHAR)); >

1 0

GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9
by Michael Mc Donnell June 17, 2011

June 17, 2011

Thanks for the comments Matteo! I have a few questions below. On Mon, May 30, 2011 at 2:45 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: > 2011/5/29 Michael Mc Donnell <michael(a)mcdonnell.dk>: >> On Sat, May 28, 2011 at 10:08 PM, Stefan Dösinger >> <stefandoesinger(a)gmx.at> wrote: >>> On Saturday 28 May 2011 19:55:55 you wrote: >>> >>>> It was implemented by setting the vertex declaration to null when the >>>> declaration is invalid. That seems to match what happens on Windows, >>>> because if a program has set an invalid declaration and then calls >>>> GetDeclaration or DrawSubset it will fail with an access violation. >>>> Should Wine also fail with an access violation or should it be >>>> slightly more graceful and return E_FAIL instead (which my new patch >>>> does)? >>> Yes, I think returning E_FAIL is a good idea, but write a WARN if >>> CreateVertexDeclaration fails or when you catch a NULL vdecl. A WARN will not >>> be shown by default(unlike a FIXME), it is used when the app does something >>> fishy but you think you handled it correctly. >>> >>>> + if (FAILED(hr)) >>>> + new_vertex_declaration = NULL; >>>> + >>>> + /* Free old vertex declaration */ >>>> + if (This->vertex_declaration) >>>> + IDirect3DVertexDeclaration9_Release(This->vertex_declaration); >>> In this situation you can just release the old decl before calling >>> CreateVertexDeclaration and pass &This->vertex_declaration to CreateVdecl. >>> That simplified the code a bit. Don't forget to set This->vertex_declaration to >>> NULL in case of an error though >> >> Ok I've simplified the code. I've also inserted three WARNs. I've >> skipped a WARN when freeing the mesh because that works fine on >> Windows with an invalid declaration. >> >>>> + /* Set the position type to color instead of float3 (invalid >>> declaration) */ >>>> + hr = mesh->lpVtbl->UpdateSemantics(mesh, declaration_wrong_type_color); >>>> + todo_wine ok(hr == D3D_OK, "Test UpdateSematics wrong type color, " >>>> + "got %#x expected D3D_OK\n", hr); >>> Is this really invalid? I don't know it on top of my head, but in general >>> types and usage are independent, especially with shaders. I don't see a check >>> in d3d9 or wined3d for this case. >> >> You're right it isn't invalid. It doesn't crash, it just doesn't draw >> anything. I've change that comment and re-ordered the tests. >> >>> Beyond that I think the patches look OK. I'd recommend to send them to wine- >>> devel one more time and then next week to wine-patches. Henri is on Vacation >>> this week, and he's usually more picky than I am. I also hope some of the >>> people who regularly contribute to d3dx9 will have a look and comment. >> >> Ok, I've attached the newest version. I'll let the patches sit here >> for a week, and start working on the other functions in the mean time. >> > > The patches are OK with me, just some nitpicking: That sounds great :-) > + testContext = HeapAlloc(GetProcessHeap(), 0, sizeof(struct TestContext)); > > We usually use something like HeapAlloc(GetProcessHeap(), 0, > sizeof(*testContext)); instead. Ok, I guess that's better in case the type of the variable changes at some point? > I think you can remove some obvious comments (like /* Create the test > mesh */). Also, check your whitespaces. Is it too much or too little whitespace? Could you give me an example? Thanks!

5 13

Re: [2/2] imm32: Factor out the message checks from ImmIsUIMessageA/W and fix them
by Marvin June 16, 2011

June 16, 2011

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=11778 Your paranoid android. === WINEBUILD (build) === Patch failed to apply

2 1

app freezes during WM_KILLFOCUS
by Eric Frias June 16, 2011

June 16, 2011

I've been chasing a bug for a few days, and finally have a good idea of what is going on. When a program brings up a modal dialog box in response to a WM_KILLFOCUS message, the program will freeze. I've attached a simple test case, all boilerplate except for: case WM_KILLFOCUS: MessageBox(hWnd, _T("Killing Focus"), _T("Note"), MB_OK); in the main event loop. If you run the program and then give focus to another application, the messagebox will pop up and then the program will become unresponsive. What is happening is: - x11drv gets a FocusOut event and calls the event handler, which ends up SendMessage-ing a WM_KILLFOCUS to the program. - The programs's code creates a modal dialog like the MessageBox above, and enters the dialog's event loop, which winds up calling GetMessage which calls MsgWaitForMultipleObjectsEx to get more X11 events. - X11DRV_MsgWaitForMultipleObjectsEx executes this: if (data->current_event) mask = 0; /* don't process nested events */ where data->current_event is true since we're still handling the FocusOut event. Since we're not processing nested events, we'll never be able to process the usual events like keys or mouse events which could exit this nested event loop. This type of error occurs in a few places in our application, where a WM_KILLFOCUS message to an edit box triggers some input validation and displays an error on failure. This works correctly in the normal case, where the user tabs out or clicks on another control in the dialog box containing the edit control -- since everything stays in the same top-level window, there is no FocusOut event from X11 and the WM_KILLFOCUS is generated internally by wine in response to, say, a KeyEvent. I think these key or mouse events are just posted to the windows message queue, and the X11 event handler has already exited by the time the application gets around to processing the ensuing WM_KILLFOCUS. But in the case where focus is lost by clicking outside the dialog containing the edit control, the WM_KILLFOCUS is generated by the FocusOut handler which is still executing, and we lock up. I suspect this is related to bug 11595 "Notepad++ freezes if native application changes a file it has open (dogfood)". My guess is in that case, a FocusIn event triggers something like a WM_ACTIVATE, in which notepad++ notices a file is changed and displays a messagebox asking if you want to reload. Since wine is still processing the FocusIn event, the modal message loop's call to MsgWaitForMultipleObjectsEx will never process any useful events because it's a nested event. Our application's built-in text editor has this same problem. I don't understand the event handling code well enough to propose a solution. As a test, I allowed nested event processing and it solved the problem without introducing any obvious errors, but I'm sure that check was in there for a reason. It's probably better to keep the application code from being executed from inside the event handler, if that's possible. Eric

2 2

Re: [PATCH 1/6] server: Add server-side raw input functions definitions
by Vitaliy Margolen June 16, 2011

June 16, 2011

On 06/14/2011 02:45 AM, Vincas Miliūnas wrote: > This is a partial raw input implementation for mouse and keyboard. > > I've read in the IRC chat, that products of tools/make_request should > not be included in a patch, so I left them out. > > My first attempts were by leeching input events from queue_mouse_message and queue_keyboard_message functions, but I couldn't get mouse cursor movement right. Instead, I copied the way dinput receives input - by installing low level hooks and forwarding input to the server. This is just to get "try 1" out of the door. Of course, it would be better if the input feed would be server-side only. > > I hit a very weird bug, where in peek_message function, info.msg.lParam was impossible to set with the proper lParam (it was correct in reply->lparam) value, instead is was overwritten by some constant. That was happening only for the messages sent by the patch using post_message. I guess the tales of server-side dragons hold some truth... If anyone wants to try to reproduce it, just recompile without the unused #include "user.h" in raw_input.c and run user32 tests. > > This patch game-tested and works for Quake Live (in_mouse 2 for raw input), World of Tanks and Risen. I'm not sure I like your approach and that AJ would like it either. Raw input is an "override" of regular messages caused by mouse/keyboard input. Wine server already receives these messages from x11drv (see __wine_send_input & co). When applications requests raw input events you need to tell wine server if it needs to send regular events, or ignore all events, and produce raw input events instead. Vitaliy.

2 3

Re: [3/3] d3dx9/tests: Implemented tests for ID3DXLine_GetDevice.
by Matteo Bruni June 16, 2011

June 16, 2011

2011/6/16 Charles Welton <charleswfb(a)gmail.com>: > > + IDirect3DDevice9_Release(return_device); > + ID3DXLine_Release(line); You could add a couple of ok() checks for the values returned by Release, to ensure nothing strange is happening. Btw, thanks for contributing. :)

1 0

Re: [1/3] d3dx9/line: Added device pointer to ID3DXLineImpl and initialize it.
by Matteo Bruni June 16, 2011

June 16, 2011

2011/6/16 Charles Welton <charleswfb(a)gmail.com>: > > @@ -27,6 +27,8 @@ static const struct ID3DXLineVtbl ID3DXLine_Vtbl; > typedef struct ID3DXLineImpl { > ID3DXLine ID3DXLine_iface; > LONG ref; > + > + IDirect3DDevice9 * device; > } ID3DXLineImpl; You also have to Release() the device in ID3DXLineImpl_Release when ref reaches 0. Additional style nitpick, there should be no space between '*' and the field name.

1 0