#2 GLOW crash - _ExecuteDeferred bug

[John Nagle]

Found a serious crash bug in GLOW.

GLUT for Win32 will make focus callbacks from inside a glutDestroyWindow call. GLOW isn't
prepared for this, and crashes.

I have several windows, each with several subwindows. I close one of the windows, and
GLOW puts the window to be closed on Glow::_closeList. Then, at the next _executeDeferred
call, GLOW calls "delete" on the window. The destructor ~GlowSubwindow is called, and
calls glutDestroyWindow. But at that time, the window being deleted is still on
Glow::closeList, because it's deleted from closeList only when the
destructor ~glowComponent is executed. That's a base-level
destructor, and hasn't been called yet when control is in
the ~GlowSubwindow destructor.

Inside glutDestroyWindow, GLUT makes focus callbacks, and
GLOW::_EntryFunc gets called. This calls _ExecuteDeferred,
which starts working through the close list again. But
the original window being closed is still on the close list, so
it is deleted again. A crash follows shortly thereafter.

I tried a workaround in glow.c, putting a flag in _ExecuteDeferred
so that it returns immediately if entered recursively, which stopped
the crashes. That's just a test; though. The real problem is that
events can come in during some GLUT calls, and GLOW needs to
be prepared for all of them.

[John Nagle]

Logged In: YES
user_id=5571

Temporary fix, as a patch to glow.cpp:

55d54
<
890c889
<
---
> static bool inExecuteDeferred = false;
894c893,895
<
---
> if (inExecuteDeferred)
// if already in here
> { return; }
// busy
> inExecuteDeferred = true;
// avoid reentrancy
920a922
> inExecuteDeferred = false;
// unlock
1505c1507,1516
< ::glutDestroyWindow(_windowNum);
---
> ::glutSetWindow(_windowNum);
// make subwindow being deleted current
> GLOW_ASSERT(::glutGetWindow() == _windowNum); //
validate subwindow being d
> // Clear entry callback to prevent callbacks
during glutDestroyWindow.
> // Note that we can still get callbacks for
other windows during
> // glutDestroyWindow; this just prevents
callbacks for the one being delete
> ::glutEntryFunc(NULL);
// cancel entry callback
> // Subwindow being deleted must not have
further subwindows at this point.
> // If it does, GLUT and GLOW will get out of
sync.
> GLOW_ASSERT(::glutGet(GLUT_WINDOW_NUM_CHILDREN) ==
0);
> ::glutDestroyWindow(_windowNum);
// finally destroy subwindow

[Daniel Azuma]

Logged In: YES
user_id=18901

Good catch on that. I checked in an exception-safe variation
of John's patch into both the trunk and the 1.0.x branch.

I did a cursory review of a few other potential trouble
spots (in particular, Glow::clock_) and I think we're okay,
but this issue could use some more rigorous review and test,
as I designed GLOW with the assumption that events are
reported sequentially and never re-enter. I put some
instrumentation in the dev branch that will spit out warning
messages if there's ever re-entry on any of the event handlers.

I've marked this bug fixed, but don't close it yet.