R devel - May 2019 - [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

Hi Peter

Yes, that looks roughly right to me.  I would be in favour of your 
option (b), partly because it is probably easiest and partly because 
that retains the basic graphics device startup logic pattern that is 
replicated across all(?) graphics devices.

Paul

On 28/04/19 11:39 AM, peter dalgaard wrote:
> I had a look at the current code, and AFAICT it has essentially the same
structure as it did back then. I think it may have finally dawned upon me what
the issue really is:
> 
> The logic is that in Rf_addX11Device, we have
> 
>          if (!X11DeviceDriver(dev, display, width, height,
>                               ps, gamma, colormodel, maxcubesize,
>                               bgcolor, canvascolor, sfonts, res,
>                               xpos, ypos, title, useCairo, antialias,
family)) {
>              free(dev);
>              errorcall(call, _("unable to start device %s"),
devname);
>          }
>          dd = GEcreateDevDesc(dev);
>          GEaddDevice2(dd, devname);
> 
> i.e., we start the device driver, and if it fails, we throw away the
"dev" structure and call it a day. If it succeeds, we proceed to
create a device descriptor structure and add it to the list of open devices.
> 
> This approach means that X11DeviceDriver() cannot do anything that
potentially accesses the dd structure because it isn't there yet, and the
things it cannot do apparently includes calling R_ProcessX11Events(). [To be
completely sure that this is actually still true, I'd need to have a closer
look at what handleEvent() does.]
> 
> So to fix things, it would seem that you could (a) add the device before
attempting to start the driver, preparing to back it out if the driver fails to
start, or (b) add a call to R_ProcessX11Events() _after_ the GEaddDevice2(dd,
devname). Option (b) is probably the easiest.
> 
> Paul: Does this analysis look roughly right?
> 
> -pd
> 
> 
> 
> 
>> On 26 Apr 2019, at 01:23 , frederik at ofb.net wrote:
>>
>> Thanks Professor Dalgard.
>>
>> If you have a different way to fix the bug then I'd be happy to
test
>> it.
>>
>> Or whatever. I understand that maybe some data was being referenced
>> before it had been initialized. I could also support moving the
>> R_ProcessEvents call in another place, but it seems one would also
>> like to generate some kind of warning message, at the location of the
>> bad reference, rather than segfaulting. Was it not possible to
>> identify this location? I'm guessing that Valgrind is a bit more
>> mature now than it was in 2001...?
>>
>> Frederick
>>
>> On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:
>>> OK, so I did the archaeology anyway....
>>>
>>>
>>> This was the story, R-core November 29, 2001. Part of thread
"X11 still segfaults".
>>>
>>> ------------>>
>>> .....
>>> Gah. I've been too tired today. Why did that take me so long?
>>>
>>> The culprit seems to be
>>>
>>> R_ProcessEvents((void*) NULL)
>>>
>>> in newX11DeviceDriver
>>>
>>> This gets called *before* this stuff at the end of Rf_addX11Device
>>>
>>> 	dd = GEcreateDevDesc(dev);
>>> 	addDevice((DevDesc*) dd);
>>> 	initDisplayList((DevDesc*) dd);
>>>
>>> and it is that "dd" that gets called by
Rf_playDisplayList. Removing
>>> the offending line stops the segfaulting, seemingly with no ill
>>> effects.
>>>
>>> I'm not really sure what the use of that line ever was; it
might be
>>> necessary to make the call somewhere later, but it appears to have
>>> been possible to race past it before receiving any events all the
>>> time.
>>>
>>> I also changed a couple of spots missing dd->newDevStruct=1
>>>
>>> Will commit in a moment.
>>> <<------------
>>>
>>> And the following day, in "graphics saga part III", we
had
>>>
>>> ------------->>
>>> ...
>>>
>>> I can't make it happen in 1.3.1 but...
>>>
>>> It is probably not unrelated to the R_ProcessEvents line that
>>> I took out, but that was definitely wrong. However, one might
reenable
>>> it if one could change this bit of code
>>>
>>> 	if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width,
height, ps, gamma,
>>> 				      colormodel, maxcubesize, canvascolor)) {
>>> 	    free(dev);
>>> 	    errorcall(gcall, "unable to start device %s",
devname);
>>>       	}
>>> 	gsetVar(install(".Device"), mkString(devname),
R_NilValue);
>>> 	dd = GEcreateDevDesc(dev);
>>> 	addDevice((DevDesc*) dd);
>>> 	initDisplayList((DevDesc*) dd);
>>>
>>>
>>> and put the if-clause last. A cursory clance through the three
>>> functions that are being called didn't reveal anything that
would rely
>>> on having opened the device driver first.
>>>
>>> Paul?
>>>
>>> (I might try it locally, but I'm not sure I should commit
anything.)
>>>
>>> <<-----------
>>>
>>> It seems that the suggestion was never followed up on?
>>>
>>> -pd
>>>
>>>
>>>> On 24 Apr 2019, at 11:42 , peter dalgaard <pdalgd at
gmail.com> wrote:
>>>>
>>>> I don't recall exactly what I did 18 years ago eiher and I
likely don't have the time to dig into the archives and reconstruct.
>>>>
>>>> I can imagine that the issue had to do with the protocol around
creating and mapping windows. Presumably the segfault comes from looking for
events on a window that hasn't been created yet, or has already been
destroyed, leading to a NULL reference somewhere. I have a vague recollection
that the issue was window manager dependent (in 2001 probably not twm, more
likely xvwm on RedHat if it was affecting me).
>>>>
>>>> A proper fix should go via proper understanding of the X11
protocol - uncommenting a line is as bad as commenting it in the 1st place....
So more like "wait for window to exist THEN process events" -- but the
1st part may be WM specific, etc.
>>>>
>>>> I recall docs being quite obtuse, and the X11 "mechanism
not policy" credo doesn't help as WMs are not obliged to (say) send
notifications, so you can end up stalling, waiting for events that never happen.
>>>>
>>>> It is entirely possible that there is stuff in here that I
didn't understand properly at the time, and still don't!
>>>>
>>>> - pd
>>>>
>>>>> On 24 Apr 2019, at 02:30 , Paul Murrell <paul at
stat.auckland.ac.nz> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> Sorry, I can't offer an explanation for the
commented-out line.
>>>>> However, regarding your final question of avoiding the
R-core bottleneck, you do have the option of creating a third-party graphics
device package.  See, for example, the 'tikzDevice' and
'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>>>
>>>>> Paul
>>>>>
>>>>> On 20/04/2019 5:27 p.m., frederik at ofb.net wrote:
>>>>>> Dear R Devel,
>>>>>>
>>>>>> I know that someone put this line in
src/modules/X11/devX11.c:2824 for
>>>>>> a reason, because commenting it out causes R to miss an
important
>>>>>> ConfigureNotify event in my window manager. The result
is that plots
>>>>>> are initially drawn off the window borders, unreadable.
>>>>>>
>>>>>>   R_ProcessX11Events((void*) NULL);
>>>>>>
>>>>>> Unfortunately for me, this line is commented in the
standard release
>>>>>> of R, it has "#if BUG ... #endif" around it.
>>>>>>
>>>>>> I guess it is also unfortunate for anyone who uses the
same window
>>>>>> manager as I do, namely i3, which I think is pretty
popular among Unix
>>>>>> power users these days; not to mention other
full-screen window
>>>>>> managers which probably exhibit the same bug in R.
>>>>>>
>>>>>> Maybe everyone on the Core team uses twm as their
window manager? Or
>>>>>> RStudio on Windows? Which would be sad because then
we're not
>>>>>> representing an important user demographic, namely
those who prefer
>>>>>> software which is modern and powerful, yet simple to
understand and
>>>>>> modify; fully configurable and interoperable and so on.
>>>>>>
>>>>>> I first reported this bug 3 years ago. In doing
research for my bug
>>>>>> report, I found that the line was commented out by
Peter Dalgaard in
>>>>>> 2001 with the explanation "X11 segfault fix - I
hope".
>>>>>>
>>>>>> I don't know what the way forward is. Obviously the
Core Team has
>>>>>> reason to say, "look, this isn't very
important, it's been broken
>>>>>> since 2001, maybe fixing it will cause the undocumented
segfault bug
>>>>>> to reappear, clearly no one here uses your window
manager". Do I have
>>>>>> to submit a correctness proof for the proposed change?
What do I do?
>>>>>>
>>>>>>
https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>>>
>>>>>> As mentioned in my bug report, I checked using gdb that
>>>>>> ConfigureNotify is indeed being received by the call to
>>>>>> R_ProcessX11Events() when it is uncommented. I
haven't experienced any
>>>>>> segfaults.
>>>>>>
>>>>>> It's good that Peter left evidence that
"R_ProcessX11Events" was being
>>>>>> called 18 years ago from X11DeviceDriver(). If he had
deleted the
>>>>>> line, rather than commenting it, then discovering the
reason for the
>>>>>> window rendering bug would have been much harder for
me.
>>>>>>
>>>>>> However, the downside is that now it is not just a
matter of inserting
>>>>>> the line where it belongs; I also feel a bit like I
have to explain
>>>>>> why it was initially removed. But although I've
given it some thought,
>>>>>> I still have no idea.
>>>>>>
>>>>>> Somewhat tangentially, I am wondering if there is some
way that we
>>>>>> could make the development of R's graphics code
proceed at a faster
>>>>>> rate, for example by pulling it out into a separate
module, so that
>>>>>> people could offer alternative implementations via CRAN
etc., rather
>>>>>> than having R Core be the bottleneck. Would this make
sense? Has it
>>>>>> already been done?
>>>>>>
>>>>>> Thank you,
>>>>>>
>>>>>> Frederick
>>>>>>
>>>>>> ______________________________________________
>>>>>> 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/
>>>>>
>>>>> ______________________________________________
>>>>> R-devel at r-project.org mailing list
>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>
>>>> --
>>>> Peter Dalgaard, Professor,
>>>> Center for Statistics, Copenhagen Business School
>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>> Phone: (+45)38153501
>>>> Office: A 4.23
>>>> Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>> -- 
>>> Peter Dalgaard, Professor,
>>> Center for Statistics, Copenhagen Business School
>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>> Phone: (+45)38153501
>>> Office: A 4.23
>>> Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
> 
-- 
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/

