Re: [PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 3)

[Dmitry Timoshkov]

Christian Costa  wrote:
> BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt )
>  {
> - DWORD error = GetLastError();
> -
> - if (!hwnd)
> - {
> -
>    SetLastError( ERROR_INVALID_WINDOW_HANDLE );
> -
>    return FALSE;
> - }
> -
> - SetLastError( 0xdeadbeef );
> MapWindowPoints( hwnd, 0, lppnt, 1 );
> -
> - if (GetLastError() != 0xdeadbeef)
> -
>    return FALSE;
> -
> - SetLastError(error);
> return TRUE;
>
> }

As been said before these games with saving/restoring last error value
are broken.

[PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 3)

[Christian Costa]

2012/10/24 Dmitry Timoshkov
> Christian Costa  wrote:
> > BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt )
> >  {
> > - DWORD error = GetLastError();
> > -
> > - if (!hwnd)
> > - {
> > -
> >    SetLastError( ERROR_INVALID_WINDOW_HANDLE );
> > -
> >    return FALSE;
> > - }
> > -
> > - SetLastError( 0xdeadbeef );
> > MapWindowPoints( hwnd, 0, lppnt, 1 );
> > -
> > - if (GetLastError() != 0xdeadbeef)
> > -
> >    return FALSE;
> > -
> > - SetLastError(error);
> > return TRUE;
> >
> > }
>
> As been said before these games with saving/restoring last error value
> are broken.
>
> Last time you said wrong so what do you mean by wrong or broken?

The only way to know if MapWindowPoints fails is to set last error first
and check it
after. If I don't restore the previous value, the tests will not pass. Of
course I can
arrange the tests but hey. And I can check windows handle here as Alexandre
said.

[PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 3)

[Nikolay Sivov]

On 10/24/2012 11:33, Christian Costa wrote:
> 2012/10/24 Dmitry Timoshkov <[e-mail] >
> Christian Costa <[e-mail]
> <mailto:[e-mail]>> wrote:
>
> >  BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt )
> >  {
> > +    DWORD error = GetLastError();
> > +
> > +    if (!hwnd)
> > +    {
> > +        SetLastError( ERROR_INVALID_WINDOW_HANDLE );
> > +        return FALSE;
> > +    }
> > +
> > +    SetLastError( 0xdeadbeef );
> >      MapWindowPoints( hwnd, 0, lppnt, 1 );
> > +
> > +    if (GetLastError() != 0xdeadbeef)
> > +        return FALSE;
> > +
> > +    SetLastError(error);
> >      return TRUE;
> >  }
>
> As been said before these games with saving/restoring last error value
> are broken.
>
>
> Last time you said wrong so what do you mean by wrong or broken?
> The only way to know if MapWindowPoints fails is to set last error 
> first and check it
> after. If I don't restore the previous value, the tests will not pass. 
> Of course I can
> arrange the tests but hey. And I can check windows handle here as 
> Alexandre said.

If error reporting from MapWindowPoints is not sufficient to use it 
somewhere else you can always move its guts to
a separate function and report failure as you want. By the way, it only 
fails on invalid window handles I guess, is that right?
If you need this behaviour you have a failing WIN_GetPtr() in this case. 
Messing with last error is not a solution obviously,
and I don't see where MapWindowPoints even sets it.

[PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 3)

[Christian Costa]

2012/10/24 Nikolay Sivov
> On 10/24/2012 11:33, Christian Costa wrote:
>
> 2012/10/24 Dmitry Timoshkov
> > Christian Costa  wrote:
> > > BOOL WINAPI ClientToScreen( HWND hwnd, LPPOINT lppnt )
> > >  {
> > > - DWORD error = GetLastError();
> > > -
> > > - if (!hwnd)
> > > - {
> > > -
> > >    SetLastError( ERROR_INVALID_WINDOW_HANDLE );
> > > -
> > >    return FALSE;
> > > - }
> > > -
> > > - SetLastError( 0xdeadbeef );
> > > MapWindowPoints( hwnd, 0, lppnt, 1 );
> > > -
> > > - if (GetLastError() != 0xdeadbeef)
> > > -
> > >    return FALSE;
> > > -
> > > - SetLastError(error);
> > > return TRUE;
> > >
> > > }
> >
> > As been said before these games with saving/restoring last error value
> > are broken.
> >
> > Last time you said wrong so what do you mean by wrong or broken?
>
> The only way to know if MapWindowPoints fails is to set last error first
> and check it
> after. If I don't restore the previous value, the tests will not pass. Of
> course I can
> arrange the tests but hey. And I can check windows handle here as
> Alexandre said.
>
> If error reporting from MapWindowPoints is not sufficient to use it
> somewhere else you can always move its guts to
> a separate function and report failure as you want. By the way, it only
> fails on invalid window handles I guess, is that right?
> If you need this behaviour you have a failing WIN_GetPtr() in this case.
> Messing with last error is not a solution obviously,
> and I don't see where MapWindowPoints even sets it.
>
> Sounds good. I'll do that. Basically WINPOS_GetWinOffset get most part the

job in our case and I already modified it to return an error.
Yes it fails only for invalid handles. WIN_GetPtr is not enough because it
just return WND_OTHER_PROCESS to call the server to ask
if it exists on another process. It's what IsWindow does
(and WINPOS_GetWinOffset).
MapWindowPoints can fail (see test). MSDN says to call SetLastError first
before calling it and read the value.
I will send a patch using WINPOS_GetWinOffset. Thanks.