Can confirm with GDM 3.14.1 and a 1-letter password. Can't reproduce with a 3-letter password or more, I guess that's why it went unnoticed ;)

I run into this often with a longer password although I usually remember to wait now.  If you hit enter before it's "ready" (whatever that involves), it just hangs until you hit ESC or the cancel button.  At least the shield needs to be up and the password field needs to be fully activated, it seems to blink once.  I probably run into this easier, because I just start typing without raising the shield first and I'm usually coming back from a suspend which may slow things down a bit.

Currently gnome-shell 3.14 on Fedora 21.

Created attachment 300426 [details] [review]
authPrompt: Fix race condition during login

Currently when password is entered too quickly without lifting the
screen shield, then there is a race between gdmclient identifying the
service and authprompt trying to verify the password entry with the
service, due to which the login screen hangs. Ideally gdmclient should
identify the service first then authPrompt should try to verify the
password entry with that service but when typed in too quickly the
reverse happens.
To make the login proceed, password verification is redone after
setting _queryingService = serviceName.

(In reply to Shivam Mishra from comment #3)
> Created attachment 300426 [details] [review] [review]
> authPrompt: Fix race condition during login
> 
> Currently when password is entered too quickly without lifting the
> screen shield, then there is a race between gdmclient identifying the
> service and authprompt trying to verify the password entry with the
> service, due to which the login screen hangs. Ideally gdmclient should
> identify the service first then authPrompt should try to verify the
> password entry with that service but when typed in too quickly the
> reverse happens.
> To make the login proceed, password verification is redone after
> setting _queryingService = serviceName.

When password is typed too quickly without lifting the screen shield, then there seems to be a race condition between identifying the service name (in this case "gdm-password") done in _onAskQuestion() and verifying the password (this._userVerifier.answerQuery(this._queryingService, this._entry.text)) done after "next" signal is emitted. The 2nd one out runs the first one whereas the opposite should happen, i.e. first _onAskQuestion() should be called and set
	this._queryingService = serviceName;
then only
        this.connect('next', Lang.bind(this, function() {
                         this.updateSensitivity(false);
                         this.startSpinning();
                         if (this._queryingService) {
                             this._userVerifier.answerQuery(this._queryingService, this._entry.text);
                         } else {
                             this._preemptiveAnswer = this._entry.text;
                         }
                     }));
should be run.
To fix this, i could think of 3 solutions:
-Introduce some sort of delay before "if (this._queryingService)" during which "this._queryingService = serviceName;" would get executed in _onAskQuestion()
-Check after "this._queryingService = serviceName;" if there is an existing _preemptiveAnswer then call
	this._userVerifier.answerQuery(this._queryingService, this._preemptiveAnswer);
-Check when "next" signal is emitted, if(this._queryingService==null) then reset the login process without lifting the screen shield and let user login again

In this patch I have implemented the 2nd solution, but I am not sure if its the best one or if there is something else that could be done. I would want some review on that.
Thanks

Ray, can you take a look at this patch when you get a chance? It's for a GSoC application.

Easiest way to reproduce the reported bug is to use a VM with low memory. If the system is swapping, it's pretty much impossible to log in until you figure out the trick that you have to wait for the Password prompt.

yea of course.  thanks for looking into this, will review shortly.

Review of attachment 300426 [details] [review]:

Hey thanks again for looking into this. I appreciate the investigation and code. I don't think the patch is quite right, but it's close.  The way the code is supposed to work, in the race you're talking about, is:

1) user types password very quickly and hits enter
2) "next" signal is emitted since enter key was hit
3) the "next" handler code you pasted is run
4) this._queryingService is null since the user hit enter before it got set in onAskQuestion
5) Since this._queryingService is null, the "next" handler doesn't call answerQuery but instead just stuffs what the user
typed into a variable (this._preemptiveAnswer)
6) GDM finally catchess up with the user and onAskQuestion is called
7) onAskQuestion notices this._preemptiveAnswer is set, so rather than ask the question it just calls answerQuery straight away.

In theory I think the above logic is right, it's just you've found a problem in the implementation of step 7.  The code tries to get the service name to answer from this._queryingService, which as you pointed out, doesn't get set until later in that function.  I think the easiest/smallest change is to use serviceName , not this._queryingService when doing a preemptive answer.  I think that should fix the core issue, without any other changes.

want to give it a try? But onto your patch:

