ClosedPublic
Actions

Authored by elexis on Oct 11 2019, 7:27 PM.

Details

Reviewers

nani

Commits

rP23068: Remove catch of critical PSERROR_Scripting exceptions in the GUI that is…

Summary

As discovered during the review of D2363, the LoadGlobalScriptFile and LoadGlobalScript calls do not require a catch following rP14496 anymore.

These PSERROR_Scripting cases are only thrown when very serious errors occur, that is when there is out-of-memory happening when creating a JS object or array,
or when a C++ author wanted to register a JS class more than once, or use one that doesn't exist.

In that case the error should not be caught and runtime continued like nothing of concern to the remaining code happened, but end the program.
Or if some C++ code doesnt rely on a JS class being registered, then it should itself catch this exception.

Notice that with current code, it is actually possible to have an XML page that loads JS scripting code that calls C++ code that triggers a C++ exception where the exception is caught in the XML parsing code,
i.e. C++ (catch) -> XML -> JS -> C++ (throw).

This will, with the current code, result in the exception caught, printed as LOGERROR then segfault.
With this patch applied, it will result in the exception being triggered and ended without subsequent error.

Test Plan

Reproduce the segfault using:

Index: binaries/data/mods/public/gui/gamesetup/gamesetup.xml
===================================================================
--- binaries/data/mods/public/gui/gamesetup/gamesetup.xml	(revision 23065)
+++ binaries/data/mods/public/gui/gamesetup/gamesetup.xml	(working copy)
@@ -3,10 +3,12 @@
 <objects>
 
 	<script directory="gui/common/"/>
 	<script directory="gui/gamesetup/"/>
 
+	<script>Engine.terror();</script>
+
 	<!-- Add a translucent black background to fade out the menu page -->
 	<object type="image" style="ModernWindow">
 
 		<object style="TitleText" type="text" size="50%-128 4 50%+128 36">
 			<translatableAttribute id="caption">Match Setup</translatableAttribute>
Index: source/gui/Scripting/JSInterface_GUIManager.cpp
===================================================================
--- source/gui/Scripting/JSInterface_GUIManager.cpp	(revision 23065)
+++ source/gui/Scripting/JSInterface_GUIManager.cpp	(working copy)
@@ -93,10 +93,15 @@ bool JSI_GUIManager::TemplateExists(Scri
 CParamNode JSI_GUIManager::GetTemplate(ScriptInterface::CxPrivate* UNUSED(pCxPrivate), const std::string& templateName)
 {
 	return g_GUI->GetTemplate(templateName);
 }
 
+void terror(ScriptInterface::CxPrivate* pCxPrivate)
+{
+	throw PSERROR_Scripting_DefineType_AlreadyExists();
+}
+
 void JSI_GUIManager::RegisterScriptFunctions(const ScriptInterface& scriptInterface)
 {
 	scriptInterface.RegisterFunction<void, std::wstring, JS::HandleValue, JS::HandleValue, &PushGuiPage>("PushGuiPage");
 	scriptInterface.RegisterFunction<void, std::wstring, JS::HandleValue, &SwitchGuiPage>("SwitchGuiPage");
 	scriptInterface.RegisterFunction<void, std::string, JS::HandleValue, &SetGlobalHotkey>("SetGlobalHotkey");
@@ -105,6 +110,7 @@ void JSI_GUIManager::RegisterScriptFunct
 	scriptInterface.RegisterFunction<JS::Value, std::string, &GetGUIObjectByName>("GetGUIObjectByName");
 	scriptInterface.RegisterFunction<std::wstring, std::wstring, &SetCursor>("SetCursor");
 	scriptInterface.RegisterFunction<void, &ResetCursor>("ResetCursor");
 	scriptInterface.RegisterFunction<bool, std::string, &TemplateExists>("TemplateExists");
 	scriptInterface.RegisterFunction<CParamNode, std::string, &GetTemplate>("GetTemplate");
+	scriptInterface.RegisterFunction<void, &terror>("terror");
 }