Cursor patches

[H. Verbeet]

Attached to this mail are a couple of patches that should fix some
issues with mouse cursors. It would be nice if some people could have
a look and see if the patches break anything. Patches 1-4 move cursors
into the server, 5 adds support for Xcursor cursors, 6 & 7 are
cleanups, 8 fixes loading of .cur cursors, 9 adds support for animated
cursors and 10 adds support for 32bpp cursors.

[Robert Shearman]

H. Verbeet wrote:
> Attached to this mail are a couple of patches that should fix some
> issues with mouse cursors. It would be nice if some people could have
> a look and see if the patches break anything. Patches 1-4 move cursors
> into the server, 5 adds support for Xcursor cursors, 6 & 7 are
> cleanups, 8 fixes loading of .cur cursors, 9 adds support for animated
> cursors and 10 adds support for 32bpp cursors.

Overall, I'm very impressed with the patch set.

However, I did find two things that I think need consideration or 
improvement:

From 01_server_cursoricon.diff.txt:
> +/* Destroy a cursor */
> +DECL_HANDLER(destroy_cursor)
> +{
> - unsigned int i;
> - cursor_t *cursor = free_user_handle( req->handle );
> -
> - if (!cursor) return;
> -
> - for (i = 0; i < cursor->num_frames; ++i)
> - {
> -
>    if (cursor->frames[i].bits) free( cursor->frames[i].bits );
> - }
> -
> - free( cursor->frames );
>
> +}

This appears to be the only place where a cursor is destroyed. Thus, a 
bad application could end up wineserver to leak memory. I think you need 
to investigate when Windows frees cursors that have not had 
DestroyCursor called on them. My guess is that it frees them on process 
destruction.

From 09_user_ani_cursor.diff.txt:
> --- a/dlls/user/Makefile.in
> +++ b/dlls/user/Makefile.in
> @@ -6,7 +6,7 @@ VPATH     = @srcdir@
>  MODULE    = user32.dll
>  IMPORTLIB = libuser32.$(IMPLIBEXT)
>  IMPORTS   = gdi32 advapi32 kernel32 ntdll
> -DELAYIMPORTS = imm32
> +DELAYIMPORTS = imm32 winmm
>  EXTRALIBS = $(LIBUNICODE)
>
> SPEC_SRCS16 = \

I'm not sure it is acceptable to import winmm from user32, even if it is 
a delay import.

[Dmitry Timoshkov]

"Robert Shearman"  wrote:
> > -DELAYIMPORTS = imm32
> > +DELAYIMPORTS = imm32 winmm
> >  EXTRALIBS = $(LIBUNICODE)
> >
> > SPEC_SRCS16 = \
>
> I'm not sure it is acceptable to import winmm from user32, even if it is 
> a delay import.

Rob is right, native user32 doesn't import winmm by any means. Looking
at user32 in a viewer it's easy to find chunk names such as RIFF, LIST,
rate, anih although. That means that user32 has an internal implementation
of mmioDescend which should be enough to parse animated cursor files.

[H. Verbeet]

On 07/07/06, Robert Shearman  wrote:
> This appears to be the only place where a cursor is destroyed. Thus, a
> bad application could end up wineserver to leak memory. I think you need
> to investigate when Windows frees cursors that have not had
> DestroyCursor called on them. My guess is that it frees them on process
> destruction.

It does look like that's the case.

On 07/07/06, Dmitry Timoshkov  wrote:
> Rob is right, native user32 doesn't import winmm by any means. Looking
> at user32 in a viewer it's easy to find chunk names such as RIFF, LIST,
> rate, anih although. That means that user32 has an internal implementation
> of mmioDescend which should be enough to parse animated cursor files.

That's a bit unfortunate. I guess that means there's no way around
duplicating a significant part of that code into user32 then?

[H. Verbeet]

