I am still seeing crashes with the damage extension when using the generic rootless layer. (This bug is related to #1032, which is also about damage-rootless incompatibility, but is a different problem with a different cause.) Here is a sample back trace: #0 0x001d4ca8 in damageCopyArea (pSrc=0x21ae000, pDst=0x5a94760, pGC=0x5a743a0, srcx=0, srcy=0, width=5, height=15, dstx=4, dsty=2) at damage.c:744 #1 0x00038ce8 in ProcCopyArea (client=0x5a704f0) at dispatch.c:1758 #2 0x00034868 in Dispatch () at dispatch.c:455 #3 0x0000a624 in main (argc=3, argv=0xbffffb0c, envp=0xbffffb1c) at main.c:442 Notice that RootlessCopyArea() is not in the backtrace. This is because the destination drawable is an unviewable window. Damage is crashing on checkGCDamage(pDst, pGC). Rootless does not do anything with windows until they are realized. It looks like Damage is crashing because it is trying to find a valid pixmap for an unrealized window. Should Damage be tracking unrealized windows? Any ideas on how to resolve this?
As long as GetWindowPixmap returns NULL for unrealized windows, the damage code should operate correctly even in this case. However, we can easily short-circuite the code in the unrealized window case with a simple patch. Can you test the diff I've attached?
Created attachment 715 [details] [review] Check for unviewable windows patch to skip unviewable windows
Created attachment 721 [details] [review] Check for unviewable windows in getDrawableDamageRef() The previously proposed patch for unviewable windows does not fix the crash although it almost does. This is because the crash is in checkGCDamage(), which uses getDrawableDamage() and not getWindowDamage(). The patch attached here does fix the crash. I think this is how we should go, modulo any code style changes you might prefer. You are right that the interaction problem is that Rootless does not set the window pixmap to NULL for unrealized windows. Glancing at the code it appears that if it did set these to NULL, Damage would have fallen back to the screen pixmap and been happy. Changing Rootless in this way is likely to be higher risk/impact, however.
Keith, is the patch suggested by Torrey correct/sufficient?
No, the suggested patch isn't correct. DamageRegister will do "bad things" as all unviewable windows will end up getting Damage structures hooked up to the global variable. I don't have a good suggestion at this point; Damage expects that GetWindowPixmap will either return legit data or NULL; there isn't any way to avoid calling it and still have the Damage structures connected to the right place.
(In reply to comment #5) > I don't have a good suggestion at this point Okay, since we are close to the release (Tuesday), we need to decide if this is truly a blocker that must hold up the release or if we can go ahead with the release as is and fix it later.
I can't see how to fix this in Damage, which means any fix would be in the rootless code. It seems too close to the release to try and fix that in scary ways at the moment. I suggest we disable Damage and Composite on any X servers using the rootless layer for this release and plan on placing the simple-but-scary fix in the rootless code after the release.
Yes, this bug is definitely a release blocker for any X servers using rootless in combination with damage. I don't mind disabling damage for rootless X servers for this release, but last I checked it looked like there were issues building without damage. I'll check....
Regardless of the short term resolution for this Damage-Rootless incompatibility, I want to clarify the issues so others can better understand them and/or correct my misunderstandings. The current rootless policy is to leave the window pixmap as rootless found it. When needed for a drawing operation, rootless asks the implementation what the pixmap should be. It stores the old pixmap (whatever it was) replaces it with the new pixmap and then lets drawing proceed. When drawing is finished, rootless puts the old pixmap back into place. Rootless does not care about what this "non- drawing pixmap" is and never looks at it. At one point in development putting back the original pixmap was necessary, but it has been a long time now so I don't know if this is still true. It is also possible that non-Mac OS X rootless implementations could use this non-drawing pixmap for something so NULLing it out would be a bad idea. None of these are show stoppers for making the change Keith suggested, but it points out why it is not a good 11th hour change to make. As he mentioned, maybe after the release it would make sense. On the other hand I would argue that after the release it would make more sense to have Damage not try to track unviewable windows at all. Requiring anything of unviewable windows does not seem like the best design. My patch intended to make Damage ignore unviewable windows. I see that it has side effects, but I think this is still a good goal. There are a number of other ways Damage could fairly easily be made to ignore unviewable windows. My patch just had the smallest line diff count. (Unfortunately it was too simple.) It is probably too late for even this kind of simple change to Damage, but it would be good to get consensus on what the appropriate strategy should be after the release.
Now I'm confused -- there is a putative underlying pixmap which should be able to support Damage for unmapped windows, and you call SetWindowPixmap when you 'rootless-ify' windows which Damage will track and automatically migrate the damage records to. Why doesn't this work? Is there just a tiny bug in the code which is supposed to manage this underlying pixmap? If we can't get this working, I suggest that we take your original patch and enable it for rootless servers so that Damage will at least not crash. Adding a config file value to disable per-window pixmaps for unrealized windows seems like a straightforward kludge-around for the moment.
Created attachment 798 [details] [review] workaround patch Keith, is this patch what you have in mind for the workaround? If so, then we should probably go with it for this release.
>Now I'm confused -- there is a putative underlying pixmap which should be able >to support Damage for unmapped windows, and you call SetWindowPixmap when you >'rootless-ify' windows which Damage will track and automatically migrate the >damage records to. I'm not sure what the underlying pixmap is, rootless does not really care about it or create it. Rootless keeps around whatever is set by fb when windows are created. This is the screen pixmap? In rootless mode the screen pixmap is purposely tweaked to essentially not have any size. See RootlessUpdateScreenPixmap() in miext/rootless/rootlessScreen.c for details if you are curious. You don't want a big screen pixmap sucking up memory if you are never going to use it. >Why doesn't this work? Is there just a tiny bug in the code which is supposed >to manage this underlying pixmap? Good question. I don't know, but I do plan to look into this when I have time. FWIW, I did try the trivial change of setting all window pixmaps to NULL when I was not drawing to them and I got the following crash backtrace: 0 org.xfree86.XDarwin 0x0005cd74 fbPutXYImage + 0x70 1 ??? 0x007e4974 rlImageGlyphBlt + 0x2b0 2 org.xfree86.XDarwin 0x001035dc miImageText8 + 0x70 3 org.xfree86.XDarwin 0x000f74b4 damageImageText8 + 0xe0 4 org.xfree86.XDarwin 0x0001a3f8 doImageText + 0x1fc 5 org.xfree86.XDarwin 0x0001a514 ImageText + 0x74 6 org.xfree86.XDarwin 0x0001fe4c ProcImageText8 + 0x198 7 org.xfree86.XDarwin 0x0001b8e4 Dispatch + 0x228 8 org.xfree86.XDarwin 0x000032f8 main + 0x5dc 9 org.xfree86.XDarwin 0x0004a694 -[XServer run] + 0x74 10 com.apple.Foundation 0x90a39b74 forkThreadForFunction + 0x6c 11 libSystem.B.dylib 0x900246e8 _pthread_body + 0x28 The lack of RootlessImageText8 in the backtrace means the drawable was not a viewable window. I think that other layers need some trivial but real pixmap in place even when no drawing is going to take place. I don't know why this does not work for Damage. I can spend more time on this once the release is not breathing down our backs.
On today's release wranglers call, we decided to set a deadline for patches of today at 6pm EDT. If patches are available by then, are small and self-contained, they can still be integreated into the release. Otherwise, we will move the remaining open bugs to the release notes bug 999 for documentation. If it is possible to come up with a better solution today, that would be great; however, if not, would the workaround patch that I added be sufficient for this release?
Workaround patch checked in after Torrey reviewed it. Closing.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.