Page MenuHomeWildfire Games

Register JSI_Debug script functions for hwdetect.js
ClosedPublic

Authored by historic_bruno on Jul 24 2019, 8:05 PM.

Details

Summary

hwdetect.js has the option to display error dialogs, currently used in only one case, but this relies on the existence of Engine.DisplayErrorDialog which is now in the JSI_Debug namespace.

Probable origin: rP14496

Test Plan
  1. Modify hwdetect.js by adding a line in RunDetection(): dialog_warnings.push("This is a test");
  2. When the game is started, a debug warning dialog should popup.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

historic_bruno created this revision.Jul 24 2019, 8:05 PM
elexis added a subscriber: elexis.EditedJul 24 2019, 8:10 PM

hwdetect.js has the option to display error dialogs, currently used in only one case, but this relies on the existence of Engine.DisplayErrorDialog which is now in the JSI_Debug namespace.

Now being a sleepless night in #4772. Edit: or was it broken before too?

If that is so it sounds better to move that individual functions from JSI*Debug to HWDetect.cpp.

historic_bruno added a comment.EditedJul 24 2019, 8:40 PM

It could be argued that hwdetect doesn't need all the JSI_Debug functions, but I also felt it strange to duplicate that function implementation instead of reusing the handy RegisterScriptFunctions.

It definitely doesn't need all of the GUI script functions, so JSI_Debug would be the minimal set, without re-implementing anything.

Crash, DebugWarn nothing uses that, shall we delete?
DisplayErrorDialog only hwdetect.js uses it
GetBuildDate, GetBuildRevision - displayed in UI menu
GetBuildTimestamp - unused, possibly useful
GetMicroseconds - I added that for occasional UI performance debugging replacing janwas "cheezy" custom UI profiler. Later (a24) added for MapGenerator too but always used there.

It could be argued that hwdetect doesn't need all the JSI_Debug functions, but I also felt it strange to duplicate that function implementation instead of reusing the handy RegisterScriptFunctions.

It definitely doesn't need all of the GUI script functions, so JSI_Debug would be the minimal set, without re-implementing anything.

So actually I still wonder why JSI_Debug is included when it only uses one function and none of the others, and that what currently does include JSI_Debug uses all but this one function.

Now the question is - was there a reason why Philip moved it to the UI context while all other functions are in HWDetect.cpp in rP8663?

I tried the function for the first time, to me its the same as std::terminate but without termination...

It seems more appropriate if the JS RunHardwareDetect would return some JS object or string or exception and then HWDetect.cpp or the UI decide itself what to do with it?

Why is DisplayErrorDialog called to begin with in JS when it is triggered by output.dialog_warnings.length which is constructed in C++?

Perhaps the dumb function could be replaced with something else as dumb that doesnt expose it to JS via RegisterScriptFunction.

historic_bruno added a comment.EditedJul 24 2019, 9:30 PM
In D2123#88689, @elexis wrote:
Crash, DebugWarn nothing uses that, shall we delete?
DisplayErrorDialog only hwdetect.js uses it
GetBuildDate, GetBuildRevision - displayed in UI menu
GetBuildTimestamp - unused, possibly useful
GetMicroseconds - I added that for occasional UI performance debugging replacing janwas "cheezy" custom UI profiler. Later (a24) added for MapGenerator too but always used there.

Crash and DebugWarn can be useful when you need to simulate those conditions from JS. You wouldn't leave them in permanently, but for debugging purposes, which explains the lack of them in the codebase.

It could be argued that hwdetect doesn't need all the JSI_Debug functions, but I also felt it strange to duplicate that function implementation instead of reusing the handy RegisterScriptFunctions.

It definitely doesn't need all of the GUI script functions, so JSI_Debug would be the minimal set, without re-implementing anything.

So actually I still wonder why JSI_Debug is included when it only uses one function and none of the others, and that what currently does include JSI_Debug uses all but this one function.