On 07/07/06, Robert Shearman  wrote:
> However, I did find two things that I think need consideration or
> improvement:
>
> From 01_server_cursoricon.diff.txt:
> > +/* Destroy a cursor */
> > +DECL_HANDLER(destroy_cursor)
> > +{
> > - unsigned int i;
> > - cursor_t *cursor = free_user_handle( req->handle );
> > -
> > - if (!cursor) return;
> > -
> > - for (i = 0; i < cursor->num_frames; ++i)
> > - {
> > -
> >    if (cursor->frames[i].bits) free( cursor->frames[i].bits );
> > - }
> > -
> > - free( cursor->frames );
> >
> > +}
>
> This appears to be the only place where a cursor is destroyed. Thus, a
> bad application could end up wineserver to leak memory. I think you need
> to investigate when Windows frees cursors that have not had
> DestroyCursor called on them. My guess is that it frees them on process
> destruction.
>
> From 09_user_ani_cursor.diff.txt:
> > --- a/dlls/user/Makefile.in
> > +++ b/dlls/user/Makefile.in
> > @@ -6,7 +6,7 @@ VPATH     = @srcdir@
> >  MODULE    = user32.dll
> >  IMPORTLIB = libuser32.$(IMPLIBEXT)
> >  IMPORTS   = gdi32 advapi32 kernel32 ntdll
> > -DELAYIMPORTS = imm32
> > +DELAYIMPORTS = imm32 winmm
> >  EXTRALIBS = $(LIBUNICODE)
> >
> > SPEC_SRCS16 = \
>
> I'm not sure it is acceptable to import winmm from user32, even if it is
> a delay import.
>
> --
> Rob Shearman

Attached is a slightly updated version of the patchset. I got rid of
the winmm import, and it should now destroy cursors / icons when the
process is destroyed. The changes are in parts 1 & 9, the rest should
be pretty much unchanged.

[Dmitry Timoshkov]

"H. Verbeet"  wrote:
> Attached is a slightly updated version of the patchset. I got rid of
> the winmm import, and it should now destroy cursors / icons when the
> process is destroyed. The changes are in parts 1 & 9, the rest should
> be pretty much unchanged.

I've looked only at 09_user_ani_cursor.diff.txt:

1. CURSORICON_LoadFromFile returns 0 on failure, so load_ani should return 0
on failure as well not INVALID_HANDLE_VALUE (which usually is returned only
for kernel objects).

2. the loop in riff_find_chunk uses memcmp to compare DWORDs, comparing
DWORD values directly is more efficient, and alignment is not a problem
since all the fields and chunks are DWORD aligned. Making ptr of a type
DWORD should help with that, although that makes pointer arithmetic
slightly more tricky, that's still will look more readable IMO.

[Vijay Kiran Kamuju]

Hi,

These are the warnings i get when i compile ur patch
os kubuntu, gcc 4.0.3

cursoricon.c: In function 'CURSORICON_Load':
cursoricon.c:1289: warning: 'frame_bits' may be used uninitialized in
this function
mouse.c: In function 'create_cursor':
mouse.c:566: warning: pointer targets in passing argument 6 of
'XCreateImage' differ in signedness
Wine build complete.

cheers,
VJ

On 7/21/06, Dmitry Timoshkov  wrote:
> "H. Verbeet"  wrote:
> > Attached is a slightly updated version of the patchset. I got rid of
> > the winmm import, and it should now destroy cursors / icons when the
> > process is destroyed. The changes are in parts 1 & 9, the rest should
> > be pretty much unchanged.
>
> I've looked only at 09_user_ani_cursor.diff.txt:
> - CURSORICON_LoadFromFile returns 0 on failure, so load_ani should return 0
>
> on failure as well not INVALID_HANDLE_VALUE (which usually is returned only
> for kernel objects).
> - the loop in riff_find_chunk uses memcmp to compare DWORDs, comparing
>
> DWORD values directly is more efficient, and alignment is not a problem
> since all the fields and chunks are DWORD aligned. Making ptr of a type
> DWORD should help with that, although that makes pointer arithmetic
> slightly more tricky, that's still will look more readable IMO.
>
> --
> Dmitry.

[H. Verbeet]

Another update.
  - The patches don't use INVALID_HANDLE_VALUE anymore (I was using
that in some of the other patches as well).
  - riff_find_chunk() compares DWORDs
  - Code using get_cursor_frame() now has some error handling, in case
the handle is invalid
  - Warnings should be fixed now, although I don't get the second one myself.

[H. Verbeet]

On 25/07/06, H. Verbeet  wrote:
> Another update.

Ignore that. It doesn't actually work.

[H. Verbeet]

On 25/07/06, H. Verbeet  wrote:
> On 25/07/06, H. Verbeet  wrote:
> > Another update.
>
> Ignore that. It doesn't actually work.

In riff_find_chunk():
*((DWORD *)ptr) + 2 == chunk_id
Should be:
*((DWORD *)ptr + 2) == chunk_id