Since the fix for bug 1553938 where we include js-confdefs.h (in order to get #defines that affect the layout of public structures) we also bring in a bunch of other #defines exported by Autoconf such as HAVE_UNISTD_H. These can conflict with #defines that embedders will have in their own config.h if they use Autoconf. (At least, they should be #defined to the same value on the same system, but the compiler will complain about them being redefined)

Is there any possibility of filtering the #defines exported in js-confdefs.h?

This is hard to fix and keep it fixed. Possibilities:

• This issue is something autoconf has coped with in the past, and there's a standard way to do it (wishful thinking, maybe)

• Take this whole header private and stop having public structs that depend on #defines (lots of work, so pretty unlikely)

• Rename these macros to have a JS_ prefix (problematic: we did this once with DEBUG so now we have both DEBUG and JS_DEBUG; also note that SM shares DEBUG with Firefox, I guess on purpose.)

A low priority for us. We'd take a patch if there's a solid plan.

Unfortunately, the standard way that Autoconf copes with it is their guideline to "never install config.h", but that ship has sailed :-/

This is a fairly high priority for me because it means that embedders basically can't use Autoconf.

I think maybe the best solution, given the constraints you mentioned, is to add HAVE_UNISTD_H (and possibly some other Autoconf-defined ones pre-emptively) to _NON_GLOBAL_ACDEFINES here (https://searchfox.org/mozilla-central/source/build/autoconf/config.status.m4#152) which filters them out of js-confdefs.h.

However, a much more sustainable solution would be to include only defines in js-confdefs.h that do affect the layout of public structs, instead of including them all and removing them one by one as they cause problems.

Ugh, messy.

It's tempting to add a build step that does a grep -v HAVE_. The only other one that looked bad at a glance was STDC_HEADERS. But there are probably others. And the HAVE_ thing isn't perfect, though the only one I see offhand is HAVE_64BIT_BUILD which will be used to determine the type of JitExceptionHandler on Windows.

Would it work to change the resolution of bug 1553938, and say that anything that affects the structure layout must show up in js-config.h instead?

Then we'd want the SM(pkg) job to fail if we mess it up.

Yes, that would also work. I'm not sure how we'd get the SM(pkg) job to fail though, since I'm not sure how it would be possible to catch the original mismatch that prompted bug 1553938 in an automated way.

In ESR68, HAVE_64BIT_BUILD is used in jsfriendapi.h and mfbt/HashTable.h, and HAVE_STRNDUP is used in mfbt/mozalloc.h.

I like sfink's suggestion best, but without some automation to catch regressions, we'd just end up in the same situation as bug 1553938 again, so I'm probably more inclined to go the _NON_GLOBAL_ACDEFINES route.

Unless ... these defines would be included used by other parts of Firefox? I'm hoping that's not the case.

Sadly, the defines seem to be used, so we'll have to revert bug 1553938 and go with the other solution.

I've done a bit more research on which defines appear where in the esr68 branch. I've grepped all the symbols that are used in an #if or #ifdef in mfbt/*.h, js/public/*.h, or js/src/*.h, skipping ones that are defined by the compiler or defined in another one of these header files themselves. That's not to say that all these symbols affect the layout of structs, but they do affect the contents of header files that embedders (who after the revert wouldn't be including js-confdefs.h anymore) would see.

These symbols affect header files, are defined by configure in js-confdefs.h, and also appear in js-config.h, so are accessible to embedders:

• JS_DEBUG
• JS_GC_SMALL_CHUNK_SIZE
• JS_GC_ZEAL
• JS_HAS_CTYPES
• JS_NUNBOX32
• JS_PUNBOX64

These symbols affect header files, are defined by configure in js-confdefs.h, but don't appear in js-config.h, so embedders don't have them:

• ENABLE_TYPED_OBJECTS
• ENABLE_WASM_CRANELIFT
• ENABLE_WASM_GC
• EXPOSE_INTL_API
• FUZZING
• HAVE_64BIT_BUILD
• HAVE_STRNDUP
• HAVE_THREAD_TLS_KEYWORD
• HAVE_VISIBILITY_ATTRIBUTE
• JS_64BIT
• JS_BUILD_BINAST
• JS_CODEGEN_X64
• JS_CODEGEN_X86
• JS_JITSPEW
• JS_OOM_BREAKPOINT
• JS_STANDALONE
• JS_TRACE_LOGGING
• JS_UNALIGNED_PRIVATE_VALUES
• MOZ_ASAN
• MOZ_CLANG_PLUGIN
• MOZ_DEBUG
• MOZ_DEV_EDITION
• MOZ_HAS_MOZGLUE
• MOZ_UBSAN
• MOZ_VALGRIND
• MOZILLA_INTERNAL_API
• MOZILLA_OFFICIAL
• NIGHTLY_BUILD
• STATIC_JS_API
• XP_MACOSX
• XP_UNIX
• XP_WIN

These symbols affect header files, but don't seem to be defined anywhere, so the checks could likely be removed:

• ENABLE_SHARED_ARRAY_BUFFER
• JS_OLD_GETTER_SETTER_METHODS
• JS_OOM_DO_BACKTRACES
• JS_USE_CUSTOM_ALLOCATOR - possibly this is supposed to be defined by the embedder? Does supplying a custom allocator even work?
• LZ4_DLL_EXPORT - this is probably vestigial, from the vendored lz4.h header, should probably be left alone to avoid diverging from lz4
• LZ4_PUBLISH_STATIC_FUNCTIONS - ditto
• MOZ_HAVE_BITSCAN64 - this is only checked in order to #undef it if it's defined, maybe it can be removed?
• XGILL_PLUGIN - I assume this is defined when compiling with the plugin?

Additionally, there are a few symbols that appear in js-config.h but don't affect anything, maybe they can be removed?

• JS_NO_JSVAL_JSID_STRUCT_TYPES
• JS_THREADSAFE

My plan is to ensure that all the symbols in the second group above are defined in js-config.h. That's a lot of symbols and they're not namespaced very well, for example HAVE_STRNDUP will still conflict with embedders if they use AC_CHECK_FUNCS([strndup]) in their build files, so maybe I should start with only the ones that actually affect struct layouts.

sfink, your opinion appreciated!

This macro is not defined anywhere and has no effect in the end whether
it's defined or not.

This macro is never defined, and the functions that it hides are missing
function definitions anyway.

Depends on D51767

This macro isn't defined anywhere and doesn't seem to do anything. It
affects the oom-backtraces property of the build configuration object in
the testing functions, but since the macro is never defined, it seems to
be always set to false anyway, so just hardcode it.

Depends on D51768

This macro seems to have been introduced in
https://bugzilla.mozilla.org/attachment.cgi?id=772469&action=diff
but there doesn't seem to be any mention of it anywhere else. Currently
it's only checked in order to #undef it if it's defined.

Depends on D51769

This macro is defined when building the Rust bindings, but it doesn't
actually affect anything.

Depends on D51770

This macro is never defined and has no effect anywhere, JS_THREADSAFE
builds seem to have been removed long ago.

Depends on D51771

Attached a first wave of patches to remove macros that seem to be obsolete.

Here's what I've found out in the meantime about what the defines in the second group affect:

These affect struct layout in public headers:

• ENABLE_WASM_CRANELIFT (ContextOptions)
• ENABLE_WASM_GC (ContextOptions)
• FUZZING (ContextOptions)
• JS_64BIT -> (shadow::String via JS_BITS_PER_WORD)
• JS_OOM_BREAKPOINT (AutoEnterOOMUnsafeRegion)

These don't affect struct layout, but do affect which function prototypes are visible:

• ENABLE_TYPED_OBJECTS
• EXPOSE_INTL_API (renamed to ENABLE_INTL_API in mozilla-central)
• JS_BUILD_BINAST
• JS_JITSPEW
• MOZ_DEBUG
• MOZ_HAS_MOZGLUE/MOZILLA_INTERNAL_API (via MOZ_HAS_JSRUST)
• NIGHTLY_BUILD

These affect something else important that's publicly visible:

• HAVE_64BIT_BUILD (affects static assert in mfbt/HashTable.h)
• HAVE_THREAD_TLS_KEYWORD (affects how thread local storage is implemented)
• HAVE_VISIBILITY_ATTRIBUTE (affects definition of MOZ_EXPORT and JS_PUBLIC_API)
• JS_CODEGEN_X64/JS_CODEGEN_X86 (affects JS_SWEPT_CODE_PATTERN)
• JS_STANDALONE (affects definition of MFBT_API and MFBT_DATA)
• JS_TRACE_LOGGING (if not defined then some APIs are inline no-ops so probably will conflict if the library was built with it defined)
• JS_UNALIGNED_PRIVATE_VALUES (esr68 only; affects behaviour of Value)

These are probably OK to ignore and don't have to go in js-config.h:

• MOZ_ASAN/MOZ_UBSAN/MOZ_VALGRIND (although maybe embedders should define them when running asan/ubsan/valgrind on their own code?)
• MOZ_CLANG_PLUGIN
• MOZ_DEV_EDITION
• MOZILLA_OFFICIAL (only affects whether MOZ_DBG is available)
• STATIC_JS_API (affects definition of JS_PUBLIC_API etc but embedders probably shouldn't use this)

Not sure about XP_MACOSX/XP_UNIX/XP_WIN, they seem to be definitely needed for stuff, but also... they seem to get defined somehow anyway?

In order to not depend on the preprocessor macros that JS was built with
being defined in public headers, use sizeof(void*) in this static assert
to determine whether pointers are 8 bytes or not.

Depends on D51772

We should have the same public API available whenever possible, and make
it a no-op or make it throw immediately if JS was built without support
for it, instead of showing or hiding the API in header files using
configure macros. Otherwise embedders can easily get mismatches between
a library with functionality and header files without it, or vice versa.

Depends on D52123

If only relying on JS_64BIT to determine whether the system is 64-bits,
then the result will be incorrect when the header is installed as a
public header for use by embedders, and since JS_BITS_PER_WORD affects
the layout of structs in header files, things will go badly wrong.

This uses two other ways of determining pointer width, hopefully
cross-platform enough. SIZEOF_POINTER is a GCC-ism and probably
works in Clang as well. UINTPTR_MAX is hopefully sufficiently
cross-platform as a last resort.

Depends on D52124

These are configure macros that start with JS_ and have an effect on the
public API declared in JSAPI header files, so they should be included in
the installed js-config.h header file.

Depends on D52458

Previously, if SpiderMonkey embedders linked to a copy of libmozjs built
with --enable-cranelift, --enable-wasm-gc, or --enable-fuzzing, then the
size of the ContextOptions data structure declared in the header file
would be different than the size of ContextOptions in the library,
likely leading to crashes. This makes all members of ContextOptions
independent of preprocessor macros. Any options not compiled into
SpiderMonkey will still be no-ops.

Depends on D52459

Whether ENABLE_INTL_API and ENABLE_TYPED_OBJECTS are defined, affects
the behaviour of JS_FOR_PROTOTYPES for the prototypes of Intl and
TypedObject. Therefore, these macros have to be available to embedders.
Rename them to JS_HAS_INTL_API and JS_HAS_TYPED_OBJECTS (in line with
the existing JS_HAS_CTYPES) everywhere they are used, and add them to
js-config.h.in.

Depends on D52460

It turns out that we don't actually want to install js-confdefs.h
because it contains macro definitions that can conflict when embedders
include JSAPI headers in their Autotools projects.

Depends on D52461

This should specifically prevent bug 1553938 from happening in the
future. Unfortunately it won't prevent other similar bugs from
happening.

Depends on D52462

OK, I think this is the best we can do for now. Any thoughts on how to programmatically enforce

The only patches out of this series that would really need to be uplifted to esr68 are:

• "Determine 64-bit in public JS header without configure macro", to ensure JS_BITS_PER_WORD is defined;
• "Include JS_FOO macros that influence public API in js-config.h", taking care to include JS_UNALIGNED_PRIVATE_VALUES and JS_CODEGEN_FOO in there even though those aren't needed on central;
• "Revert bug 1553938".

It's debatable whether to uplift "Make ENABLE_INTL_API and ENABLE_TYPED_OBJECTS into js-config macros", they do break the behaviour of JS_FOR_PROTOTYPES for embedders, but I'd say for the amount of churn that patch introduces (and the amount of tedium to backport it since ENABLE_INTL_API is EXPOSE_INTL_API on esr68) that breakage is probably minor enough and can be worked around.

https://hg.mozilla.org/mozilla-central/rev/fee367f20825
https://hg.mozilla.org/mozilla-central/rev/cee3ca35e416
https://hg.mozilla.org/mozilla-central/rev/bedd96446c98
https://hg.mozilla.org/mozilla-central/rev/b98e3a2058c0
https://hg.mozilla.org/mozilla-central/rev/41cd25e887f0
https://hg.mozilla.org/mozilla-central/rev/35fb1e29016b

That was only half the patches, reopening.

Comment on attachment 9107685 [details]
Bug 1590907 - Determine 64-bit in public JS header without configure macro. r?jwalden

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: These patches remove preprocessor dependence from public SpiderMonkey header files. This is intended fix the recurring problem of embedders using installed SpiderMonkey header files and not having the same preprocessor macros defined.

I'm requesting uplift for as few patches as possible, operating under the assumption that uplifts should only fix serious bugs on ESR68, but below are potential reasons why some others of the remaining patches might be considered for uplift:

• "Remove obsolete macro" patches - these don't fix anything, they are harmless cleanups of unused code.
• "Stop configure macros from masking function prototypes in public JS headers" - this is fairly invasive, and embedders wouldn't have been able to use these APIs before anyway because they wouldn't have been visible in the public headers, so I think it's OK to leave it be.
• "Remove preprocessor dependence from size of ContextOptions" - this still leaves a potential bug for embedders on ESR68, but it seems unlikely that embedders are using SpiderMonkey builds with the affected options.
• "Make ENABLE_INTL_API and ENABLE_TYPED_OBJECTS into js-config macros" - this also still leaves a potential bug for embedders on ESR68, but it only affects the behaviour of JS_FOR_EACH_PROTOTYPE which is quite minor, and since the ENABLE_INTL_API option is called EXPOSE_INTL_API on mozilla-esr68 it would be quite tedious to backport, so I think it's minor enough to leave it be.
• "Add safeguard when JS_BITS_PER_WORD affects struct layout" - this doesn't fix anything, it just adds an extra compile-time safeguard in case someone forgets to include js-config.h.

• User impact if declined: Embedders would need to build SpiderMonkey with these or similar patches and the patches would need to be carried by downstream Linux distributions.
• Fix Landed on Version: 72
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): If these patches were to cause any regressions, they'd most likely be obvious compile-time errors, or far less likely obvious crashes such as the one in bug 1553938.
• String or UUID changes made by this patch: None

Failed to land, please rebase:

On Tue, November 26, 2019, 2:37 AM GMT+2, by ccoroiu@mozilla.com.
Revisions: D52124 diff 197869 ← D52458 diff 197870 ← D52459 diff 197871 ← D52460 diff 197872 ← D52461 diff 197873 ← D52462 diff 197874 ← D52463 diff 197875
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpXW9Y8d js/src/vm/OffThreadScriptCompilation.cpp Hunk #1 FAILED at 203. 1 out of 1 hunk FAILED -- saving rejects to file js/src/vm/OffThreadScriptCompilation.cpp.rej js/src/jsapi.h Hunk #4 FAILED at 1943. 1 out of 4 hunks FAILED -- saving rejects to file js/src/jsapi.h.rej js/src/jsapi.cpp Hunk #3 FAILED at 3660. Hunk #4 FAILED at 4584. 2 out of 4 hunks FAILED -- saving rejects to file js/src/jsapi.cpp.rej js/public/OffThreadScriptCompilation.h Hunk #1 FAILED at 131. 1 out of 1 hunk FAILED -- saving rejects to file js/public/OffThreadScriptCompilation.h.rej abort: patch command failed: exited with status 256

Rebased now, thanks!

Backed out 7 changesets (Bug 1590907) for build bustages (hazard) at jsfriendapi.cpp.

https://hg.mozilla.org/integration/autoland/rev/98fd7a2227453a52213c0345605958f29a2a7f7b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=d6ac9325cb2cbeca7460b967d080d17fc85cb9b6&selectedJob=278150981

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=278150981&repo=autoland&lineNumber=4171

Apparently some APIs changed between the time I rebased it and the time it landed... I've rebased all the patches again and fixed the breakage. Here's a try run with the three jobs that failed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb99bca14aed596d918454e92b54ee0db351a723

https://hg.mozilla.org/mozilla-central/rev/2f5288e7e77e
https://hg.mozilla.org/mozilla-central/rev/1fcd2dd27766
https://hg.mozilla.org/mozilla-central/rev/2e8022ea6b75
https://hg.mozilla.org/mozilla-central/rev/965e57bb64a3
https://hg.mozilla.org/mozilla-central/rev/bfdd5f34f2f4
https://hg.mozilla.org/mozilla-central/rev/06611450ed60
https://hg.mozilla.org/mozilla-central/rev/454ac96ed988

Here's a backport of one patch to ESR68, which needs to include an extra option.

The other patches requested for backport I think should apply cleanly.

Comment on attachment 9107685 [details]
Bug 1590907 - Determine 64-bit in public JS header without configure macro. r?jwalden

Various SpiderMonkey build fixes to make life better for embedders. No affect on Mozilla-produced builds. Approved for 68.4esr.

https://hg.mozilla.org/releases/mozilla-esr68/rev/3d4060cdea68
https://hg.mozilla.org/releases/mozilla-esr68/rev/11af3f9251b5
https://hg.mozilla.org/releases/mozilla-esr68/rev/085c965a7830
https://hg.mozilla.org/releases/mozilla-esr68/rev/f79bfc535dd9

Does this bug still need to be open?

No need to be open, I think it's finally resolved! thanks all