OK, this is now in R-devel, but only superficially tested (b/c this is a Mac).
Please check it out.

-pd
> On 30 Apr 2019, at 23:09 , Paul Murrell <paul at stat.auckland.ac.nz>
wrote:
> 
> Hi Peter
> 
> Yes, that looks roughly right to me.  I would be in favour of your option
(b), partly because it is probably easiest and partly because that retains the
basic graphics device startup logic pattern that is replicated across all(?)
graphics devices.
> 
> Paul
> 
> On 28/04/19 11:39 AM, peter dalgaard wrote:
>> I had a look at the current code, and AFAICT it has essentially the
same structure as it did back then. I think it may have finally dawned upon me
what the issue really is:
>> The logic is that in Rf_addX11Device, we have
>>         if (!X11DeviceDriver(dev, display, width, height,
>>                              ps, gamma, colormodel, maxcubesize,
>>                              bgcolor, canvascolor, sfonts, res,
>>                              xpos, ypos, title, useCairo, antialias,
family)) {
>>             free(dev);
>>             errorcall(call, _("unable to start device %s"),
devname);
>>         }
>>         dd = GEcreateDevDesc(dev);
>>         GEaddDevice2(dd, devname);
>> i.e., we start the device driver, and if it fails, we throw away the
"dev" structure and call it a day. If it succeeds, we proceed to
create a device descriptor structure and add it to the list of open devices.
>> This approach means that X11DeviceDriver() cannot do anything that
potentially accesses the dd structure because it isn't there yet, and the
things it cannot do apparently includes calling R_ProcessX11Events(). [To be
completely sure that this is actually still true, I'd need to have a closer
look at what handleEvent() does.]
>> So to fix things, it would seem that you could (a) add the device
before attempting to start the driver, preparing to back it out if the driver
fails to start, or (b) add a call to R_ProcessX11Events() _after_ the
GEaddDevice2(dd, devname). Option (b) is probably the easiest.
>> Paul: Does this analysis look roughly right?
>> -pd
>>> On 26 Apr 2019, at 01:23 , frederik at ofb.net wrote:
>>> 
>>> Thanks Professor Dalgard.
>>> 
>>> If you have a different way to fix the bug then I'd be happy to
test
>>> it.
>>> 
>>> Or whatever. I understand that maybe some data was being referenced
>>> before it had been initialized. I could also support moving the
>>> R_ProcessEvents call in another place, but it seems one would also
>>> like to generate some kind of warning message, at the location of
the
>>> bad reference, rather than segfaulting. Was it not possible to
>>> identify this location? I'm guessing that Valgrind is a bit
more
>>> mature now than it was in 2001...?
>>> 
>>> Frederick
>>> 
>>> On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:
>>>> OK, so I did the archaeology anyway....
>>>> 
>>>> 
>>>> This was the story, R-core November 29, 2001. Part of thread
"X11 still segfaults".
>>>> 
>>>> ------------>>
>>>> .....
>>>> Gah. I've been too tired today. Why did that take me so
long?
>>>> 
>>>> The culprit seems to be
>>>> 
>>>> R_ProcessEvents((void*) NULL)
>>>> 
>>>> in newX11DeviceDriver
>>>> 
>>>> This gets called *before* this stuff at the end of
Rf_addX11Device
>>>> 
>>>> 	dd = GEcreateDevDesc(dev);
>>>> 	addDevice((DevDesc*) dd);
>>>> 	initDisplayList((DevDesc*) dd);
>>>> 
>>>> and it is that "dd" that gets called by
Rf_playDisplayList. Removing
>>>> the offending line stops the segfaulting, seemingly with no ill
>>>> effects.
>>>> 
>>>> I'm not really sure what the use of that line ever was; it
might be
>>>> necessary to make the call somewhere later, but it appears to
have
>>>> been possible to race past it before receiving any events all
the
>>>> time.
>>>> 
>>>> I also changed a couple of spots missing dd->newDevStruct=1
>>>> 
>>>> Will commit in a moment.
>>>> <<------------
>>>> 
>>>> And the following day, in "graphics saga part III",
we had
>>>> 
>>>> ------------->>
>>>> ...
>>>> 
>>>> I can't make it happen in 1.3.1 but...
>>>> 
>>>> It is probably not unrelated to the R_ProcessEvents line that
>>>> I took out, but that was definitely wrong. However, one might
reenable
>>>> it if one could change this bit of code
>>>> 
>>>> 	if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width,
height, ps, gamma,
>>>> 				      colormodel, maxcubesize, canvascolor)) {
>>>> 	    free(dev);
>>>> 	    errorcall(gcall, "unable to start device %s",
devname);
>>>>      	}
>>>> 	gsetVar(install(".Device"), mkString(devname),
R_NilValue);
>>>> 	dd = GEcreateDevDesc(dev);
>>>> 	addDevice((DevDesc*) dd);
>>>> 	initDisplayList((DevDesc*) dd);
>>>> 
>>>> 
>>>> and put the if-clause last. A cursory clance through the three
>>>> functions that are being called didn't reveal anything that
would rely
>>>> on having opened the device driver first.
>>>> 
>>>> Paul?
>>>> 
>>>> (I might try it locally, but I'm not sure I should commit
anything.)
>>>> 
>>>> <<-----------
>>>> 
>>>> It seems that the suggestion was never followed up on?
>>>> 
>>>> -pd
>>>> 
>>>> 
>>>>> On 24 Apr 2019, at 11:42 , peter dalgaard <pdalgd at
gmail.com> wrote:
>>>>> 
>>>>> I don't recall exactly what I did 18 years ago eiher
and I likely don't have the time to dig into the archives and reconstruct.
>>>>> 
>>>>> I can imagine that the issue had to do with the protocol
around creating and mapping windows. Presumably the segfault comes from looking
for events on a window that hasn't been created yet, or has already been
destroyed, leading to a NULL reference somewhere. I have a vague recollection
that the issue was window manager dependent (in 2001 probably not twm, more
likely xvwm on RedHat if it was affecting me).
>>>>> 
>>>>> A proper fix should go via proper understanding of the X11
protocol - uncommenting a line is as bad as commenting it in the 1st place....
So more like "wait for window to exist THEN process events" -- but the
1st part may be WM specific, etc.
>>>>> 
>>>>> I recall docs being quite obtuse, and the X11
"mechanism not policy" credo doesn't help as WMs are not obliged
to (say) send notifications, so you can end up stalling, waiting for events that
never happen.
>>>>> 
>>>>> It is entirely possible that there is stuff in here that I
didn't understand properly at the time, and still don't!
>>>>> 
>>>>> - pd
>>>>> 
>>>>>> On 24 Apr 2019, at 02:30 , Paul Murrell <paul at
stat.auckland.ac.nz> wrote:
>>>>>> 
>>>>>> Hi
>>>>>> 
>>>>>> Sorry, I can't offer an explanation for the
commented-out line.
>>>>>> However, regarding your final question of avoiding the
R-core bottleneck, you do have the option of creating a third-party graphics
device package.  See, for example, the 'tikzDevice' and
'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>>>> 
>>>>>> Paul
>>>>>> 
>>>>>> On 20/04/2019 5:27 p.m., frederik at ofb.net wrote:
>>>>>>> Dear R Devel,
>>>>>>> 
>>>>>>> I know that someone put this line in
src/modules/X11/devX11.c:2824 for
>>>>>>> a reason, because commenting it out causes R to
miss an important
>>>>>>> ConfigureNotify event in my window manager. The
result is that plots
>>>>>>> are initially drawn off the window borders,
unreadable.
>>>>>>> 
>>>>>>>  R_ProcessX11Events((void*) NULL);
>>>>>>> 
>>>>>>> Unfortunately for me, this line is commented in the
standard release
>>>>>>> of R, it has "#if BUG ... #endif" around
it.
>>>>>>> 
>>>>>>> I guess it is also unfortunate for anyone who uses
the same window
>>>>>>> manager as I do, namely i3, which I think is pretty
popular among Unix
>>>>>>> power users these days; not to mention other
full-screen window
>>>>>>> managers which probably exhibit the same bug in R.
>>>>>>> 
>>>>>>> Maybe everyone on the Core team uses twm as their
window manager? Or
>>>>>>> RStudio on Windows? Which would be sad because then
we're not
>>>>>>> representing an important user demographic, namely
those who prefer
>>>>>>> software which is modern and powerful, yet simple
to understand and
>>>>>>> modify; fully configurable and interoperable and so
on.
>>>>>>> 
>>>>>>> I first reported this bug 3 years ago. In doing
research for my bug
>>>>>>> report, I found that the line was commented out by
Peter Dalgaard in
>>>>>>> 2001 with the explanation "X11 segfault fix -
I hope".
>>>>>>> 
>>>>>>> I don't know what the way forward is. Obviously
the Core Team has
>>>>>>> reason to say, "look, this isn't very
important, it's been broken
>>>>>>> since 2001, maybe fixing it will cause the
undocumented segfault bug
>>>>>>> to reappear, clearly no one here uses your window
manager". Do I have
>>>>>>> to submit a correctness proof for the proposed
change? What do I do?
>>>>>>> 
>>>>>>>
https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>>>> 
>>>>>>> As mentioned in my bug report, I checked using gdb
that
>>>>>>> ConfigureNotify is indeed being received by the
call to
>>>>>>> R_ProcessX11Events() when it is uncommented. I
haven't experienced any
>>>>>>> segfaults.
>>>>>>> 
>>>>>>> It's good that Peter left evidence that
"R_ProcessX11Events" was being
>>>>>>> called 18 years ago from X11DeviceDriver(). If he
had deleted the
>>>>>>> line, rather than commenting it, then discovering
the reason for the
>>>>>>> window rendering bug would have been much harder
for me.
>>>>>>> 
>>>>>>> However, the downside is that now it is not just a
matter of inserting
>>>>>>> the line where it belongs; I also feel a bit like I
have to explain
>>>>>>> why it was initially removed. But although I've
given it some thought,
>>>>>>> I still have no idea.
>>>>>>> 
>>>>>>> Somewhat tangentially, I am wondering if there is
some way that we
>>>>>>> could make the development of R's graphics code
proceed at a faster
>>>>>>> rate, for example by pulling it out into a separate
module, so that
>>>>>>> people could offer alternative implementations via
CRAN etc., rather
>>>>>>> than having R Core be the bottleneck. Would this
make sense? Has it
>>>>>>> already been done?
>>>>>>> 
>>>>>>> Thank you,
>>>>>>> 
>>>>>>> Frederick
>>>>>>> 
>>>>>>> ______________________________________________
>>>>>>> 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/
>>>>>> 
>>>>>> ______________________________________________
>>>>>> R-devel at r-project.org mailing list
>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>> 
>>>>> --
>>>>> Peter Dalgaard, Professor,
>>>>> Center for Statistics, Copenhagen Business School
>>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>>> Phone: (+45)38153501
>>>>> Office: A 4.23
>>>>> Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>> 
>>>> -- 
>>>> Peter Dalgaard, Professor,
>>>> Center for Statistics, Copenhagen Business School
>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>> Phone: (+45)38153501
>>>> Office: A 4.23
>>>> Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
> 
> -- 
> 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/
-- 
Peter Dalgaard, Professor,
Center for Statistics, Copenhagen Business School
Solbjerg Plads 3, 2000 Frederiksberg, Denmark
Phone: (+45)38153501
Office: A 4.23
Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com

