Rootless code debugging notes

8 Dec 2007

      Ken Thomases of CodeWeavers (who makes an excellent product called  
CrossOver Mac, which you should all go try) and I have been mailing  
back and forth for months now, having a fairly sparse (time-wise)  
conversation about debugging the Rootless subsystem.  It resulted in  
several small patches, but we obviously still have far to go.

He's given me permission to collect some of the bits and post them  
here -- hopefully they'll inspire one of you!

I'm going to order this "backwards", with the most recent stuff at the  
top, because in many cases it supersedes the older stuff.  However,  
there's still a lot of knowledge there.
...
I remembered something else that I fixed in the X server.  I could  
have sworn that Torrey applied this suggestion way back around  
6.9.x, though.  On the other hand, I don't see the fixes in the  
current git branch.
The code in miext/rootless/accel is essentially a copy+paste+hack  
job of similar code in the fb directory.  However, the fb code  
continued to be updated after the time of the copy, and fixes there  
didn't make it into the rootless code.  The classic problem with  
code duplication.  This is an excellent candidate for consolidation,  
but I have to admit I took the coward's way out and just updated the  
rootless/accel code to incorporate the fixes to the corresponding fb  
code.
[http://gitweb.freedesktop.org/?p=xorg/xserver.git;a=commit;h=70374a58937d7a6... 
]
[http://gitweb.freedesktop.org/?p=xorg/xserver.git;a=commit;h=3f857e129df7ce4... 
]
...
...
I'll apply it (Patch to avert (some) damage / rootless crashes,  
courte ... xorg-server-1.2-apple), too, but I'm still getting  
rootless crashes just like ones identified below -- for example,
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xb09a7ffe
[Switching to process 64128 thread 0x2d03]
0x000ba460 in fbSolid (dst=0xb09a7ffc, dstStride=1, dstX=16,  
bpp=16, width=4560, height=1, and=0, xor=1523997398) at fbsolid.c:67
67		    FbDoLeftMaskByteRRop(dst,startbyte,startmask,and,xor);
(gdb) bt
#0  0x000ba460 in fbSolid (dst=0xb09a7ffc, dstStride=1, dstX=16,  
bpp=16, width=4560, height=1, and=0, xor=1523997398) at fbsolid.c:67
#1  0x000bcb96 in fbFillRegionSolid (pDrawable=0x4f8b90,  
pRegion=0x13110280, and=0, xor=1523997398) at fbwindow.c:247
#2  0x0002693d in SafeAlphaPaintWindow (pWin=0x4f8b90,  
pRegion=0x13110280, what=0) at safeAlphaWindow.c:148
#3  0x00106f1a in damagePaintWindow (pWindow=0x4f8b90,  
prgn=0x13110280, what=0) at damage.c:1593
#4  0x00025a88 in RootlessPaintWindowBackground (pWin=0x4f8b90,  
pRegion=0x13110280, what=0) at rootlessWindow.c:1542
#5  0x00035655 in miWindowExposures (pWin=0x4f8b90,  
prgn=0x13110280, other_exposed=0x0) at miexpose.c:565
#6  0x000164ac in DRIWindowExposures (pWin=0x4f8b90,  
prgn=0xb09a7ffe, bsreg=0xb09a7ffe) at dri.c:609
#7  0x0004922c in miHandleValidateExposures (pWin=0x428470) at  
miwindow.c:470
#8  0x0008a9a6 in MapWindow (pWin=0x4f67d0, client=0x4f10d0) at  
window.c:2803
#9  0x000688b9 in ProcMapWindow (client=0x1) at dispatch.c:696
#10 0x0006f5b5 in Dispatch () at dispatch.c:457
#11 0x0007e001 in main (argc=2, argv=0xbffff4f8, envp=0xbffff504)  
at main.c:445
Hmm.  I had thought I had caught all of the remaining crashes.  It's  
of course possible that new bugs have been introduced between the  
code base that I'm working with and X.org 7.2.  The most likely  
candidate would be someplace where a function is unwrapped on an  
object (a screen, a window, a graphics context, etc.) and there's a  
code path that fails to rewrap it when it should.  This is what I  
fixed in the earlier commit <http://gitweb.freedesktop.org/?p=xorg/xserver.git;a=commit;h=38965768636aef5...
...
.  A thorough grepping of the code can find places where something  
is unwrapped, and you can then examine each place to make sure the  
proper rewrap happens in every case.
Something that can be useful when debugging what's going on is  
malloc_history and the MallocStackLoggingNoCompact environment  
variable.  You may find Xray (ahem, "Instruments" ;) can also help.
When I was debugging, I actually did something I thought was kind of  
neat.  It's pretty common to litter code with printfs (or ErrorF) to  
debug.  Well, I did that and included the pointer address of each  
interesting object at each point -- when a PixMap was freed, I  
printed the address of the pixmap object, for example.  What was  
neat though, is that I also did a small allocation and included the  
address of that, and then leaked the allocated buffer.  For example:
ErrorF("Did something interesting with pixmap %p.  %p\n", pixmap,  
malloc(1));
Then, as I examined the flood of logging messages, when I found a  
suspicious operation, I would use malloc_history on the address of  
the small leaked buffer.  Since I was leaking it, then the last  
entry in the history had to be for the log line in question, and  
malloc_history could provide a stack trace for where/when that  
allocation happened.  That gave me invaluable insight into what the  
code was doing.
It's quite possible that DTrace is powerful enough, though, that  
this technique can be considered "archaic".  :)
... and this next bit is excerpted from an email thread between him  
and Torrey Lyons that he sent me.
...
...
...
...
...
We're experiencing a significant number of crashes when we  
resize windows.  They're all within the Damage extension.  I  
found bug 1032 in X.org's bugzilla <https://bugs.freedesktop.org/show_bug.cgi?id=1032
...
, but haven't seen recent activity.
That bug also includes a note claiming that Damage is disabled  
for rootless servers, but I find that that isn't true.  In fact,  
I attempted to disable the extension but found it impossible due  
to the cursor code requiring it.
Hmm. The comment on bug <https://bugs.freedesktop.org/show_bug.cgi?id=1032
...
from ajax@nwnk.net is incorrect as I recall and as you noted.  
Damage is used. My understanding was that all crashes related to  
damage were fixed. The bug was left open to remind me to fix a  
potential incorrect behavior in CopyWindow. However, I had never  
actually observed any problems related to this. You may have  
stumbled upon one. Can you please, try the following:
1. Rebuild X.Org with the debug flag on so you get a more  
complete backtrace when the crash occurs. You can do this by  
inserting a host.def file into /xc/config/cf with the line:
#define DefaultCDebugFlags              -g
2. Duplicate the crash and send me the backtrace. You should be  
able to get the XDarwin backtrace from Console.app. You can also  
post it on the above mentioned bug.
Thanks for your reply.
I have attached crash reports showing two crashes related to the  
Damage extension.
Here are some gdb backtraces I just generated:
-------------
Program received signal:  "EXC_BAD_ACCESS".
[Switching to process 5606 thread 0x4603]
(gdb) bt
#0  0x01f45dd9 in rlBlt (srcLine=0x109a4838, srcStride=512,  
srcX=0, pDstScreen=0x1979590, dstLine=0x294, dstStride=1, dstX=29,  
width=271, height=326, alu=3, pm=4294967295, bpp=1, reverse=0,  
upsidedown=0) at rlBlt.c:327
#1  0x01f445ff in rlCopyNtoN (pSrcDrawable=0x109a4000,  
pDstDrawable=0x10094dc0, pGC=0x1008f010, pbox=0xb009dc78, nbox=0,  
dx=-573, dy=-147, reverse=0, upsidedown=0, bitplane=0,  
closure=0x0) at rlCopy.c:60
#2  0x001aeec5 in fbCopyRegion (pSrcDrawable=0x109a4000,  
pDstDrawable=0x10094dc0, pGC=0x1008f010, pDstRegion=0xb009dc78,  
dx=-573, dy=-147, copyProc=0x1f443cf <rlCopyNtoN>, bitPlane=0,  
closure=0x0) at fbcopy.c:366
#3  0x001af38f in fbDoCopy (pSrcDrawable=0x109a4000,  
pDstDrawable=0x10094dc0, pGC=0x1008f010, xIn=0, yIn=0,  
widthSrc=271, heightSrc=468, xOut=573, yOut=147,  
copyProc=0x1f443cf <rlCopyNtoN>, bitPlane=0, closure=0x0) at  
fbcopy.c:566
#4  0x01f446ad in rlCopyArea (pSrcDrawable=0x109a4000,  
pDstDrawable=0x10094dc0, pGC=0x1008f010, xIn=0, yIn=0,  
widthSrc=271, heightSrc=468, xOut=569, yOut=124) at rlCopy.c:101
#5  0x000dead0 in damageCopyArea (pSrc=0x109a4000,  
pDst=0x10094dc0, pGC=0x1008f010, srcx=0, srcy=0, width=271,  
height=468, dstx=569, dsty=124) at damage.c:762
#6  0x000bfecb in ProcCopyArea (client=0x100886d0) at dispatch.c: 
1758
#7  0x000bc943 in Dispatch () at dispatch.c:455
#8  0x0002d5ad in main (argc=1, argv=0xbffff9ec, envp=0xbffff9f4)  
at main.c:451
-------------
-------------
Program received signal:  "EXC_BAD_ACCESS".
[Switching to process 5906 thread 0x4603]
(gdb) bt
#0  0x9035ab8e in sseCGSFill8by1 ()
#1  0x9a73c9a6 in xp_fill_bytes ()
#2  0x01f46b85 in rlSolid (pScreen=0x1979a00, dst=0x209f58,  
dstStride=1280, dstX=0, bpp=32, width=8320, height=2, and=0,  
xor=15000804) at rlSolid.c:77
#3  0x01f4615f in rlFill (pDrawable=0xfc8ecd0, pGC=0xfc8b870,  
x=726, y=417, width=260, height=2) at rlFill.c:49
#4  0x01f44e32 in rlPolyFillRect (pDrawable=0xfc8ecd0,  
pGC=0xfc8b870, nrect=0, prect=0x108c806c) at rlFillRect.c:79
#5  0x000e0858 in damagePolyFillRect (pDrawable=0xfc8ecd0,  
pGC=0xfc8b870, nRects=5, pRects=0x108c8044) at damage.c:1205
#6  0x000c1339 in ProcPolyFillRectangle (client=0xfc7b6f0) at  
dispatch.c:1978
#7  0x000bc943 in Dispatch () at dispatch.c:455
#8  0x0002d5ad in main (argc=1, argv=0xbffff9ec, envp=0xbffff9f4)  
at main.c:451
-------------
This next one is most reminiscent of the known Damage bug, but I  
suspect the ones above are related:
-------------
Program received signal:  "EXC_BAD_ACCESS".
[Switching to process 6021 thread 0x4603]
(gdb) bt
#0  0x000de948 in damageCopyArea (pSrc=0x10877000, pDst=0xfba1830,  
pGC=0xfba6a50, srcx=0, srcy=0, width=628, height=26, dstx=0,  
dsty=51) at damage.c:748
#1  0x000bfecb in ProcCopyArea (client=0xfb986a0) at dispatch.c:1758
#2  0x000bc943 in Dispatch () at dispatch.c:455
#3  0x0002d5ad in main (argc=1, argv=0xbffff9ec, envp=0xbffff9f4)  
at main.c:451
-------------
As you probably remember, what's happening is that  
getDrawableDamageRef is returning a bogus pointer which is  
dereferenced by the getDrawableDamage macro (used by  
checkGCDamage).  getDrawableDamageRef returns a bogus pointer  
because it uses the (*pScreen->GetWindowPixmap) function pointer,  
which is _fbGetWindowPixmap in this case.  That devolves down to  
the macro fbGetWindowPixmap(pWin) which is defined as ((PixmapPtr)  
((WindowPtr) (pWin))->devPrivates[fbGetWinPrivateIndex()].ptr).   
And finally we get to the immediate problem: there is no pointer  
stashed in that private index entry.  However, what I don't  
understand is why that private index entry hasn't been  
initialized.  That's where I get lost in the maze that is X11.
[...]
...
...
...
t took me a while to get around to it, but I have tested with the  
6.9 tip and am still able to reproduce the crashes.  Here are some  
gdb backtraces of a few of the crashes:
Program received signal:  "EXC_BAD_ACCESS".
[Switching to process 16351 thread 0x4607]
(gdb) bt
#0  0x001b107b in fbEvenTile (dst=0x6cb60, dstStride=900, dstX=0,  
width=5504, height=22, tile=0x103ee2b8, tileHeight=26, alu=3,  
pm=4294967295, xRot=28832, yRot=-3) at fbtile.c:94
#1  0x001b130e in fbTile (dst=0x6bd40, dstStride=1072, dstX=28928,  
width=5504, height=23, tile=0x103ee2b8, tileStride=1,  
tileWidth=32, tileHeight=26, alu=3, pm=4294967295, bpp=32,  
xRot=28832, yRot=-3) at fbtile.c:193
#2  0x01f466f1 in rlFill (pDrawable=0x103981a0, pGC=0x103a1850,  
x=904, y=103, width=172, height=23) at rlFill.c:142
#3  0x01f44f11 in rlPolyFillRect (pDrawable=0x103981a0,  
pGC=0x103a1850, nrect=0, prect=0x107c50cc) at rlFillRect.c:108
#4  0x000e1714 in damagePolyFillRect (pDrawable=0x103981a0,  
pGC=0x103a1850, nRects=1, pRects=0x107c50c4) at damage.c:1205
#5  0x000c21f5 in ProcPolyFillRectangle (client=0x10399130) at  
dispatch.c:1978
#6  0x000bd7ff in Dispatch () at dispatch.c:455
#7  0x0002e469 in main (argc=1, argv=0xbffff9ec, envp=0xbffff9f4)  
at main.c:451
--------------------
Program received signal:  "EXC_BAD_ACCESS".
[Switching to process 16734 thread 0x4607]
(gdb) bt full
#0  0x000df804 in damageCopyArea (pSrc=0x1099c000,  
pDst=0x10094b00, pGC=0x100947e0, srcx=0, srcy=0, width=628,  
height=26, dstx=0, dsty=51) at damage.c:748
  ret = (RegionPtr) 0x100947e0
  pGCPriv = (DamageGCPrivPtr) 0x10094868
  oldFuncs = (GCFuncs *) 0x23a120
