port to spidermonkey 60

[jiangphcn]

Overview

Port SpiderMonkey from 1.8.5 to 6.0, i.e mozjs-60.1.1pre3 from https://ftp.mozilla.org/pub/spidermonkey/prereleases/60/pre3/ aligning with the version used in planned CouchDB 4.0. This allows CouchDB 3.0 to address existing issues in legacy spidermonkey version and use new features provided in new version of spidermonkey.

One outstanding topic is about CouchDB-style function declaration.
"map": "function(doc) {emit(doc._id, doc._id);}"

By means of the original transpile idea from the couch-chakra PoC from @dmunch, currently both traditional CouchDB-style function declaration and modern function declaration like below are supported in CouchDB 3.0 if spidermonkey 6.0 is selected.

"map": "(function (doc) {emit(doc._id, doc._id);});"

In order to provide enough flexibility, one switch is introduced when running configure. Currently, the supported value is 1.8.5 and 60. (the default value is 1.8.5 and will be changed to 60 later)

./configure  --spidermonkey-version 60 (non-windows version)
powershell -ExecutionPolicy Bypass .\configure.ps1 -WithCurl -SpiderMonkeyVersion=60 (windows version)

Testing recommendations

All tests below are involved and run successfully again.

 - Make test-cluster-with-quorum
 - Make test-cluster-without-quorum
 - Make eunit
 - Make javascript
 - Make mango-test
 - Make elixir

Related Issues or Pull Requests

#2170 is PR when starting to work on this task.
apache/couchdb-glazier#11

Checklist

• Code is written and works correctly
• Changes are covered by tests
• Any new configurable parameters are documented in rel/overlay/etc/default.ini
• A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

[wohali]

Hi @jiangphcn , this is a big PR and it's taking me a bit of time to work through.

While I'm doing that, I went and had a look for pre-built versions of SM60.

libmozjs-60 is available pre-built only on:

• Debian buster (10.x) and newer.
• Ubuntu disco (19.04) and newer - but no LTS releases past bionic (18.04LTS) exist yet.

@willholley so far I cannot find libmozjs60 precompiled for any version of RHEL/CentOS, is this true?

This probably means that for the rest of our binary build matrix, we'll have to start building libmozjs60 and distributing it ourselves, as we do today for the ancient 1.8.5 library. That means building it for:

• Debian stretch (9.x)
• Ubuntu xenial (16.04LTS)
• Ubuntu bionic (18.04LTS)
• CentOS 7
• CentOS 8
• Windows 64-bit

I'm intentionally excluding CentOS 6 from that list because it will go End-Of-Life on November 2020; people stuck on it can live with 1.8.5 a little longer, I think. 😉

The alternative is that we only ship our binaries with 60 in them only for Debian buster, and maybe Windows (tech preview), then add in the other platforms over time, as packages materialise (or if we decide to pick up the slack). Perhaps we know someone over at RH/CentOS who can get a mozjs60 package released for CentOS 8 at the very least...

The good news is that the Docker container can probably be moved from stretch to buster fairly easily (there was a single bug outstanding against ppc64le, which may actually be a qemu bug) so our Docker users can enjoy the new support with this release.

What do you think?

[wohali]

@tilgovi In the past you've helped with checking out things like sandbox quality / escape mechanisms on JavaScript. Would you be able to review if things have changed with this SM60 PR?

[jiangphcn]

Hi @wohali, thanks indeed for taking time to review this PR.

Regarding pre-built version of SM60, again, thanks for thoughtful consideration for its distribution. Yes, there are limited pre-built versions for libmozjs-60. For the rest of them you mentioned, we or the users need to built them and then use spidermonkey 60 based codes.

For now, I am able to build for Debian stretch and Windows 64-bit. Because we have switch to use between 1.8.5 and 6.0 and still want to set 1.8.5 as default, I think that we can move forward step by step. As you suggested, we can ship some library (such as Debian stretch, buster, Ubuntu disco, Windows 64 bit), and ship them on request later.

Also, we can publish step I come up with to help user to build by themselves for the platforms they need.

Thanks,
Peng Hui

[wohali]

This is upstream source code licensed under BSD 2-clause. The same goes for the esprima.js in this same directory.

Keeping track of changes to these dependencies over time worries me; we'll need to think more about how to do this. Fauxton uses npm to pull the latest, but so far, we've managed to avoid that for core CouchDB itself.

