nanogui: Thread: fixed: potentional buggy of 0.88pre11:nanox

/*
 * Destroy windows and eventclient structures used by client.
 * Called by GsDropClient after a client has exited to clean
 * up resources.
 */
void
GsDestroyClientResources(GR_CLIENT * client)
{
 GR_WINDOW     * wp;
 GR_WINDOW     * nwp;
 GR_PIXMAP     * pp;
 GR_PIXMAP     * npp;
 GR_EVENT_CLIENT *ecp;
 GR_EVENT_CLIENT *necp;
 GR_EVENT_CLIENT *pecp = NULL;

 /* search window list, destroy windows owned by client*/
 for(wp=listwp; wp; wp=nwp) {
  nwp = wp->next;
  if (wp->owner == client) {
+#if 1//jmt: nanox0.88pre11buggy fixed
+   /*
+    * Remove eventclient structures for
+    * windows this client did own
+    */
+   ecp = wp->eventclients;
+   while (ecp) {
+    necp = ecp->next;
+    if (ecp->client == client) {
+     if (ecp == wp->eventclients)
+      wp->eventclients = ecp->next;
+     else
+      pecp->next = ecp->next;
+     free(ecp);
+    } else
+     pecp = ecp;
+    ecp = necp;
+   }
+#endif
   /* FIXME: change to call GsDestroyWindow()*/
    GsWpUnmapWindow(wp);
   GsDeliverUpdateEvent(wp, GR_UPDATE_UNMAP, 0, 0, 0, 0);
   GsWpDestroyWindow(wp);
  } else {
   /*
    * Remove eventclient structures for
    * windows this client didn't own
    */
   ecp = wp->eventclients;
   while (ecp) {
    necp = ecp->next;
    if (ecp->client == client) {
     if (ecp == wp->eventclients)
      wp->eventclients = ecp->next;
     else
      pecp->next = ecp->next;
     free(ecp);
    } else
     pecp = ecp;
    ecp = necp;
   }
  }
 }

 /* search pixmap list, destroy pixmaps owned by client*/
 for(pp=listpp; pp; pp=npp) {
  npp = pp->next;
  if (pp->owner == client)
   GrDestroyWindow(pp->id);
 }
}

Hello!

In the "soon to be released bug-fixes mail" by me some time back, we
fixed this too, slightly different.  Since we know which GR_EVENT_LIST
needs to be destroyed completely (the one associated with the client
to be cleaned up), we did it the simple way and just free it after all
the other code has run in GsDestroyClientResources.  I can't see how
this might fail.

We have been busy moving to a new office location the last couple of
days, but we are online again now, and here is one set of patches that
we have discovered.  Some of the patches (memory leak above) etc. where
detected by using the "Checker" utility or a commercial license of
purify.

The patch is against 0.88pre8, unfortunatlely, but are from what I have
read, still relevant and against code that hasn't changed much.

The "client craches => nano-X server craches IF mouse button pressed"
problem was tracked down as the result of a more deliberate bug-hunt.

I have no idea how the clipping code error was discovered -- I have not
talked to the developer about this.  You may want to verify that it is
a correct and relevant patch.  I'll forward questions regarding the
clipping code patch to the developer if there are any.

There are more patches I try to clean up and include in our CVS tree
that we are currently building; I will keep sending patches as this
work continues.  Be prepared for some new features as well.

Regards,
Morten Rolland, Screen Media

PS: I include the patch both as an attachent and text.