#1  0x000c0d87 in ProcCopyArea (client=0x1008d330) at dispatch.c: 
1758
  pDst = (DrawablePtr) 0x10094b00
  pSrc = (DrawablePtr) 0x1099c000
  pGC = (GC *) 0x100947e0
  stuff = (xCopyAreaReq *) 0x1094c3b4
  pRgn = (RegionPtr) 0x1
#2  0x000bd7ff in Dispatch () at dispatch.c:455
  clientReady = (int *) 0x206ae00
  result = 28
  client = (ClientPtr) 0x1008d330
  nready = 0
  icheck = (HWEventQueuePtr *) 0x27d2b0
  start_tick = 11920
#3  0x0002e469 in main (argc=1, argv=0xbffff9ec, envp=0xbffff9f4)  
at main.c:451
  i = 1
  j = 2
  k = 2
  error = 0
  xauthfile = 0x0
  alwaysCheckForInput = {0, 1}
  adjustedRgbPath = "/Users/ken/work/cxmac/macosx/CrossOver/build/ 
Debug/CrossOver.app/Contents/SharedSupport/X11/lib/X11/rgb", '\0'  
<repeats 920 times>
(gdb) p pDst->pScreen->GetWindowPixmap
$1 = (GetWindowPixmapProcPtr) 0x1573f9 <_fbGetWindowPixmap>
(gdb) p ((PixmapPtr) ((WindowPtr) (pDst))-
...
devPrivates[fbWinPrivateIndex].ptr)
$2 = (struct _Pixmap *) 0x100bcd10
(gdb) p *((PixmapPtr) ((WindowPtr) (pDst))- 
devPrivates[fbWinPrivateIndex].ptr)
$3 = {
drawable = {
  type = 32 ' ',
  class = 157 '\235',
  depth = 128 '\200',
  bitsPerPixel = 160 '\240',
  id = 271240,
  x = 0,
  y = 0,
  width = 58736,
  height = 400,
  pScreen = 0x3,
  serialNumber = 6
},
refcnt = 600000,
devKind = -1824067958,
devPrivate = {
  ptr = 0x50797472,
  val = 1350136946,
  uval = 1350136946,
  fptr = 0x50797472
},
devPrivates = 0xea4d230,
screen_x = 0,
screen_y = 0
}
(gdb) p damagePixPrivateIndex
$4 = 0
(gdb) p $2->devPrivates[damagePixPrivateIndex].ptr
$5 = (pointer) 0xa0801fa8
(gdb) p (DamagePtr*) &($2->devPrivates[damagePixPrivateIndex].ptr)
$6 = (DamagePtr *) 0x4f0074
(gdb) p **$6
$9 = {
pNext = 0x18,
pNextWin = 0x1440000,
damage = {
  extents = {
    x1 = 242,
    y1 = -17399,
    x2 = 24,
    y2 = 0
  },
  data = 0x1440000
},
damageLevel = 3155558643,
isInternal = 24,
closure = 0x1440000,
isWindow = -1138687756,
pDrawable = 0x18,
damageReport = 0x1440000,
damageDestroy = 0xbc3900f6
}
--------------------
Program received signal:  "EXC_BAD_ACCESS".
[Switching to process 17098 thread 0x4607]
(gdb) bt full
#0  0x90385a1a in CGBlt_copyBytes ()
No symbol table info available.
#1  0x9a73c8f3 in xp_copy_bytes ()
No symbol table info available.
#2  0x01f452b6 in rlBlt (srcLine=0x107c1038, srcStride=628,  
srcX=0, pDstScreen=0x197b5d0, dstLine=0x7fffffff, dstStride=0,  
dstX=512, width=80384, height=1, alu=3, pm=4294967295, bpp=128,  
reverse=0, upsidedown=0) at rlBlt.c:93
  src = (FbBits *) 0x102e1fd0
  dst = (FbBits *) 0x102e8ad0
  leftShift = 32725726
  rightShift = -1838100790
  startmask = 0
  endmask = 0
  bits = 2456866803
  bits1 = 0
  n = -1
  nmiddle = 2512
  destInvarient = 1
  startbyte = 0
  endbyte = 0
  _ca1 = 0
  _cx1 = 0
  _ca2 = 4294967295
  _cx2 = 0