::: js/gdm/authPrompt.js
@@ +196,2 @@
     _onAskQuestion: function(verifier, serviceName, question, passwordChar) {
+        if (this._preemptiveAnswer && this._queryingService) {

So I don't believe this code will ever run if you add this._queryingService to the check. You could just remove the whole block especially since you do it again now below.

@@ +205,2 @@
         this._queryingService = serviceName;
+        if (this._preemptiveAnswer) {

You've effectively moved the block down to later in function.  This way this._queryingService is set and we won't end up calling answerQuery with an unset service. That's interesting fix, and a potentially viable alternative to my proposal above of using serviceName instead of this._queryingService. 

I don't think I like this solution, as much, though, because it means _queryingService is set when a service isn't actively waiting on a query response. I think this._queryingService should only be set if there's a service waiting for an answer. 

Your solution also means this.clear() will get called.  this.clear() does two things:

1) stop the spinner that's started when the user hits enter
2) clear the entry box.

Off hand I'm not sure if we want to do those two things or not when answering preemptively.  What do you think?

Hi, thanks a lot for reviewing the patch and i totally agree with you, even I feel that my solution wasnt really a very clean one but at that time I couldnt think of anything better so i thought i should submit my patch and will improve based on reviews.

I would surely like to try what you said about using serviceName instead of this._queryingService when doing a preemptive answer, that seems to be a very clean solution but there is just one thing i am still unsure of:

Lets say there is a situation when we receive an answer before receiving the query but before we could set this answer into _preemptiveAnswer, the first "if" condition of _onAskQuestion is already executed and at that moment this._preemptiveAnswer was null and therefore we moved pass that "if" condition. Now things would go as normal, answer would be stored in _preemptiveAnswer and serviceName would be stored in _queryingService (if not rechecking for existence of _preemptiveAnswer after setting _queryingService)

Now the next time we receive a query whose answer is yet not received then this time it would consider _preemptiveAnswer to be the answer of serviceName (if using serviceName insted of _queryingService in "if" condition) whereas that answer should have been for the previous query. It seems very unlikely and hypothetical situation and i am not even sure if something like this would ever happen considering the number of instructions in both threads, but i just want to confirm it. Otherwise using serviceName seems like a lot cleaner approach.

And for your 2nd question, as of now i really dont whether this.clear() should be called or not but i would surely think on that.

Thanks

Hi,

(In reply to Shivam Mishra from comment #8)
> Lets say there is a situation when we receive an answer before receiving the
> query but before we could set this answer into _preemptiveAnswer, the first
> "if" condition of _onAskQuestion is already executed
can't happen because all the code in question happens on the same thread.

> i am not even sure if something like this would ever happen considering 
> the number of instructions in both threads
All javascript code runs on one thread.

Oh I am sorry, I didnt know that.
In that case,  as you said, using serviceName instead of _queryingService seems a lot cleaner approach.
I will try that and will let you know in case of any issue.

Thanks

(replying to myself)
> You've effectively moved the block down to later in function.  This way
> this._queryingService is set and we won't end up calling answerQuery with an
> unset service. That's interesting fix, and a potentially viable alternative
> to my proposal above of using serviceName instead of this._queryingService. 
> 
> I don't think I like this solution, as much, though, because it means
> _queryingService is set when a service isn't actively waiting on a query
> response. I think this._queryingService should only be set if there's a
> service waiting for an answer. 
So looking at the code closer, this doesn't seem to be a real invariant. in the normal case we keep _queryingService set even after the user answers, so I think we should behave the same way with the preemptive answer. So, i like your solution better now.  Drop the spurious hunk at the top and let's go with that.

Created attachment 301089 [details] [review]
authPrompt: Fix hang if user types password really fast

It's possible for a user to type their password so quickly
that GDM hasn't even had time to ask for the password yet,
much less have time to process the answer.
In that situation, we tuck the user response away as
_preemptiveAnswer, and pass it along to GDM when GDM is finally
ready for it.
The problem is, there's a bug in the code, where we send
null for the service name in the answer, instead of the right
service name (say "gdm-password").
This commit addresses the bug by making sure we don't pass the
answer along, until the service name is properly set in
_queryingService. To ensure that, answering query (answerQuery)
based on _preemptiveAnswer has been shifted right below
this._queryingService = serviceName;

Review of attachment 301089 [details] [review]:

great thanks.

Attachment 301089 [details] pushed as 6660342 - authPrompt: Fix hang if user types password really fast