I tested it. It fixes the bug and didn't seem to produce any errors. Thank
you Professor Dalgaard! I'm so glad this has finally been addressed. I will
update the bug report.

(https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702)

On Thu, May 02, 2019 at 04:48:51PM +0200, peter dalgaard
wrote:
>OK, this is now in R-devel, but only superficially tested (b/c this is a
Mac). Please check it out.
>
>-pd
>
>> On 30 Apr 2019, at 23:09 , Paul Murrell <paul at
stat.auckland.ac.nz> wrote:
>>
>> Hi Peter
>>
>> Yes, that looks roughly right to me.  I would be in favour of your
option (b), partly because it is probably easiest and partly because that
retains the basic graphics device startup logic pattern that is replicated
across all(?) graphics devices.
>>
>> Paul
>>
>> On 28/04/19 11:39 AM, peter dalgaard wrote:
>>> I had a look at the current code, and AFAICT it has essentially the
same structure as it did back then. I think it may have finally dawned upon me
what the issue really is:
>>> The logic is that in Rf_addX11Device, we have
>>>         if (!X11DeviceDriver(dev, display, width, height,
>>>                              ps, gamma, colormodel, maxcubesize,
>>>                              bgcolor, canvascolor, sfonts, res,
>>>                              xpos, ypos, title, useCairo,
antialias, family)) {
>>>             free(dev);
>>>             errorcall(call, _("unable to start device
%s"), devname);
>>>         }
>>>         dd = GEcreateDevDesc(dev);
>>>         GEaddDevice2(dd, devname);
>>> i.e., we start the device driver, and if it fails, we throw away
the "dev" structure and call it a day. If it succeeds, we proceed to
create a device descriptor structure and add it to the list of open devices.
>>> This approach means that X11DeviceDriver() cannot do anything that
potentially accesses the dd structure because it isn't there yet, and the
things it cannot do apparently includes calling R_ProcessX11Events(). [To be
completely sure that this is actually still true, I'd need to have a closer
look at what handleEvent() does.]
>>> So to fix things, it would seem that you could (a) add the device
before attempting to start the driver, preparing to back it out if the driver
fails to start, or (b) add a call to R_ProcessX11Events() _after_ the
GEaddDevice2(dd, devname). Option (b) is probably the easiest.
>>> Paul: Does this analysis look roughly right?
>>> -pd
>>>> On 26 Apr 2019, at 01:23 , frederik at ofb.net wrote:
>>>>
>>>> Thanks Professor Dalgard.
>>>>
>>>> If you have a different way to fix the bug then I'd be
happy to test
>>>> it.
>>>>
>>>> Or whatever. I understand that maybe some data was being
referenced
>>>> before it had been initialized. I could also support moving the
>>>> R_ProcessEvents call in another place, but it seems one would
also
>>>> like to generate some kind of warning message, at the location
of the
>>>> bad reference, rather than segfaulting. Was it not possible to
>>>> identify this location? I'm guessing that Valgrind is a bit
more
>>>> mature now than it was in 2001...?
>>>>
>>>> Frederick
>>>>
>>>> On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:
>>>>> OK, so I did the archaeology anyway....
>>>>>
>>>>>
>>>>> This was the story, R-core November 29, 2001. Part of
thread "X11 still segfaults".
>>>>>
>>>>> ------------>>
>>>>> .....
>>>>> Gah. I've been too tired today. Why did that take me so
long?
>>>>>
>>>>> The culprit seems to be
>>>>>
>>>>> R_ProcessEvents((void*) NULL)
>>>>>
>>>>> in newX11DeviceDriver
>>>>>
>>>>> This gets called *before* this stuff at the end of
Rf_addX11Device
>>>>>
>>>>> 	dd = GEcreateDevDesc(dev);
>>>>> 	addDevice((DevDesc*) dd);
>>>>> 	initDisplayList((DevDesc*) dd);
>>>>>
>>>>> and it is that "dd" that gets called by
Rf_playDisplayList. Removing
>>>>> the offending line stops the segfaulting, seemingly with no
ill
>>>>> effects.
>>>>>
>>>>> I'm not really sure what the use of that line ever was;
it might be
>>>>> necessary to make the call somewhere later, but it appears
to have
>>>>> been possible to race past it before receiving any events
all the
>>>>> time.
>>>>>
>>>>> I also changed a couple of spots missing
dd->newDevStruct=1
>>>>>
>>>>> Will commit in a moment.
>>>>> <<------------
>>>>>
>>>>> And the following day, in "graphics saga part
III", we had
>>>>>
>>>>> ------------->>
>>>>> ...
>>>>>
>>>>> I can't make it happen in 1.3.1 but...
>>>>>
>>>>> It is probably not unrelated to the R_ProcessEvents line
that
>>>>> I took out, but that was definitely wrong. However, one
might reenable
>>>>> it if one could change this bit of code
>>>>>
>>>>> 	if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display,
width, height, ps, gamma,
>>>>> 				      colormodel, maxcubesize, canvascolor)) {
>>>>> 	    free(dev);
>>>>> 	    errorcall(gcall, "unable to start device
%s", devname);
>>>>>      	}
>>>>> 	gsetVar(install(".Device"), mkString(devname),
R_NilValue);
>>>>> 	dd = GEcreateDevDesc(dev);
>>>>> 	addDevice((DevDesc*) dd);
>>>>> 	initDisplayList((DevDesc*) dd);
>>>>>
>>>>>
>>>>> and put the if-clause last. A cursory clance through the
three
>>>>> functions that are being called didn't reveal anything
that would rely
>>>>> on having opened the device driver first.
>>>>>
>>>>> Paul?
>>>>>
>>>>> (I might try it locally, but I'm not sure I should
commit anything.)
>>>>>
>>>>> <<-----------
>>>>>
>>>>> It seems that the suggestion was never followed up on?
>>>>>
>>>>> -pd
>>>>>
>>>>>
>>>>>> On 24 Apr 2019, at 11:42 , peter dalgaard <pdalgd at
gmail.com> wrote:
>>>>>>
>>>>>> I don't recall exactly what I did 18 years ago
eiher and I likely don't have the time to dig into the archives and
reconstruct.
>>>>>>
>>>>>> I can imagine that the issue had to do with the
protocol around creating and mapping windows. Presumably the segfault comes from
looking for events on a window that hasn't been created yet, or has already
been destroyed, leading to a NULL reference somewhere. I have a vague
recollection that the issue was window manager dependent (in 2001 probably not
twm, more likely xvwm on RedHat if it was affecting me).
>>>>>>
>>>>>> A proper fix should go via proper understanding of the
X11 protocol - uncommenting a line is as bad as commenting it in the 1st
place.... So more like "wait for window to exist THEN process events"
-- but the 1st part may be WM specific, etc.
>>>>>>
>>>>>> I recall docs being quite obtuse, and the X11
"mechanism not policy" credo doesn't help as WMs are not obliged
to (say) send notifications, so you can end up stalling, waiting for events that
never happen.
>>>>>>
>>>>>> It is entirely possible that there is stuff in here
that I didn't understand properly at the time, and still don't!
>>>>>>
>>>>>> - pd
>>>>>>
>>>>>>> On 24 Apr 2019, at 02:30 , Paul Murrell <paul at
stat.auckland.ac.nz> wrote:
>>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> Sorry, I can't offer an explanation for the
commented-out line.
>>>>>>> However, regarding your final question of avoiding
the R-core bottleneck, you do have the option of creating a third-party graphics
device package.  See, for example, the 'tikzDevice' and
'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>>>>>
>>>>>>> Paul
>>>>>>>
>>>>>>> On 20/04/2019 5:27 p.m., frederik at ofb.net wrote:
>>>>>>>> Dear R Devel,
>>>>>>>>
>>>>>>>> I know that someone put this line in
src/modules/X11/devX11.c:2824 for
>>>>>>>> a reason, because commenting it out causes R to
miss an important
>>>>>>>> ConfigureNotify event in my window manager. The
result is that plots
>>>>>>>> are initially drawn off the window borders,
unreadable.
>>>>>>>>
>>>>>>>>  R_ProcessX11Events((void*) NULL);
>>>>>>>>
>>>>>>>> Unfortunately for me, this line is commented in
the standard release
>>>>>>>> of R, it has "#if BUG ... #endif"
around it.
>>>>>>>>
>>>>>>>> I guess it is also unfortunate for anyone who
uses the same window
>>>>>>>> manager as I do, namely i3, which I think is
pretty popular among Unix
>>>>>>>> power users these days; not to mention other
full-screen window
>>>>>>>> managers which probably exhibit the same bug in
R.
>>>>>>>>
>>>>>>>> Maybe everyone on the Core team uses twm as
their window manager? Or
>>>>>>>> RStudio on Windows? Which would be sad because
then we're not
>>>>>>>> representing an important user demographic,
namely those who prefer
>>>>>>>> software which is modern and powerful, yet
simple to understand and
>>>>>>>> modify; fully configurable and interoperable
and so on.
>>>>>>>>
>>>>>>>> I first reported this bug 3 years ago. In doing
research for my bug
>>>>>>>> report, I found that the line was commented out
by Peter Dalgaard in
>>>>>>>> 2001 with the explanation "X11 segfault
fix - I hope".
>>>>>>>>
>>>>>>>> I don't know what the way forward is.
Obviously the Core Team has
>>>>>>>> reason to say, "look, this isn't very
important, it's been broken
>>>>>>>> since 2001, maybe fixing it will cause the
undocumented segfault bug
>>>>>>>> to reappear, clearly no one here uses your
window manager". Do I have
>>>>>>>> to submit a correctness proof for the proposed
change? What do I do?
>>>>>>>>
>>>>>>>>
https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>>>>>
>>>>>>>> As mentioned in my bug report, I checked using
gdb that
>>>>>>>> ConfigureNotify is indeed being received by the
call to
>>>>>>>> R_ProcessX11Events() when it is uncommented. I
haven't experienced any
>>>>>>>> segfaults.
>>>>>>>>
>>>>>>>> It's good that Peter left evidence that
"R_ProcessX11Events" was being
>>>>>>>> called 18 years ago from X11DeviceDriver(). If
he had deleted the
>>>>>>>> line, rather than commenting it, then
discovering the reason for the
>>>>>>>> window rendering bug would have been much
harder for me.
>>>>>>>>
>>>>>>>> However, the downside is that now it is not
just a matter of inserting
>>>>>>>> the line where it belongs; I also feel a bit
like I have to explain
>>>>>>>> why it was initially removed. But although
I've given it some thought,
>>>>>>>> I still have no idea.
>>>>>>>>
>>>>>>>> Somewhat tangentially, I am wondering if there
is some way that we
>>>>>>>> could make the development of R's graphics
code proceed at a faster
>>>>>>>> rate, for example by pulling it out into a
separate module, so that
>>>>>>>> people could offer alternative implementations
via CRAN etc., rather
>>>>>>>> than having R Core be the bottleneck. Would
this make sense? Has it
>>>>>>>> already been done?
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>>
>>>>>>>> Frederick
>>>>>>>>
>>>>>>>> ______________________________________________
>>>>>>>> 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/
>>>>>>>
>>>>>>> ______________________________________________
>>>>>>> R-devel at r-project.org mailing list
>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>
>>>>>> --
>>>>>> Peter Dalgaard, Professor,
>>>>>> Center for Statistics, Copenhagen Business School
>>>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>>>> Phone: (+45)38153501
>>>>>> Office: A 4.23
>>>>>> Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Peter Dalgaard, Professor,
>>>>> Center for Statistics, Copenhagen Business School
>>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>>> Phone: (+45)38153501
>>>>> Office: A 4.23
>>>>> Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>
>> --
>> 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/
>
>-- 
>Peter Dalgaard, Professor,
>Center for Statistics, Copenhagen Business School
>Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>Phone: (+45)38153501
>Office: A 4.23
>Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
>
>
>
>
>
>
>
>
>