#3  0x01f445ff in rlCopyNtoN (pSrcDrawable=0x107c1000,  
pDstDrawable=0x10291680, pGC=0x10290010, pbox=0x102e8608, nbox=2,  
dx=-4, dy=-74, reverse=0, upsidedown=0, bitplane=0, closure=0x0)  
at rlCopy.c:60
  alu = 3 '\003'
  pm = 4294967295
  src = (FbBits *) 0x107c1038
  srcStride = 628
  srcBpp = 32
  srcXoff = 0
  srcYoff = 0
  dst = (FbBits *) 0x7fffffff
  dstStride = 0
  dstBpp = 128
  dstXoff = 0
  dstYoff = 0
#4  0x001afd81 in fbCopyRegion (pSrcDrawable=0x107c1000,  
pDstDrawable=0x10291680, pGC=0x10290010, pDstRegion=0xb009dc78,  
dx=-4, dy=-74, copyProc=0x1f443cf <rlCopyNtoN>, bitPlane=0,  
closure=0x0) at fbcopy.c:366
  careful = 0
  reverse = 0
  upsidedown = 0
  pbox = (BoxPtr) 0x102e8608
  nbox = 3
  pboxNew1 = (BoxPtr) 0x0
  pboxNew2 = (BoxPtr) 0x0
  pboxBase = (BoxPtr) 0x102b7d30
  pboxNext = (BoxPtr) 0x197b5d0
  pboxTmp = (BoxPtr) 0x1971020