Now the question is - was there a reason why Philip moved it to the UI context while all other functions are in HWDetect.cpp in rP8663?

I tried the function for the first time, to me its the same as std::terminate but without termination...

It seems more appropriate if the JS RunHardwareDetect would return some JS object or string or exception and then HWDetect.cpp or the UI decide itself what to do with it?

Why is DisplayErrorDialog called to begin with in JS when it is triggered by output.dialog_warnings.length which is constructed in C++?

Perhaps the dumb function could be replaced with something else as dumb that doesnt expose it to JS via RegisterScriptFunction.

Well DisplayErrorDialog is a "nicer" way of warning the user about something from JS (see RunDetection() in hwdetect.js, which can optionally push such warnings).

There's a chance the GUI wouldn't even display with a bad driver (and it might not cause a detectable problem in GL init, which is why we check GPU/driver names), so there would be no other way for the user to know what had happened before it crashed. That's why this error dialog is used instead of something else. I think there are other similar cases where something goes wrong during OpenGL init and we need to show an error dialog without relying on the GUI.

We could certainly replace it, but it current;y serves its purpose (or would if it hadn't been broken accidentally).

The idea is as we get reports about other drivers that are broken, we can add them to the warning list. But we need to collect and aggregate such reports, which we haven't been doing.

Crash and DebugWarn can be useful when you need to simulate those conditions from JS. You wouldn't leave them in permanently, but for debugging purposes, which explains the lack of them in the codebase.

Can we name such a situation? I guess one dig through the revisions to find one relating to the commit that introduced them. To me it seems purely hypothetical, in such a case the developer can compile. If we want to save compile time for hypothetical things that almost never occur, we could add arbitrary lots of hypotheticals, no?
And do we want to distribute that to 100k computers that can install mods from shady sources?

DisplayErrorDialog

I object more to the way the C++ function is exposed to JS instead of using the return value of the function.

If you don't want to change the implementation or the way it is subscribed, I don't mind, but I don't understand why JSI*Debug should completely be subscribed if only one function is used, and if that function is never used in the UI context, and rather appears a dirty leftover. IMO move that one function over to HWDetect.cpp, clean the UI this way a bit and grouping it in the place its intended to be used from?

Philip clearly had some intention to use this in the UI, otherwise he'd put it into HWDetect.cpp. Perhaps IRClogs (2003-2019) can show something. Only occurrence of that word is in

2016-03-02-QuakeNet-#0ad-dev.log:16:28 < dalerank> i see... seems it need to call DisplayErrorDialog from scripts

I guess one could check the day and days leading to the commit, but meh.

It was added to GUI scripts because it was envisioned it might be useful elsewhere besides hwdetect (remember this is when "GUI scripts" was a global scriptinterface and applied to hwdetect too).

The other functions in hwdetect are really hacks, that is why there are TODOs in HWDetect.cpp and hwdetect.js:

// The Set* functions will override the default behaviour, unless the user
// has explicitly set a config variable to override that.
// (TODO: This is an ugly abuse of the config system)
// TODO: add some mechanism for setting config values
	// (overriding default.cfg, but overridden by local.cfg)

So eventually we would want to use something like the JSI_ConfigDB functions instead, but I don't know if they support the required functionality yet (see TODO above). That's beyond the scope of this diff, however.

I think the point about removing Crash and DebugWarn for security reasons is probably the strongest. I would say nobody talks about them, because nobody knows they are there :)

How else can I break into my JS script and see the engine callstack? We have no integrated JS debugger anymore. Think of it as a poor man's debugger. I would be fine with selectively enabling those features (without having to recompile), as average users won't want or need them

elexis accepted this revision.Jul 24 2019, 10:16 PM

It was added to GUI scripts because it was envisioned it might be useful elsewhere besides hwdetect (remember this is when "GUI scripts" was a global scriptinterface and applied to hwdetect too).

The other functions in hwdetect are really hacks, that is why there are TODOs in HWDetect.cpp and hwdetect.js:

