Created attachment 151238 [details] [review]
Remove display from XDMCP factory store when the display is finished or fail

After investigation, I found it is because the display is not remove when it is finished. Same with what we did in daemon/gdm-local-display-factory.c, we should remove the display when its  status changed to GDM_DISPLAY_FINISHED and GDM_DISPLAY_FAILED in daemon/gdm-xdmcp-display-factory.c.

So one thing I don't understand is...The code already does:

display_dispose_check (factory, hostname, clnt_dspnum);

upon a new connection which looks like it should clean things up.  Do you know why that isn't working?

Also, I notice the other display types have a finish method that calls unmanage, but xdmcp doesn't.  I wonder if that's relevant.

I think I need to understand a little bit better what's going on here, before I'd feel comfortable committing attachment 151238 [details] [review]

(In reply to comment #2)
> So one thing I don't understand is...The code already does:
> 
> display_dispose_check (factory, hostname, clnt_dspnum);
> 
> upon a new connection which looks like it should clean things up.  Do you know
> why that isn't working?
With debugging, display_dispose_check() is invoked when getting REQUEST from remote hosts.

At that time the displays in store are not created as GDM_XDMCP_DISPLAY, so it return FALSE in remove_host(). The following messages should be helpful (some message are added in my local code base). 

Jan 14 12:06:52 judo gdm-binary[18541]: [ID 702911 daemon.warning] DEBUG(+): GdmXdmcpDisplayFactory: Received opcode REQUEST from client 129.158.217.94 : 388
98
Jan 14 12:06:52 judo gdm-binary[18541]: [ID 702911 daemon.warning] DEBUG(+): GdmXdmcpDisplayFactory: Got REQUEST from 129.158.217.94
Jan 14 12:06:52 judo gdm-binary[18541]: [ID 702911 daemon.warning] DEBUG(+): GdmXdmcpDisplayFactory: xdmcp_pending=0, MaxPending=4, xdmcp_sessions=0, MaxSess
ions=16, ManufacturerID=
Jan 14 12:06:52 judo gdm-binary[18541]: [ID 702911 daemon.warning] DEBUG(+): GdmXdmcpDisplayFactory: entered =1
Jan 14 12:06:52 judo gdm-binary[18541]: [ID 702911 daemon.warning] DEBUG(+): GdmXdmcpDisplayFactory: display_dispose_check (129.158.217.94:0)
Jan 14 12:06:52 judo gdm-binary[18541]: [ID 702911 daemon.warning] DEBUG(+): display_dispose_check count=0
Jan 14 12:06:52 judo gdm-binary[18541]: [ID 702911 daemon.warning] DEBUG(+): display_dispose_check is not xdmcp display
Jan 14 12:06:52 judo gdm-binary[18541]: [ID 702911 daemon.warning] DEBUG(+): GdmXdmcpDisplayFactory: Creating xdmcp display for 129.158.217.94:0

I think that is right behavior because this display is not managed during REQUEST phase, am I right? 
> 
> Also, I notice the other display types have a finish method that calls
> unmanage, but xdmcp doesn't.  I wonder if that's relevant.
I have no idea, do you need me do some trying?

> 
> I think I need to understand a little bit better what's going on here, before
> I'd feel comfortable committing attachment 151238 [details] [review]

Ping, does the patch looks good?

I tested this patch and it does allow for re-logging in after logging out using XDMCP (see bug #610264).  However, multiple logins (at the same time) using XDMCP does not seem possible.  Maybe this bug can address that also?

*** Bug 610264 has been marked as a duplicate of this bug. ***

When you say that multiple logins at the same time are not allowed, you need to provide more detail about this.  Note that GDM does have configuration options like xdmcp/DisplaysPerHost which is set to 1 by default, only allowing one login from the same machine at a time.  If you are trying to login to XDMCP multiple times from the same machine, then you need to configure GDM to allow this.  Refer to the manual:

http://library.gnome.org/admin/gdm/2.28/configuration.html.en#xdmcpsection

If this doesn't fix the problem, please edit /etc/gdm/custom.conf so it has these lines:

[debug]
Enable=true

Then, restart GDM.  Now GDM will output debug messages to your syslog (/var/log/messages or /var/adm/messages depending on your system).

Now try to login multiple times so that the failure happens, and after the failure happens, attach the gdm-related debug messages from the bottom of your syslog to the bug report so we can help with analysis.  Usually the debug messages will highlight what is going wrong.

Okay.  From the information you provided, there is no problem with multiple logins.  Increasing the number of displays per host for XDMCP allows multiple logins with no problems.

If there is no other fix for this bug, I recommend that this patch be applied so that there are no problems when logging out.

I can now report that this bug is not an issue in release 2.29.6.  Thanks.

Jose, I'm confused.  The fix for this bug did not go into 2.29.6.  Are you sure that the problem is not happening anymore?  

What version of GDM were you using before when it didn't work?  I don't see any fixes to the XDMCP code between version 2.29.5 and 2.29.6.

That's odd.  I was experiencing this on this system (ubuntu 10.04) before a recent gdm upgrade.  I wanted to see if the bug was still there after the upgrade out of curiosity and now the bug is gone.

Honestly, it's hard to know what was happening before.  All I can say is that I was experiencing this bug, the patch fixed it, removing the patch broke it, but this most recent upgrade does not exhibit the behavior.  I did not apply the patch for the bug to disappear.

Maybe it was inadvertently fixed while attending another issue. I can definitely confirm that I'm not using a locally compiled version but rather the system package that was provided with the upgrade.

Maybe the original reporter can test also.

(In reply to comment #11)
Jose, if I did not apply this patch on gdm-2.29.92 release on OpenSolaris, this bug is still there.

How did you test on Ubuntu then?

Here is my steps on OpenSolaris:
1) Edit /etc/gdm/custom.conf
   Add Enable=true in section [xdmcp]. Add Enable=true also in [debug] for issue tracking.
2) #svcadm restart gdm
   #svcadm enable xvnc-inetd
3) On client, vncviewer <my_server>,  close it after connected.
4) Do step 3) again
Expected result:
  vncviewer can connect my server more than once.

