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

[Paul Murrell]

Hi

Thanks for the further analysis.  Sounds reasonable to me.

What would be good to get from you is:

- a test that does recursive getGraphicsEvent() calls, that is fixed by 
your patch

- a test that produces the newline prompt from getGraphicsEvent(), that 
is fixed by your other patch

- a test that loses event handling with setTimeLimit(), that is fixed by 
your other other patch.

I have a partial list of packages that are known to make use of 
getGraphicsEvent(), so we can hopefully involve those package authors in 
testing for backward compatibility.

Paul

On 21/09/16 23:23, Richard Bodewits wrote:
> 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
>>>
>>
>
> ______________________________________________
> 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/