Fix XmppClient upon entering wrong password following rP23172/D2412
ClosedPublic
Actions

Authored by elexis on Dec 18 2019, 5:36 PM.

Details

Reviewers: None
Commits: rP23323: Fix XmppClient upon registration or entering wrong password following…

Summary

As reported by user1 to me, when one enters the wrong password or registers a new account, the login page will not respond.
The purpose of this patch is to fix this regression.

Test Plan

The regression comes from rP23172/D2412.
It was the delayed sending of GUIMessages until m_initialLoadComplete.
This delay is necessary if we want to setup the playerlist only once instead of numPlayers/25 times (which is visually irritating).
Since the client never reaches the m_initialLoadComplete state if the password is incorrect, the login page just hangs there.

The patch is correct exactly if m_initialLoadComplete will be complete or if the other condition is true exactly if m_initialLoadComplete will never be true.
I.e. the patch is correct if and only if m_initialLoadComplete will always become true if m_isConnected remains true.
Othewise there would be another circumstance under which the loginpage just sits there without reaction.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Dec 18 2019, 5:36 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/787/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1303/display/redirect

Harbormaster completed remote builds in B10361: Diff 10636.Dec 18 2019, 5:44 PM

elexis edited the summary of this revision. (Show Details)Jan 1 2020, 9:32 PM

There is another logic error in the change from pulling individual messages to pulling an array of messages in rP23172:

Upon successful registration, the client will disconnect, push the registered GUI message, disconnect, then push the`disconnect` GUI message.
Then the prelobby feedback.js` page will receive the array containing both of these messages.
Then that prelobby GUI page will call StopXmppClient when processing the registered GUI message and also call StopXmppClient when calling the disconnected GUI message.
This will trigger the ENSURE in the StopXmppClient function when it attempts to delete the instance the second time.
Prior to rP23172, the StopXmppClient upon the registered message deleted the client, so the disconnected GUI message was created but subsequently deleted before it was processed, thus not triggering the disconnected case and thus not triggering the StopXmppClient ENSURE.

Also the idea was that JS can never trigger C++ breakpoints or crashes, since the purpose of C++ breakpoints is to trace C++ errors, but this is a JS error, so there should be a catchable JS exception instead.

Fix registration case too.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/923/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/18/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lobby/XmppClient.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1441/display/redirect

Harbormaster completed remote builds in B10591: Diff 10840.Jan 2 2020, 1:36 AM

elexis added inline comments.Jan 2 2020, 1:42 AM

binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
30 ↗	(On Diff #10840)	This is not obvious why it's here, precisely like the `StopXmppClient` in `disconnected`, the `StopXmppClient` in `registered` and the `disconnect` in `handleRegistrationResult`. The meaning of these calls is revealed to the one reading throgh the entire stack however. (And not adding a comment to explain it to people not working with the stack seems unreasonable, because then one would have to add those comments in every other line of code while not achieving any benefit for the developers actually working with the code who already did read the code, not even saving time as they have to read through it already with a conscious mind.) Another option would be adding a state global (for instance g_Registered). But that seems even uglier, not only because its a global (lessening cohesion), but also because it would be logically more complex, easier to add cases that are not handled correctly (less/simpler code that does the same is less prone to errors). Reverting to pulling only one message at a time seems undesirable, not only because its a performance loss on init, but also because it is irritating to the users if the playerlist is updated 5 times on init instead of once (first frame 20 users online, second frame 40 users online, third frame 60 users online etc).

elexis retitled this revision from Fix XmppClient Client upon entering wrong password following rP23172/D2412 to Fix XmppClient upon entering wrong password following rP23172/D2412.Jan 2 2020, 11:10 PM

This revision was not accepted when it landed; it landed in state Needs Review.Jan 2 2020, 11:14 PM

Closed by commit rP23323: Fix XmppClient upon registration or entering wrong password following… (authored by elexis). · Explain Why

This revision was automatically updated to reflect the committed changes.

elexis added a commit: rP23323: Fix XmppClient upon registration or entering wrong password following….

Owners added a subscriber: Restricted Owners Package.Jan 2 2020, 11:14 PM