At a minimum, we need to disclose this in the top-level NOTICE file. It would be best to link there to the original source locations for these.

@janl any advice as our resident JS ecosystem expert?

[davisp]

FWIW, the esprima.js that's included here is actually patched from the upstream version (because it implements the spec precisely which we don't want). So we'll always have a slight fork there (two lines if memory serves).

This escodegen.js bit is annoying. I spent some time awhile back trying to figure out how to get a version that's not minified so that tracking changes would be much easier. However I couldn't figure out the JS build system for getting a single file version of it. Any help from resident JS experts there would be quite helpful.

[wohali]

That's going to be difficult to maintain. It would be best if, before we merge this, we have at least a README that contains the diffs we apply to esprima.js and a method for applying them, if not a Makefile target (possibly supplemented by npm/webpack/etc tooling that can pull the latest code, apply patches, and build a minified version.

He's probably not yet caught up on email but @janl might be best, or maybe @garrensmith knows someone in the Fauxton team who could help?

[jiangphcn]

thanks @wohali and @davisp. I discussed with Paul, and tried to use 88f8b06 to add necessary information for reference of esprima.js and escodegen.js. Just let me know whether this is fine or anything else to follow?

[wohali]

@jiangphcn Sounds like we agree on this.

I will review the code more closely tomorrow; I want to understand how sandboxing is affected by the move to SM60, and I want to get some more hands-on time with the code.

[jiangphcn]

Sure, thanks @wohali

[wohali]

@jiangphcn on Debian Buster, I did the following:

sudo apt install libmozjs-60-0 libmozjs-60-dev
git clone https://github.com/apache/couchdb
cd couchdb
git checkout sm60-davisp
./configure --spidermonkey-version 60 -c
make couch

This results in the following warnings at build time:

Compiling priv/couch_js/60/http.cpp
Compiling priv/couch_js/60/main.cpp
In file included from /usr/include/time.h:29,
                 from /usr/include/pthread.h:24,
                 from /usr/include/x86_64-linux-gnu/c++/8/bits/gthr-default.h:35,
                 from /usr/include/x86_64-linux-gnu/c++/8/bits/gthr.h:148,
                 from /usr/include/c++/8/ext/atomicity.h:35,
                 from /usr/include/c++/8/bits/basic_string.h:39,
                 from /usr/include/c++/8/string:52,
                 from /usr/include/c++/8/stdexcept:39,
                 from /usr/include/c++/8/array:39,
                 from /usr/include/mozjs-60/mozilla/Span.h:32,
                 from /usr/include/mozjs-60/mozilla/Range.h:12,
                 from /usr/include/mozjs-60/jsapi.h:15,
                 from priv/couch_js/60/main.cpp:23:
/usr/include/mozjs-60/js/Proxy.h: In static member function 'static size_t js::BaseProxyHandler::offsetOfFamily()':
/usr/include/mozjs-60/js/Proxy.h:212:25: warning: offsetof within non-standard-layout type 'js::BaseProxyHandler' is conditionally-supported [-Winvalid-offsetof]
         return offsetof(BaseProxyHandler, mFamily);
                         ^
priv/couch_js/60/main.cpp: In function 'bool req_ctor(JSContext*, unsigned int, JS::Value*)':
priv/couch_js/60/main.cpp:90:32: warning: too many arguments for format [-Wformat-extra-args]
         JS_ReportErrorUTF8(cx, "Failed to create CouchHTTP instance.\n", NULL);
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Compiling priv/couch_js/60/utf8.cpp
In file included from /usr/include/strings.h:23,
                 from /usr/include/string.h:431,
                 from /usr/include/c++/8/cstring:42,
                 from /usr/include/mozjs-60/mozilla/Span.h:33,
                 from /usr/include/mozjs-60/mozilla/Range.h:12,
                 from /usr/include/mozjs-60/jsapi.h:15,
                 from priv/couch_js/60/utf8.cpp:13:
/usr/include/mozjs-60/js/Proxy.h: In static member function 'static size_t js::BaseProxyHandler::offsetOfFamily()':
/usr/include/mozjs-60/js/Proxy.h:212:25: warning: offsetof within non-standard-layout type 'js::BaseProxyHandler' is conditionally-supported [-Winvalid-offsetof]
         return offsetof(BaseProxyHandler, mFamily);
                         ^

Do you see these as well?

I do see that make javascript passes, which is good. Next step for me is to read the code a bit more closely. @davisp if you're able to describe the changes you're making to the upstream JS libraries that'd be most useful.