Actual result:
  vncviewer can not connect my server at second time.

My custom.conf file is similar to yours (see bug #610264).  The way I tested this patch was to use the apt-build tool on ubuntu so I could download the system package source, apply the patch and then have the tool re-build the package which could then be installed in place of the system package for testing.  This was the easiest way I could find to make sure the patch was good.  When I reverted to the system package, the bug re-appeared.

A few days later, there were one or two other gdm update for this system.  Since I know this bug has been reported, I would see if this bug disappeared, testing the same way you describe in steps 3 and 4 (making sure that gdm had been restarted, of course).  With a recent update, I noticed that multiple login/logout sessions were now possible so I thought the bug had been fixed.  But apparently it is not fixed for you.

Just now, I removed the apt-build package line from my system package repositories (so packages that apt-build made would not be available for installation), reloaded the sources and re-installed gdm (the same version that you just reported you used on OpenSolaris -- 2.29.92), rebooted the system (to be sure that everything new would be used), and tested multiple login/logout sessions witch Vnc successfully.  I just don't know what changed, but the bug has disappeared for me.

Oh well.  If more information is needed from me I'd be happy to provide it.

Now I'm experiencing the bug again.  I honestly don't know what's going on so please ignore my postings about this bug disappearing until the developers can determine what is really happening with this bug.

Could you apply the patch and try again?

The patch still applies cleanly and makes the bug disappear when it is exhibitted.

Nice, Ray and Brian, does it make sense to commit the patch?

I am seeing a probably related bug in 2.28 on Fedora 12 (see https://bugzilla.redhat.com/show_bug.cgi?id=562143 ), where gdm doesn't clean up properly after an xdmcp session ends resulting in gdm leaking file descriptors and eventually breaking when all the file descriptors are used up. I have proposed a patch https://bugzilla.redhat.com/attachment.cgi?id=399058 which fixes the problem I am seeing and may also fix your problem.

I tried the patch in comment #18 without my patch in comment #1. This bug still appears.

Now that 2.30.0 is out the door we should make sure we land this, the patch from comment 18, or some combination there of for the next stable release.

I committed the patch attached and the patch in comment #18, so marking this bug as fixed.