Created attachment 80924 [details]
test case

test case to expose the bug

Created attachment 80925 [details] [review]
patch

It is believed that , what is happening is that the window was moved to 505, 222 by the first map adding the frame. The XWithdrawWindow call causes the window manager to do XReparentWindow setting x and y back to 500, 200.
  
The XtVaSetValues RootGeometryManager code sees the oldx, oldy values as 505,
222 and therefore the XConfigureWindow request as changing the x and y to 500,
200. The request is redirected to the window manager, since the window is
withdrawn it just re-issues the XConfigureWindow call (see display.c in the
metacity code about line 2010). However, the server sees this as a null-op so
doesn't generate the ConfigureNotify event but the Xt code expects a change in
position so waits for that event.

The relevant mwm code for handling this is in WMManage.c from mwm :

/*
* Get window attribute information; this is used later on
* to decide if a synthetic ConfigureNotify event should
* be send to the client.
*/

if (WmGetWindowAttributes (configureEvent->window))
{
   configChanged =
(wmGD.windowAttributes.x != configureEvent->x) ||
(wmGD.windowAttributes.y != configureEvent->y) ||
(wmGD.windowAttributes.width != configureEvent->width) ||
(wmGD.windowAttributes.height != configureEvent->height) ||
(wmGD.windowAttributes.border_width !=
      configureEvent->border_width) ||
(configureEvent->value_mask & (CWSibling|CWStackMode));

           /*
            * The window is not (yet) managed.  Do the window
    * configuration.
            */

   if (configChanged)
   {
       values.x = configureEvent->x;
       values.y = configureEvent->y;
       values.width = configureEvent->width;
       values.height = configureEvent->height;
       values.border_width = configureEvent->border_width;
       values.sibling = configureEvent->above;
       values.stack_mode = configureEvent->detail;
       XConfigureWindow (DISPLAY, configureEvent->window,
           (unsigned int) (configureEvent->value_mask), &values);
   }

   /*
    * Some clients expect a ConfigureNotify event even if the
    * XConfigureWindow call has NO effect.  Send a synthetic
    * ConfigureNotify event just to be sure.
    */

   if (!configChanged)
   {
       notifyEvent.type = ConfigureNotify;
       notifyEvent.display = DISPLAY;
       notifyEvent.event = configureEvent->window;
       notifyEvent.window = configureEvent->window;
       notifyEvent.x = configureEvent->x;
       notifyEvent.y = configureEvent->y;
       notifyEvent.width = configureEvent->width;
       notifyEvent.height = configureEvent->height;
       notifyEvent.border_width = configureEvent->border_width;
       notifyEvent.above = None;
       notifyEvent.override_redirect = False;

       XSendEvent (DISPLAY, configureEvent->window, False,
           StructureNotifyMask, (XEvent *)&notifyEvent);
           }
       }

Note the explanation of producing the events. Applying a similar patch to
metacity seems to have fixed things.  

Thanks, some minor comments

The ChangeLog note should go in ChangeLog rather than the comment at the top.

XGetWindowAttributes is a round trip, so at minimum the second arg to error_trap_pop after it would change. I think it might be better, though, to just always send the synthetic event because it will be cheaper than the round trip and I can't imagine it causing a problem.

I'm not sure this change is right, though. The general rule is that clients should see the same thing with or without a WM. In this case, if no WM were running, Xt would not get a configure event. So this change introduces a "dependency" on the WM to be running, doesn't it? I think this may simply be an Xt bug.

The other change (the non-withdrawn window case) I remember there are complex rules for and I don't remember what they are, so I'm not sure whether the change is right. Someone will need to research what ICCCM and EWMH have to say on this.

One stylistic thing though, no need to document or comment out bugs that used to exist; just fix the bug so the code looks correctly-written in the first place, then describe what was fixed in ChangeLog and leave the rest to SVN history.