Index: ChangeLog
===================================================================
RCS file: /sw/CVSROOT/microwin/src/ChangeLog,v
retrieving revision 1.1.1.1
retrieving revision 1.3
diff -u -6 -r1.1.1.1 -r1.3
--- ChangeLog   2000/07/28 09:44:06     1.1.1.1
+++ ChangeLog   2000/07/28 18:13:51     1.3
@@ -1,6 +1,27 @@
+2000-07-28  Jan Vehusheia
+
+       * nanox/srvnet.c  Free allocated events to freelist when destroying
+         a client.
+       * nanox/srvutil.c Correctly reassign mouse pointer information when
+         an application crashes while a mouse button is held pressed down.
+
+2000-07-28  Jon K Hellan  ####@####.####
+
+       * nanox/srvmain.c (GsInitialize): Initialize wp->owner to NULL.
+       * nanox/srvnet.c  Remove call to GsWpUnmapWindow and
+         GsDeliverUpdateEvent. GsWpUnmapWindow is called anyway by
+         GsWpDestroyWindow.
+       * engine/devrgn.c (REGION_SubtractO): Add a missing test for
+         having finished. 
+       * engine/devfont.c (GdCreateFont): Don't crash on NULL font name.
+       * drivers/kbd_x11.c (X11_Read): Initialize modifiers also when no
+         key press pending.
+       * demos/nanox/demo5.c (main): Correct height of bitmap2(fg|bg) in
+         GrSetCursor call.
+
 Version 0.88pre8 - 23rd May 2000 - ####@####.####
        * added Murphy's X vs Microwindows speed tester in contrib/speedtst
        * fixed convbdf to not emit nonascii characters
        * added Victor's DJGPP port
        * removed automatic portrait mode
        * added extern "C" to nano-X.h
Index: demos/nanox/demo5.c
===================================================================
RCS file: /sw/CVSROOT/microwin/src/demos/nanox/demo5.c,v
retrieving revision 1.1.1.1
retrieving revision 1.2
diff -u -6 -r1.1.1.1 -r1.2
--- demos/nanox/demo5.c 2000/07/28 09:44:08     1.1.1.1
+++ demos/nanox/demo5.c 2000/07/28 17:25:20     1.2
@@ -137,13 +137,13 @@
        bitmap2bg[2] = MASK(X,X,X,X,X,X,X);
        bitmap2bg[3] = MASK(X,X,X,X,X,X,X);
        bitmap2bg[4] = MASK(_,X,X,X,X,X,_);
        bitmap2bg[5] = MASK(_,_,X,X,X,_,_);
 
        GrSetCursor(w1, 7, 7, 3, 3, WHITE, BLACK, bitmap1fg, bitmap1bg);
