Re: d3d9: update locked_rect only if wined3d_surface_map succeeded (try 3)

[Nikolay Sivov]

On 10/15/2013 18:57, Lasse Rasinen wrote:
> According to debugging output, Artemis Spaceship Bridge Simulator 2.0
> calls LockRect twice on the same texture (for whatever reason) and crashes.
>
> http://bugs.winehq.org/show_bug.cgi?id=34271
>
> The return values are cleared explicitly --- game is playable.
>
> Now with appropriate assignments and test output.
> ---
>   dlls/d3d9/surface.c      | 9 +++++++--
>   dlls/d3d9/tests/device.c | 4 ++++
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/d3d9/surface.c b/dlls/d3d9/surface.c
> index 99f0b0f..20ccc68 100644
> --- a/dlls/d3d9/surface.c
> +++ b/dlls/d3d9/surface.c
> @@ -293,8 +293,13 @@ static HRESULT WINAPI d3d9_surface_LockRect(IDirect3DSurface9 *iface,
>       hr = wined3d_surface_map(surface->wined3d_surface, &map_desc, rect, flags);
>       wined3d_mutex_unlock();
>
> -    locked_rect->Pitch = map_desc.row_pitch;
> -    locked_rect->pBits = map_desc.data;
> +    if (SUCCEEDED(hr)) {
> +        locked_rect->Pitch = map_desc.row_pitch;
> +        locked_rect->pBits = map_desc.data;
> +    } else {
> +        locked_rect->Pitch = 0;
> +        locked_rect->pBits = NULL;
> +    }
>
> return hr;
>   }
> diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c
> index 02b4174..90d5c38 100644
> --- a/dlls/d3d9/tests/device.c
> +++ b/dlls/d3d9/tests/device.c
> @@ -5404,6 +5404,10 @@ static void test_lockrect_invalid(void)
>       ok(SUCCEEDED(hr), "Failed to lock surface with rect NULL, hr %#x.\n", hr);
>       hr = IDirect3DSurface9_LockRect(surface, &locked_rect, NULL, 0);
>       ok(hr == D3DERR_INVALIDCALL, "Got unexpected hr %#x.\n", hr);
> +    locked_rect.pBits = (BYTE *)0xdeadbeef;
> +    hr = IDirect3DSurface9_LockRect(surface, &locked_rect, NULL, 0);
> +    ok(!SUCCEEDED(hr) && locked_rect.pBits == NULL, "Failed to clear pBits (%p)\n",
> +            locked_rect.pBits);
>       hr = IDirect3DSurface9_UnlockRect(surface);
>       ok(SUCCEEDED(hr), "Failed to unlock surface, hr %#x.\n", hr);

It looks like you could use existing LockRect() call that's already 
tested as failing. Also I think FAILED() is a preferred way to
test HRESULT value for failure, but as existing code does - you could 
also test for exact failure code.

d3d9: update locked_rect only if wined3d_surface_map succeeded (try 3)

[Henri Verbeet]

On 15 October 2013 18:10, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
> > diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c
> > index 02b4174..90d5c38 100644
> > --- a/dlls/d3d9/tests/device.c
> > +++ b/dlls/d3d9/tests/device.c
> > @@ -5404,6 +5404,10 @@ static void test_lockrect_invalid(void)
> >       ok(SUCCEEDED(hr), "Failed to lock surface with rect NULL, hr
> > %#x.\n", hr);
> >       hr = IDirect3DSurface9_LockRect(surface, &locked_rect, NULL, 0);
> >       ok(hr == D3DERR_INVALIDCALL, "Got unexpected hr %#x.\n", hr);
> > +    locked_rect.pBits = (BYTE *)0xdeadbeef;
> > +    hr = IDirect3DSurface9_LockRect(surface, &locked_rect, NULL, 0);
> > +    ok(!SUCCEEDED(hr) && locked_rect.pBits == NULL, "Failed to clear
> > pBits (%p)\n",
> > +            locked_rect.pBits);
> >       hr = IDirect3DSurface9_UnlockRect(surface);
> >       ok(SUCCEEDED(hr), "Failed to unlock surface, hr %#x.\n", hr);
>
> It looks like you could use existing LockRect() call that's already tested
> as failing. Also I think FAILED() is a preferred way to
> test HRESULT value for failure, but as existing code does - you could also
> test for exact failure code.

Yeah, I think it really just needs the intialization of pBits before
the existing LockRect() call, and an extra ok() call after the
existing one.