Now that Bug 1401624 has landed; can I ask folks to look at this again and give some guidance on whether triggering this assertion is worrying and needs investigation?

Our previous representation of private values assumed that the private pointer was aligned, and did some bit twiddling to try to disguise it as a double. Since bug 1293313, it has been unnecessary to set the top bit for a double, so that bit twiddling is unnecessary. There are actual use cases where private values are unaligned, so we should fix this.

While cleaning this up, I also removed unboxPrivateValue, because its only use could be better written using loadPrivateValue directly.

Justification for the above patch: I've run myself around in circles a couple of times, but now I think I'm convinced that we were already implicitly relying on the top bit to be clear under the existing implementation, so it should not be a problem to make it explicit.

Specifically, because we set the sign bit in isDouble before doing the comparison, the value of the high bit is irrelevant when it comes to disguising a pointer as a double. All that matters is that the value of the pointer (after the high bit is set) must be less than JSVAL_SHIFTED_TAG_MAX_DOUBLE, which is 0xfff8_0000_ffff_ffff.

The bottom half of the standard actually-only-48-bits address space runs from 0x0 to 0x0000_7fff_ffff_ffff, which trivially satisfies the requirement. The top half of the address space (typically reserved for the kernel) runs from 0xffff_8000_0000_0000 to 0xffff_ffff_ffff_ffff. Shifting the low end of that range by 1 gives 0x7fff_c000_0000_0000, which (with the top bit set) is still greater than JSVAL_SHIFTED_TAG_MAX_DOUBLE.

So there's actually no need to do any shifting or bit-twiddling. Kernel pointers don't work either way. Any pointer that we could store under the old system can just be stored as a raw pointer, and everything will work out fine.

https://hg.mozilla.org/mozilla-central/rev/fbb42a9d132e

Is this backportable to ESR68 and ESR60 without also backporting the object-biased boxing patch? In bug 1566883 I described a use case which produces some fairly surprising behaviour for embedders due to this bug.

The patch would look different, because object-biased NaN-boxing did a lot of tidying, but this change can be made independently.

That said, I don't think uplifting the entire patch is realistic. The Value representation is a pretty fundamental part of the engine. What we might be able to do is uplift a minimal alternate API to ESR68: something along the lines of "getUnalignedPrivate/toUnalignedPrivate", using the new representation. There would be no in-tree uses of the API, but embedders could use it to store unaligned pointers.

The API would probably get deprecated in the next ESR release.

Would that be worthwhile for you?

To be worthwhile for my use case, I would need ArrayBuffer to use that unaligned private API: https://searchfox.org/mozilla-esr68/source/js/src/vm/ArrayBufferObject.cpp#983

Would that be a possibility?

I would like whatever is necessary to remove that assertion from esr68; for the MinGW build to work.

This assert should probably be a MOZ_RELEASE_ASSERT. The current 68 code has a hard requirement on the alignment (since it drops the low bit internally).

One option may be a compile-time #ifdef for 68 so that we can safely uplift and only things like mingw builds would opt in. Having release asserts in the key choke-points like NewArrayBuffer are probably worthwhile for all users and is a low risk change as well (since if the assert trips, the browser is about to experience far worse problems).

After talking it over with RyanVM, the plan is to uplift the core of the patch (minus some incidental cleanups) to ESR68, hidden behind an #ifdef. See bug 1568564.

Appreciated, thanks!