1422930 - [meta] Prepare a SpiderMonkey ESR68 standalone version

Status: NEW

(Core :: JavaScript Engine, task, P3)

Description

[Ted Campbell [:tcampbell]]

Now that we are in the 59 cycle, we should review any remaining obstacles to having an official SpiderMonkey ESR release for embedders.

A few things that come to mind:
 - Upload tarball to https://archive.mozilla.org/pub/spidermonkey/releases/
 - Make sure MDN docs have links and a brief changelog
 - Have a plan for ESR updates and point releases

We should also gather feedback from embedders here and track issues that block 59 embedding.

Comment 1

[Ted Campbell [:tcampbell]]

A few more things after some IRC discussions...

- MDN docs are really old and should at a minimum have warnings at the top of them
- Doing some cleanup in jsapi.h of docs and structure might be helpful to embedders who must turn to code for answers.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Ted Campbell [:tcampbell] from comment #0)
> A few things that come to mind:
>  - Upload tarball to https://archive.mozilla.org/pub/spidermonkey/releases/

See also Bug 1422937.

Comment 3

[Ted Campbell [:tcampbell]]

As a first step, I have selected a release from mozilla-central to be a first "alpha" of SM59.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&selectedJob=149799451

The packaged tarball for that job is: https://queue.taskcluster.net/v1/task/YhbEOycATOiblYHIKGAoUQ/runs/0/artifacts/public/build/mozjs-59.0a1.0.tar.bz2

Philip, give this a try and let us know any feedback or problems with it.

Comment 4

[Steve Fink [:sfink] [:s:]]

Just to make it easier to remember what's what, I've uploaded the above tarball to https://ftp.mozilla.org/pub/spidermonkey/prereleases/59/pre1/mozjs-59.0a1.0.tar.bz2

Comment 5

[Philip Chimento [:ptomato]]

I gave this a try. The good news is that I didn't have to port much of my codebase at all, apart from trivial things. The one exception was the disappearance of getProperty and setProperty from JSClass which will be tedious, but probably not too difficult, to rewrite to use resolve.

The bad news is still mozglue. My programs will compile but not link. It turns out libmozglue is built as a separate shared library which 'make install' does not install, so libmozjs refers to it but it can't be found.

I am pretty sure that mozglue is not supposed to be a shared library for embedded SpiderMonkey, but instead compiled into the main library. I am testing so far on macOS, and I believe that is why we get the shared library, so I made this change:

--- a/mozglue/build/moz.build
+++ b/mozglue/build/moz.build
@@ -4,9 +4,12 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-# Build mozglue as a shared lib on Windows, OSX and Android.
+# Build mozglue as a shared lib on Windows, OSX and Android. But not for
+# embedders!
 # If this is ever changed, update MOZ_SHARED_MOZGLUE in browser/installer/Makefile.in
-if CONFIG['OS_TARGET'] in ('WINNT', 'Darwin', 'Android'):
+if CONFIG['JS_STANDALONE']:
+    Library('mozglue')
+elif CONFIG['OS_TARGET'] in ('WINNT', 'Darwin', 'Android'):
     SharedLibrary('mozglue')
 else:
     Library('mozglue')

This should bring the sharedness of mozglue for embedded SpiderMonkey on any platform in line with what it would be on Linux x64.
However this makes SpiderMonkey not build.

libmozglue.a
rm -f libmozglue.a libmozglue.a.desc
make[3]: Nothing to be done for `target'.
/Users/ptomato/.cache/jhbuild/build/mozjs/_virtualenv/bin/python /Users/ptomato/jhbuild/source/mozjs/config/expandlibs_exec.py --extract -- ar crs libmozglue.a  ../../memory/build/libmemory.a ../../memory/mozalloc/libmemory_mozalloc.a ../../mozglue/misc/libmozglue_misc.a ../../mfbt/libmfbt.a
make[3]: Nothing to be done for `target'.
Executing: ar crs libmozglue.dylib ../../memory/build/Unified_cpp_memory_build0.o ../../memory/mozalloc/mozalloc_abort.o ../../memory/mozalloc/Unified_cpp_memory_mozalloc0.o ../misc/AutoProfilerLabel.o ../misc/ConditionVariable_posix.o ../misc/Mutex_posix.o ../misc/Printf.o ../misc/StackWalk.o ../misc/TimeStamp.o ../misc/TimeStamp_darwin.o ../../mfbt/lz4.o ../../mfbt/Compression.o ../../mfbt/Decimal.o ../../mfbt/Unified_cpp_mfbt0.o ../../mfbt/Unified_cpp_mfbt1.o
ar: libmozglue.dylib: Inappropriate file type or format
make[3]: *** [libmozglue.a] Error 1
make[2]: *** [mozglue/build/target] Error 2
make[1]: *** [compile] Error 2
make: *** [default] Error 2