I believe this may be the same as bug 322840, but I don't have time to investigate and verify at the moment.  But regardless the same information used to research that bug may be useful here, so...  That bug is about Metacity handling of ConfigureNotify events in responses to MapRequest events; due to my investigation of the issue I thought an EWMH extension/clarification was needed and brought it up on wm-spec-list at http://mail.gnome.org/archives/wm-spec-list/2006-April/msg00045.html.  I thought Dan had an insightful response at http://mail.gnome.org/archives/wm-spec-list/2006-May/msg00000.html about what the clarification should be.  My investigation of other WMs showed that they weren't affected by this bug due to just punting and sending excessive ConfigureNotify events (though it really doesn't seem to hurt much).

> In this case, if no WM were running, Xt would not get a configure event. So this
> change introduces a "dependency" on the WM to be running, doesn't it? I think 
> this may simply be an Xt bug.

With no window manager running, the dialog appears immediately . 

> With no window manager running, the dialog appears immediately .

Why? What am I missing?

> Why? What am I missing?

i'am a bit confused. this bug is not reproducible 
when using mwm, and no window manager afaik.

with other wm : icewm, blackbox, windowmaker, metacity this bug is seen.

I'm not arguing with you on when the bug happens, I'm asking why.
The theory is that Xt hangs when it gets no ConfigureNotify, right?

Well, with no WM, if you configure request a window to its actual current size, I believe you get no ConfigureNotify. (Is this wrong?) So my question is, why does Xt work in that case but not with metacity.

Created attachment 86200 [details] [review]
Alternative solution

This patch was based off the following part of the explanation from Ritesh:

  The XWithdrawWindow call causes the window manager to do XReparentWindow
  setting x and y back to 500, 200.  [However,] The XtVaSetValues
  RootGeometryManager code sees the oldx, oldy values as 505, 222 and
  therefore the XConfigureWindow request as changing the x and y to 500, 200.

So the app was essentially lied to, because I think the ICCCM requires us to tell the app about any change in position with ConfigureNotify events (or something like that, I've never been able to figure out what the real requirements are here) and we didn't do that.  I think this is the bug.  We should just inform the app of its correct position, then all future ConfigureRequests while withdrawn (if any) behave as expected.

Does this sound right?

IIRC the ICCCM doesn't categorically require synthetic ConfigureNotify; it requires ConfigureNotify in some cases where, since the size/position are unchanged from an X server perspective, the app would not get a ConfigureNotify. IIRC the main issue is we have to generate a ConfigureNotify when the frame moves, since the app's window is not moving relative to the frame, there is no ConfigureNotify from the server in that case. We thus essentially simulate the app not having a frame by sending a synthetic ConfigureNotify with coords relative to the root window.

The basic idea is to hide the existence of the frame from the app.

I guess the bug here is that the ReparentWindow does not result in a ConfigureNotify?

The thing I don't understand though is why Xt thinks it will get a ConfigureNotify when calling XWithdrawWindow, or why it gets one when no WM is running. That doesn't make any sense. Unmapping a window just plain shouldn't change its position.

Elijah, I'm a little skeptical of your comment about gravity in the patch, or don't understand it. The gravity just specifies how the WM interprets configure requests. I don't think it should change what's in a ConfigureNotify. The ConfigureNotify would always just have the actual X server position of the window with respect to its parent (or for a reparented window, with respect to the root window, which is the parent the app expects).

(In reply to comment #11)
> I guess the bug here is that the ReparentWindow does not result in a
> ConfigureNotify?

Yeah, this probably needs an EWMH 'clarification'.  See also bug 322840 and Dan's suggested clarification relative to that bug at http://mail.gnome.org/archives/wm-spec-list/2006-May/msg00000.html; under the same logic, I believe we'd need to send a ConfigureNotify here.

> The thing I don't understand though is why Xt thinks it will get a
> ConfigureNotify when calling XWithdrawWindow, or why it gets one when no WM is
> running. That doesn't make any sense. Unmapping a window just plain shouldn't
> change its position.

Unless I'm mistaken, unmapping does change the position of the client window under metacity due to the use of window->frame->rect.[xy] in the XReparentWindow call in meta_window_destroy_frame().

I'm pretty sure Xt doesn't get and doesn't expect to get a ConfigureNotify when no WM is running, due to the fact that it has correct information about the position of its window.  The problem is that Xt thinks it knows the position of the window from past ConfigureNotify events, but when running under metacity that isn't true due to the XReparentWindow call at the time the window is withdrawn.


> Elijah, I'm a little skeptical of your comment about gravity in the patch, or
> don't understand it. The gravity just specifies how the WM interprets configure
> requests. I don't think it should change what's in a ConfigureNotify. The
> ConfigureNotify would always just have the actual X server position of the
> window with respect to its parent (or for a reparented window, with respect to
> the root window, which is the parent the app expects).

Oh, right.  Ignore that.  I was just intently set on matching the FIXMEs in meta_window_destroy_frame() and send_configure_notify(), and went too far.

Ah ok, the "what I was missing" is that ReparentWindow lets you specify new coordinates.

The app could in theory look at the x,y in the ReparentNotify but I guess since we don't want apps to know about the reparenting into a frame, it would make sense to send a configure notify after reparenting.

Created attachment 86221 [details] [review]
Fix the FIXME(s), as pointed out by Havoc

I tested the patch earlier today and don't see the problem with the new patch, but could duplicate without it, so I'm committing the patch and marking this bug as fixed.  :)

Um, right.  Let's try that again:  I'm marking the bug as fixed...