[PATCH] Remove g_ScriptingHost and implement global to compartment 1 to 1 relation

[Yves]

Here's the description copied from the Spidermonkey upgrade forum thread:

Each GUI page in its own compartment

The first major change is having each GUI Page (CGUI Object) in its own compartment using its own ScriptInterface instance. Currently the GUI uses two global objects. One is a page global for each GUI page which should separate global variables between the pages. The other global is used for all pages and contains engine function definitions and such. The new Spidermonkey associates a compartment with each global object and this design doesn't work anymore. At least I didn't find a way to make it work with minor changes. Cross compartment calls and wrapper objects could provide a solution here but I decided the other approach looks more promising. At the moment the plan is to register all GUI functions for all pages. I don't think registering maybe 100 functions for each GUI page should be a performance problem or memory problem. If it becomes a problem we can still implement something that lets GUI pages include functions they need.

Getting rid of g_ScriptingHost

We are still using the old ScriptingHost that had been created before ScriptInterface was developed. That's unneeded duplication and should be replaced by ScriptInterface. It's sometimes a bit tricky because a lot of code simply accesses this global object and some thoughts are required that this code uses the right context/ScriptInterface/compartment in the future.

I'm currently working on a patch but it's not quite ready yet.

[Yves]

I'm currently facing the problem that v1.8.5 fails to create a structured clone when the object contains wrappers. That's bad because I have to use a slightly different solution for 1.8.5 and for the new Spidermonkey in this patch. I've listed the possible approaches I see along with some pros and cons. I currently tend to the last approach.

Structured clones instead of wrappers
	-- Testing the patch under the right conditions is NOT possible
	- Can't test performance differences
	- Bad for performance
	- some additional things to change for the final solution


One compartment per Runtime
	-- Testing the patch under the right conditions is NOT possible
	- Can't test performance differences
	+ No known performance drawbacks
	- some additional things to change for the final solution

Patch Spidermonkey 1.8.5
	+ Testing the patch under the right conditions is possible
	+ Testing performance differences is possible
	+ No known performance drawbacks
	+ As close to the final solution as possible
	-- We drop support for bundled 1.8.5 libraries


Patch Spidermonkey 1.8.5 for testing the patch, but use one compartment per runtime in the first committed patch version
	+ Testing the patch under the right conditions is possible
	+ Testing performance differences is possible
	+ No known Performance drawbacks
	- some additional things to change for the final solution
	- unknown if it works as expected
	- more work (creating patch for 1.8.5 that will be thrown away, maintainging and testing two different versions of the patch)

[Yves]

1. Structured clones don't work because there's recursion somewhere.
2. Using the current approach (one compartment per runtime) doesn't work because there's a problem with registring the GUIObject type and its functions. It currently looks like a Spidermonkey bug but I could investigate further if I decide to spend another day or two on it with low chances of success.
3. The Patch seems to have a lot of prerequisites and it would probably be too much work to create it and test it properly.

I could wait with that commit until the new version of Spidermonkey is ready to commit, but that would mean combining two very large patches and having to review one "megapatch". I would like to avoid that if possible.

[Yves]

A quick hack that fixes the cloning of objects containing wrappers.

[Yves]

This is the official patch that fixed this problem: ​http://hg.mozilla.org/mozilla-central/rev/1fb77c1bb742

Unfortunately it relies on other patches and can't be easily applied. I've attached my own workaround for this problem. My solution works for us, but it probably doesn't work correctly with principals. On the other hand, this case shouldn't affect any other users of the library anyway because cloning of objects containing wrappers doesn't work at all at the moment.

[Yves]

In 14102:

Remove the old and no longer used archive builder that rebuilt the archive while the game is running.
This approach isn't used anymore. Now we start the archive building with parameters to pyrogeneis.

Refs #2241 (the code used g_ScriptingHost which is going to be removed)

[Yves]

In 14199:

