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

[Dmitry Timoshkov]

Christian Costa <titan.costa(a)gmail.com> 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.

-- 
Dmitry.

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

[Christian Costa]

2012/10/24 Dmitry Timoshkov <dmitry(a)baikal.ru>
> Christian Costa <titan.costa(a)gmail.com> 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 <dmitry(a)baikal.ru <mailto:dmitry(a)baikal.ru>>
>
> Christian Costa <titan.costa(a)gmail.com
>     <mailto:titan.costa(a)gmail.com>> 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 <bunglehead(a)gmail.com>
> On 10/24/2012 11:33, Christian Costa wrote:
>
> 2012/10/24 Dmitry Timoshkov <dmitry(a)baikal.ru>
> > Christian Costa <titan.costa(a)gmail.com> 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.