Config.js doesn't block callers when loading. Causing invalid values to be returned and crashes MapEngine.

[redpolline]

Describe the bug
Config.js needs to iterate through a loop to load the server's config. This takes time and is done async. As a result, any calls to Config.get() or Config.getServer() before that loop is finished, results in the default values being given back to the callers. Even if the actual config (such as from index.html) says otherwise.

The biggest issue caused by this is within MapEngine. Where CheckAttendance's and MapName's require() are behind a call to Config.get() === true. The default value is false for both, and a false result from Config.get() will result in the require() never being executed.

If this bug occurs and the server has one or both of these UIs enabled, then after Config finishes loading subsequent calls to Config.get() will return true, but the UI objects in MapEngine will still be undefined. This results in MapEngine calling functions like prepare() or append() on an undefined value, crashing MapEngine during init(), and leaving the user with an unusable black screen.

Screenshots (if applicable)

Packet/Client version
PACKETVER 20210406

Browser/device info

• Browser: Version 126.0.6478.126 (Official Build) built on Debian 12.5, running on Debian 12.6 (64-bit)
• Device: Debian 12 / Linux PC

Additional context
The correct fix would be to block callers to Config's getter and setter methods, until the init loop finishes execution. But a quick attempt at fixing it by modifying Config.js, results in a hung UI that stops loading anything after Online.js.

[MrAntares]

I am puzzled by how is this even possible, because you can't even login until the config is finished and READY message is sent to the client main module. Map engine is only started when you enter the game, by that time you requited at least a dozen configs that are essential to the setup of the ui, the login screen, the char creation and select screen. Isn't this the result of some change or some other bug? I never had any issue like this with configs

[redpolline]

The changes in the screenshots were made just for testing whether or not the values were defined. They are not part of any other change set. The last commit on my server was commit 8ad379b on master. (It had the two pull requests I sent manually applied.)

[redpolline]