#5  0x001b024b in fbDoCopy (pSrcDrawable=0x107c1000,  
pDstDrawable=0x10291680, pGC=0x10290010, xIn=0, yIn=0,  
widthSrc=628, heightSrc=26, xOut=4, yOut=74, copyProc=0x1f443cf  
<rlCopyNtoN>, bitPlane=0, closure=0x0) at fbcopy.c:566
  prgnSrcClip = (RegionPtr) 0x0
  freeSrcClip = 0
  prgnExposed = (RegionPtr) 0x0
  rgnDst = {
extents = {
  x1 = 4,
  y1 = 74,
  x2 = 632,
  y2 = 100
},
data = 0x102e8600
}
  dx = -4
  dy = -74
  numRects = 3
  box_x1 = 4
  box_y1 = 74
  box_x2 = 632
  box_y2 = 100
  fastSrc = 1
  fastDst = 0
  fastExpose = 1
#6  0x01f446ad in rlCopyArea (pSrcDrawable=0x107c1000,  
pDstDrawable=0x10291680, pGC=0x10290010, xIn=0, yIn=0,  
widthSrc=628, heightSrc=26, xOut=0, yOut=51) at rlCopy.c:101
  copy = (fbCopyProc) 0x1f443cf <rlCopyNtoN>
#7  0x000df98c in damageCopyArea (pSrc=0x107c1000,  
pDst=0x10291680, pGC=0x10290010, srcx=0, srcy=0, width=628,  
height=26, dstx=0, dsty=51) at damage.c:762
  ret = (RegionPtr) 0x10290010
  pGCPriv = (DamageGCPrivPtr) 0x10290098
  oldFuncs = (GCFuncs *) 0x23a120
