From pd@|gd @end|ng |rom gm@||@com  Wed Apr 24 15:12:55 2019
From: pd@|gd @end|ng |rom gm@||@com (peter dalgaard)
Date: Wed, 24 Apr 2019 15:12:55 +0200
Subject: [Rd] 
 [FORGED]  src/modules/X11/devX11.c, can we remove "#if BUG" yet
In-Reply-To: <4FEA2AD4-C0A6-4CEE-AB30-CD8AE209ED24@gmail.com>
References: <20190420052728.7tgjr4zxqdddsimm@localhost>
 <3badc471-0698-42e1-581a-2988eb3bf818@stat.auckland.ac.nz>
 <4FEA2AD4-C0A6-4CEE-AB30-CD8AE209ED24@gmail.com>
Message-ID: <3C40D480-61B5-40D1-A592-1890B090C124@gmail.com>

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



[Rd] [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet
peter dalgaard