In #2241 I'm going to change the GUI to have one ScriptInterface for each GUI page because that will be required for the Spidermonkey upgrade (#1886).
The Multiplayer lobby needs some changes to avoid compartment mismatches. Instead of initializing it with a ScriptInterface and storing script values at different locations, it takes a ScriptInterface argument in the functions that really need to read or write some script values and avoids storing values as script values with an associated compartment where possible.
The scripting interface of the lobby is also adjusted to use JSInterface_Lobby.h/cpp files as other components instead of adding all functions to ScriptFunctions.cpp. This makes it a bit more clearly arranged IMO.

Fixes #2267
Refs #2241
Refs #1886

[Yves]

Here's the first version of the patch ready for tests and review. It will need a lot of testing and careful review! ;) There are most likely some issues remaining, but these should be small enough for a review to make sense (especially if something bigger is brought up during the review). I will continue my own tests, but help with testing is very welcome!

How to apply

1. Apply FixSpidermonkeyWrappers_v1.0.diff to solve the wrapping issue of Spidermonkey 1.8.5
2. Apply RemoveScriptingHost_oneCompartmentPerGUIPage_[VERSION].diff
3. Run clean-workspaces and update-workspaces and DON'T use --with-system-mozjs185

The patch is tested for rev 14197.

Known issues

1. I have to take a closer look at some warnings in debug mode: [debug]JS engine warning: leaking GC root 'CScriptValRooted' at 0xa9b4d50

[mimo]

I get a crash when building with --without-lobby which should be due to this commit

Linking pyrogenesis
../../../binaries/system/libgui.a(ScriptFunctions.o): dans la fonction « call<bool (*)(void*)> »:
/build/workspaces/gcc/../../../source/scriptinterface/NativeWrapperDefns.h:33: référence indéfinie vers « JSI_Lobby::HasXmppClient(void*) »
collect2: error: ld returned 1 exit status
make[1]: *** [../../../binaries/system/pyrogenesis] Erreur 1
make: *** [pyrogenesis] Erreur 2

[Yves]

Thanks, it's fixed here: r14203

[Yves]

Version 1.1 of the patch fixes the leaking GC root issue and a renames "fileExists" to "Engine.FileExists" in lobby.js.

Some additional things to do about the Spidermonkey wrapping issue (thanks to Philip for the input):

1. Check if the buggy functions could be copied to our code to avoid compatibility problems with Spidermonkey 1.8.5 packages (probably difficult, but I will try that).
2. Otherwise try to detect a wrong version of the library at build time. Some defines could be added to jsversion.h and checked in the engine code for example.
3. If repackaging of is needed, contact packages maintainers.

[Yves]

I've tried to copy the modified functions into our code (point 1). Unfortunately I came to the conclusion that this is not going to work. This code depends on way too much other code that is not in the public API.

Now I've modified the patch a little bit to inform package maintainers and people who compile from source by causing a compile error that should point them to this ticket (point 2).

I think I will wait until after the release of Alpha 15. I hope the planned release date (5th of December) will be kept. After that I can start contacting package maintainers (point 3).

[Yves]

This version adds an error at compile-time to inform about the need to patch Spidermonkey

[Yves]

I discovered some problems with hotloading.

[Yves]

The hotloading problems are solved in my next version of the patch, but there are other problems with wrapping and serialization and I noticed that this patch makes the performance significantly worse. I think the problem is the wrapping and this could also be the problem with the new SpiderMonkey version. I don't mean creating the wrappers but how the wrappers affect the JIT compiler.

[Yves]

The next version with some serialization fixed

[Yves]

In 14441:

Moves AI players to one global using the module pattern.
This avoids wrapping overhead that would otherwise be required because multiple globals per compartment aren't supported anymore in newer versions of SpiderMonkey.

Check the ticket for a detailed explanation.

Closes #2322
Refs #2241
Refs #1886

[Yves]

Some comments:

