Page MenuHomeWildfire Games

Replace Eval calls with new ScriptInterface CreateObject and CreateArray functions
ClosedPublic

Authored by elexis on Jul 15 2019, 4:23 PM.

Details

Summary

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.

Test Plan

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Jul 20 2019, 7:33 PM
source/scriptinterface/ScriptInterface.h
135 ↗(On Diff #9014)
149 ↗(On Diff #9014)

or CreatePlainObject

154 ↗(On Diff #9014)

nope

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)

In D2080#87697, @elexis wrote:

How should errors be treated? throw something? The ScriptInterface should return a boolean, but the users ought to catch errors and report them somehow. I guess per-case basis.

I guess returning null value when an object is expected would be sufficient to be treated as an error?

Krinkle added inline comments.Sat, Jul 20, 10:09 PM
source/scriptinterface/ScriptInterface.h
137 ↗(On Diff #9014)

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() utility also works this way, it has an optional second parameter for property descriptors.

elexis updated this revision to Diff 9045.EditedSun, Jul 21, 9:56 PM

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

Krinkle added inline comments.Sun, Jul 21, 10:51 PM
source/ps/GameSetup/GameSetup.cpp
539 ↗(On Diff #9045)

Should this go into the variadic signature as well, using a line for each key/value pair?

elexis added inline comments.Sun, Jul 21, 10:59 PM
source/ps/GameSetup/GameSetup.cpp
539 ↗(On Diff #9045)

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.
It's a JS::Value so it needs to be RootedValue in function scope, HandleValue or MutableHandle value when passed as an argument. I tried with AssignOrToJSVal, but it keeps complaining. I'm not so fond of the idea of having to add many function variations for specific types like we see with SetProperty, perhaps it's only one needed.

Krinkle accepted this revision.Sun, Jul 21, 11:44 PM

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.

This revision is now accepted and ready to land.Sun, Jul 21, 11:44 PM
elexis added a comment.EditedMon, Jul 22, 4:16 PM
  • 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:

  • 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 ↗(On Diff #9045)

(Example for file-dependent throw/catch here:
Some exceptions in this file are caught with self->m_ScriptInterface->ReportError, push_back can throw http://www.cplusplus.com/reference/new/bad_alloc/ on OOM as well)

source/lobby/XmppClient.cpp
545 ↗(On Diff #9045)

Nuking those as well in favor of scriptInterface.SetPropertyInt(ret, i++, game)

source/ps/VisualReplay.cpp
196 ↗(On Diff #9045)

static_cast

source/scriptinterface/ScriptInterface.h
165 ↗(On Diff #9045)

(The two CreateObject functions should remain grouped)

elexis retitled this revision from Remove pointless Eval calls creating empty objects and arrays to Replace Eval calls with new ScriptInterface CreateObject and CreateArray functions.Mon, Jul 22, 4:18 PM

This looks much cleaner to me. OK with exceptions in CreateObject/CreateArray, since those would likely be unrecoverable.

source/scriptinterface/ScriptInterface.h
159 ↗(On Diff #9045)

Still think this comment can be improved, probably by simplifying it.

165 ↗(On Diff #9045)

It might be worth moving CreateArray before CreateObject

elexis added inline comments.Mon, Jul 22, 4:40 PM
source/ps/GameSetup/HWDetect.cpp
247 ↗(On Diff #9045)

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 ↗(On Diff #9045)

The other functions have a variant for wstring, so it would be more consistent if this one had one too (or abstracted for both).

elexis added inline comments.Mon, Jul 22, 5:59 PM
source/ps/GameSetup/GameSetup.cpp
539 ↗(On Diff #9045)

(fixed, see previous main comment)

source/ps/scripting/JSInterface_ModIo.cpp
148 ↗(On Diff #9045)

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 ↗(On Diff #9014)

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.
SetProperty seems like it should throw a PSERROR as well if it fails rather than silently dropping errors (269 or 272 occurrences of SetProperty ignore the return value, three others catch and return undefined, two of them with a TODO report error.)

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 ↗(On Diff #9045)

(inlining, as JS::Value is now supported)

139 ↗(On Diff #9045)

(inlining, as JS::Value is now supported)

elexis updated this revision to Diff 9061.Mon, Jul 22, 6:03 PM

Heal the world.

historic_bruno requested changes to this revision.Mon, Jul 22, 6:13 PM
historic_bruno added a subscriber: historic_bruno.
historic_bruno added inline comments.
source/lobby/XmppClient.cpp
507 ↗(On Diff #9061)

not a big fan of re-using i here

This revision now requires changes to proceed.Mon, Jul 22, 6:13 PM
wraitii added inline comments.Mon, Jul 22, 6:14 PM
source/scriptinterface/ScriptInterface.h
150 ↗(On Diff #9061)

Feels like you could template this too.

historic_bruno added inline comments.Mon, Jul 22, 6:17 PM
source/lobby/XmppClient.cpp
507 ↗(On Diff #9061)

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 ↗(On Diff #9061)

This is never thrown (anymore?) (refs Errors.cpp needing regeneration.)

source/ps/SavedGame.cpp
235 ↗(On Diff #9061)

here

source/ps/scripting/JSInterface_ModIo.cpp
94 ↗(On Diff #9061)

Using new semantics here.

elexis updated this revision to Diff 9066.Mon, Jul 22, 7:31 PM

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 ↗(On Diff #9066)

testable by starting jebel barkal tiny, performed

source/graphics/MapReader.cpp
375 ↗(On Diff #9066)

Testable by opening the gamesetup, performed.

source/gui/IGUIObject.cpp
467 ↗(On Diff #9066)

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 ↗(On Diff #9066)

Testable by clicking with the mouse on the minimap, performed.

source/gui/scripting/JSInterface_IGUIObject.cpp
752 ↗(On Diff #9061)

This function getTextSize can be tested by opening the structree, performed.

110 ↗(On Diff #9066)

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.

123 ↗(On Diff #9066)

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.

783 ↗(On Diff #9066)

Can be tested by opening the gamesetup, works.

source/lobby/XmppClient.cpp
522 ↗(On Diff #9066)

Tested by joining hte lbby , works.

550 ↗(On Diff #9066)

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.

576 ↗(On Diff #9066)

Cant be tested as we dont have rated games in a24 and not gonna test with local lobby now, code looks good.?

603 ↗(On Diff #9066)

same

647 ↗(On Diff #9066)

Can be tested by reading chat, pushing and popping the lobby dialog, works.

684 ↗(On Diff #9066)

same

source/network/NetClient.cpp
305 ↗(On Diff #9066)

CAn be tested by hosting a game and joining with another instance, works

772 ↗(On Diff #9066)

Can be tested by kicking, works.

source/network/NetClientTurnManager.cpp
159 ↗(On Diff #9066)

Can be tested by starting with the host, modifying cost of a template and rejoin, works.

source/network/StunClient.cpp
395 ↗(On Diff #9066)

joined lobby iwth 2 player, nonfowraded port works blaa

source/ps/GameSetup/GameSetup.cpp
542 ↗(On Diff #9066)

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.

1277 ↗(On Diff #9066)

This must be an array! Found with mentiond test.

source/ps/GameSetup/HWDetect.cpp
124 ↗(On Diff #9066)

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
152 ↗(On Diff #9066)

Can be tested by opening the replay screen and triggering the compatibiltiy checkbox, seems to work.

source/ps/ProfileViewer.cpp
541 ↗(On Diff #9066)

Tested by examining profile2.jsonp after a nonvisual replay, looks nice, didnt open in the profile viewer though.

source/ps/SavedGame.cpp
287 ↗(On Diff #9066)

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 ↗(On Diff #9066)

Can be teted by loading the screen, works.

142 ↗(On Diff #9066)

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.

terminate called after throwing an instance of 'PSERROR_Scripting_CreateObjectFailed'

what():  Scripting_CreateObjectFailed

Aborted (core dumped)

source/simulation2/Simulation2.cpp
986 ↗(On Diff #9066)

CAn be tested by configuring the AI in thegamesetup screen, works.

source/simulation2/components/CCmpAIManager.cpp
633 ↗(On Diff #9066)

CAn be tested by running a match with the AI and looking if it crashes.

source/simulation2/components/ICmpAIManager.cpp
67 ↗(On Diff #9066)

same

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
152 ↗(On Diff #9066)

Can be tested by performing random mapgen in atlas.

elexis updated this revision to Diff 9070.Mon, Jul 22, 8:38 PM

Fix oversights found by testing every call.

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

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Jul 22, 9:35 PM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package, Restricted Owners Package.Mon, Jul 22, 9:35 PM