Just checked the current master for both main and the new remoteClient repo. (Which has it's own issue with a hard coded path to AI/Consts.lua. Every other file loads fine though, assuming you fix the missing .htaccess file.)

The bug is still present.

For the record, the content of that patch is this:

diff --git a/src/Engine/MapEngine.js b/src/Engine/MapEngine.js
index 3973ae9..2fc2d62 100644
--- a/src/Engine/MapEngine.js
+++ b/src/Engine/MapEngine.js
@@ -44,10 +44,6 @@ define(function( require )
 	var ChatBoxSettings  = require('UI/Components/ChatBoxSettings/ChatBoxSettings');
 	var StatusConst        = require('DB/Status/StatusState');
 
-	if(Configs.get('enableCheckAttendance') && PACKETVER.value >= 20180307) {
-		var CheckAttendance  = require('UI/Components/CheckAttendance/CheckAttendance');
-	}
-
 	var WinStats         = require('UI/Components/WinStats/WinStats');
 	var Inventory        = require('UI/Components/Inventory/Inventory');
 	var CartItems        = require('UI/Components/CartItems/CartItems');
@@ -72,9 +68,6 @@ define(function( require )
 	var RodexIcon        = require('UI/Components/Rodex/RodexIcon');
 	var PetInformations  = require('UI/Components/PetInformations/PetInformations');
 	var HomunInformations= require('UI/Components/HomunInformations/HomunInformations');
-	if(Configs.get('enableMapName')){
-		var MapName          = require('UI/Components/MapName/MapName');
-	}
 	var PluginManager    = require('Plugins/PluginManager');
 
 	var UIVersionManager      = require('UI/UIVersionManager');
@@ -85,6 +78,10 @@ define(function( require )
 	var Quest     = require('UI/Components/Quest/Quest');
 	var PlayerViewEquip     = require('UI/Components/PlayerViewEquip/PlayerViewEquip');
 
+	// Config enabled UIs (Cannot require here; only in the init function. As Config may not be fully initialized yet due to JS async execution.)
+	var CheckAttendance  = undefined;
+	var MapName          = undefined;
+
 	/**
 	 * @var {string mapname}
 	 */
@@ -121,6 +118,26 @@ define(function( require )
 	 */
 	MapEngine.init = function init( ip, port, mapName )
 	{
+		// Require Config enabled UIs.
+		if (Configs.get('enableCheckAttendance')) {
+			console.log("CheckAttendance UI is enabled in config.");
+			if (PACKETVER.value >= 20180307) {
+				console.log("Activating CheckAttendance.");
+				CheckAttendance  = require('UI/Components/CheckAttendance/CheckAttendance');
+			} else {
+				console.log("PACKETVER does not support CheckAttendance, you should disable it in the config.");
+			}
+		} else {
+			console.log("CheckAttendance UI is disabled in config.");
+		}
+
+		if (Configs.get('enableMapName')){
+			console.log("MapName UI is enabled in config. Activating MapName.");
+			var MapName          = require('UI/Components/MapName/MapName');
+		} else {
+			console.log("MapName UI is disabled in config.");
+		}
+
 		_mapName = mapName;
 
 		// Connect to char server
@@ -275,8 +292,12 @@ define(function( require )
 			Rodex.prepare();
 			RodexIcon.prepare();
 
-			if(Configs.get('enableMapName')){
-				MapName.prepare();
+			if(Configs.get('enableMapName')) {
+				if (MapName !== undefined) {
+					MapName.prepare();
+				} else {
+					console.log("WARNING: MapName UI is undefined, despite being enabled in the config. UI won't be available.");
+				}
 			}
 
 			if(Configs.get('enableCashShop')){
@@ -288,7 +309,11 @@ define(function( require )
 			}
 
 			if(Configs.get('enableCheckAttendance') && PACKETVER.value >= 20180307) {
-				CheckAttendance.prepare();
+				if (CheckAttendance !== undefined) {
+					CheckAttendance.prepare();
+				} else {
+					console.log("WARNING: CheckAttendance UI is undefined, despite being enabled in the config. UI won't be available.");
+				}
 			}
 
 			// Bind UI
@@ -585,8 +610,10 @@ define(function( require )
 			MiniMap.getUI().append();
 			MiniMap.getUI().setMap( MapRenderer.currentMap );
 			if(Configs.get('enableMapName')){
-				MapName.setMap( MapRenderer.currentMap );
-				MapName.append();
+				if (MapName !== undefined) {
+					MapName.setMap( MapRenderer.currentMap );
+					MapName.append();
+				}
 			}
 			ChatBox.append();
 			ChatBoxSettings.append();
@@ -623,7 +650,9 @@ define(function( require )
 			}
 
 			if(Configs.get('enableCheckAttendance') && PACKETVER.value >= 20180307) {
-				CheckAttendance.append();
+				if (CheckAttendance !== undefined) {
+					CheckAttendance.append();
+				}
 			}
 
 			// Reload plugins

I just haven't submitted it, because it's more of a hack to get it working on my set up than a proper fix. (All it does is prevent the crash by checking for an undef'd value before calling a method, and potentially give the JS thread more clock cycles to get Config loaded and win the race. But it doesn't actually fix the race condition.)

[redpolline]

I am puzzled by how is this even possible, because you can't even login until the config is finished and READY message is sent to the client main module. Map engine is only started when you enter the game, by that time you requited at least a dozen configs that are essential to the setup of the ui, the login screen, the char creation and select screen. Isn't this the result of some change or some other bug? I never had any issue like this with configs

As for the working login / char select / etc: I think that they are working despite the bug largely due to how Configs is used for them.

After doing some looking around, I've only found 4 other places where Configs.get() is called inside of a define() function. (Not including the MapEngine.js file nor any of the non-remote client app stuff.)

1. src/Network/NetworkManager.js line 58: Checks the packetDump option.

2. src/Network/PacketVerManager.js line 25: Checks the renewal option. (That's the one I added.)

3. src/Network/PacketStructure.js line 21: Checks the renewal option.

4. src/Core/AIDriver.js: line 13: Checks the remoteClient option.

The first one calls Configs.get() in it's define(), just like MapEngine.js. However, this one works if it's enabled. (Although all it does is control whether or not a console dump is made, it doesn't load nor depend on additional objects via require().)

The second one has 'Core/Configs' listed as a dependency to it's define() function. (So Configs should be fully loaded before the call to that function is made. Which seems to be inline with the API rules.)

The third one is the same as the second one. (I.e. 'Core/Configs' is listed as a dep to define().)

The fourth one is just weird. It gives a dependency list to define() that doesn't include 'require' nor 'Core/Configs'. Yet it calls require('Core/Configs') like MapEngine.js, despite that being against the API rules of requirejs. This one however is used for dynamic code generation and execution, and doesn't use it to require() anything else.

Basically the only use of Configs.get() that can result in executing a function call in an undefined value is apparently in MapEngine.js. The rest are either done in methods called after all of the initial require() calls are completed, only get the returned result evaluated by a future method call, or have Configs as a hard dependency for their define().

The first and second choices are what my workaround in the diff above does, and is known to work, but may not be a proper solution and could potentially break in the future. The third option seems like a better choice, but would require duplicating all of the require() calls in MapEngine's define() to it's dependency list. (And optionally making them arguments.) Which I haven't tested.

Although, if it's desired, I can try to test the third option. Or I can just submit the workaround as a pull request. Unless anyone has a better idea?

[redpolline]

If anyone wants to check the bug's behavior on their own, I've pushed the branch I was running debugging checks on to https://github.com/redpolline/roBrowserLegacy/tree/Configs_Check_Behavior

On my system it shows that both Configs.init() and MapEngine's define() is executed before src/App/Online.js's calls to Plugins.init() and GameEngine.init(). But that at the time of MapEngine's define(), Configs.get() returns false for enableMapName and enableCheckAttendance, despite apparently executing the for loop in Config.init().

[Samuel23]

Isn't this just from a wrong setup? I am confused with how Configs doesn't load in here. The last time I had a problem with config was because there was a typo that leads to even being set as true inside the config in the html results into false

how do you setup your config files and how do you run your robrowser?

in the first picture your problem is with CheckAttendance UI itself, because it read that your config for it is enabled and your packetver supports it, but CheckAttendance UI can't be appended.

[MrAntares]

I just downloaded the latest version and everything works fine for me. Is there a specific way/setting to reproduce this?

[redpolline]

Isn't this just from a wrong setup? I am confused with how Configs doesn't load in here. The last time I had a problem with config was because there was a typo that leads to even being set as true inside the config in the html results into false

how do you setup your config files and how do you run your robrowser?

in the first picture your problem is with CheckAttendance UI itself, because it read that your config for it is enabled and your packetver supports it, but CheckAttendance UI can't be appended.

Welp, I found it.

In attempting to show the index.html that contains the server config in a screenshot, I found an indented additional config block at the global scope that I did not add myself. (It's indented so much that my editor didn't show it. I had to zoom out to see it.) That block configures enableCheckAttendance and friends to false, while my server config block ( under "servers: [ { ... } ]" ) enables it.

Changing the global block to match the server block fixes the bug, and explains the behavior: Globally those UI components are disabled, but they get enabled when a server is selected that has those options enabled in it's server config block. The client code doesn't handle this case.

So if anything really needs to be "fixed", it's either delaying the UI appending for for enable* options until MapEngine.init(), - OR - rejecting the options at server config level and leaving everything else alone.

Sorry for the noise, I just didn't see the extra config block until I attempted to take a screenshot of the entire index.html....