#8  0x000c0d87 in ProcCopyArea (client=0x1028a090) at dispatch.c: 
1758
  pDst = (DrawablePtr) 0x10291680
  pSrc = (DrawablePtr) 0x107c1000
  pGC = (GC *) 0x10290010
  stuff = (xCopyAreaReq *) 0x1073b3b4
  pRgn = (RegionPtr) 0x1
[...]
-----------------
Do you know what's going wrong and how to fix it?
If there's some diagnostic steps I could take that would be  
informative, please let me know.
[...]
So, I have some primary suspects.  I think it's the handling of  
the window pixmaps which is done by RootlessStartDrawing and  
RootlessStopDrawing.  Basically, RootlessStopDrawing is sometimes  
restoring a stale or deallocated pixmap to the window.
Here are a couple of scenarios to illustrate the issue:
Scenario 1: Calling RootlessStartDrawing twice on a window
Assume we have Window A.  Its top-level parent is T.   
OriginalPixmapA is the current pixmap of Window A before the calls.
First call to RootlessStartDrawing(A):
      Sets WINREC(T)->pixmap to a scratch pixmap configured as  
appropriate.  We'll call this ScratchPixmap.
      Sets WINREC(T)->is_drawing
      Sets WINREC(T)->oldPixmap to OriginalPixmapA
      Sets pixmap of Window A to WINREC(T)->pixmap, which is  