// The Set* functions will override the default behaviour, unless the user
// has explicitly set a config variable to override that.
// (TODO: This is an ugly abuse of the config system)
// TODO: add some mechanism for setting config values
	// (overriding default.cfg, but overridden by local.cfg)

So eventually we would want to use something like the JSI_ConfigDB functions instead, but I don't know if they support the required functionality yet (see TODO above). That's beyond the scope of this diff, however.

I think the point about removing Crash and DebugWarn for security reasons is probably the strongest. I would say nobody talks about them, because nobody knows they are there :)

How else can I break into my JS script and see the engine callstack?

throw in JS gives a callstack about what's wrong in JS
won't the C++ callstack always be the one that performs the current JS script, i.e. not the relevant part of the bug and also determinable from the knowledge which C++ file performs the JS script?

We have no integrated JS debugger anymore. Think of it as a poor man's debugger. I would be fine with selectively enabling those features (without having to recompile), as average users won't want or need them

I just know that every 'debug' feature will be abused at some point, and that if we distribute this software to possibly 100k people I rather remove hypothetically (not actually) useful hypothetically harmful things.

0x00007ffff62cf636 in waitpid () from /usr/lib/libpthread.so.0
(gdb) info stack
#0  0x00007ffff62cf636 in waitpid () from /usr/lib/libpthread.so.0
#1  0x0000555555b50b26 in try_gui_display_error (no_continue=false, allow_suppress=false, manual_break=false, text=0x7fffffffce50 L"") at ../../../source/lib/sysdep/os/unix/unix.cpp:165
#2  sys_display_error (text=text@entry=0x7fffffffd150 L"meh", flags=flags@entry=8) at ../../../source/lib/sysdep/os/unix/unix.cpp:215
#3  0x0000555555aed973 in CallDisplayError (flags=<optimized out>, text=0x7fffffffd150 L"meh") at ../../../source/lib/debug.cpp:383
#4  debug_DisplayError (description=<optimized out>, flags=<optimized out>, flags@entry=8, context=context@entry=0x0, lastFuncToSkip=lastFuncToSkip@entry=0x0, pathname=pathname@entry=0x0, line=line@entry=0, func=0x0, suppress=0x0)
    at ../../../source/lib/debug.cpp:452
#5  0x0000555555be033e in JSI_Debug::DisplayErrorDialog (msg=L"meh") at /usr/include/c++/9.1.0/bits/basic_string.h:2300
#6  ScriptInterface_NativeWrapper<void>::call<void (ScriptInterface::CxPrivate*, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&), std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > >(JSContext*, JS::MutableHandle<JS::Value>, void (ScriptInterface::CxPrivate*, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&), std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >) (fptr=<optimized out>, cx=0x555556bfd080) at ../../../source/scriptinterface/NativeWrapperDefns.h:86
#7  ScriptInterface::call<void, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, &JSI_Debug::DisplayErrorDialog> (cx=cx@entry=0x555556bfd080, argc=<optimized out>, vp=0x555555f52ae0)
    at ../../../source/scriptinterface/NativeWrapperDefns.h:125
#8  0x00007ffff77c8b12 in js::CallJSNative (args=..., 
    native=0x555555be01d0 <ScriptInterface::call<void, std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >, &JSI_Debug::DisplayErrorDialog>(JSContext*, unsigned int, JS::Value*)>, cx=0x555556bfd080)
    at ../../dist/include/js/CallArgs.h:204
