Even with constant strings to create empty objects, the Eval function is one that is healthy to avoid where possible, otherwise someone might take it as an example to call it with possibly malicious user-provided input.
Details
Check the current instances of the two calls being present already in some source files, for example JSI*Replay*.cpp and some gui/graphics code.
Read the specs and see that the code behavior is the same.
Consider the possibility of implementing some ScriptInterface function to construct these and possibly reject it due to the argument that the developer needs to know about JS internals if he is coding with them in C++.
Notice that all the other Eval occurrenes are in testfiles where they have some legitimacy.
The only other call is in CStdDeserializer::ReadScriptVal:
case SCRIPT_TYPE_OBJECT_SET: { JS::RootedValue setVal(cx); m_ScriptInterface.Eval("(new Set())", &setVal);
which might be better treated in an independent diff.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 8512 Build 13923: Vulcan Build Jenkins Build 13922: arc lint + arc unit
Event Timeline
I'm wondering if you couldn't create a "plain object" wrapper with a setProperty method, and convertible to HandleValue. You'd write 'Js::HandleValue jx = PlainObject().SetProp("XX", YY).SetProp("AA", BB)
I guess returning null value when an object is expected would be sufficient to be treated as an error?
source/scriptinterface/ScriptInterface.h | ||
---|---|---|
137 | Right. It's less confusing to sometimes pass properties as second param to a method called CreateObject, then to sometimes not pass properties to a method called CreateObjectWithProps. Incidentally, the standard [Object.create()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create) utility also works this way, it has an optional second parameter for property descriptors. |
CreateObject, CreateArray.
To explore:
- Doesn't support passing of JS::RootedValue, despite SetProperty supporting that?!
- Move to ScriptInterface.cpp, linker didn't like it.
- Copyright year number changes
- Exception / unexpected-return-value handling
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/CCmpAIManager.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" source/ps/Mod.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" source/ps/ProfileViewer.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" source/simulation2/Simulation2.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" source/network/StunClient.cpp | 1| /*·Copyright·(C)·2017·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2017" source/ps/scripting/JSInterface_ModIo.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" source/network/NetClientTurnManager.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" source/gui/MiniMap.cpp | 1| /*·Copyright·(C)·2017·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2017" source/simulation2/components/ICmpAIManager.cpp | 1| /*·Copyright·(C)·2017·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2017" source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp | 211| » » g_Renderer.GetWaterManager(),·g_Renderer.GetSkyManager(), | | [MAJOR] CPPCheckBear (syntaxError): | | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'. source/network/NetClient.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/179/display/redirect
source/ps/GameSetup/GameSetup.cpp | ||
---|---|---|
539 | Should this go into the variadic signature as well, using a line for each key/value pair? |
source/ps/GameSetup/GameSetup.cpp | ||
---|---|---|
539 | Should yes, does it work with the current implementation or the one I tried? No. That's why it's in the "to explore" list of the patch upload comment. |
LGTM! Statically reviewers and everything looks functionally identical, with one or two naming improvements.
There's more that could be done, it certainly a huge improvement as-is.
- Doesn't support passing of JS::RootedValue, despite SetProperty supporting that?!
Done:
The error that keeps popping up when passing a JS::RootedValue that works with SetProperty but not CreateObject:
../../../source/ps/GameSetup/GameSetup.cpp: In function ‘void InitPsAutostart(bool, JS::HandleValue)’: ../../../source/ps/GameSetup/GameSetup.cpp:538:105: error: use of deleted function ‘JS::Rooted<T>::Rooted(const JS::Rooted<T>&) [with T = JS::Value]’ 538 | scriptInterface.CreateObject(&sessionInitData, "attribs", attrs, "playerAssignments", playerAssignments); | ^ In file included from ../../../libraries/source/spidermonkey/include-unix-release/js/CallArgs.h:38, from ../../../libraries/source/spidermonkey/include-unix-release/jsapi.h:25, from ../../../source/scriptinterface/ScriptTypes.h:61, from ../../../source/scriptinterface/ScriptInterface.h:23, from ../../../source/graphics/MapReader.h:26, from ../../../source/ps/GameSetup/GameSetup.cpp:35: ../../../libraries/source/spidermonkey/include-unix-release/js/RootingAPI.h:818:5: note: declared here 818 | Rooted(const Rooted&) = delete; | ^~~~~~ In file included from ../../../source/graphics/MapReader.h:26, from ../../../source/ps/GameSetup/GameSetup.cpp:35: ../../../source/scriptinterface/ScriptInterface.h:144:110: note: initializing argument 5 of ‘bool ScriptInterface::CreateObject(JS::MutableHandleValue, const char*, const T&, Args ...) const [with T = JS::Handle<JS::Value>; Args = {const char*, JS::Rooted<JS::Value>}; JS::MutableHandleValue = JS::MutableHandle<JS::Value>]’ 144 | bool CreateObject(JS::MutableHandleValue objectValue, const char* propertyName, const T& propertyValue, Args... args) const | ~~~~^~~~~~~~
So it's the parameter pack that wants to assing a RootedValue to a RootedValue, which goes against the rooting concept of spidermonkey, see JSRootingGuide.
Searching the web a bit, I realized there is just a const& missing to make it happen.
So the pack parameter arguments were actually copied for no reason, i.e. a hidden performance disadvantage!
- Move to ScriptInterface.cpp, linker didn't like it.
Done:
- CreateArray and CreateObject without arguments were moved to cpp.
- The template function CreateObject can't be cleanly moved to the implementation file with current c++ standard, see https://stackoverflow.com/questions/495021/why-can-templates-only-be-implemented-in-the-header-file
- Copyright year number changes
Done
- Exception / unexpected-return-value handling
Done:
It seems catching OOM seems like a difficult / offtopic / questionable task, because
(1) Many other c++ statements allocate something somewhere and may trigger an currently caught OOM, not only the added ones, so one would have to revise every modified function and possibly the callers to those, and the callers to the callers.
(2) Every call needs a different exception handling dependent on callstack
(3) If OOM was caught, it most likely is still unrecoverable, so not much can be saved other than the earliest callstack (which might be running into the exception without catching it.
So it seems either one needs to revise OOM exception handling throughout the engine, or keep as is, or throw instead of returning false if JS object creation fails.
Actually, let's throw a std::bad_alloc here as well if someone ordered as JS object but couldn't get one due to OOM. If this error is considered unrecoverable, then it must throw a stacktrace asap.
One could consider shutting down with writing logfiles, but true logfiles should be written before OOM or crash, and that's currently the case, so it should be right to throw in CreateObject instead of returning silently false!?
std::bad_alloc might be nice, but more consistent use would be a PSERROR exception (see other throw statements in this file), looking at Errors.cpp we find PSERROR_Scripting_CreateObjectFailed which fits perfectly it seems.
This type is in fact unused currently. svn blame says it was introduced unused in rP769?
Thanks for the review and comments!
source/graphics/MapGenerator.cpp | ||
---|---|---|
408 | (Example for file-dependent throw/catch here: | |
source/lobby/XmppClient.cpp | ||
545 | Nuking those as well in favor of scriptInterface.SetPropertyInt(ret, i++, game) | |
source/ps/VisualReplay.cpp | ||
196 | static_cast | |
source/scriptinterface/ScriptInterface.h | ||
165 | (The two CreateObject functions should remain grouped) |
source/ps/GameSetup/HWDetect.cpp | ||
---|---|---|
247 | Look at all of these properties. It seems like a usecase for SetProperties with a pack argument, but actually I think the idea was to put this into custom objects rather than having a flat 1d one? So declaring out of scope, it will be very easy to add this function if a use case arises. | |
source/scriptinterface/ScriptInterface.h | ||
164 | The other functions have a variant for wstring, so it would be more consistent if this one had one too (or abstracted for both). |
source/ps/GameSetup/GameSetup.cpp | ||
---|---|---|
539 | (fixed, see previous main comment) | |
source/ps/scripting/JSInterface_ModIo.cpp | ||
148 | Could use the template function, but then there'd be a scriptInterface->FreezeObject call, but then the other CreateObject call above would be inconsistent, so I'll just not touch this. | |
source/scriptinterface/ScriptInterface.h | ||
149 | See IRC discussion today on whether to use return false, throw PSERROR_Scripting or ENSURE. It seems first the SM API was mirrored that returns false on exceptions, then PSERRORs were introduced. So the non-template CreateObject() has to throw if we want to report an OOM immediately and it has to return a pointless boolean that is always true as long as SetPropery uses that as error handling, and can consider insertion of throw with a new PSERROR type in a separate patch. | |
source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp | ||
107 | (inlining, as JS::Value is now supported) | |
139 | (inlining, as JS::Value is now supported) |
source/lobby/XmppClient.cpp | ||
---|---|---|
506 | not a big fan of re-using i here |
source/scriptinterface/ScriptInterface.h | ||
---|---|---|
150 | Feels like you could template this too. |
source/lobby/XmppClient.cpp | ||
---|---|---|
506 | Not this one, but the ones below where an inner loop shadows the outer i |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp | 211| » » g_Renderer.GetWaterManager(),·g_Renderer.GetSkyManager(), | | [MAJOR] CPPCheckBear (syntaxError): | | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/191/display/redirect
Thanks for the find historic_bruno! Indeed a pitfall to shadow that variable.
Using the Value syntax in JSInterface_VFS.cpp and few other places where the Value variant is requested.
Making CreateArray void, since it has no actual return value opposed to current CreateObject (SetProperty) implementation .
source/gui/scripting/JSInterface_IGUIObject.cpp | ||
---|---|---|
752 | This is never thrown (anymore?) (refs Errors.cpp needing regeneration.) | |
source/ps/SavedGame.cpp | ||
237 | here | |
source/ps/scripting/JSInterface_ModIo.cpp | ||
94 | Using new semantics here. |
Replace some more JS_NewArray instances, don't shadow that array counter variable, void CreateArray.
Yet to be tested.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp | 211| » » g_Renderer.GetWaterManager(),·g_Renderer.GetSkyManager(), | | [MAJOR] CPPCheckBear (syntaxError): | | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/196/display/redirect
Found two bugs while testing, one missing SetPropertyInt change, one Array/Object confusion, fixed.
Done with this patch now.
source/graphics/MapGenerator.cpp | ||
---|---|---|
407 | testable by starting jebel barkal tiny, performed | |
source/graphics/MapReader.cpp | ||
375–376 | Testable by opening the gamesetup, performed. | |
source/gui/IGUIObject.cpp | ||
467 | It seems like these coordinates are currently unused from the JS side, at least I couldn't find the use: gui/COList.cpp: ScriptEvent("selectioncolumnchange"); gui/CSlider.cpp: ScriptEvent("valuechange"); gui/CList.cpp: ScriptEvent("selectionchange"); gui/CList.cpp: ScriptEvent("hoverchange"); gui/CList.cpp: ScriptEvent("hoverchange"); and would benefit from receiving arguments relating to the semantics of the GUI object instead. Perhaps one can just delete the arguments in a separate commit? Hypothetically useful, whatever So can only test by uneval(). Performed, works. | |
source/gui/MiniMap.cpp | ||
246 | Testable by clicking with the mouse on the minimap, performed. | |
source/gui/scripting/JSInterface_IGUIObject.cpp | ||
111 | This can be tested by opening the gamesetup, version this comment relates to was visibly broken as JS_SetElement needs to be changed to SetProperty working on val instead of the shadowed obj. | |
124 | This is ScriptInterface::ToJSVal<const char*> from ScriptConversions.cpp. There is no user of this property, since the name is the hardcoded identifier in JS to begin with. So it can only be tested with F9 -> warn(Engine.GetGUIObjectByName("dev_commands").name), performed, works. | |
752 | This function getTextSize can be tested by opening the structree, performed. | |
784 | Can be tested by opening the gamesetup, works. | |
source/lobby/XmppClient.cpp | ||
521 | Tested by joining hte lbby , works. | |
546 | Can be tested by hosting a game in the lobby and opening the popup dialog. Doestn replace the test with multiple games. Code looks good though. | |
569 | Cant be tested as we dont have rated games in a24 and not gonna test with local lobby now, code looks good.? | |
593 | same | |
637–638 | Can be tested by reading chat, pushing and popping the lobby dialog, works. | |
674 | same | |
source/network/NetClient.cpp | ||
305 | CAn be tested by hosting a game and joining with another instance, works | |
772 | Can be tested by kicking, works. | |
source/network/NetClientTurnManager.cpp | ||
159 | Can be tested by starting with the host, modifying cost of a template and rejoin, works. | |
source/network/StunClient.cpp | ||
395 | joined lobby iwth 2 player, nonfowraded port works blaa | |
source/ps/GameSetup/GameSetup.cpp | ||
540 | Tested using -autostart="random/jebel_barkal" -autostart-seed=-1 -autostart-players=2 -autostart-civ=1:athen -autostart-civ=2:brit -autostart-ai=1:petra -autostart-ai=2:petra -autostart-player=-1 as per readme.txt. | |
1275 | This must be an array! Found with mentiond test. | |
source/ps/GameSetup/HWDetect.cpp | ||
124 | This file can be tested at most by running the UserReporter, looking at that new user reporter json file from a23b, but has to look at the code rather that the properties remain correct. .config/0ad/logs/userreport_hwdetect.txt seems to work | |
source/ps/Mod.cpp | ||
158 | Can be tested by opening the replay screen and triggering the compatibiltiy checkbox, seems to work. | |
source/ps/ProfileViewer.cpp | ||
541–542 | Tested by examining profile2.jsonp after a nonvisual replay, looks nice, didnt open in the profile viewer though. | |
source/ps/SavedGame.cpp | ||
287 | File can be tested by saving and loading agame, playing with the menu and checking that the camera is in the correct location, works. | |
source/ps/scripting/JSInterface_ModIo.cpp | ||
80 | Can be teted by loading the screen, works. | |
144 | Downloaded a mod, worked. View works too. | |
source/ps/scripting/JSInterface_VFS.cpp | ||
153 ↗ | (On Diff #9066) | Can be tested by looking at loading screen tips, works. |
source/scriptinterface/ScriptInterface.cpp | ||
592 ↗ | (On Diff #9066) | Tested what happens if the throw is triggered by removing the condition, looks nice.
|
source/simulation2/Simulation2.cpp | ||
986 | CAn be tested by configuring the AI in thegamesetup screen, works. | |
source/simulation2/components/CCmpAIManager.cpp | ||
633 | CAn be tested by running a match with the AI and looking if it crashes. | |
source/simulation2/components/ICmpAIManager.cpp | ||
67 | same | |
source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp | ||
152 | Can be tested by performing random mapgen in atlas. |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp | 211| » » g_Renderer.GetWaterManager(),·g_Renderer.GetSkyManager(), | | [MAJOR] CPPCheckBear (syntaxError): | | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/198/display/redirect