I have asked @tilgovi to look at the sandboxing for this code and SM60. He says he hopes to have time this weekend for a code review.

[jiangphcn]

Thanks Joan @wohali. I only saw two of them before.

In file included from priv/couch_js/main.cpp:26:
In file included from /usr/local/include/mozjs-60/js/Wrapper.h:12:
/usr/local/include/mozjs-60/js/Proxy.h:212:16: warning: offset of on non-standard-layout type 'js::BaseProxyHandler' [-Winvalid-offsetof]
        return offsetof(BaseProxyHandler, mFamily);

In file included from priv/couch_js/utf8.cpp:16:
In file included from /usr/local/include/mozjs-60/js/Wrapper.h:12:
/usr/local/include/mozjs-60/js/Proxy.h:212:16: warning: offset of on non-standard-layout type 'js::BaseProxyHandler' [-Winvalid-offsetof]
        return offsetof(BaseProxyHandler, mFamily);
                       ^

Above looks to be related to spidermonkey so I leave them for now.

However, I addressed the second one in your list using f6830c9.

Also, thank you for involving more people on this PR.

[davisp]

Overall this looks good. I had a few comments and questions inline.

One thing I'm not seeing is where you moved the 1.8.5 JS files. I may be missing them in the diff but it looks like maybe they're missing from this PR? Also do we actually need two versions of those? If memory serves the changes were minimal and were just slightly different syntactically. Can we not just tweak the few places to run on both 1.8.5 and 60 both?

[davisp]

This is wrong. You're setting the filename and line number of the javascript code to "main.cpp", 258. With 1.8.5 we would set this to an optional second paramter and hard code the start line to 1 (which is appropriate when you're doing the equivalent of evaling a whole file.

[jiangphcn]

ah, as you mentioned, there is different parameters for eval in spidermonkey 6.0. I replaced it with "<unknown>", 1

[davisp]

Same comment as above about setting file/line numbers.

[davisp]

Here the __FILE__ should be args->scripts[i] which is easier. __LINE__ should still be 1.

[davisp]

Why are we removing these cleanup bits in tests?

[jiangphcn]

We used 15986 here. As you suggested, I changed to _node instead.

[davisp]

Why was this removed? If this test passes then we should be asserting that we have the expected error here.

[jiangphcn]

there is different error generated for spidermonkey 6.0. I added both for that now 9d1cb18#diff-4aba4f69c2f30c9686f1c16ea474a9ddR157

[davisp]

I'll compile and run make javascript on both 1.8.5 and 60 and report back.

[jiangphcn]

thanks @davisp. The javascript layout for spidermonkey 1.8.5 and 60 can be seen from https://github.com/apache/couchdb/pull/2345/files#diff-a28dc96b01dfdd66c40acdcc4704ea16R44. In general, there are 3 files to be changed in 1.8.5 for 60. They are not missing. I will try to see whether we can unify them today. Of course, esprima.js and escodegen.js are only for spidermonkey 60.

[jiangphcn]

hey Paul, @davisp I think that I addressed all existing comments from your side. Would you please take another look at them when getting time? The left is esprima.js and escodegen.js.

[wohali]

Very minor changes; I will approve (since I won't be back to look at this again until after 12/25) but please review my comments.

[wohali]

Looking good.

We still need to place a copy of these upstream dependencies untouched into an Apache repository, as @janl said on IRC. If we made modifications, we should track those in a branch in that repo as well. If we can't solve the build problem now (how to generate these files from those repos), please open a new ticket and mention Jan and Garren in it so we can look at fixing that for the future.

I want to be sure that if these libraries change in the future, we can easily absorb the fixes and improvements into our code, especially if they're security-related patches.

@davisp can help you with the repo importation into Apache, he's done it recently for part of the 4.0 libraries.

[jiangphcn]

Thanks for your suggestions, @wohali I created #2372 for now.

[davisp]

@wohali @jiangphcn Yap, will do that in a few minute after I finish testing this PR.

[wohali]

Thank you for doing both configure and configure.ps1.

[wohali]

Ideally we should manually test -S <size> and --eval as I know multiple users who need these options immediately. I don't think we a test case set up for either of these, unfortunately, because changing launch options isn't easy. Sorry that I ran out of time for this.

[jiangphcn]

thanks for your tip. I will test them manually at least.

[wohali]

These changes look right. If we've broken a platform we haven't tested (like freebsd) we can fix it before we release.

[jiangphcn]

Yes, currently, I tested against

• xenial for linux
• MacOS
• Windows 10

We can react if there is more issue before release.

[wohali]

Is this a difference between 1.8.5 and 60? If so, is there any better error message we can emit? unnamed_error is a bit worrying, but if we really don't know, there's not much we can do.

[jiangphcn]

Yes, they are different. For 1.8.5, we have os_process_error while it is unnamed_error in 6.0. Took some time to get better error message, but don't get bottom yet. So stayed with this for now.

[wohali]

Thanks again @jiangphcn .

[tilgovi]

Apologies, but I've not gotten to this yet. I plan to do so this week.

[jiangphcn]

Thanks again @wohali and @tilgovi. I will take care of these comments. And enjoy your coming holidays.

[tilgovi]

I'm trying to reproduce such a parse tree with the esprima online parser, and I just get errors for things like this:

function(doc) {}

Here's a link: https://esprima.org/demo/parse.html?code=function(doc)%20%7B%7D

Does this actually parse both forms we want it to?

[jiangphcn]

It looks that the esprima online parser doesn't like function(doc) {}. Using esprima in this PR, we can have output. I tried both (function (doc) {emit(doc._id, doc._id);}); and function (doc) {emit(doc._id, doc._id);}. The result are listed below respectively.

(function (doc) {emit(doc._id, doc._id);});

{"type":"Program","body":[{"type":"ExpressionStatement","expression":{"type":"FunctionExpression","id":null,"params":[{"type":"Identifier","name":"doc"}],"body":{"type":"BlockStatement","body":[{"type":"ExpressionStatement","expression":{"type":"CallExpression","callee":{"type":"Identifier","name":"emit"},"arguments":[{"type":"MemberExpression","computed":false,"object":{"type":"Identifier","name":"doc"},"property":{"type":"Identifier","name":"_id"}},{"type":"MemberExpression","computed":false,"object":{"type":"Identifier","name":"doc"},"property":{"type":"Identifier","name":"_id"}}]}}]},"generator":false,"expression":false,"async":false}}],"sourceType":"script"}

function (doc) {emit(doc._id, doc._id);}

{"type":"Program","body":[{"type":"FunctionDeclaration","id":null,"params":[{"type":"Identifier","name":"doc"}],"body":{"type":"BlockStatement","body":[{"type":"ExpressionStatement","expression":{"type":"CallExpression","callee":{"type":"Identifier","name":"emit"},"arguments":[{"type":"MemberExpression","computed":false,"object":{"type":"Identifier","name":"doc"},"property":{"type":"Identifier","name":"_id"}},{"type":"MemberExpression","computed":false,"object":{"type":"Identifier","name":"doc"},"property":{"type":"Identifier","name":"_id"}}]}}]},"generator":false,"expression":false,"async":false}],"sourceType":"script"}

One is ExpressionStatement while another is FunctionDeclaration

[davisp]

@tilgovi The esprima used by this PR is patched to avoid that error so that we can rewrite it. Basically we have to violate the spec as existed in old SpiderMonkey so we can tweak the AST so that it conforms. One of the to-do items we have is to extract that patch so we can apply it to a standard esprima.js to show the diff better.

If memory serves it's something like two lines to remove a boolean check for that "anonymous function at global scope" error.

[tilgovi]

Right, that's the context I was forgetting. Thank you, both!

[tilgovi]

I'll pick up more review tomorrow. This is looking good!

[davisp]

I've tested make javascript on both ./configure --dev and ./configure --dev --spidermonkey-version=1.8.5 and that passes. Having moved on to using --spidermonkey-version=60 I'm getting these errors consistently:

Error: chttpd_view_test:chttpd_view_test:75 should_succeed_on_view_with_queries_keys

::in function lists:nth/2 (lists.erl, line 170)
  called as nth(1,undefined)
in call from chttpd_view_test:'-should_succeed_on_view_with_queries_keys/1-fun-2-'/1 (test/eunit/chttpd_view_test.erl, line 85)
**error:function_clause

Error: chttpd_view_test:chttpd_view_test:91 should_succeed_on_view_with_queries_limit_skip

::in function chttpd_view_test:'-should_succeed_on_view_with_queries_limit_skip/1-fun-1-'/1 (test/eunit/chttpd_view_test.erl, line 98)
in call from chttpd_view_test:'-should_succeed_on_view_with_queries_limit_skip/1-fun-4-'/1 (test/eunit/chttpd_view_test.erl, line 98)
**error:{assertEqual,[{module,chttpd_view_test},
              {line,98},
              {expression,"RC"},
              {expected,200},
              {value,404}]}

Error: chttpd_view_test:chttpd_view_test:108 should_succeed_on_view_with_multiple_queries

::in function chttpd_view_test:'-should_succeed_on_view_with_multiple_queries/1-fun-1-'/1 (test/eunit/chttpd_view_test.erl, line 116)
in call from chttpd_view_test:'-should_succeed_on_view_with_multiple_queries/1-fun-5-'/1 (test/eunit/chttpd_view_test.erl, line 116)
**error:{assertEqual,[{module,chttpd_view_test},
              {line,116},
              {expression,"RC"},
              {expected,200},
              {value,404}]}

Error: chttpd_security_tests:fixture setup [39,1,1,14]

::in function chttpd_security_tests:create_user/4 (test/eunit/chttpd_security_tests.erl, line 91)
in call from chttpd_security_tests:setup/0 (test/eunit/chttpd_security_tests.erl, line 51)
**error:{badmatch,{ok,500,
              [{"Cache-Control","must-revalidate"},
               {"Content-Length","5555"},
               {"Content-Type","application/json"},
               {"Date","Wed, 18 Dec 2019 16:07:37 GMT"},
               {"Server","CouchDB/3.0.0-61486a5e4 (Erlang OTP/20)"},
               {"X-Couch-Request-ID","7707c95a33"},
               {"X-CouchDB-Body-Time","0"}],
              <<"{\"error\":\"compilation_error\",\"reason\":\"(new SyntaxError(\\\"function statement requires a name\\\", \\\"<unknown>\\\", 2)) (\\n    function(newDoc, oldDoc, userCtx, secObj) {\\n        if (newDoc._deleted === true) {\\n            // allow deletes by admins and matching users\\n            // without checking the other fields\\n            if ((userCtx.roles.indexOf('_admin') !== -1"...>>}}

I haven't looked at them in any depth. That last one is obviously us missing a call to rewriteFun somewhere though.

[davisp]

Also we should rebase this against master which includes those fixes for JavaScript tests.

[davisp]

I've created the requested mirrors:

https://github.com/apache/couchdb-esprima
https://github.com/apache/couchdb-escodegen

The esprima repo in particular is nearly 60MiB so we'll want to figure out some sort of script to avoid having to download that for every build. Not sure how all the JS tooling works for that sort of thing.

[davisp]

Finally managed to get the esprima.js thing "compiled" and importable into SpiderMonkey. In the couchdb-esprima repo you can do this (assuming you have TypeScript and webpack installed, both available in Homebrew):

tsc -p src/
webpack --mode none

This gives a human readable dist/esprima.js

The patch that CouchDB needs to make this work is:

--- esprima.js	2019-12-18 12:59:30.000000000 -0600
+++ esprima.couchdb.js	2019-12-18 13:05:04.000000000 -0600
@@ -7,7 +7,7 @@
 		exports["esprima"] = factory();
 	else
 		root["esprima"] = factory();
-})(window, function() {
+})(this, function() {
 return /******/ (function(modules) { // webpackBootstrap
 /******/ 	// The module cache
 /******/ 	var installedModules = {};
@@ -3468,7 +3468,9 @@
                     statement = this.parseLexicalDeclaration({ inFor: false });
                     break;
                 case 'function':
-                    statement = this.parseFunctionDeclaration();
+                    // Apache CouchDB modification: add true to tolerate
+                    // missing function identifiers.
+                    statement = this.parseFunctionDeclaration(true);
                     break;
                 case 'class':
                     statement = this.parseClassDeclaration();

[davisp]

I can't figure out how to get a standalone escodegen.js as it has commonjs stuff and the npm run release script fails. If any JS folks want to take a look at that I'd be much obliged.

[jiangphcn]

@davisp Hey Paul, regarding #2345 (comment), it is due to our refactoring of bffa5e6 this week. Removing this line will resolve issue. I also rebased from master along with this change.

[davisp]

+1 Awesome work @jiangphcn!

This passes all of the test suite including the sandboxing tests. Given that its not the default yet and everything passes I think its fine to go ahead and merge.

[jiangphcn]

Thanks @davisp @wohali @janl @tilgovi @garrensmith and all who helped make this happen.

[janl]

great job everybody!

[jiangphcn]

thanks @janl