R devel - Sep 2016 - getGraphicsEvent() and setTimeLimit() bug and compatibility patch.

Hey all.

Setting a time limit with setTimeLimit(), and then using
getGraphicsEvent(), will cause graphics event handling for the current
device to break on timeout, until the device is destroyed and recreated.

The problem lies in do_getGraphicsEvent() checking the value of
dd->gettingEvent and concluding it's being called recursively
(ironically this same test fails to detect actual recursion). The
gettingEvent value is not reset on error conditions, so the device
becomes unusable for the remainder of its life.

As far as I've been able to tell, the values set with setTimeLimit() are
checked in R_ProcessEvents(), which is defined separately in
gnuwin32/system.c and unix/sys-unix.c. If a time limit is exceeded, it
error()s out. That in turn percolates up and through jump_to_top_ex(),
as defined in main/errors.c, which eventually calls GEonExit() in
main/engine.c, a function meant to reset some state on graphics devices
and which from the look of it and its comments was added to fix a
similar bug with recordGraphics().

I've added a single line to GEonExit(), to also reset gettingEvent back
to FALSE, which seems to have no side effects, and serves to make
getGraphicsEvent() timeout-friendly. It errors out as expected, and can
then be reused immediately. The patch is attached to this mail.

One problem with this, is recursion. My previous patch fixes the problem
of do_getGraphicsEvent() being called recursively from its own handlers,
but without that patch it becomes possible that R_ProcessEvents()
triggers a timeout while we're in a recursive call of
do_getGraphicsEvent(). Resetting gettingEvent would then potentially
affect all parent do_getGraphicsEvent() calls in the recursion stack.

Currently do_getGraphicsEvent() does not actually check the value of
gettingEvent after it would have triggered a recursion, but this seems
like a landmine for future changes to step on. To support setTimeLimit()
properly without applying the recursion fix, would require some sort of
stack-aware alternative to gettingEvent's current single boolean
implementation.

So short version; this patch builds on my do_getGraphicsEvent()
recursion patch, and will fix getGraphicsEvent() choking on
setTimeLimit() timing out.


- Richard Bodewits
-------------- next part --------------
Index: src/main/engine.c
==================================================================---
src/main/engine.c	(revision 71293)
+++ src/main/engine.c	(working copy)
@@ -3043,6 +3043,7 @@
 	    gd = GEgetDevice(devNum);
 	    gd->recordGraphics = TRUE;
 	    dd = gd->dev;
+	    dd->gettingEvent = FALSE; // Added to allow setTimeLimit() to be used
with getGraphicsEvent().
 	    if (dd->onExit) dd->onExit(dd);
 	    devNum = nextDevice(devNum);
 	}