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.
Details
- Reviewers
- None
- Commits
- rP23323: Fix XmppClient upon registration or entering wrong password following…
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
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 10361 Build 17667: Vulcan Build Jenkins Build 17666: Vulcan Build (Windows) Jenkins Build 17665: arc lint + arc unit
Event Timeline
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
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.
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
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. 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). |