Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for SpiderMonkeys 102, 115 and 128 #5314

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

janl
Copy link
Member

@janl janl commented Oct 19, 2024

This PR combines work from #4305 (@nickva) #5146 (@big-r81) & and #5306 (@indutny) plus some more original work from @indutny to swap out our usage of our homegrown seal() function for Object.freeze().

Compiles and make check passes on macOS with SpiderMonkey 91, 102, 115 and 128.

It’d be great if we can test this on other systems as well and double check that Object.freeze() has the security properties be want (but I think it does, it might even be stricter than before, not allowing reassignment of values to existing properties).

The underlying issue is that starting with SpiderMonkey 102 one of our original seal() calls would seal the State.funs array, which then makes a subsequent State.funs.push() fail with the type error reported in #4305 and #5146. (h/t to @indutny for teasing this out).

This should unblock support for Ubuntu 24.04, which ships with SpiderMonkey 128.

It feels like the build args lists in src/couch/rebar.config.script are due for a refactor, but I didn’t want to mess too much with the shape of the project just yet.

Similarly, the differences in couch_js/main.cpp and couch_js/util.cpp from 91 to 128 are not a lot and one could argue we should start switching to ifdef the relevant bits, rather than duplicating a lot of code. On the other hand, this will make it easier to drop versions later, if we ever want to.

Thanks all for doing the hard work here, I’m just pulling it all together to validate this is (hopefully) a way forward ✌️

@big-r81
Copy link
Contributor

big-r81 commented Oct 19, 2024

Hey,

that looks really great!
I think we could merge 102 and 115 together like we did in 86 & 91.

In v115 I explicitly removed the global_ops var to not deal with the record members. This should be handled by the class DefaultGlobalClassOps … But need to deep dive into it again ...

@janl
Copy link
Member Author

janl commented Oct 20, 2024

that looks really great! I think we could merge 102 and 115 together like we did in 86 & 91.

yeah, that’s what I meant. My question is “should we?”.

@big-r81
Copy link
Contributor

big-r81 commented Oct 20, 2024

btw, we should test this in a branch starting with jenkins-... to test full CI runs , so we could se problems with SM 78 ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants