Open Bug 1422930 Opened 2 years ago Updated 3 months ago

[meta] Prepare a SpiderMonkey ESR68 standalone version

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

Tracking Status
firefox-esr68 --- affected
firefox59 --- affected

People

(Reporter: tcampbell, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: leave-open, meta)

Attachments

(1 file)

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.
Depends on: 1385450
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.
(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.
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.
Flags: needinfo?(philip.chimento)
Priority: -- → P3
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
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.
(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.
The next ESR will be 60 rather than 59.
Summary: SpiderMonkey ESR59 → SpiderMonkey ESR60
(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?
(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).
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
Flags: needinfo?(philip.chimento)
If you have an alpha of ESR60, I'll give it another try.
Steve, are you able to cut an "alpha" of ESR60 onto the ftp to look at?
Flags: needinfo?(sphink)
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
Flags: needinfo?(sphink)
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.
Attachment #8975487 - Flags: review?(mh+mozilla)
Assignee: nobody → philip.chimento
Status: NEW → ASSIGNED
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 on attachment 8975487 [details] [diff] [review]
Fix SpiderMonkey includedir installs

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

FWIW, bug 1427455 is what changed.
Attachment #8975487 - Flags: review?(mh+mozilla) → review+
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.
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77041935decb
Fix SpiderMonkey includedir installs. r=glandium
Keywords: checkin-needed
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.
Attachment #8975487 - Flags: approval-mozilla-esr60?
Duplicate of this bug: 1460276
Comment on attachment 8975487 [details] [diff] [review]
Fix SpiderMonkey includedir installs

spidermonkey makefile fix, approved for 60.1esr
Attachment #8975487 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
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.
Attachment #8975487 - Flags: approval-mozilla-esr60+ → checkin+
Depends on: 1465038
Depends on: 1464912
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?
Blocks: 1482179
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

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

Flags: needinfo?(sdetar)

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

Flags: needinfo?(sdetar) → needinfo?(philip.chimento)
Summary: SpiderMonkey ESR60 → Prepare a SpiderMonkey ESR60 standalone version

: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.
Flags: needinfo?(philip.chimento)

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.
Assignee: philip.chimento → nobody
Status: ASSIGNED → NEW
Keywords: meta
Summary: Prepare a SpiderMonkey ESR60 standalone version → [meta] Prepare a SpiderMonkey ESR60 standalone version
Type: defect → task

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

Flags: needinfo?(sdetar)

This is should still stay open

Flags: needinfo?(sdetar)

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

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

Summary: [meta] Prepare a SpiderMonkey ESR60 standalone version → [meta] Prepare a SpiderMonkey ESR68 standalone version
You need to log in before you can comment on or make changes to this bug.