Fix missing JSAutoRequest before JS_ReportError
ClosedPublic
Actions

Authored by elexis on Sep 3 2019, 3:54 PM.

Details

Reviewers: None
Commits: rP22949: Fix missing JSAutoRequest before JS_ReportError in various commits.

Summary

It is easy to forget request mode when coding JS from C++, because many calls dont need it and many do need it.
JS_ReportError is one of the calls that do need it but had it missed in various places, this patch fixes that.

Test Plan

Consider whether using ScriptInterface::ReportError would be better. The difference is that its one line shorter when the ScriptInterface is already known, but longer if it has to be derived from the pCxPrivate thing, even slower if that is the case.
There is a TODO in ReportError about pending exceptions not working. Perhaps these JS_ReportError calls should call something about pending exceptions.

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

elexis created this revision.Sep 3 2019, 3:54 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/63/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/scripting/EngineScriptConversions.cpp
| 189| »   »   FAIL_VOID("Failed·to·get·Vector3D·constructor");
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/572/display/redirect

Harbormaster completed remote builds in B9204: Diff 9615.Sep 3 2019, 4:01 PM

What does JSAutoRequest do exactly ? I didn't find much documentation (I didn't exactly dig deep either)

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Internals#Garbage_collector
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_BeginRequest

Requests constrain garbage collection. If any thread is in a requests, garbage collection can happen only when that thread calls into the JSAPI. If one thread needs garbage collection, it blocks until each other thread makes a JSAPI call. It is therefore imperative that native code executing within an active request on cx not block, or simply take too long, outside the JSAPI.

Also notice how gcc segfaulted due to one missing AutoRequest previously: #4053, rP18451.

source/ps/scripting/JSInterface_Game.cpp
115 ↗	(On Diff #9615)	from rP14496

I cannot find the willingness to go through and test all JS_ReportError calls and figure out for every case individually which error handling would be better than the current one.

In some cases one should return false, in some cases one should use the implict error handling, in some cases we might already report twice, in some cases we might report too little.
At least that was what I found last time I tried fixing this TODO.

void ScriptInterface::ReportError(const char* msg) const
{
	JSAutoRequest rq(m->m_cx);
	// JS_ReportError by itself doesn't seem to set a JS-style exception, and so
	// script callers will be unable to catch anything. So use JS_SetPendingException
	// to make sure there really is a script-level exception. But just set it to undefined
	// because there's not much value yet in throwing a real exception object.
	JS_SetPendingException(m->m_cx, JS::UndefinedHandleValue);
	// And report the actual error
	JS_ReportError(m->m_cx, "%s", msg);

	// TODO: Why doesn't JS_ReportPendingException(m->m_cx); work?
}

Meanwhile it breaks GCC not to have these autorequests.

elexis added inline comments.Sep 20 2019, 9:16 PM