Don't unsubclass the surface's output window if the window has been destroyed...

Don't unsubclass the surface's output window if the window has been destroyed.


Committed: http://code.google.com/p/angleproject/source/detail?r=499

[apatrick1, 2010-11-30 21:21:00]

http://codereview.appspot.com/3357041/diff/1/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3357041/diff/1/src/libEGL/Surface.cpp#newcode347
src/libEGL/Surface.cpp:347: // If the window has died unexpectedly, return
rather than explode
How does it explode? It looks to me like all the Win32 calls below will fail
uneventfully and the ASSERT will catch the problem in debug builds. Also, I
think there's a race here. The window could be destroyed at any point after
IsWindow returns true.

[nduca, 2010-11-30 21:38:29]

It should read, "Return rather than assert."

When you forcibly terminate the browser, e.g. shift-F5 in visual studio during a
debug session, the browser process gets killed destroying the window. If you're
not debugging the GPU process, it will perform a shutdown. The assert will
explode not because the window isn't subclassed but rather because it does not
exist at all.

This having been said, I tweaked the proposed patch slightly to avoid leaking
properties.

On 2010/11/30 21:21:00, apatrick1 wrote:

[apatrick1, 2010-11-30 22:29:58]

http://codereview.appspot.com/3357041/diff/6001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3357041/diff/6001/src/libEGL/Surface.cpp#newcod...
src/libEGL/Surface.cpp:353: ASSERT(GetWindowLong(mWindow, GWL_WNDPROC) ==
reinterpret_cast<LONG>(SurfaceWindowProc));
We've been avoiding having ANGLE assume the window might be invalid and taking
special case action. From ANGLE's point of view, it is the client's
responsibility to ensure that the window handle is valid for the lifetime of the
associated EGL surface, beyond doing something really bad, like assuming
GetClientRect succeeds and using an uninitialized width and height to allocate a
buffer, for example.

How about changing the assert to check that the returned WNDPROC is either what
we expect or NULL (the latter indicating GetWindowLong failed)?

Also, I think RemoveProp will be a no-op on an invalid mWindow. Windows is good
about not recycling those.

[nduca, 2010-12-06 20:09:22]

Does the new patch look OK?

[apatrick1, 2010-12-06 23:14:29]

Looks good for my part. I added Daniel to see if he has any objection.

[dgkoch, 2010-12-07 02:30:55]

On 2010/12/06 23:14:29, apatrick1 wrote:
> Looks good for my part. I added Daniel to see if he has any objection.

No issues here.