[Rd] Handlers in setGraphicsEventHandlers() can recursively call getGraphicsEvent(). Intended behavior?

[Richard Bodewits]

Hey all.

As in general it's a bad idea to allow an event handler to generate an
event, and as comments in the code seem to suggest this isn't the
intention either, I was wondering about recursion in getGraphicsEvent().
In main/gevents.c, both doMouseEvent() and doKeybd() have the following
line;

    dd->gettingEvent = FALSE; /* avoid recursive calls */

And they reset it to TRUE before returning. The effective result of this
is that the event handlers on the R side are allowed to call
getGraphicsEvent(), and recurse until they eventually would run out of
stack space. Though R does catch and handle that cleanly with the error;

    Error: evaluation nested too deeply: infinite recursion /
options(expressions=)?

A quick scan of the SVN logs suggests this code has been untouched since
its introduction in 2004, so I'm left to wonder if this is intended
behavior. It stands out as a bit of a sore thumb due to the generic
check for recursion in do_getGraphicsEvent() in the same file, which
will error out with error(_("recursive use of 'getGraphicsEvent' not
supported")) if dd->gettingEvent is already set to TRUE. Which would
suggest recursively calling it is very much not intended to be possible.

To me, setting gettingEvent to FALSE seems like an easy mistake to make
if you temporarily interpret gettingEvent to mean that event(s) are
allowed to still come in. But the actual interpretation in
do_getGraphicsEvents() is the opposite, as it's interpreted as an
indicator of whether or not an event is currently being processed.

I've removed the gettingEvent altering lines from both doMouseEvent()
and doKeybd() to no ill effect, and doing so disabled the ability to
call getGraphicsEvent() from inside one of the assigned handlers as
expected. But after 12 (!) years, it's conceivable that people have come
to depend on this behavior in existing scripts. Is this something that
should be left alone to minimize disruption? Or should this be fixed (if
it is indeed unintended) for the sake of protecting people from infinite
recursions?

I've included a small patch as attachment that removes the offending lines.


- Richard Bodewits
-------------- next part --------------
Index: src/main/gevents.c
===================================================================
--- src/main/gevents.c	(revision 71293)
+++ src/main/gevents.c	(working copy)
@@ -211,8 +211,6 @@
     int i;
     SEXP handler, bvec, sx, sy, temp, result;
 
-    dd->gettingEvent = FALSE; /* avoid recursive calls */
-
     PROTECT(handler = findVar(install(mouseHandlers[event]), dd->eventEnv));
     if (TYPEOF(handler) == PROMSXP) {
 	handler = eval(handler, dd->eventEnv);
@@ -242,7 +240,7 @@
 	R_FlushConsole();
     }
     UNPROTECT(1); /* handler */
-    dd->gettingEvent = TRUE;
+
     return;
 }
 
@@ -257,8 +255,6 @@
 {
     SEXP handler, skey, temp, result;
 
-    dd->gettingEvent = FALSE; /* avoid recursive calls */
-
     PROTECT(handler = findVar(install(keybdHandler), dd->eventEnv));
     if (TYPEOF(handler) == PROMSXP) {
 	handler = eval(handler, dd->eventEnv);
@@ -277,6 +273,6 @@
 	R_FlushConsole();
     }
     UNPROTECT(1); /* handler */
-    dd->gettingEvent = TRUE;
+
     return;
 }