Page MenuHomeWildfire Games

Fix missing JSAutoRequest before JS_ReportError

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



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

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 3 2019, 3:54 PM
Vulcan added a comment.Sep 3 2019, 3:56 PM

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

Link to build:

Vulcan added a comment.Sep 3 2019, 4:01 PM

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

Linter detected issues:
Executing section Source...

| 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:

Stan added a subscriber: Stan.Sep 3 2019, 4:08 PM

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

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.

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

From rP22663

This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2019, 9:18 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 20 2019, 9:18 PM