Bug 1032

I'd see if GetDrawablePixmap (damage.c:65) returns a pixmap which hasn't been
correctly initialized (no pixmap private allocated).  Is it possible that
DamageSetup hasn't been called?

(at some point, we should be able to use damage and composite to implement
pieces of the rootless code too).

Perhaps bug 961 is related?

I've whacked Damage to deal with DDXen which have no GetWindowPixmap (or where
GetWindowPixmap returns NULL).  Would you check and see if this helps XDarwin at
all?

Damage still have issues. Some notes:

DamageSetup() is being called; it is called twice actually. It is called first by miSpriteInitialize() and once 
by DamageExtensionInit().

A new backtrace:

#0  0x001d6938 in damageDamageRegion (pDrawable=0x57588a0, pRegion=0xf005fbf0, clip=1) at 
damage.c:166
#1  0x001d6f28 in damageDamageBox (pDrawable=0x57588a0, pBox=0xf005fc60) at damage.c:288
#2  0x001da1cc in damagePolyRectangle (pDrawable=0x57588a0, pGC=0x5755560, nRects=1, 
pRects=0x210d40c) at damage.c:1004
#3  0x00039ddc in ProcPolyRectangle (client=0x57541e0) at dispatch.c:1904
#4  0x00034868 in Dispatch () at dispatch.c:455
#5  0x0000a624 in main (argc=2, argv=0xbffffe68, envp=0xbffffe74) at main.c:442

It looks like pDamage is garbage. For this crash it was:

(gdb) print *pDamage
$2 = {
  pNext = 0xa0a0a038, 
  pNextWin = 0xa0a05a18, 
  damage = {
    extents = {
      x1 = -28493, 
      y1 = 15668, 
      x2 = 0, 
      y2 = 0
    }, 
    data = 0x189
  }, 
  damageLevel = DamageReportNone, 
  isInternal = 0, 
  closure = 0xc091b0, 
  isWindow = -1870004328, 
  pDrawable = 0x0, 
  damageReport = 0, 
  damageDestroy = 0
}

Also note the generic rootless layer does try to setup a trivial screen pixmap. See 
RootlessUpdateScreenPixmap() in miext/rootless/rootlessScreen.c for details.

Could there be a problem with the ordering of the wrapping? Rootless wraps screen functions so it can 
put valid window pixmaps in place as they are needed. Subsequent drawing calls to a window are 
grouped together, but between drawing calls groups the window pixmaps are not valid.

Yes, that pointer is clearly garbage...

DamageSetup should be invoked by any subsystem which uses the miext/damage code
-- software cursors use it, the miext/shadow frame buffer code uses it, and both
the Damage and Composite extensions use it.  That function should be dealing
with multiple invocations just fine.

Damage uses pixmap privates to hold a list of clients with interest in damage to
a particular pixmap -- it uses pixmaps so that rendering to any window in the
hierarchy which shares the same pixmap will be noted as damage.

It expects that the pixmap for each window will not change except through calls
to SetWindowPixmap, which it should handle correctly.

I fear you're correct about the initialization order; it's always tricky to get
things layered correctly in the X server, and with multiple sub-systems using
the Damage code, it's probably even harder...

Damage must be initialized *after* all of the rendering functions are set in the
screen structure.  For the Composite extension to work properly, it must be *on
top* of the new Composite Wrapper interface.

If the rootless code is swapping window pixmaps around, it may be that the
damage code needs to be underneath that though, just as it must be underneath
the Composite code (which does the same thing).

A stack trace from something like damageCreateGC might show us where the order
is incorrect.

Switching the initialization order to make sure DamageSetup() is run before RootlessInit() fixed the 
crash.

There is probably a small issue remaining, but it is likely not trivial to fix. The generic rootless layer will 
sometimes call accelerated drawing functions instead of unwrapping and calling GC ops below it. In this 
case, Damage would never know a part of the window had been changed. Probably the best fix is to just 
have the accelerated routines call the corresponding Damage routine directly. Any thoughts?

In any case the major blocker has been fixed. I'm not sure I'll have time to fix this other issue until late 
next week.

The release wranglers decided to leave this as a blocker for now to keep track
of whether the bug can be fixed before the release, but it won't hold up the
release if the fix is too invasive or can't be fixed in time.

Moving to "held open" bug since this is no longer a blocker

Created attachment 668 [details] [review]
Patch to make rootless acceleration compatible with Damage

This patch makes rootless acceleration compatible with Damage. It does this by
pulling the rootless acceleration wrapping out of the generic rootless layer's
CreateGC function. Rootless acceleration must now be separately initialized
after fb but before DamageSetup(). The patch adds the new initialization
function, RootlessAccelInit() to XDarwin's xpr and cr implementations and to
Cygwin's corresponding screen initialization. Cygwin guys, please check that
everything is kosher for you.

Could the other people who depend on the the rootless layer please test the
patch to see if it works for them and doesn't cause any other problems?  It
would be nice to get this fixed for the release but want to make sure that the
change does not cause other problems.

Hmpf. Even with the above patch there is at least one place that the damage functions will not get 
called. The rootless screen function for CopyWindow nevers calls to any underlying wrapped 
CopyWindow function. The relevant code is RootlessCopyWindow() in Xserver/miext/rootless/
rootlessWindow.c. One solution is to split RootlessCopyWindow() into two levels of screen function 
wrapping but this is suboptimal and not trivial to get right for this function. Probably the best solution 
is just to call DamageDamageRegion() directly inside RootlessCopyWindow(). Is there anything to watch 
out for? What tests can I use to verify that damage is actually workinig?

Comment on attachment 668 [details] [review]
Patch to make rootless acceleration compatible with Damage

The patch for rootless acceleration has been applied. RootlessCopyWindow()
still needs to be fixed for Rootless to be completely compatible with Damage.

aiui Damage is still disabled in the rootless servers, so this isn't a blocker
for 7.

(In reply to comment #13)
> aiui Damage is still disabled in the rootless servers, so this isn't a blocker
> for 7.

Kicking to 7.2, not strong enough to block 7.1.

shunting off the radar, since we don't appear to have any more maintained
rootless ddxes.  if someone really cares, a big FIXME should do the trick.

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.