#9  js::Invoke (cx=0x555556bfd080, args=..., construct=<optimized out>) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-38.0.0/js/src/vm/Interpreter.cpp:498
#10 0x00007ffff77bda65 in Interpret (cx=0x555556bfd080, state=...) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-38.0.0/js/src/vm/Interpreter.cpp:2602
#11 0x00007ffff77c879b in js::RunScript (cx=cx@entry=0x555556bfd080, state=...) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-38.0.0/js/src/vm/Interpreter.cpp:448
#12 0x00007ffff77d76e5 in js::ExecuteKernel (cx=cx@entry=0x555556bfd080, script=script@entry=..., scopeChainArg=..., thisv=..., type=type@entry=js::EXECUTE_GLOBAL, evalInFrame=..., evalInFrame@entry=..., result=0x7fffffffdfa0)
    at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-38.0.0/js/src/vm/Interpreter.cpp:654
#13 0x00007ffff77d914b in js::Execute (cx=cx@entry=0x555556bfd080, script=script@entry=..., scopeChainArg=..., rval=0x7fffffffdfa0) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-38.0.0/js/src/vm/Stack.h:242
#14 0x00007ffff7b1e389 in Evaluate (cx=0x555556bfd080, obj=..., optionsArg=..., srcBuf=..., rval=...) at ../../dist/include/js/RootingAPI.h:996
#15 0x00007ffff7b1e8e5 in Evaluate (rval=..., length=<optimized out>, chars=<optimized out>, optionsArg=..., obj=..., cx=<optimized out>)
    at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-38.0.0/js/src/jsapi.cpp:4107
#16 JS::Evaluate (cx=<optimized out>, obj=..., obj@entry=..., optionsArg=..., chars=<optimized out>, length=<optimized out>, rval=..., rval@entry=...)
    at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-38.0.0/js/src/jsapi.cpp:4161
#17 0x000055555577b39a in ScriptInterface::Eval_ (this=this@entry=0x555556bfc980, code=code@entry=0x555555eb3620 L"Engine.DisplayErrorDialog(\"meh\")", rval=..., rval@entry=...) at /usr/include/c++/9.1.0/bits/unique_ptr.h:357
#18 0x00005555557a3dc8 in ScriptInterface::Eval<wchar_t> (ret=..., code=0x555555eb3620 L"Engine.DisplayErrorDialog(\"meh\")", this=0x555556bfc980) at ../../../source/scriptinterface/ScriptInterface.h:571
#19 CConsole::ProcessBuffer (this=0x555555dae620, szLine=0x555555eb3620 L"Engine.DisplayErrorDialog(\"meh\")") at ../../../source/ps/CConsole.cpp:563
#20 0x00005555557a46c0 in CConsole::InsertChar (this=0x555555dae620, szChar=<optimized out>, cooked=0 L'\000') at ../../../source/ps/CConsole.cpp:329
#21 0x00005555557a4c22 in conInputHandler (ev=0x7fffffffe210) at ../../../source/ps/CConsole.cpp:690
#22 0x0000555555b33e0a in in_dispatch_event (ev=0x7fffffffe210) at ../../../source/lib/input.cpp:62
#23 0x00005555555f4ce4 in PumpEvents () at ../../../source/main.cpp:227
#24 Frame () at ../../../source/main.cpp:367
#25 0x00005555555f92fd in RunGameOrAtlas (argc=<optimized out>, argv=<optimized out>) at ../../../source/main.cpp:638
#26 0x00005555555e9eaa in main (argc=1, argv=0x7fffffffe908) at ../../../source/main.cpp:684

The idea is that one can switch into the debugger if some JS conditions are met?

JSI_Debug::Crash still seems gruesome to me, but whatever.

If you want this one, you may want the other ones as well, so you're welcome this file exists :P

I didn't test the diff, but it looks good to me and I guess you did test.
Thanks for the fix!

This revision is now accepted and ready to land.Jul 24 2019, 10:16 PM
In D2123#88693, @elexis wrote:

The idea is that one can switch into the debugger if some JS conditions are met?

JSI_Debug::Crash still seems gruesome to me, but whatever.

Exactly, and of course it's an ugly hack :) There is a lot of debugging code that we'll eventually want to disable in release builds, but has been extremely useful to have during development. I'm not sure when exactly we make that call though.

This revision was automatically updated to reflect the committed changes.