ScratchPixmap
Second call to RootlessStartDrawing(A)
      Since WINREC(T)->is_drawing, do not get another scratch pixmap
      Sets WINREC(T)->oldPixmap to the *current* pixmap of Window A,  
which is ScratchPixmap.  This loses the value of OriginalPixmapA!
      Sets pixmap of Window A to WINREC(T)->pixmap, which is  
ScratchPixmap
Call to RootlessStopDrawing(A)
      Calls FreeScratchPixmapHeader on WINREC(T)->pixmap, which is  
ScratchPixmap.  This either deallocates ScratchPixmap or sets  
ScratchPixmap->devPrivate to NULL.
      Sets pixmap of Window A to WINREC(T)->oldPixmap, which is  
ScratchPixmap, not OriginalPixmapA.  ScratchPixmap is, of course,  
invalid.
      Clears WINREC(T)->pixmap and WINREC(T)->is_drawing
The fix for this case is to not overwrite WINREC(T)->oldPixmap if  
the pixmap of Window A is already equal to WINREC(T)->pixmap.   
However, this fix doesn't fix all of the cases, as illustrated  
by...  *drum roll* Scenario 2.  Nobody expects Scenario 2!     
*cough*  (Sorry.)
Scenario 2: Calling RootlessStartDrawing on multiple windows with  
the same top-level parent.
In addition to Window A, there is now also Window B.  They share  
the same top-level parent, T.  OriginalPixmapB is the current  
pixmap of Window B before the calls.
Call to RootlessStartDrawing(A):
      Sets WINREC(T)->pixmap to a scratch pixmap configured as  