• The patch changes how messageboxes work (check the comment in functions_global_object.js)
• The fixed renderpath message now pops up after the Splashscreen is closed instead of right after it's opened. This has to be done because the callback code relies on the stack behaviour of the GUI page stack (LIFO order) to find in which page it should try to call the callback function.
• The JS console runs in the active GUI page instead of the g_ScriptingHost context (we don't have direct access to the simulation data or AI data anyway, so it shouldn't matter).
• The size in this line is quite arbitrary (as it was before too): g_ScriptRuntime = ScriptInterface::CreateRuntime(128 * 1024 * 1024);
• Tested so far: Replay mode, hotloading, serialization (full hash at the end of an AI game matches), performance (no problems anymore)
• Tested on rev r14453

[Yves]

This version works without the SpiderMonkey? patch and because of #2322 there are no performance problems anymore. Hotloading works and serialization works too.

[wraitii]

Tested this morning. C++ side code seems clean, it's readable, it's even clearer in several spots at first sight, I like it, and I haven't noticed any format typo (though I haven't looked at it with extreme scrutiny). It compiles cleanly on my OS. There is a useless line ending change in ComponentManagerSerialization.cpp though.

JS side you've missed a commented one l294 of selection_details.j You haven't changed help.txt You've missed "endgame" in CancelOnError in functions_utility_errors.js Some search reports no other misses.

It'd need a bit of update for the latest GUI updates I assume, but nothing un-doable. I'd say we should commit this as soon as possible.

[Yves]

Addresses wraitii's input and upldates to r14461

[Yves]

I'm trying something to ease the review of this patch. Should be ready in a few hours.

[Yves]

a table classifying the complexity of changes per file (should help with reviewing).

[Yves]

Some minor fixes, but it doesn't yet fix the build problem on Windows that was reported

[Yves]

This version fixes the build errors on Windows

[Yves]

Concerning the JS Debugger, I think I'll have to remove it together with this patch. Currently it crashes after closing the splash screen. I'm quite sure the reason is what's described in #1890. That problem there becomes bigger with this patch because now there's only one runtime and a lot of contexts per runtime. It would require a major redesign of the JS debugger which will be required anyway for the SpiderMonkey upgrade. It would not make sense to fix that now and then do it from scratch again after updating SpiderMonkey.

[Yves]

This version fixes saving and loading

[Yves]

Modified CGUIManager::SwitchPage to work around a crash that only happened when using --with-system-mozjs for an unknown reason. Applies cleanly with r14478.

[Yves]

In 14496:

Removes g_ScriptingHost and implements global to compartment 1 to 1 relation.
Each GUI Page gets its own compartment and all ScriptInterfaces in the same thread should now use the same JS Runtime.
This is required for the SpiderMonkey upgrade.
Check the ticket for details.

Closes #2241
Refs #1886
Refs #1966

[Yves]

In 14498:

Fixes assertion failure when clicking the settings button (aiconfig) on the match setup screen.
This problem happens because callback functions need to be passed to GUI pages differently since r14496.

Refs #2241

[Yves]

In 14499:

Fixes assertion failure when opening the ingame manual.
This problem happens because callback functions need to be passed to GUI pages differently since r14496.

Refs #2241

[Yves]

In 14506:

Disables the JS debugger.
It's completely broken since r14496 and will have to be updated for the new SpiderMonkey API.
I only uncomment it at the moment because I plan to fix/reimplement it soon after the upgrade.

Closes #2348
Refs #2241
Refs #1886

[Yves]

In 14507:

Fixes an occasional crash introduced in #2241 (hopefully).

Refs #2241

[Yves]

In 14556:

Fixes a problem when messageboxes were called without callback function.
Fixes #2367
Refs #2241

[Yves]

In 14573:

Fixes an occasional crash when hotloading GUI files.
Refs #2241

[historic_bruno]

Replying to Yves:

