Page MenuHomeWildfire Games

Update to Spidermonkey 45.0.2
ClosedPublic

Authored by Itms on May 20 2018, 3:55 PM.

Details

Reviewers
wraitii
historic_bruno
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22627: Upgrade SpiderMonkey to version 45.0.2, refs #4893.
Trac Tickets
#4893
Summary

This upgrades 0 A.D. to work with Spidermonkey 45.0.2

Here is the list of changes, for eased review:

Test Plan

Try compiling spider monkey 45, compiling the game, starting up some games.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8471
Build 13843: Vulcan BuildJenkins
Build 13842: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Dec 26 2018, 3:49 PM
binaries/data/mods/public/globalscripts/vector.js
436 ↗(On Diff #6598)

I thought more of https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_GetPrototype but I didn't check this yet. (Also didn't check yet whether if Itms branch has a fix for this hunk.)

lyv added inline comments.Dec 26 2018, 4:27 PM
binaries/data/mods/public/globalscripts/vector.js
436 ↗(On Diff #6598)

That is more straightforward and from the looks of it, the proper way to do it.

Stan added a comment.Dec 26 2018, 4:31 PM

Anything I can do to help ?

build/premake/extern_libs5.lua
556

45

572

45 for all above

libraries/source/spidermonkey/patch.sh
13–26

45

wraitii marked an inline comment as done.Dec 28 2018, 10:54 AM

@elexis feel free to commandeer.

I suggest to commit the JS::ObjectValue changes separately, ahead, and every other change that works with sm38.

Meh, we're so far behind anyways that it's not going to help much and the changes are kinda safe. But if you feel like splitting, go ahead.

binaries/data/mods/public/globalscripts/vector.js
436 ↗(On Diff #6598)

Should be checked with upstream.

Itms commandeered this revision.Dec 28 2018, 11:24 AM
Itms edited reviewers, added: wraitii; removed: Itms.

I am working on this and so far I am not having issues, it's just a bit time-consuming (so holidays are perfect for me). So if you don't mind someone else commandeering this @wraitii, I'm taking it.

I'll try to use exclusively Phabricator for sharing my work, so that information is not scattered. If I'm blocked by something, we can look for a solution.

Itms planned changes to this revision.Dec 28 2018, 11:24 AM

Meh, we're so far behind anyways that it's not going to help much and the changes are kinda safe. But if you feel like splitting, go ahead.

It's not a split into two revision proposals, but making this revision proposal shorter.
We always attempt to commit into atomic steps (not only the last spidermonkey where it were like 30 commits).
Also the first diff of the sm45 diffs was already committed in rP20062.
The OBJECT_TO_JSVAL change really looks like a commit that we can do already to shorten the rest of the diff.
Also it appears to be the only change that can be split off and used with sm38, so there would not be more that could be split off as far as I see (if want that every revision of the repository is compilable and usable).

elexis updated the Trac tickets for this revision.Dec 29 2018, 3:16 PM

@Itms you mentioned somewhere to plan to go to sm60 after sm52, but I still didn't see any information that indicates that sm60 is planned as a release (Only FF).
(If we don't see sufficient benefit to stick to releases, then one can consider going to more recent versions than v60) Also the sm52 release looks less complete than our a23 one, only a pre-release package.

The difference between an actual sm "release" and the rest is not only testing and a compatibility difference, but also that the releases might be more distributed to different platforms.
Consider the ALT's e2k port mentioned in #4893 as an example.
On the other hand I have seen that debian also provides an sm60 package (although sm60 is not marked as a release).
Knowing about these things beforehand may increase the platforms 0ad can run on, so it may be worth to explore (or describe if already explored).

binaries/data/mods/public/globalscripts/vector.js
436 ↗(On Diff #6598)

(What upstream in particular? The release announcement or specs explaining some change that explains this behavior difference?
We should check that the this instance actually refers to the intended instance.
The GUIManager can sometimes unintentionally call functions defined in page X on the context of page Y (examples in D1701).
Otherwise, this is a globalscript, and the globalscripts are loaded in the SciprInterface constructor, it may be a loading order thing.)

Itms updated this revision to Diff 7128.Dec 29 2018, 10:52 PM
Itms edited the summary of this revision. (Show Details)

This works for me ? and is based on D1716. arcanist should theoretically be able to patch in chain...

I updated the description with useful information for the reviewers.

Owners added a subscriber: Restricted Owners Package.Dec 29 2018, 10:52 PM
Itms added a comment.Dec 29 2018, 11:03 PM
In D1510#67827, @elexis wrote:

@Itms you mentioned somewhere to plan to go to sm60 after sm52, but I still didn't see any information that indicates that sm60 is planned as a release [...]
Knowing about these things beforehand may increase the platforms 0ad can run on, so it may be worth to explore (or describe if already explored).

About the ESR versions of SpiderMonkey: yes the documentation is poorly maintained, and releases of SpiderMonkey are not well announced (more on that below). However, I confirm the past released versions of SM were 38, 45 and 52. The current version is 60. Version 68 is the upcoming ESR. SM60 started to be developed on 2018-01-22, was released 2018-05-19, and will receive fixes until 2019-10-21 (two versions after the release of SM68). Those dates are taken from https://wiki.mozilla.org/Release_Management/Calendar: SM releases match the releases of Firefox ESR, as stated in the documentation.

This information was confirmed by the SM maintainers on IRC. On their IRC, one can find a link to https://github.com/spidermonkey-embedders/spidermonkey-embedding-examples, which looks like an attempt at a new format of the documentation for the embedders; this repo does describe SM60 as the current release. Other projects who embed SpiderMonkey confirm that this is the correct list of releases: most notably GNOME, who migrated from SM52 to SM60 in this issue . This commit explicitly shows that the previous version they used was 52. On the Wikipedia page, one can find the list of releases I'm speaking about, as well as other embedders that can be consulted (some of them embed all the versions, not just the released ones).

About releases not being nicely released: given the low number of embedders, SpiderMonkey does not have very robust standalone release scripts. We often need to patch SM against bugs that only affect embedders, and not Firefox, and those bugs are not quickly fixed. SM also receives regular small updates during the lifetime of a release, so it would only make sense for them to fully release once the version has reached end-of-life. But we cannot wait for that moment, because then we would not be able to report bugs. Additionally, they don't go through the hassle of bundling a version that was first released more than a year before.

Also the sm52 release looks less complete than our a23 one, only a pre-release package.

Are you talking about the size of the bundle? sm45 was abnormaly big, so sm52 and 60 look tiny in comparison. If you are talking about the "pre-" in the name, it's just that they didn't bother moving 52 to releases/ while they were already working on Firefox 62. But we could ask them to.

So what about us?
45 and 52 will not receive any updates anymore, so we can include them. For 60, we have the choice. We could wait for the release of Firefox 70, i.e. for 60 to be EOL and without any updates, to include it. But we have at least the responsibility, as downstream, to make it work for us before that, so that we can report bugs and have them fixed while we can. Committing it is another matter: I am in favor of doing so because

  • it gives us a recent version with performance improvements and fixes
  • it is painless to upgrade to a minor version of the current ESR if we need to, or if we want the latest after its EOL
  • it allows package maintainers on Linux to use the mozjs release from Firefox ESR (especially on Debian and Ubuntu)

but I agree there are cons.
Additionally, I think we mustn't try to upgrade to a new version as long as the previous one is not EOL (don't try to upgrade to SM68 while Firefox 70 isn't released).

In conclusion:
I hope this long overview is clear enough; don't hesitate to ask more about the Firefox release process, which I find neat. ?

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|   1|    |-/////////////////////////////////////////////////////////////////////
|    |   1|+// ///////////////////////////////////////////////////////////////////
|   2|   2| //	Vector2D
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|   3|   3| //
|   4|   4| //	Class for representing and manipulating 2D vectors
|   5|   5| //
|   6|    |-/////////////////////////////////////////////////////////////////////
|    |   6|+// ///////////////////////////////////////////////////////////////////
|   7|   7| 
|   8|   8| // TODO: Type errors if v not instanceof Vector classes
|   9|   9| // TODO: Possibly implement in C++
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 240| 240| 	return sum;
| 241| 241| };
| 242| 242| 
| 243|    |-/////////////////////////////////////////////////////////////////////
|    | 243|+// ///////////////////////////////////////////////////////////////////
| 244| 244| //	Vector3D
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 245| 245| //
| 246| 246| //	Class for representing and manipulating 3D vectors
| 247| 247| //
| 248|    |-/////////////////////////////////////////////////////////////////////
|    | 248|+// ///////////////////////////////////////////////////////////////////
| 249| 249| 
| 250| 250| function Vector3D(x = 0, y = 0, z = 0)
| 251| 251| {
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["Vector2Dprototype"] is better written in dot notation.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 437| 437| // https://trac.wildfiregames.com/ticket/5376
| 438| 438| 
| 439| 439| // make the prototypes easily accessible to C++
| 440|    |-this["Vector2Dprototype"] = Vector2D.prototype;
|    | 440|+this.Vector2Dprototype = Vector2D.prototype;
| 441| 441| this["Vector3Dprototype"] = Vector3D.prototype;
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["Vector3Dprototype"] is better written in dot notation.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/vector.js
| 438| 438| 
| 439| 439| // make the prototypes easily accessible to C++
| 440| 440| this["Vector2Dprototype"] = Vector2D.prototype;
| 441|    |-this["Vector3Dprototype"] = Vector3D.prototype;
|    | 441|+this.Vector3Dprototype = Vector3D.prototype;

binaries/data/mods/public/globalscripts/vector.js
| 440| this["Vector2Dprototype"]·=·Vector2D.prototype;
|    | [NORMAL] JSHintBear:
|    | ['Vector2Dprototype'] is better written in dot notation.

binaries/data/mods/public/globalscripts/vector.js
| 441| this["Vector3Dprototype"]·=·Vector3D.prototype;
|    | [NORMAL] JSHintBear:
|    | ['Vector3Dprototype'] is better written in dot notation.

Link to build: https://jenkins.wildfiregames.com/job/differential/851/

I think trying to move to SM60 will be a positive because I believe that includes the JS_Runtime changes already, which is the hard part to update.

It still seems very wrong to neglect the differences between a SpiderMonkey pre-release, a SpiderMonkey release and a Mozilla Extended Support Release....

SM52, SM59, SM60 are currently still pre-releases, not releases. https://ftp.mozilla.org/pub/spidermonkey/
Notice they also have SM59 as a pre-release, whereas FF 59 was not an ESR...

ESR is a specific process that is applied to the mozilla products (Firefox + Thunderbird):
https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
https://wiki.mozilla.org/Enterprise/Firefox/ExtendedSupport:Proposal
https://www.mozilla.org/en-US/firefox/organizations/

(ESR) Releases will be maintained for more than a year, with point releases containing security updates coinciding with regular Firefox releases.
The ESR will also have a two cycles (12 week) overlap between the time of a new release and the end-of-life of the previous release to permit testing and certification prior to deploying a new version.
Mozilla relies on Firefox ESR users to provide feedback for the new ESR releases. During the first two cycles, please report any bugs about web compatibility regressions, stability issues, and so on.
Maintenance of each ESR, through point releases, is limited to high-risk/high-impact security vulnerabilities and in rare cases may also include off-schedule releases that address live security vulnerabilities. Backports of any functional enhancements and/or stability fixes are not in scope.

I think SM releases are derived from the Mozilla ESRs, but that doesn't mean that SM is also under the ESR process.

Version 68 is the upcoming ESR

ESR refers to the Mozilla Products (FF + thunderbird), not SM.

SM60 started to be developed on 2018-01-22, was released 2018-05-19... Those dates are taken from https://wiki.mozilla.org/Release_Management/Calendar

But that's the calendar of Mozilla products, it doesn't speak about the SM releases.

SM releases match the releases of Firefox ESR, as stated in the documentation.

I didn't see this on any documentation site, link?
I do see an indication for that in our SM38 README.txt:

This release is based on a revision of Mozilla 38:
http://hg.mozilla.org/releases/

Also the sm52 release looks less complete than our a23 one, only a pre-release package.

Are you talking about the size of the bundle?

With incomplete I refer to the fact that

https://github.com/spidermonkey-embedders/spidermonkey-embedding-examples this repo does describe SM60 as the current release

"The information on this esr60 branch applies to SpiderMonkey 60.x, an Extended Support Release (ESR). "

esr60 are the Mozilla products which are an ESR release that contain SpiderMonkey 60. This quote doesn't imply that the individually released SpiderMonkey 60 receives the ESR process as well.

Other projects who embed SpiderMonkey confirm that this is the correct list of releases

They upgraded to the most recent pre-release and projects also have the freedom to go beyond release if they experience bugs.

But these last three points are very circumstancial, it would be more indicative to stick with the primary source.
For example showing how a past SM release was affected by the ESR process (as defined by the Mozilla products).
(But using the terms they are defined in the docs seems easier than trying to prove something that isn't mentioned in the docs)

elexis added inline comments.Dec 30 2018, 12:45 PM
binaries/data/mods/public/globalscripts/vector.js
436 ↗(On Diff #6598)

Did you try JS_GetPrototype?

elexis added inline comments.Dec 30 2018, 12:46 PM
binaries/data/mods/public/globalscripts/vector.js
436 ↗(On Diff #6598)

Sorry, didn't see #5376.

Itms added a comment.Dec 30 2018, 1:18 PM
In D1510#68005, @elexis wrote:

It still seems very wrong to neglect the differences between a SpiderMonkey pre-release, a SpiderMonkey release and a Mozilla Extended Support Release....

Yes, sorry about that. However SM developers themselves use the terms "release" and "ESR" liberally, for instance in bug reports, hence my sloppy use of the terms. I'll be more cautious. To clarify: when Firefox has an ESR release, SM developers create a pre-release of SM as soon as possible, and mark it as a release when they are confident the product is good enough.

Most remarks you are making are addressed in this bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1422930, which should give you a better overview of why the SM releases are not always completed and stay in the "pre-release" state. It is not clear for them when to say that the pre-release has been sufficiently tested to become a release, because of the low number of embedders. The low number of embedders also makes their job to develop SM as a piece of Firefox more important than giving love to the standalone releases, which explains the lack of documentation and PR.

Notice they also have SM59 as a pre-release, whereas FF 59 was not an ESR...

That is because 59 was supposed to be ESR (24,31,38,45,52,59,66...) but Mozilla changed the ESR cycle from 7 Firefox releases to 8 between 52 and 60. See the bug report above, again.

SM releases match the releases of Firefox ESR, as stated in the documentation.

I didn't see this on any documentation site, link?

Indeed, I can't find a link from a primary source for that (I probably remembered our README), but that is still what SM developers attempt to do. Would a statement from the developers on their IRC be enough for you? Or is the discussion in the bug report above clear enough to lift your uncertainty?

With incomplete I refer to the fact that [...]

Yes. Standalone releases are not their priority, that is a fact.

They upgraded to the most recent pre-release and projects also have the freedom to go beyond release if they experience bugs.

And it is also our responsibility to try pre-releases (not necessarily commit them) so that we can give feedback to them in the form of: "we have a bug, can it be fixed?" or "it works for us, we think you can release officially".

(But using the terms they are defined in the docs seems easier than trying to prove something that isn't mentioned in the docs)

Yes I'll stick with the correct terms in the future.

Stan added a comment.Dec 30 2018, 1:24 PM

Smiley raised a good point on irc today. Does it make sense to use eom release of Spidermonkey which means we are always lacking in terms of security ?
To me yes because we only run our code or code from mods we accept and we are not liable for any third party mod we didn't accept. Also it means we don't have to keep patching to keep up with upstream.

Itms planned changes to this revision.Jan 13 2019, 5:39 PM

I'm rebasing this one, and I'll try to include the binaries for Windows in the diff.

@Itms Hm, so what's the status on this?

Krinkle added inline comments.Jun 16 2019, 10:12 PM
binaries/data/mods/public/globalscripts/vector.js
436 ↗(On Diff #6598)

This was probably changed from var to const at some point, which was a mistake because where var is lexically scoped, const is block scoped.

This means that under standard ES6 behaviour, const variables do not become properties of the global object. I don't know for sure, but I guess the previous version of SpiderMonkey exposed constants as global variables by accident, and it was fixed between SM 38 and 45?

Restoring this to var would make it work without the verboseness of this assignments, which is a bit unusual.

Note that I'm proposing to remove this code in D1991.

wraitii added inline comments.Jun 17 2019, 10:12 AM
binaries/data/mods/public/globalscripts/vector.js
436 ↗(On Diff #6598)

Right.

I think I prefer the verbose solution because what we are trying to do is make them properties of the global object, and relying on var doing that is not very obvious.

That being said, we probably do want a better solution.

Itms updated this revision to Diff 9006.Jul 20 2019, 4:50 AM

Rebased; this is a RC.

Much to my regret arc doesn't want to upload the tarball. You can find it at https://ftp.mozilla.org/pub/spidermonkey/releases/45.0.2/.
You can test the git branch, complete with binaries including the Windows ones, at https://github.com/na-Itms/0ad/tree/sm45.

Happy testing!

Build failure - The Moirai have given mortals hearts that can endure.

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

Compiles, in-games correctly.

There's a problem on MacOS (on my machine™) - the libmozjs45-ps-release.a library is generated in mozjs-45.0.2/js/src/build-release/js/srcand not linked in /Users/Lancelot/Desktop/git-0AD/libraries/source/spidermonkey/mozjs-45.0.2/js/src/build-release/dist/sdk/lib, so it's missed by the script when putting it in libraries/spidermonkey/lib.
I'm not entirely sure why tbh.
I've made a very basic PR to fix it on your branch, simply copying the file from its proper place. TBH there's probably a better fix in the configure, but I tried one setting and then got annoyed by the recompilation times :p .

(not requesting changes so that people notice this needs more testing)

source/scriptinterface/ScriptInterface.cpp
114

Not a blocking concern: It sounds like we could just expose __stackTrace as an engine function cleanly in ScriptInterface instead of evaluating ing it here?

historic_bruno added inline comments.Jul 20 2019, 9:22 AM
libraries/source/spidermonkey/build.sh
108

Would be nice to replace these long lines with each flag on a separate line, as in rP22492 (notice the diff here isn't very clean, but compare to build-osx-libs.sh diff)

clean-workspaces.sh should be patched: P157

Clang build is very noisy, see P155. Most errors are [-Wundefined-var-template]. We could suppress this warning, but it's not available in the oldest compilers we support.

Here is an optional patch backported from upstream (50.0.1) that fixes those warnings: P156
Sources: https://hg.mozilla.org/mozilla-central/rev/6c37be9cee51 and https://hg.mozilla.org/mozilla-central/rev/4548ba932bde3067a722b267f9b1e43256740d4e

All tests passed or skipped in release build (some of them timeout in debug build)

Results: P158

ROSA applied some other patches to their lib64mozjs45-devel package for fixing arm/aarch64 support:

  • P159
  • P160
  • P161
elexis added inline comments.Aug 2 2019, 4:21 PM
source/gui/scripting/JSInterface_IGUIObject.cpp
47
Itms marked an inline comment as done.Aug 5 2019, 12:13 PM

I rebased my branch and added a commit addressing comments: https://github.com/na-Itms/0ad/tree/sm45.

If that looks good to you guys, I'll update the diff here for getting a final green light.

I've made a very basic PR to fix it on your branch, simply copying the file from its proper place. TBH there's probably a better fix in the configure

Nah let's not meddle with the configure file. If the files are there, let's get them from there. Included your PR.

clean-workspaces.sh should be patched: P157

Included.

Clang build is very noisy, see P155. Most errors are [-Wundefined-var-template]. We could suppress this warning, but it's not available in the oldest compilers we support.

SM build is quite noisy everywhere, so if you don't mind I'll leave those warnings and not patch the library for this.

ROSA applied some other patches to their lib64mozjs45-devel package for fixing arm/aarch64 support:

  • P159
  • P160
  • P161

Included those three as a single patch called FixNonx86.diff with short description in patch.sh.

libraries/source/spidermonkey/build.sh
108

Done!

source/scriptinterface/ScriptInterface.cpp
114

I'm not 100% sure what you propose, you want to add an Engine.stacktrace? If it's not used elsewhere I don't see the point? Or maybe create a new diff, independent from the SM upgrade, demonstrating how we could use that new engine function ?

elexis added inline comments.Aug 5 2019, 12:20 PM
source/scriptinterface/ScriptInterface.cpp
111

I wonder if one can't obtain the readable stack without an Evaluate

113

Rooting in a variable usually mean that one is in JS Request mode for that context.

elexis added inline comments.Aug 5 2019, 7:40 PM
libraries/source/spidermonkey/patch.sh
43

See todays IRC discussions, clang build failure, this was missing follow up commits from https://bugzilla.mozilla.org/show_bug.cgi?id=1316079#c3 (which is fixed on Itms branch now)

elexis edited the summary of this revision. (Show Details)Aug 6 2019, 6:21 PM
elexis edited the summary of this revision. (Show Details)Aug 6 2019, 6:33 PM
elexis edited the summary of this revision. (Show Details)Aug 6 2019, 7:46 PM
elexis requested changes to this revision.Aug 6 2019, 8:08 PM
  • gcc 9.0.1. sm45 build succeeds (with 91k lies build output for SM and FCollada)

gcc + sm45 + 0ad also succeeds:


One compiler warning (see inline comments) from this sm45 branch, the rest are reported ones.

  • clang 8.0.1 build now fails the same way it fails with SM38, so there is no regression introduced (the one clang regression was fixed by pulling follow ups to ExportJSPropertyDescriptor.diff).

F1033276

unit test binary succeeds.

Verification:

Various build changes

Hmm

removed the NSPR dependency on Unix

Ack
Sounds good if it was unused, it makes the osx build script much shorter and avoids useless library altogether.

Add js/Initialization.h to source/scriptinterface/ScriptEngine.h

Ack
Confirmed, the specs have it in their example:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_Init

Use nullptr instead of JS::NullPtr() https://bugzilla.mozilla.org/show_bug.cgi?id=1164602

Ack
I see it in the link that they removed it, and I also don't see anymore references to NullPtr in the SM code, so I agree it has to be removed from the 0ad codebase too.
In svn it was used for JS::HandleObject arguments, so it should be equal to using nullptr.

Remove JS::RuntimeOptionsRef.varObjFix https://bugzilla.mozilla.org/show_bug.cgi?id=1171177

Ack.
I witness they removed it, and they removed it without replacement, so it seems just to also remove it without replacement in svn.
I won't question the validity of the removal upstream now.

Remove uses of AutoIdArray https://bugzilla.mozilla.org/show_bug.cgi?id=1191529

Nack
Confirmed that all incidences are removed, I find only three comments in spidermonkey code where it wasnt removed upstream.
It was replaced in ScriptInterface::EnumeratePropertyNamesWithPrefix and CBinarySerializerScriptImpl::HandleScriptVal.
The former is called

  • in CComponentManager::Script_RegisterComponentType_Common to find all On event subscriptions
  • by XmppClient::SendIqRegisterGame and XmppClient::SendIqGameReport to create the Stanza from the respective JS Object

The latter is called for serialization of JS entity components in the simulation.
I see in https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/f8da9d2fc8dd that your replacement differs from theirs.
Reading https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS::Rooted again,
the second argument is the initial value, so passing context there is quite wrong, because the JSContext is not the JS::IdVector.
Not sure why it never surfaces, but I would object to that init unless the existence of some implicit conversion is demonstrated.
Otherwise should be correct, especially if its the same as the SM devs have.

JS_InternUCStringN has been renamed https://bugzilla.mozilla.org/show_bug.cgi?id=1178581

Ack.
I witnessed it, fine.

JS::Evaluate now takes scope chains explicitly Use the oppoertunity to improve the ErrorReporter https://bugzilla.mozilla.org/show_bug.cgi?id=1097987

Nack.
See inline comment, the code looks like it should be removed instead of upgraded and extended.

Array functions are fallible and output to params https://bugzilla.mozilla.org/show_bug.cgi?id=1179003

Ack
JS_IsArrayObject that is.
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_IsArrayObject

Remove workaround in our code https://bugzilla.mozilla.org/show_bug.cgi?id=1236373

Ack.
JSCLASS_CACHED_PROTO_WIDTH that is.
All (1) occurrences removed.

Remove compile'n go https://bugzilla.mozilla.org/show_bug.cgi?id=679939

Question below.

setCompileAndGo that is.
I see those two diffs removing those
https://hg.mozilla.org/integration/mozilla-inbound/rev/5336ae0fe919
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1a45bc093a
and I see that there are literally 0 occurrences of that function in the spidermonkey folder.
The question is whether it should be replaced with something or just deleted without replacement.
Looking at the course of https://bugzilla.mozilla.org/show_bug.cgi?id=679939 again,
there are a number of cleanup preparation commits, where compile flags were set.
For example there is a new setTreatAsRunOnce.
Should that be used for ScriptInterface::LoadScript (since that runs only once), but not for the reoccurringly processed GUI ScriptHandlers?

Mark shared memory in direct access operations https://bugzilla.mozilla.org/show_bug.cgi?id=1176214

Ack.
JS_GetUint16ArrayData and JS_GetUint8ArrayData that is.
It receives a boolean.
Search for JS_*ArrayData yielded 4 results outside of spidermonkey/.

JS::ObjectOpResult. I chose to follow the same convention as the SM developers, which is to return true when there is a failure. That changes our code behavior, but this is ES6-compliant as far as I understood.
https://bugzilla.mozilla.org/show_bug.cgi?id=1113369

Ack.
I don't know what .fail and .success return, so I don't know if there was a behavior change, but the semantics are used in accordance with the code (success only called when it is intended to succeed etc.)
(That area is subject to change / merge conflicts)

As wraitii pointed out, it's easy to miss references to mozjs-38, so I did a grep and confirmed that I didnt find any other remains than the one intentional line removing.

libraries/source/spidermonkey/patch.sh
48

Confirmed, all removed issues are resolved by SM45, all remaining ones are resolved at a later stage.

source/scriptinterface/ScriptConversions.h
57

same warning about parentheses (x && y) || z, despite operator precedence

source/scriptinterface/ScriptInterface.cpp
98
102

Evaluate is bad practice in general unless one really really, really wants it.
And here it is pointless and quite ugly.
All of this code seems like weird workarounds and hacks stacked on top of each other, and what this proposed code does is add documentation to it.
If the first sentence is // TODO: this violates the docs then it already tells you that it should be exchanged for something different.
The disabling of the JS exception state, just to be able to run some JS script when the specs say one shouldn't do that, just to perform some JS code that makes it just harder to obtain the value.
The complete hunk falls apart in a separate diff that just replaces this with JS_GetProperty and FromJSVal, unless I miss an important feature (I don't consider indentation to be one).

rP8063 added the exception printing, rP8975 added the violation TODO. I don't see any rights to the codes existence if the whole circus is done solely for one tab indentation.

I've created D2152 which should remove this hunk.

772

(same, both)

source/simulation2/serialization/BinarySerializer.cpp
103

This gives compile warnings on GCC about missing parentheses.

In file included from ../../../source/scriptinterface/ScriptConversions.cpp:20:
../../../source/scriptinterface/ScriptConversions.h: In instantiation of ‘bool FromJSVal_vector(JSContext*, JS::HandleValue, std::vector<_Tp>&) [with T = int; JS::HandleValue = JS::Handle<JS::Value>]’:
../../../source/scriptinterface/ScriptConversions.cpp:305:1:   required from here
../../../source/scriptinterface/ScriptConversions.h:57:44: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
   57 |  if (!(JS_IsArrayObject(cx, obj, &isArray) && isArray || JS_IsTypedArrayObject(obj)))
      |        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
305

This seems broken, the initial value should not be the cx but an JS::IdVector!

307

unneeded variable hasProps

This revision now requires changes to proceed.Aug 6 2019, 8:08 PM
Itms updated this revision to Diff 9263.Aug 7 2019, 5:04 PM
Itms marked an inline comment as done.

Updated RC following comments by historic_bruno, wraitii and elexis.

Vulcan added a comment.Aug 7 2019, 5:18 PM

Build failure - The Moirai have given mortals hearts that can endure.

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

elexis accepted this revision.EditedAug 7 2019, 8:59 PM
  1. Verification of the SpiderMonkey zip:
sha256sum mozjs-45.0.2.tar.bz2
570530b1e551bf4a459d7cae875f33f99d5ef0c29ccc7742a1b6f588e5eadbee  mozjs-45.0.2.tar.bz2

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Releases/45
Ack

  1. Verification of the applied and removed spidermonkey patches, see previous article.
  1. Testing: We did a multiplayer game against Petra AIs on Jebel Barkal (and they only won because they had one dock remaining while we didn't). No regression was observed. All GUI Object types and visible properties were tested prima facie.

It was tested on clang, gcc, VS2015. We will see how Jenkins will digest this.

It was tested on windows and linux, not so much on macOS, aside from wraitii and historic_bruno testing compilation.
It will remain as important to have SVN testgames, as we saw for a24 already and with the newly discovered svn OOS in that testmatch (#5552).

Support for other platforms should be kept on the radar too, for example D1592.

Thanks to everyone who participated in the SM 45 upgrade!

(Edit: I didn't check the windows headers and dlls)

This revision is now accepted and ready to land.Aug 7 2019, 8:59 PM
This revision was landed with ongoing or failed builds.Aug 8 2019, 12:37 AM
This revision was automatically updated to reflect the committed changes.