appropriate.  We'll call this ScratchPixmap.
      Sets WINREC(T)->is_drawing
      Sets WINREC(T)->oldPixmap to OriginalPixmapA
      Sets pixmap of Window A to WINREC(T)->pixmap, which is  
ScratchPixmap
Call to RootlessStartDrawing(B)
      Since WINREC(T)->is_drawing, do not get another scratch pixmap
      Sets WINREC(T)->oldPixmap to the current pixmap of Window B,  
which is OriginalPixmapB.  This loses the value of OriginalPixmapA!
      Sets pixmap of Window B to WINREC(T)->pixmap, which is  
ScratchPixmap
Call to RootlessStopDrawing(A)
      Calls FreeScratchPixmapHeader on WINREC(T)->pixmap, which is  
ScratchPixmap.  This either deallocates ScratchPixmap or sets  
ScratchPixmap->devPrivate to NULL.
      Sets pixmap of Window A to WINREC(T)->oldPixmap, which is  
OriginalPixmapB, not OriginalPixmapA.  This is not right!
      Clears WINREC(T)->pixmap and WINREC(T)->is_drawing
Note: at this point Window B's pixmap is set to ScratchPixmap,  
which is invalid.
Call to RootlessStopDrawing(B)
      Since WINREC(T)->is_drawing has been cleared, this does nothing.
      Thus, Window B's pixmap is still left at ScratchPixmap, which is  