• The fixed renderpath message now pops up after the Splashscreen is closed instead of right after it's opened. This has to be done because the callback code relies on the stack behaviour of the GUI page stack (LIFO order) to find in which page it should try to call the callback function.

Does this mean multiple overlapping dialogs will break callbacks? That sounds like a bug :/

The workaround doesn't work here, I've forced fixed renderpath but the warning never appears after the splash screen. Also, it should appear before / on top of the splash screen, that's why it was pushed immediately after the splashscreen.

[Yves]

In 14592:

Fixes the problem that functions in globalscripts aren't available from GUI scripts anymore since r14496.
Refs #2241

[Yves]

In 14593:

Fixes a problem with the renderpath screen not being displayed.
Refs #2241

[Yves]

Replying to historic_bruno:

Replying to Yves:

• The fixed renderpath message now pops up after the Splashscreen is closed instead of right after it's opened. This has to be done because the callback code relies on the stack behaviour of the GUI page stack (LIFO order) to find in which page it should try to call the callback function.

Does this mean multiple overlapping dialogs will break callbacks? That sounds like a bug :/

It breaks callbacks in this case: Page A opens Page B and then (before B is closed) Page C.

It doesn't break callbacks in this case: PageC is opened by Page B which was opened by PageA.

In my opinion we shouldn't do this anyway. Definitely not for simple messageboxes because they are modal dialogs.

I haven't found any places other than the renderpath screen where this limitation is a problem in the current GUI. It could be a problem for future features like tabbed interfaces where tabs are represented with GUI pages. In this case the more central design fault is using a stack to store GUI pages. Using a stack there basically says its OK to accept the limitations of a stack.

Anyway, if this limitation is a problem and we need to find a solution, using private pointers (keywords: JSClass, JSCLASS_HAS_PRIVATE) in the GUI object to pass a pointer to the callback page could work (that's just a rough idea, I'd have to test it).

The workaround doesn't work here, I've forced fixed renderpath but the warning never appears after the splash screen. Also, it should appear before / on top of the splash screen, that's why it was pushed immediately after the splashscreen.

I must have broken it in one version of the patch. It worked at some point, I've tested it. Now it's fixed.

[historic_bruno]

Replying to Yves:

In this case the more central design fault is using a stack to store GUI pages. Using a stack there basically says its OK to accept the limitations of a stack.

I think my main concern isn't having limitations, but knowing those limitations and not enforcing them somehow (if that's the case here). This is an obscure aspect of the GUI engine, but one that would be easy to encounter as a GUI designer who doesn't know all the internals of SM and our GUI engine.

What happens when the breakage occurs, is it possible that it won't be obvious? I just fixed a stack-related bug in r14591 because whoever wrote the GUI didn't consider dialogs, yet we have them, it's not terrible to tweak the design to make it functional.

The workaround doesn't work here, I've forced fixed renderpath but the warning never appears after the splash screen. Also, it should appear before / on top of the splash screen, that's why it was pushed immediately after the splashscreen.

I must have broken it in one version of the patch. It worked at some point, I've tested it. Now it's fixed.

Thanks!

[mimo]

I'm facing a weird bug with messageBox. I don't know if related, but could be. The problem can be reproduced using a simple messageBox(500, 200, "test", "test", 0, ["Yes", "No"], [null, null]);

If I put this instruction at different places in the code, it works as expected, except in the function loadGame() of gui/savedgames/load.js : If I put it as first line of the function, I got a JS error saying that gameSelection is undefined, and if I put it somewhere else in that function, it is just ignored.

If anybody have some hint about it, that would be welcomed !

[mimo]

The problem reported in previous comment was mainly due to my bad usage of messageBox (thanks to Yves for the help) so not related to this ticket.

[Yves]

In 15495:

Revert obsolete workaround for structured clones.

Reverts r14573 because that workaround isn't needed anymore with SpiderMonkey 24 (and 31).

Refs #2241
Refs #1886