-       GrSetCursor(w2, 7, 7, 3, 3, WHITE, BLACK, bitmap2fg, bitmap2bg);
+       GrSetCursor(w2, 7, 6, 3, 3, WHITE, BLACK, bitmap2fg, bitmap2bg);
 
        GrRect(GR_ROOT_WINDOW_ID, gc1, 0, 0, si.cols, si.rows);
 
 
        while (1) {
                GrCheckNextEvent(&event);
Index: drivers/kbd_x11.c
===================================================================
RCS file: /sw/CVSROOT/microwin/src/drivers/kbd_x11.c,v
retrieving revision 1.1.1.1
retrieving revision 1.2
diff -u -6 -r1.1.1.1 -r1.2
--- drivers/kbd_x11.c   2000/07/28 09:44:06     1.1.1.1
+++ drivers/kbd_x11.c   2000/07/28 17:25:20     1.2
@@ -77,16 +77,17 @@
 
 static int
 X11_Read(MWUCHAR *buf, int *modifiers)
 {
     XEvent ev;
 
+    *modifiers = 0;         /* no modifiers yet */
+    
     /* check if we have a KeyPressedEvent */
     if (XCheckMaskEvent(x11_dpy, KeyPressMask, &ev)) {
        KeySym sym = XKeycodeToKeysym(x11_dpy, ev.xkey.keycode, 0);
-       *modifiers = 0;         /* no modifiers yet */
        if (sym == NoSymbol)
            return -1;
        else if (sym == XK_Escape) {
            if (ev.xkey.state & ControlMask) { 
                /* toggle grab control */
                if (grabbed) {
Index: engine/devfont.c
===================================================================
RCS file: /sw/CVSROOT/microwin/src/engine/devfont.c,v
retrieving revision 1.1.1.1
retrieving revision 1.2
diff -u -6 -r1.1.1.1 -r1.2
--- engine/devfont.c    2000/07/28 09:44:07     1.1.1.1
+++ engine/devfont.c    2000/07/28 17:25:20     1.2
@@ -172,20 +172,20 @@
        char            fontname[128];
 
        GdGetScreenInfo(psd, &scrinfo);
 
        /* if plogfont not specified, use name and height*/
        if (!plogfont) {
-               strcpy(fontname, name);
+               strcpy(fontname, name ? name : "");
                fontclass = MWLF_CLASS_ANY;
        } else {
 #if FONTMAPPER
                /* otherwise, use MWLOGFONT name and height*/
                fontclass = select_font(plogfont, fontname);
 #else
-               strcpy(fontname, name);
+               strcpy(fontname, name ? name : "");
                fontclass = MWLF_CLASS_ANY;
 #endif
                height = plogfont->lfHeight;
        }
        height = abs(height);
  
Index: engine/devrgn.c
===================================================================
RCS file: /sw/CVSROOT/microwin/src/engine/devrgn.c,v
retrieving revision 1.1.1.1
retrieving revision 1.2
diff -u -6 -r1.1.1.1 -r1.2
--- engine/devrgn.c     2000/07/28 09:44:07     1.1.1.1
+++ engine/devrgn.c     2000/07/28 17:25:20     1.2
@@ -1212,13 +1212,14 @@
                pNextRect->right = r1->right;
                pNextRect->bottom = bottom;
                pReg->numRects += 1;
                pNextRect++;
            }
            r1++;
-           left = r1->left;
+           if (r1 != r1End)
+               left = r1->left;
        }
     }
 
     /*
      * Add remaining minuend rectangles to region.
      */
Index: nanox/srvmain.c
===================================================================
RCS file: /sw/CVSROOT/microwin/src/nanox/srvmain.c,v
retrieving revision 1.1.1.1
retrieving revision 1.2
diff -u -6 -r1.1.1.1 -r1.2
--- nanox/srvmain.c     2000/07/28 09:44:08     1.1.1.1
+++ nanox/srvmain.c     2000/07/28 17:25:20     1.2
@@ -439,12 +439,13 @@
 
        /*
         * Initialize the root window.
         */
        wp->psd = psd;
        wp->id = GR_ROOT_WINDOW_ID;
+       wp->owner  = NULL;
        wp->parent = NULL;              /* changed: was = NULL*/
        wp->children = NULL;
        wp->siblings = NULL;
        wp->next = NULL;
        wp->x = 0;
        wp->y = 0;
Index: nanox/srvnet.c
===================================================================
RCS file: /sw/CVSROOT/microwin/src/nanox/srvnet.c,v
retrieving revision 1.1.1.1
retrieving revision 1.3
diff -u -6 -r1.1.1.1 -r1.3
--- nanox/srvnet.c      2000/07/28 09:44:08     1.1.1.1
+++ nanox/srvnet.c      2000/07/28 18:13:54     1.3
@@ -841,20 +841,18 @@
        GR_WINDOW     * nwp;
        GR_PIXMAP     * pp;
        GR_PIXMAP     * npp;
        GR_EVENT_CLIENT *ecp;
        GR_EVENT_CLIENT *necp;
        GR_EVENT_CLIENT *pecp = NULL;
+       GR_EVENT_LIST   *evp;
 
        /* search window list, destroy windows owned by client*/
        for(wp=listwp; wp; wp=nwp) {
                nwp = wp->next;
                if (wp->owner == client) {
-                       /* FIXME: change to call GsDestroyWindow()*/
-                       GsWpUnmapWindow(wp);
-                       GsDeliverUpdateEvent(wp, GR_UPDATE_UNMAP, 0, 0, 0, 0);
                        GsWpDestroyWindow(wp);
                } else {
                        /*
                         * Remove eventclient structures for
                         * windows this client didn't own
                         */
@@ -877,12 +875,21 @@
        /* search pixmap list, destroy pixmaps owned by client*/
        for(pp=listpp; pp; pp=npp) {
                npp = pp->next;
                if (pp->owner == client)
                        GrDestroyWindow(pp->id);
        }
+
+       /* Free events associated with client */
+       evp = client->eventhead;
+       while ( evp ) {
+               client->eventhead = evp->next;
+               evp->next = eventfree;
+               eventfree = evp;
+                evp = client->eventhead;
+        }
 }
 
 /*
  * This is used to drop a client when it is detected that the connection to
it
  * has been lost.
  */
Index: nanox/srvutil.c
===================================================================
RCS file: /sw/CVSROOT/microwin/src/nanox/srvutil.c,v
retrieving revision 1.1.1.1
retrieving revision 1.2
diff -u -6 -r1.1.1.1 -r1.2
--- nanox/srvutil.c     2000/07/28 09:44:08     1.1.1.1
+++ nanox/srvutil.c     2000/07/28 18:13:54     1.2
@@ -208,12 +208,14 @@
        }
        if (wp == focuswp) {
                focusfixed = GR_FALSE;
                focuswp = rootwp;
        }
 
+       GsCheckMouseWindow();
+
        free(wp);
 }
 
 /*
  * Clear the specified area of a window and possibly make an exposure event.
  * This sets the area window to its background color.  If the exposeflag is

: In the "soon to be released bug-fixes mail" by me some time back, we
: fixed this too, slightly different.  Since we know which GR_EVENT_LIST
: needs to be destroyed completely (the one associated with the client
: to be cleaned up), we did it the simple way and just free it after all
: the other code has run in GsDestroyClientResources.  I can't see how
: this might fail.

I have added this to 0.88, which I plan to release tonight.  In your patch,
I didn't however, remove the "rendundant" calls to GsWpUnmapWindow,
since the window isn't always unmapped by GsWpDestroyWindow
as the developer suggested.

I've also added Kevin's eventclient free() code, since it seems that
both patches free different things: yours frees events associated
with the client, and Kevin's frees eventclient structures associated with
the client.  I hope I got this right.  Perhaps both of you could check
the new nanox/srvnet.c.


: The "client craches => nano-X server craches IF mouse button pressed"
: problem was tracked down as the result of a more deliberate bug-hunt.

Added.


:
: I have no idea how the clipping code error was discovered -- I have not
: talked to the developer about this.  You may want to verify that it is
: a correct and relevant patch.  I'll forward questions regarding the
: clipping code patch to the developer if there are any.

I pulled the mult-rectangle regions code out from the X server
sources, and I just checked this again, and the current X server
sources don't include this bugfix.  Because the region intersect/or/xor/subtract
code is so complicated, I don't want to change this unless I can
see an example of this region bug.


: +       * nanox/srvnet.c  Remove call to GsWpUnmapWindow and
: +         GsDeliverUpdateEvent. GsWpUnmapWindow is called anyway by
: +         GsWpDestroyWindow.

Not patched.


: +       * engine/devrgn.c (REGION_SubtractO): Add a missing test for
: +         having finished.

Not patched.

Everything else was added.

Regards,

Greg

Hello!

I looked at the new version just a little bit, and the freeing of client
resources looks good.

> In your patch,
> I didn't however, remove the "rendundant" calls to GsWpUnmapWindow,
> since the window isn't always unmapped by GsWpDestroyWindow
> as the developer suggested.

OK, must talk to the developer about this.

> I pulled the mult-rectangle regions code out from the X server
> sources, and I just checked this again, and the current X server
> sources don't include this bugfix.  Because the region intersect/or/xor/subtract
> code is so complicated, I don't want to change this unless I can
> see an example of this region bug.

I agree, until we have an example of error...

Regards,
Morten Rolland, Screen Media