invalid.
Reversing the calls to RootlessStopDrawing so that it's called on  
B before A addresses the problem where Window A gets  
OriginalPixmapB, but does not fix the problem where one of the  
windows is left with an invalid pixmap.  Also, near as I can tell,  
there's no guarantee that the RootlessStopDrawing calls will be  
made in the "proper" order (i.e. the reverse order of the  
RootlessStartDrawing calls).
A partial fix is to have a backup pixmap not per-top-level-parent,  
but per-window.  This allows each window to be restored with the  
pixmap it originally had.
Another thing necessary to fix this scenario is for the top-level  
parent to be left in "drawing mode" (is_drawing set to TRUE, the  
scratch pixmap remaining allocated) for as long as any of its  
child windows are in drawing mode (their pixmap is set to the  
scratch pixmap).  There are two approaches to accomplishing this:  
1) when RootlessStopDrawing is called on any window, it is  
propagated to all children of the top-level parent; or 2)  
is_drawing is changed from a flag to a count of how many children  
of the top-level parent are in drawing mode, and only exit drawing  
mode for the top-level parent when all of the children have exited  
drawing mode.  I don't know enough to know which is better.
There may be more problems that I haven't fully identified yet.   
What do you think?  In particular, how would you suggest the  
problem with ending drawing mode for the top-level parent when  
multiple children are still in drawing mode?
[...]
I've implemented a fix based on my analysis.  Read on...
On Aug 21, 2006, at 5:09 PM, Ken Thomases wrote:
...
Scenario 1: Calling RootlessStartDrawing twice on a window
[...]
The fix for this case is to not overwrite WINREC(T)->oldPixmap if  
the pixmap of Window A is already equal to WINREC(T)->pixmap.
Did this.
...
Scenario 2: Calling RootlessStartDrawing on multiple windows with  
the same top-level parent.
[...]
A partial fix is to have a backup pixmap not per-top-level- 
parent, but per-window.  This allows each window to be restored  
with the pixmap it originally had.
I did this a bit inelegantly, using a second window private  
instead of juggling with the existing one.
...
Another thing necessary to fix this scenario is for the top-level  
parent to be left in "drawing mode" (is_drawing set to TRUE, the  
scratch pixmap remaining allocated) for as long as any of its  
child windows are in drawing mode (their pixmap is set to the  
scratch pixmap).  There are two approaches to accomplishing this:  
1) when RootlessStopDrawing is called on any window, it is  
propagated to all children of the top-level parent; or 2)  
is_drawing is changed from a flag to a count of how many children  
of the top-level parent are in drawing mode, and only exit  
drawing mode for the top-level parent when all of the children  
have exited drawing mode.  I don't know enough to know which is  
better.
I tried both.  Method 1 works well for the most part.  It  
occasionally causes updates to be lost.  RootlessStopDrawing is  
applying to all windows within a hierarchy, and I think it's  
"premature" for some.  They revert to the original framebuffer  
pixmap, which, if I understand correctly, is a memory buffer which  
is never really used in rootless mode.  Basically, I think this  
happens in the case where it would otherwise crash.  Fixing this  
seems to require a deeper rethinking of the rootless  
implementation.  For now, I'm going to leave that to you.
Method 2 did not work well, at all.  It resulted in no windows  
being shown on the screen.  I also got a number of libXplugin  
complaints about a window being locked when it shouldn't be.   
Essentially, RootlessStopDrawing is not called for every window on  
which RootlessStartDrawing is called.  In fact,  
RootlessStopDrawing is often called on a window for which  
RootlessStartDrawing was *not* called.  It seems it's generally  
called on the top-level window, whereas RootlessStartDrawing is  
often called on windows deeper in the hierarchy.  (This fact seems  
to confirm my method 1 approach.)  The consequence is that the  
is_drawing count was never balanced, and the real meat of  
RootlessStopDrawing was never invoked.
I've had a little time to look over your work and I think you are  
right on about the problem if not the solution. It looks pretty  
convincing that in either "Scenario 1" or "Scenario 2" you can get  
an invalid Pixmap set for a window, which later causes problems.  
Some of things you noticed in your fix are intended, however. In  
particular:
1. Rootless Start/Stop Drawing calls are not intended to be  
balanced and generally won't be.
2. StopDrawing is mainly called on the top level windows only.
3. Recording exactly which pixmap a window had is good form but not  
really necessary in practice, at least until we integrate with  
Composite better. All the windows start with the screen pixmap as  
their pixmap. We can just reset the pixmap to the screen pixmap and  
not bother with tracking if we want.
4. In debugging I see that we do commonly lose the oldPixmap.
5. Since we don't generally stop drawing in each subwindow many of  
the subwindows will not get their pixmaps reverted to the screen  
pixmap correctly even once we fix the tracking issue with the  
oldPixmap. The most straightforward way to fix that is to walk the  
window tree below each window on StopDrawing to reset the window  
pixmaps but that could be inefficient.
I'll have to think this weekend about the best way to fix this. You  
have definitely pointed out and interesting problem.

...
To summarize the previous discussion: we were having some crashes  
due to the rootless implementation.  I diagnosed the cause of the  
crashes, and fixed them.  This left us with occasional rendering  
failures -- drawing would just fail to go to the screen.  I was  
under the impression that the drawing failures were left over from  
the crashing bug -- that my fix had essentially transformed crashes  
to mere data loss.
The good news is that I was wrong.  The better news is that we have  
found and fixed the drawing failures.  On top of that, this has  
given me confidence which I didn't have when I last wrote that my  
fix for the rootless crashes is actually a complete fix, rather than  
a half-assed fix.
I'm attaching a couple of patches which fixed the drawing failures.   
The problem is that some functions would unwrap a dispatch table  
and, for some code paths, fail to rewrap them before completing.   
These patches are against X.org 6.8.2-era source, but not all that  
much seems to have changed.
[http://gitweb.freedesktop.org/?p=xorg/xserver.git;a=commit;h=8606c76a904d550... 
]

Ben Byer
CoreOS / BSD Technology Group, XDarwin maintainer

Ben Byer

tags

participants (1)