Again I've only tested this on macOS so far, I won't be back to my Linux machine until after the winter break.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Philip Chimento [:ptomato] from comment #5)
> The one exception was the
> disappearance of getProperty and setProperty from JSClass which will be
> tedious, but probably not too difficult, to rewrite to use resolve.

It depends on your use case: resolve hooks work fine for some of them, proxies might be better for other objects. Bug 1389949 has a patch converting NPAPI NPObjWrapper from getProperty/setProperty Class hooks to a proxy object. Hope this helps.

Comment 7

[Julien Cristau [:jcristau]]

The next ESR will be 60 rather than 59.

Comment 8

[Philip Chimento [:ptomato]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Philip Chimento [:ptomato] from comment #5)
> > The one exception was the
> > disappearance of getProperty and setProperty from JSClass which will be
> > tedious, but probably not too difficult, to rewrite to use resolve.
> 
> It depends on your use case: resolve hooks work fine for some of them,
> proxies might be better for other objects. Bug 1389949 has a patch
> converting NPAPI NPObjWrapper from getProperty/setProperty Class hooks to a
> proxy object. Hope this helps.

Looking at my use case it seems that resolve would probably make sense, but out of curiosity do you have a criterion for deciding which approach to take?

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Philip Chimento [:ptomato] from comment #8)
> Looking at my use case it seems that resolve would probably make sense, but
> out of curiosity do you have a criterion for deciding which approach to take?

I'd use resolve hooks when there's a fixed number of lazy properties, properties you want to define when they're accessed. Proxies are useful when you want to intercept a large number of properties (say integer indexes) or when the values of these properties are dynamic (with a resolve hook you will have to define a getter/setter, then figure out which property was accessed, etc).

Comment 10

[Philip Chimento [:ptomato]]

To follow up on this, I've set up Travis CI on my personal mozjs GitHub repo [1] and pushed my work to the mozjs59 branch. I have it building, testing the JS shell, and linking a test embedder application with Clang, GCC 6, GCC 7 on Linux, and Clang on macOS [2]. This should make it easier to test future alphas of ESR60. I updated my patch that fixes building and linking on macOS and will attach it on bug 1176787.

I'll continue to port my code base and see what else comes up.

Note that bug 1430438 will be semi-required for me to move my code base to ESR60 and get rid of getProperty/setProperty hooks. I can do without it, but in that case I'll have performance regressions due to copying byte arrays where previously that wasn't necessary.

[1] https://github.com/ptomato/mozjs/tree/mozjs59
[2] https://travis-ci.org/ptomato/mozjs/builds/328959639

Comment 11

[Philip Chimento [:ptomato]]

If you have an alpha of ESR60, I'll give it another try.

Comment 12

[Ted Campbell [:tcampbell]]

Steve, are you able to cut an "alpha" of ESR60 onto the ftp to look at?

Comment 13

[Philip Chimento [:ptomato]]

I used a tarball marked 60.0.0 from Treeherder. Here's the branch in my personal mozjs repo: [1]

Relative to the 59a1 that I tested before, there's one regression: the header files are installed into the wrong directory. I will attach a patch to fix this, but since the code that I change in my patch wasn't changed recently, I'm not sure what caused the regression, and if it's necessary to fix this problem somewhere else than I did in the patch. I'm guessing Mike would be the most appropriate reviewer for this?

BTW, I wonder if the latest commit on [1] might be useful to you, to prevent these kinds of regressions - it uses Travis CI to compile, link, and run a minimal embedder program with the just-built SpiderMonkey library.

[1] https://github.com/ptomato/mozjs/tree/mozjs60

Comment 14

[Philip Chimento [:ptomato]]

Attachment: Fix SpiderMonkey includedir installs

Somehow the header files were being installed directly into
$prefix/include, rather than $prefix/include/mozjs-60. Something else
changed somewhere that affected this, since this code was the same in
older mozjs versions, but this seems the most logical place to fix it.

Comment 15

[Steve Fink [:sfink] [:s:]]

Hm, I bet the https://archive.mozilla.org/pub/spidermonkey/prereleases/60/pre1/mozjs-60.0.1.tar.bz2 that I uploaded at around the same time probably has the same problem, then.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8975487 [details] [diff] [review]
Fix SpiderMonkey includedir installs

Review of attachment 8975487 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, bug 1427455 is what changed.

Comment 17

[Philip Chimento [:ptomato]]

Thanks. I suppose I could add back that variable, but it would be easy to remove accidentally again, and this keeps the tarball-install-specific stuff in the same place.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77041935decb
Fix SpiderMonkey includedir installs. r=glandium

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/77041935decb

Comment 20

[Philip Chimento [:ptomato]]

Comment on attachment 8975487 [details] [diff] [review]
Fix SpiderMonkey includedir installs

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Generated SpiderMonkey tarballs don't work out of the box without this.
User impact if declined: Embedders can't use SpiderMonkey without patching.
Fix Landed on Version: 60
Risk to taking this patch (and alternatives if risky): Shouldn't affect Firefox, as this build code is only used when running "make install" on the SpiderMonkey tarball.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 8975487 [details] [diff] [review]
Fix SpiderMonkey includedir installs

spidermonkey makefile fix, approved for 60.1esr

Comment 23

[Steve Fink [:sfink] [:s:]]

Note that this is included in http://ftp.mozilla.org/pub/spidermonkey/prereleases/60/pre2/mozjs-60.0.2pre2.tar.bz2

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975487 [details] [diff] [review]
Fix SpiderMonkey includedir installs

https://hg.mozilla.org/releases/mozilla-esr60/rev/8e016d7b01bb

Clearing the esr60 approval flag so it doesn't show up on the needs-uplift radar anymore.

Comment 25

[Philip Chimento [:ptomato]]

Would it be possible to upload a stable release, for example https://queue.taskcluster.net/v1/task/WQHIXhTpRI-pE-2Snadfqw/runs/0/artifacts/public/build/mozjs-60.1.0.tar.bz2 (from job https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&selectedJob=183691323) to the FTP and call that an official release of standalone SpiderMonkey?

Other than that, here are some things that were mentioned earlier in this comment thread that might be considered conditions for closing this bug:

 - Have a plan for ESR updates and point releases
 - Put warnings at the top of MDN docs
 - Doing some cleanup in jsapi.h of docs and structure might be helpful to embedders who must turn to code for answers
 - Fail the standalone build if configured without --disable-jemalloc (bug 1465038)

How does that sound?

Comment 26

[Tim]

I have been testing this on Ubuntu, the 64-bit builds are working out ok, but 32-bit builds have test failures relating to double precision numbers see bug 1486328

Comment 27

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 28

[Steven DeTar [:sdetar]]

:ptomato, Should this bug still be left with the leave-open keyword?

Comment 29

[Philip Chimento [:ptomato]]

:sdetar - It really depends on whether Mozilla can devote some resources to this. I can't modify your release management process :-)

If this is not likely to be done from your side, then it may as well be closed.

Here are the items I suggested from earlier in the conversation, the first one is the most important here:

• [ ] Have a plan for ESR updates and point releases - For what it's worth it would be great if this plan was even just "manually upload a tarball at some well-defined interval to a public FTP such as https://ftp.mozilla.org/pub/spidermonkey"

The remaining items would be nice to have, but could maybe be moved to separate bugs:

• [ ] Put warnings at the top of MDN docs - this would be nice to have, as the SpiderMonkey API information on MDN is now quite outdated and misleading, but maybe we could open a separate bug for this
• [x] Doing some cleanup in jsapi.h of docs and structure might be helpful to embedders who must turn to code for answers - tcampbell has been doing this, improved the situation a lot
• [ ] Fail the standalone build if configured without --disable-jemalloc (bug 1465038) - sfink or someone else could weigh in on this bug, if it was likely to be accepted then I would write the patch.

Comment 30

[Ted Campbell [:tcampbell]]

I'll try and turn this into a meta-bug so the relman bots leave us alone. Thanks for the great summary Philip!

Additional data points:

• We are be migrated away from MDN (as it becomes a resource for the Web at-large instead of firefox-specific) and those docs should probably just deleted. I took at archive of the wikis already.

Comment 31

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 32

[Steven DeTar [:sdetar]]

This is should still stay open

Comment 33

[Ryan VanderMeulen [:RyanVM]]

What's the status of this bug now that ESR60 has reached EOL status?

Comment 34

[Philip Chimento [:ptomato]]

I think it can be renamed to "Prepare a SpiderMonkey ESR68 standalone version", here's what's remaining from the above checklist:

• [ ] Have a plan for ESR updates and point releases - For what it's worth it would be great if this plan was even just "manually upload a tarball at some well-defined interval to a public FTP such as https://ftp.mozilla.org/pub/spidermonkey"
• [ ] Either put warnings or delete the outdated MDN pages on the SpiderMonkey API: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey and subpages, especially https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference

Comment 35

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 36

[Steven DeTar [:sdetar]]

Ted, can you comment on whether we should close this bug or leave it open