R devel - Sep 2016 - Handlers in setGraphicsEventHandlers() can recursively call getGraphicsEvent(). Intended behavior?

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;
 }

Hi

Is the correct patch to remove the setting of the gettingEvent flag or 
would it be better to flip the TRUE/FALSE setting (set to TRUE before 
handling then reset to FALSE after handling) ?

Also, for this patch and for the other two you sent, one difficulty will 
be with testing the patches.  I have no testing code for this, so would 
need at least a test or two from you (ideally someone would also have 
some regression tests, beyond ?getGraphicsEvent, to ensure continuity of 
previous behaviour).

Paul

On 18/09/2016 3:29 a.m., Richard Bodewits wrote:
> 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
>
>
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>
-- 
Dr Paul Murrell
Department of Statistics
The University of Auckland
Private Bag 92019
Auckland
New Zealand
64 9 3737599 x85392
paul at stat.auckland.ac.nz
http://www.stat.auckland.ac.nz/~paul/

doKeybd() gets called in CHelpKeyIn() and NHelpKeyIn() in
library/grDevices/src/devWindows.c, where the call is encapsulated in an
'if (dd->gettingEvent)' block. So the only times this code ever calls
doKeybd() is when gettingEvent is in fact set.

Further, it's called in two locations in X11_eventHelper(), in
modules/X11/devX11.c, where the state of gettingEvent seems to be moot
for its handling.

doMouseEvent() in turn is called in HelpMouseClick(), HelpMouseMove(),
and HelpMouseUp() in devWindows.c, where again each call is only made
after checking if gettingEvent is true.

In devX11.c it's called once in X11_eventHelper(), after checking if
gettingEvent is true.

Other than these calls, gettingEvent does not get checked anywhere
outside of main/gevents.c, and nothing called in doKeybd() or
doMouseEvent() depends on its state being toggled either which way.

As far as I've been able to ascertain these lines are completely
superfluous, as well as introducing a recursion issue that they seem to
have been meant to somehow prevent.

As for tests, I can look into cooking something up, though I'm going to
be spread a little thin for the next three months.

After 12 years of this code being in place I doubt I'll be able to cover
every imaginable usecase scenario by myself though.

But thanks for replying. Was starting to think I was screaming into the
void. ;-)


- Richard Bodewits


On 09/21/2016 03:42 AM, Paul Murrell wrote:
> Hi
> 
> Is the correct patch to remove the setting of the gettingEvent flag or
> would it be better to flip the TRUE/FALSE setting (set to TRUE before
> handling then reset to FALSE after handling) ?
> 
> Also, for this patch and for the other two you sent, one difficulty will
> be with testing the patches.  I have no testing code for this, so would
> need at least a test or two from you (ideally someone would also have
> some regression tests, beyond ?getGraphicsEvent, to ensure continuity of
> previous behaviour).
> 
> Paul
> 
> On 18/09/2016 3:29 a.m., Richard Bodewits wrote:
>> 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
>>
>>
>>
>> ______________________________________________
>> R-devel at r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
>