Bugzilla

Assignee

Comment 1

•

6 years ago

I would guess that bug 1479900 broke big-endian. We must be overlaying a relocation field on top of the NON_ATOM bit. Or something.

Depends on: 1479900

Flags: needinfo?(sphink)

Assignee

Comment 2

•

6 years ago

Oops, didn't mean to remove the needinfo.

Flags: needinfo?(sphink)

Comment 3

•

6 years ago

This is talking about ESR60 which got a simpler version of Bug 1479900. I'm not sure why that would fail endianess.

Comment 4

•

6 years ago

It also affects ppc64. I get assert at debug build/Firefox 62:

#0  0x00007fffee091c84 in JSString::isAtom() const (this=0x7fffd4000e00)
    at /home/stransky/rpmbuild/BUILD/firefox-62.0/js/src/vm/StringType.h:491
#1  0x00007fffef2fccb4 in JSString::nurseryCellIsString(js::gc::Cell*) (cell=0x7fffd4000e00)
    at /home/stransky/rpmbuild/BUILD/firefox-62.0/js/src/vm/StringType.h:510
#2  0x00007fffef355884 in js::gc::AssertGCThingHasType(js::gc::Cell*, JS::TraceKind) (cell=0x7fffd4000e00, kind=JS::TraceKind::Object) at /home/stransky/rpmbuild/BUILD/firefox-62.0/js/src/gc/GC.cpp:8443
#3  0x00007fffe6542dc0 in JS::GCCellPtr::checkedCast(void*, JS::TraceKind) (p=0x7fffd4000e00, traceKind=JS::TraceKind::Object) at /home/stransky/rpmbuild/BUILD/firefox-62.0/objdir/dist/include/js/HeapAPI.h:330
#4  0x00007fffe656ca80 in JS::GCCellPtr::GCCellPtr<JSObject>(JSObject*) (this=0x7fffffff5f20, p=0x7fffd4000e00)
    at /home/stransky/rpmbuild/BUILD/firefox-62.0/objdir/dist/include/js/HeapAPI.h:267
#5  0x00007fffeea0f13c in JS::ExposeObjectToActiveJS(JSObject*) (obj=0x7fffd4000e00)
    at /home/stransky/rpmbuild/BUILD/firefox-62.0/objdir/dist/include/js/HeapAPI.h:662
#6  0x00007fffeea1a998 in JS_WrapObject(JSContext*, JS::MutableHandle<JSObject*>) (cx=0x100479f30, objp=...)
    at /home/stransky/rpmbuild/BUILD/firefox-62.0/js/src/jsapi.cpp:777
#7  0x00007fffe7ad00d8 in mozJSComponentLoader::Import(JSContext*, nsTSubstring<char> const&, JS::MutableHandle<JSObject*>, JS::MutableHandle<JSObject*>, bool) (this=0x1000e5e00, aCx=0x100479f30, aLocation=..., aModuleGlobal=..., aModuleExports=..., aIgnoreExports=false) at /home/stransky/rpmbuild/BUILD/firefox-62.0/js/xpconnect/loader/mozJSComponentLoader.cpp:1391

where isAtom() is perhaps wrongly interpreted so object is returned as string.

(gdb) p /x d.u1.flags
$22 = 0x7fff

Assignee

Comment 5

•

6 years ago

Thanks for that, it points directly at the problem, which appears to be very simple.

JSObject starts with a js::GCPtrObjectGroup, which is pretty much just ObjectGroup*, or a regular pointer.

JSString is confusing, but starts with

            struct {
                uint32_t           flags;               /* JSString */
                uint32_t           length;              /* JSString */
            };

and thus the flags alias the high 32 bits of the ObjectGroup pointer on big-endian (the low 32 bits on little-endian.)

So I think this just needs conditional compilation to swap those fields. Except that as of bug 1439885, which was explicitly to fix big-endian machines, RelocationOverlay is now

  struct {
      uint32_t preserve_; // used to distinguish objects from strings, using the NON_ATOM flags bit
      uint32_t magic_; // set to Relocated (0xbad0bad1)
      ...
  }

so basically I messed up in bug 1439885. My logic in comment 5 appears to be correct, except I missed one case: `flags` now aliases the high 32 bits of group_ on big-endian, which is not good. Ugh.

Flags: needinfo?(sphink)

Assignee

Comment 6

•

6 years ago

Ok, so we have

JSString.flags, JSString.length
group_.hi32, group_.lo32
RelocationOverlay.preserve_, RelocationOverlay.magic_

JSString.flags needs its lowest bit to be preserved when in a nursery cell, for use as JSString::NON_ATOM to distinguish objects from string.

RelocationOverlay.magic_ can alias JSString.length because the magic value is an invalid length.

RelocationOverlay.magic_ must not alias JSString.flags because it does not preserve the low bit.

RelocationOverlay.magic_ can alias group_.hi32 because the magic value has its high bit set and is thus an invalid pointer.

RelocationOverlay.magic_ can alias group_.lo32 because the magic value has its low bit set and is thus an unaligned pointer.

(anything can alias RelocationOverlay.preserve_.)

JSString.flags must not alias group_.hi32 because an object's group_ pointer's 32nd bit is not fixed (this bug).

JSString.flags can alias group_.lo32 because an object's group_ pointer's low bit is fixed to zero.

JSString.length can alias group_.hi32 because neither is needed to distinguish nursery objects from strings.

JSString.length can alias group_.lo32 because neither is needed to distinguish nursery objects from strings.

Ok, so with all those rules, it looks like on 64-bit big-endian we have to do:

RelocationOverlay.magic_ aliases JSString.length aliases group_.hi32

RelocationOverlay.preserve_ aliases JSString.flags aliases group_.lo32

Do we have 32-bit big-endian? I'll assume not until someone tells me otherwise. Though it seems like it'll probably just work no matter what we do, since JSObject has two aligned pointer fields.

Now tcampbell can tell me where I'm going horribly wrong this time.

Assignee

Comment 7

•

6 years ago

Attached patch JSString flags bit must alias the low 32 bits of JSObject.group_, not the high 32 (obsolete) — Details — Splinter Review

Attachment #9007853 - Flags: review?(tcampbell)

Assignee

Updated

•

6 years ago

Assignee: nobody → sphink

Status: UNCONFIRMED → ASSIGNED

Ever confirmed: true

Comment 8

•

6 years ago

Comment on attachment 9007853 [details] [diff] [review]
JSString flags bit must alias the low 32 bits of JSObject.group_, not the high 32

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

::: js/src/gc/RelocationOverlay.h
@@ +42,5 @@
>      uint32_t preserve_;
>  
>      /* Set to Relocated when moved. */
>      uint32_t magic_;
> +#else

#else if JS_BITS_PER_WORD == 64

Attachment #9007853 - Flags: review?(tcampbell) → review+

Comment 9

•

6 years ago

Or use the little-endian case for 32-bit big endian.

Assignee

Comment 10

•

6 years ago

Attached patch JSString flags bit must alias the low 32 bits of JSObject.group_, not the high 32 (obsolete) — Details — Splinter Review

Updated with review comments, re-uploading so I can request backport.

Assignee

Updated

•

6 years ago

Attachment #9007853 - Attachment is obsolete: true

Assignee

Comment 11

•

6 years ago

Comment on attachment 9007875 [details] [diff] [review]
JSString flags bit must alias the low 32 bits of JSObject.group_, not the high 32

Inheriting r=tcampbell

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This is necessary to avoid crashes (usually/always startup crashes?) on big-endian 64-bit. It does not affect any of our shipping platforms, which are all little-endian.

> User impact if declined: 

Big-endian crashes.

> Fix Landed on Version:

None. This is only needed for esr60. For other versions, the version of bug 1479900 that has landed fixed all of this in a more comprehensive way. This patch is an esr60-only fix.

> Risk to taking this patch (and alternatives if risky): 

Very little. If I messed up, we'll know quickly.

> String or UUID changes made by this patch: 

None.

Attachment #9007875 - Flags: review+

Attachment #9007875 - Flags: approval-mozilla-esr60?

Reporter

Comment 12

•

6 years ago

Attached file Complete stack trace from FF60.2ESR on s390x — Details

Reporter

Comment 13

•

6 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #11)
> Comment on attachment 9007875 [details] [diff] [review]
> JSString flags bit must alias the low 32 bits of JSObject.group_, not the
> high 32
> 

Eager for a fix I applied your patch, rebuilt FF60esr for s390x, ran and it still crashes in the same place mentioned in the description. I also built with optimization off and so have a more complete stack trace (Attached).

Also, here is my build configuration if this helps narrow it down:

Using GCC5

Compiler flags:
CFLAGS='-fmessage-length=0 -grecord-gcc-switches -fstack-protector  -Wall -D_FORTIFY_SOURCE=2 -funwind-tables -fasynchronous-unwind-tables  -g -fno-strict-aliasing'
CXXFLAGS='-fmessage-length=0 -grecord-gcc-switches -fstack-protector  -Wall -D_FORTIFY_SOURCE=2 -funwind-tables -fasynchronous-unwind-tables  -g -fno-strict-aliasing'

Configure options from mozconfig:
--enable-application=browser
--prefix=/usr
--libdir=/usr/lib64
--includedir=/usr/include
--enable-release
--enable-default-toolkit=cairo-gtk3
--enable-pie
--disable-optimize
--with-system-nspr
--with-system-nss
--with-l10n-base=/home/abuild/rpmbuild/BUILD/l10n
--with-system-zlib
--disable-updater
--disable-tests
--enable-alsa
--disable-debug
--enable-startup-notification
--enable-update-channel=esr
--with-mozilla-api-keyfile=/home/abuild/rpmbuild/SOURCES/mozilla-api-key
--with-google-api-keyfile=/home/abuild/rpmbuild/SOURCES/google-api-key
--enable-official-branding
--enable-libproxy
--disable-crashreporter
--disable-webrtc
--disable-jemalloc
--disable-updater

Assignee

Comment 14

•

6 years ago

Huh.

Any chance you could break on Marking.cpp:2919, and give the output of both `p *(JSString*)*edge` and `p *(JSObject*)*edge`?

Assignee

Comment 15

•

6 years ago

Comment on attachment 9007875 [details] [diff] [review]
JSString flags bit must alias the low 32 bits of JSObject.group_, not the high 32

Cancelling approval request until I come up with something that works.

Attachment #9007875 - Flags: approval-mozilla-esr60?

Comment 16

•

6 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #14)
> Huh.
> 
> Any chance you could break on Marking.cpp:2919, and give the output of both
> `p *(JSString*)*edge` and `p *(JSObject*)*edge`?

I'm testing that on ppc64/ESR60.2/debug build with your patch. js::gc::StoreBuffer::CellPtrEdge::trace() is not hit, it fails early as in comment 4.

Reporter

Comment 17

•

6 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #14)
> Huh.
> 
> Any chance you could break on Marking.cpp:2919, and give the output of both
> `p *(JSString*)*edge` and `p *(JSObject*)*edge`?

Thread 1 "firefox" hit Breakpoint 1, js::gc::StoreBuffer::CellPtrEdge::trace (this=0x2aa001c0088, mover=...) at /usr/src/debug/mozilla/js/src/gc/Marking.cpp:2918
2918	    if (JSString::nurseryCellIsString(*edge))
(gdb) p *(JSString*)*edge
$1 = {<js::gc::Cell> = {<No data fields>}, static NUM_INLINE_CHARS_LATIN1 = 16, static NUM_INLINE_CHARS_TWO_BYTE = 8, d = {u1 = {{flags = 1023, length = 2768715248}, flattenData = 4396520259056}, {{
        inlineStorageLatin1 = "\000\000\003\377\245\a\217\070\000\000\003\377\247\225\213\b", inlineStorageTwoByte = u"\000\x3ff\xa507\x8f38\000\x3ff\xa795\x8b08"}, s = {u2 = {
          nonInlineCharsLatin1 = 0x3ffa5078f38 "", nonInlineCharsTwoByte = 0x3ffa5078f38 u"", left = 0x3ffa5078f38}, u3 = {base = 0x3ffa7958b08, right = 0x3ffa7958b08, capacity = 4396563139336, 
          externalFinalizer = 0x3ffa7958b08}}}}, static NON_ATOM_BIT = 1, static LINEAR_BIT = 2, static HAS_BASE_BIT = 4, static INLINE_CHARS_BIT = 8, static DEPENDENT_FLAGS = 7, static UNDEPENDED_FLAGS = 23, 
  static EXTENSIBLE_FLAGS = 19, static EXTERNAL_FLAGS = 35, static FAT_INLINE_MASK = 24, static PERMANENT_ATOM_MASK = 33, static PERMANENT_ATOM_FLAGS = 32, static INIT_THIN_INLINE_FLAGS = 11, 
  static INIT_FAT_INLINE_FLAGS = 27, static INIT_ROPE_FLAGS = 1, static INIT_FLAT_FLAGS = 3, static TYPE_FLAGS_MASK = 63, static LATIN1_CHARS_BIT = 64, static INDEX_VALUE_BIT = 128, 
  static INDEX_VALUE_SHIFT = 16, static MAX_LENGTH = 268435455, static MAX_LATIN1_CHAR = 255 '\377', static TraceKind = JS::TraceKind::String}
(gdb) p *(JSObject*)*edge
$2 = {<js::gc::Cell> = {<No data fields>}, group_ = {<js::WriteBarrieredBase<js::ObjectGroup*>> = {<js::BarrieredBase<js::ObjectGroup*>> = {
        value = 0x3ffa5073df0}, <js::WrappedPtrOperations<js::ObjectGroup*, js::WriteBarrieredBase<js::ObjectGroup*> >> = {<No data fields>}, <No data fields>}, <No data fields>}, 
  shapeOrExpando_ = 0x3ffa5078f38, static TraceKind = JS::TraceKind::Object, static MaxTagBits = 3, static ITER_CLASS_NFIXED_SLOTS = 1, static MAX_BYTE_SIZE = 160}
(gdb)

Reporter

Comment 18

•

6 years ago

(In reply to Martin Stránský [:stransky] from comment #16)
> (In reply to Steve Fink [:sfink] [:s:] from comment #14)
> > Huh.
> > 
> > Any chance you could break on Marking.cpp:2919, and give the output of both
> > `p *(JSString*)*edge` and `p *(JSObject*)*edge`?
> 
> I'm testing that on ppc64/ESR60.2/debug build with your patch.
> js::gc::StoreBuffer::CellPtrEdge::trace() is not hit, it fails early as in
> comment 4.

Are you running with --enable-debug and hitting a MOZ_ASSERT macro? I discovered that MOZ_ASSERT in debug evaluates to a forced crash (if false) and execution halts. With --disable-debug MOZ_ASSERT evaluates to a no-op (if true or false) and processing continues (do {} while(false)). See mfbt/Assertions.h 

When I was running with --enable-debug I was hitting a MOZ_ASSERT and not getting to the actual problem in the code. Can you try running with --disable-debug and see what happens?

Assignee

Comment 19

•

6 years ago

(In reply to Charles Robertson from comment #17)
> (gdb) p *(JSString*)*edge
> $1 = {... d = {u1 = {{flags = 1023, length = 2768715248}, flattenData = 4396520259056}

Ok, the change is not getting applied. With my patch, you should be seeing length then flags. So MOZ_LITTLE_ENDIAN is defined??!

I guess do

    cd $objdir/js/src
    make StringType.i
    # cut and paste the command it ran for that and rerun it, adding -dD to the end
    egrep 'MOZ_LITTLE|JS_BITS_|JS_64' *.i

Also, I really should have a direct #include of mozilla/EndianUtils.h if I'm going to use MOZ_LITTLE_ENDIAN, so I'll update the patch to add it. (But it's included indirectly, so it shouldn't matter?)

Assignee

Comment 20

•

6 years ago

Attached patch JSString flags bit must alias the low 32 bits of JSObject.group_, not the high 32 (obsolete) — Details — Splinter Review

Assignee

Updated

•

6 years ago

Attachment #9007875 - Attachment is obsolete: true

Reporter

Comment 21

•

6 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #19)
> Ok, the change is not getting applied. With my patch, you should be seeing
> length then flags. So MOZ_LITTLE_ENDIAN is defined??!
> 
> I guess do
> 
>     cd $objdir/js/src
>     make StringType.i
>     # cut and paste the command it ran for that and rerun it, adding -dD to
> the end
>     egrep 'MOZ_LITTLE|JS_BITS_|JS_64' *.i

OK. Ran the make command as you said, cutting and pasting the command it ran and adding -dD to the end:

$ /usr/bin/g++-5 -std=gnu++14 -C -E -o Unified_cpp_js_src36.i -I/home/abuild/rpmbuild/BUILD/obj/dist/system_wrappers -include /home/abuild/rpmbuild/BUILD/mozilla/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DENABLE_WASM_GLOBAL -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DJS_HAS_CTYPES '-DDLL_PREFIX="lib"' '-DDLL_SUFFIX=".so"' -DMOZ_HAS_MOZGLUE -I/home/abuild/rpmbuild/BUILD/mozilla/js/src -I/home/abuild/rpmbuild/BUILD/obj/js/src -I/home/abuild/rpmbuild/BUILD/obj/dist/include -I/usr/include/nspr4 -fPIC -DMOZILLA_CLIENT -include /home/abuild/rpmbuild/BUILD/obj/js/src/js-confdefs.h -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -fno-sized-deallocation -fmessage-length=0 -grecord-gcc-switches -fstack-protector -Wall -D_FORTIFY_SOURCE=2 -funwind-tables -fasynchronous-unwind-tables -g -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -fno-omit-frame-pointer -Wno-shadow -Werror=format -fno-strict-aliasing     /home/abuild/rpmbuild/BUILD/obj/js/src/Unified_cpp_js_src36.cpp -dD
In file included from /home/abuild/rpmbuild/BUILD/obj/dist/system_wrappers/features.h:3:0,
                 from /usr/include/c++/5/s390x-suse-linux/bits/os_defines.h:39,
                 from /usr/include/c++/5/s390x-suse-linux/bits/c++config.h:2255,
                 from /usr/include/c++/5/cstddef:44,
                 from /home/abuild/rpmbuild/BUILD/obj/dist/system_wrappers/cstddef:3,
                 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/Compiler.h:49,
                 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/Attributes.h:12,
                 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/Assertions.h:16,
                 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/Array.h:12,
                 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/PodOperations.h:18,
                 from /home/abuild/rpmbuild/BUILD/mozilla/js/src/vm/StringType.h:11,
                 from /home/abuild/rpmbuild/BUILD/mozilla/js/src/vm/StringType-inl.h:10,
                 from /home/abuild/rpmbuild/BUILD/mozilla/js/src/vm/StringType.cpp:7,
                 from /home/abuild/rpmbuild/BUILD/obj/js/src/Unified_cpp_js_src36.cpp:2:
/usr/include/features.h:341:4: warning: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Wcpp]
 #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
    ^
$ egrep 'MOZ_LITTLE|JS_BITS_|JS_64' *.i
#define JS_64BIT 1
#define JS_BITS_PER_BYTE 8
#define JS_BITS_PER_BYTE_LOG2 3
#define JS_BITS_PER_WORD 64
#define MOZ_LITTLE_ENDIAN 0
$

Mike Hommey [:glandium]

Comment 22

•

6 years ago

StringType.h is using #elif defined(MOZ_LITTLE_ENDIAN) instead of #elif MOZ_LITTLE_ENDIAN

Assignee

Comment 23

•

6 years ago

Attached patch JSString flags bit must alias the low 32 bits of JSObject.group_, not the high 32 (obsolete) — Details — Splinter Review

Ugh. Any reason why it works that way? It seems atypical.

Anyway, can you give this patch a try?

Assignee

Updated

•

6 years ago

Attachment #9008209 - Attachment is obsolete: true

Mike Hommey [:glandium]

Comment 24

•

6 years ago

It's not /that/ atypical. That said, you're not fixing it in your patch.

Assignee

Comment 25

•

6 years ago

(In reply to Mike Hommey [:glandium] from comment #24)
> It's not /that/ atypical. That said, you're not fixing it in your patch.

You mean doing #undef instead of defining it to 0? I didn't have reason to think that build peer people would prefer one over the other.

Or maybe you were pointing out that I didn't fix the #elif defined(MOZ_LITTLE_ENDIAN) in mozilla-central? I just noticed that. I'll bet that's what you meant. Ugh. I've only been looking at esr60 in this bug so far. I'll fix it.

Assignee

Comment 26

•

6 years ago

Attached patch MOZ_LITTLE_ENDIAN is 0/1, not undef/1 (mozilla-central patch) — Details — Splinter Review

Fix the main branch.

Attachment #9008260 - Flags: review?(tcampbell)

Comment 27

•

6 years ago

Comment on attachment 9008260 [details] [diff] [review]
MOZ_LITTLE_ENDIAN is 0/1, not undef/1 (mozilla-central patch)

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

I think this needs to go to beta as well

Attachment #9008260 - Flags: review?(tcampbell) → review+

Comment 28

•

6 years ago

Comment on attachment 9008245 [details] [diff] [review]
JSString flags bit must alias the low 32 bits of JSObject.group_, not the high 32

I tested this patch on ppc64 and it fails to build with aserts:

StringType.h:
static_assert(offsetof(JSString, d.u1.length) == offsetof(String, length),
                      "shadow::String length offset must match JSString");
static_assert(offsetof(JSString, d.u1.flags) == offsetof(String, flags),
                      "shadow::String flags offset must match JSString");

Marking-inl.h:
static_assert(offsetof(RelocationOverlay, magic_) == offsetof(JSObject, group_) + sizeof(uint32_t),
                  "RelocationOverlay::magic_ is in the wrong location");
static_assert(offsetof(RelocationOverlay, magic_) == offsetof(js::Shape, base_) + sizeof(uint32_t),
                  "RelocationOverlay::magic_ is in the wrong location");

Reporter

Comment 29

•

6 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #23)
> Created attachment 9008245 [details] [diff] [review]
> JSString flags bit must alias the low 32 bits of JSObject.group_, not the
> high 32
> 
> Ugh. Any reason why it works that way? It seems atypical.
> 
> Anyway, can you give this patch a try?

I gave your new patch a try with no success.

Reporter

Comment 30

•

6 years ago

I'm getting the same static_assert() errors as Martin (comment 28) with attachment 9008245 [details] [diff] [review] test:

/home/abuild/rpmbuild/BUILD/mozilla/js/src/vm/StringType.h: In static member function 'static void JSString::staticAsserts()':
/home/abuild/rpmbuild/BUILD/mozilla/js/src/vm/StringType.h:332:9: error: static assertion failed: shadow::String length offset must match JSString
         static_assert(offsetof(JSString, d.u1.length) == offsetof(String, length),
         ^
/home/abuild/rpmbuild/BUILD/mozilla/js/src/vm/StringType.h:334:9: error: static assertion failed: shadow::String flags offset must match JSString
         static_assert(offsetof(JSString, d.u1.flags) == offsetof(String, flags),

...
/home/abuild/rpmbuild/BUILD/mozilla/js/src/gc/Marking-inl.h: In member function 'void js::gc::RelocationOverlay::forwardTo(js::gc::Cell*)':
/home/abuild/rpmbuild/BUILD/mozilla/js/src/gc/Marking-inl.h:95:5: error: static assertion failed: RelocationOverlay::magic_ is in the wrong location
         static_assert(offsetof(RelocationOverlay, magic_) == offsetof(JSObject, group_) + sizeof(uint32_t),
         ^
 /home/abuild/rpmbuild/BUILD/mozilla/js/src/gc/Marking-inl.h:97:5: error: static assertion failed: RelocationOverlay::magic_ is in the wrong location
         static_assert(offsetof(RelocationOverlay, magic_) == offsetof(js::Shape, base_) + sizeof(uint32_t),

Reporter

Comment 31

•

6 years ago

I removed the patch (attachment 9008245 [details] [diff] [review]) above so I could debug into where it is crashing on my s390x machine. Here are the results of that debug session. If indeed this is a big-endian alignment issue then I hope someone can point out where the miss-alignment is occurring. The patch only swaps the order of the uint32_t flags; and uint32_t length; fields of the Data struct of JSString class. I cannot see how this is related to the Zone object returned by JSString.zone().

I set a breakpoint at js::TenuringTracer::moveToTenured in js/src/gc/Marking.cpp (~3219). At about line 3225 a call to src->zone() which I stepped into. zone() returns NULL which when referenced in the zone->tenuredStrings++; statement causes a seg-fault.

Does this help enlighten things to those of you who know this code better than I? Any other debugging info I can provide please let me know.


Thread 1 "firefox" hit Breakpoint 1, js::TenuringTracer::moveToTenured (this=0x3fffffd1358, src=0x3ffa7958ae8) at /usr/src/debug/mozilla/js/src/gc/Marking.cpp:3224
3224	    AllocKind dstKind = src->getAllocKind();
(gdb) n
3225	    Zone* zone = src->zone();
(gdb) s
JSString::zone (this=0x3ffa7958ae8) at /usr/src/debug/mozilla/js/src/vm/StringType.h:556
556	        if (isTenured()) {
(gdb) n
562	        return js::Nursery::getStringZone(this);
(gdb) s
js::Nursery::getStringZone (str=0x3ffa7958ae8) at /usr/src/debug/mozilla/js/src/gc/Nursery.h:214
214	        auto layout = reinterpret_cast<const uint8_t*>(str) - offsetof(StringLayout, cell);
(gdb) p cell
No symbol "cell" in current context.
(gdb) p layout
$1 = (const unsigned char *) 0x3fffdfa3f70 ""
(gdb) p reinterpret_cast<const uint8_t*>(str)
$2 = (const uint8_t *) 0x3ffa7958ae8 ""
(gdb) n
215	        return reinterpret_cast<const StringLayout*>(layout)->zone;
(gdb) p reinterpret_cast<const StringLayout*>(layout)then 
$3 = (const js::Nursery::StringLayout *) 0x3ffa7958ae0
(gdb) p *(reinterpret_cast<const StringLayout*>(layout))
$4 = {zone = 0x0, cell = {byte = 0 '\000'}}
(gdb) p str
$5 = (const JSString *) 0x3ffa7958ae8
(gdb) p *str
$6 = {<js::gc::Cell> = {<No data fields>}, static NUM_INLINE_CHARS_LATIN1 = 16, static NUM_INLINE_CHARS_TWO_BYTE = 8, d = {u1 = {{flags = 1023, length = 2768715248}, flattenData = 4396520259056}, {{
        inlineStorageLatin1 = "\000\000\003\377\245\a\217\210\000\000\003\377\247\225\213\b", inlineStorageTwoByte = u"\000\x3ff\xa507\x8f88\000\x3ff\xa795\x8b08"}, s = {u2 = {
          nonInlineCharsLatin1 = 0x3ffa5078f88 "", nonInlineCharsTwoByte = 0x3ffa5078f88 u"", left = 0x3ffa5078f88}, u3 = {base = 0x3ffa7958b08, right = 0x3ffa7958b08, capacity = 4396563139336, 
          externalFinalizer = 0x3ffa7958b08}}}}, static NON_ATOM_BIT = 1, static LINEAR_BIT = 2, static HAS_BASE_BIT = 4, static INLINE_CHARS_BIT = 8, static DEPENDENT_FLAGS = 7, static UNDEPENDED_FLAGS = 23, 
  static EXTENSIBLE_FLAGS = 19, static EXTERNAL_FLAGS = 35, static FAT_INLINE_MASK = 24, static PERMANENT_ATOM_MASK = 33, static PERMANENT_ATOM_FLAGS = 32, static INIT_THIN_INLINE_FLAGS = 11, 
  static INIT_FAT_INLINE_FLAGS = 27, static INIT_ROPE_FLAGS = 1, static INIT_FLAT_FLAGS = 3, static TYPE_FLAGS_MASK = 63, static LATIN1_CHARS_BIT = 64, static INDEX_VALUE_BIT = 128, 
  static INDEX_VALUE_SHIFT = 16, static MAX_LENGTH = 268435455, static MAX_LATIN1_CHAR = 255 '\377', static TraceKind = JS::TraceKind::String}
(gdb) p *(reinterpret_cast<const uint8_t*>(str))
$7 = 0 '\000'
(gdb) p reinterpret_cast<const uint8_t*>(str) 
$8 = (const uint8_t *) 0x3ffa7958ae8 ""
(gdb)

Comment 32

•

6 years ago

Charles, if you remove the static_asserts but include the Comment 23 patch, does it work? Those static asserts were written a little strange and aren't quite precise enough.


The other question would be does building mozilla-central with patch in Comment 26 work? This is just to validate the current cleaned up code will work in next year's ESR.

Flags: needinfo?(cgrobertson)

Reporter

Comment 33

•

6 years ago

(In reply to Ted Campbell [:tcampbell] from comment #32)
> Charles, if you remove the static_asserts but include the Comment 23 patch,
> does it work? Those static asserts were written a little strange and aren't
> quite precise enough.

I removed the static_asserts and built with the patch in comment 23. It looks like it worked and Firefox proceeds past this point but later crashes in Rust code:

(firefox:69063): GLib-GObject-CRITICAL **: g_object_unref: assertion 'object->ref_count > 0' failed
[New Thread 0x3ffa12ff910 (LWP 69101)]
Detaching after fork from child process 69102.

Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
0x000003fffd910008 in memcpy_mvcle () from /lib64/libc.so.6
(gdb) bt
#0  0x000003fffd910008 in memcpy_mvcle () from /lib64/libc.so.6
#1  0x000003fff65d4f38 in <[T] as core::slice::SliceExt>::copy_from_slice (self=..., src=...) at /usr/src/debug/rustc-1.26.2-src/src/libcore/slice/mod.rs:712
#2  alloc::slice::<impl [T]>::copy_from_slice (self=..., src=...) at /usr/src/debug/rustc-1.26.2-src/src/liballoc/slice.rs:1693
#3  <alloc::vec::Vec<T> as alloc::vec::SpecExtend<&'a T, core::slice::Iter<'a, T>>>::spec_extend (self=0x3ff888fd010, iterator=...) at /usr/src/debug/rustc-1.26.2-src/src/liballoc/vec.rs:1886
#4  <alloc::vec::Vec<T>>::extend_from_slice (self=0x3ff888fd010, other=...) at /usr/src/debug/rustc-1.26.2-src/src/liballoc/vec.rs:1369
#5  alloc::slice::hack::to_vec (s=...) at /usr/src/debug/rustc-1.26.2-src/src/liballoc/slice.rs:169
#6  alloc::slice::<impl [T]>::to_vec (self=...) at /usr/src/debug/rustc-1.26.2-src/src/liballoc/slice.rs:1770
#7  style::gecko_string_cache::WeakAtom::to_ascii_lowercase (self=0x2aa013efda0) at servo/components/style/gecko_string_cache/mod.rs:204
#8  0x000003fff664a9d0 in <style::selector_map::MaybeCaseInsensitiveHashMap<style::gecko_string_cache::Atom, V>>::try_entry (self=0x3ffffff9d60, 
    self@entry=<error reading variable: Dwarf Error: Cannot find DIE at 0x2c0584a4 referenced from DIE at 0x2ba67aac [in module /usr/lib/debug/usr/lib64/firefox/libxul.so.debug]>, key=..., 
    quirks_mode=<optimized out>) at servo/components/style/selector_map.rs:522
(gdb)

This looks like a totally different bug (Rust related). However, it appears the Comment 23 patch fixed this problem after removing the static_asserts.

Flags: needinfo?(cgrobertson)

Reporter

Comment 34

•

6 years ago

> The other question would be does building mozilla-central with patch in
> Comment 26 work? This is just to validate the current cleaned up code will
> work in next year's ESR.

I'm still working on getting a build with mozilla-central as you have requested.

Comment 35

•

6 years ago

After some work on that (applied the patch and make the assertions happy) Firefox fails to start with some JS module loading errors:

JavaScript error: file:///home/stransky/src/src/objdir/dist/bin/components/MainProcessSingleton.js, line 15: TypeError: Components.ID is not a function
[7383, Main Thread] WARNING: Cannot create startup observer : service,@mozilla.org/main-process-singleton;1: file /home/stransky/src/src/toolkit/xre/nsAppStartupNotifier.cpp, line 78
JavaScript error: file:///home/stransky/src/src/objdir/dist/bin/components/PushComponents.js, line 50: TypeError: Components.ID is not a function
[7383, Main Thread] WARNING: Cannot create startup observer : @mozilla.org/push/Service;1: file /home/stransky/src/src/toolkit/xre/nsAppStartupNotifier.cpp, line 78
JavaScript error: file:///home/stransky/src/src/objdir/dist/bin/browser/components/WebContentConverter.js, line 16: TypeError: Components.ID is not a function
[7383, Main Thread] WARNING: Cannot create startup observer : service,@mozilla.org/embeddor.implemented/web-content-handler-registrar;1: file /home/stransky/src/src/toolkit/xre/nsAppStartupNotifier.cpp, line 78
[7383, Main Thread] WARNING: NS_ENSURE_TRUE(aPropertyList.isObject()) failed: file /home/stransky/src/src/js/xpconnect/src/XPCComponents.cpp, line 2299
JavaScript error: file:///home/stransky/src/src/objdir/dist/bin/browser/components/nsBrowserGlue.js, line 74: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.importGlobalProperties]
[7383, Main Thread] WARNING: Cannot create startup observer : service,@mozilla.org/browser/browserglue;1: file /home/stransky/src/src/toolkit/xre/nsAppStartupNotifier.cpp, line 78
JavaScript error: file:///home/stransky/src/src/objdir/dist/bin/browser/components/nsSessionStartup.js, line 362: TypeError: Components.ID is not a function
[7383, Main Thread] WARNING: Cannot create startup observer : service,@mozilla.org/browser/sessionstartup;1: file /home/stransky/src/src/toolkit/xre/nsAppStartupNotifier.cpp, line 78
JavaScript error: file:///home/stransky/src/src/objdir/dist/bin/browser/components/startupRecorder.js, line 6: TypeError: Cm is null
[7383, Main Thread] WARNING: Cannot create startup observer : service,@mozilla.org/test/startuprecorder;1: file /home/stransky/src/src/toolkit/xre/nsAppStartupNotifier.cpp, line 78
JavaScript error: file:///home/stransky/src/src/objdir/dist/bin/browser/components/EnterprisePolicies.js, line 71: TypeError: Components.ID is not a function
JavaScript error: file:///home/stransky/src/src/objdir/dist/bin/components/addonManager.js, line 251: TypeError: Components.ID is not a function
[7383, Main Thread] WARNING: Failed to create Addons Manager.: file /home/stransky/src/src/toolkit/xre/nsXREDirProvider.cpp, line 1014
Hit MOZ_CRASH(Failed to load module "file:///home/stransky/src/src/objdir/dist/bin/components/nsAsyncShutdown.js": ["TypeError: Components.ID is not a function" {file: "file:///home/stransky/src/src/objdir/dist/bin/components/nsAsyncShutdown.js", line: 181}]) at /home/stransky/src/src/js/xpconnect/loader/mozJSComponentLoader.cpp:462
Segmentation fault (core dumped)

Assignee

Updated

•

6 years ago

Priority: -- → P3

Comment 36

•

6 years ago

Attached patch Align shadow::String with JSString (obsolete) — Details — Splinter Review

This patch appears to fix the static_assert in StringType.h correctly.

Attachment #9016789 - Flags: review?(tcampbell)

Attachment #9016789 - Flags: review?(sphink)

Comment 37

•

6 years ago

Using the patch from Comment 23, the patch from Comment 36, and commenting out the static_asserts in Marking-inl.h, I'm now seeing the same result as Comment 33:

(gdb) bt
#0  0x00003fffb0b7b268 in __syscall4 (d=8, c=0, b=70367849031600, a=2, n=174) at ./arch/powerpc64/syscall_arch.h:54
#1  0x00003fffb0b7b268 in __restore_sigs (set=0x3fffcaa527b0) at src/signal/block.c:43
#2  0x00003fffb0b7b56c in raise (sig=<optimized out>) at src/signal/raise.c:11
#3  0x00003fffad521dd4 in nsProfileLock::FatalSignalHandler(int, siginfo_t*, void*) (signo=<optimized out>, info=<optimized out>, context=0x3fffcaa52a30) at /usr/src/packages/user/thunderbird/src/thunderbird-60.2.1/toolkit/profile/nsProfileLock.cpp:177
#4  0x00003fffadb5fe64 in WasmFaultHandler(int, siginfo_t*, void*) (signum=<optimized out>, info=0x3fffcaa537a8, context=0x3fffcaa52a30) at /usr/src/packages/user/thunderbird/src/thunderbird-60.2.1/js/src/wasm/WasmSignalHandlers.cpp:1501
#5  0x00003fffb0be04a8 in <signal handler called> ()
#6  0x00003fffb0b88768 in memcpy (dest=0x3fff88c9e020, src=<optimized out>, n=<optimized out>) at src/string/memcpy.c:29
#7  0x00003fffadf1e978 in core::slice::<impl [T]>::copy_from_slice (self=..., src=...) at /usr/src/packages/user/rust/src/rustc-1.29.1-src/src/libcore/slice/mod.rs:1657
#8  0x00003fffadf1e978 in <alloc::vec::Vec<T> as alloc::vec::SpecExtend<&'a T, core::slice::Iter<'a, T>>>::spec_extend (self=<optimized out>, iterator=...) at /usr/src/packages/user/rust/src/rustc-1.29.1-src/src/liballoc/vec.rs:1955
#9  0x00003fffadf1e978 in <alloc::vec::Vec<T>>::extend_from_slice (other=..., self=<optimized out>) at /usr/src/packages/user/rust/src/rustc-1.29.1-src/src/liballoc/vec.rs:1396
#10 0x00003fffadf1e978 in alloc::slice::hack::to_vec (s=...) at /usr/src/packages/user/rust/src/rustc-1.29.1-src/src/liballoc/slice.rs:168
#11 0x00003fffadf1e978 in alloc::slice::<impl [T]>::to_vec (self=...) at /usr/src/packages/user/rust/src/rustc-1.29.1-src/src/liballoc/slice.rs:369
#12 0x00003fffadf1e978 in style::gecko_string_cache::WeakAtom::to_ascii_lowercase (self=<optimized out>) at servo/components/style/gecko_string_cache/mod.rs:204
#13 0x00003fffadf9ea64 in <style::selector_map::MaybeCaseInsensitiveHashMap<style::gecko_string_cache::Atom, V>>::try_entry (self=0x3fffcaa54b48, key=..., quirks_mode=<optimized out>) at servo/components/style/selector_map.rs:522


This is Thunderbird on esr60 bookmark on Linux/ppc64/musl.

I'd like to fix this (seemingly unrelated) bug before moving on to the ppc32 issue, which I think *should* work by changing "#if MOZ_LITTLE_ENDIAN" to "#if MOZ_LITTLE_ENDIAN || JS_BITS_PER_WORD == 32".  Not entirely sure yet.

Assignee

Comment 38

•

6 years ago

Comment on attachment 9016789 [details] [diff] [review]
Align shadow::String with JSString

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

This looks right.

Attachment #9016789 - Flags: review?(sphink) → review+

Assignee

Comment 39

•

6 years ago

I'm kind of lost as to the current state here. If I understand correctly, this is the state we want for 64-bit big-endian:

(1) Field orders are

  RelocationOverlay: magic_ then preserve_
  JSString:          length then flags
  JSObject:          group_ (hi32 then lo32)

and the jsfriendapi.h shadow string stuff must match JSString.

(2) MOZ_LITTLE_ENDIAN needs to be tested with #if

(3) the static asserts are bogus in some way. They could either be fixed, or just made conditional on #if MOZ_LITTLE_ENDIAN.

Can someone test a patch that does the above things? It may still hit the Rust error, if it's unrelated. (Or it's not, and Rust is using the wrong layout when reading JSStrings. I only say that because I see 'gecko_string_cache' in the stack, though that may be gecko nsString strings and so not related.)

Assignee

Updated

•

6 years ago

Attachment #9016789 - Flags: review?(tcampbell)

Assignee

Comment 40

•

6 years ago

(In reply to Charles Robertson from comment #31)
> The patch only swaps the order of the uint32_t flags; and uint32_t length; fields of the Data struct of JSString class. I cannot see how this is related to the Zone object returned by JSString.zone().

I never answered this.

The nursery is shared by all Zones, so you can't tell just from a nursery pointer what Zone it belongs to. Tenured cells are stored in arenas that have their Zone* in a footer. Nursery JSObjects can retrieve their Zone* from their group_ field, with is an ObjectGroup*, and ObjectGroups are only allowed in the tenured heap so they can use the arena footer as above. Nursery JSStrings do not have any such field, so when they're allocated in the nursery an extra Zone* is stored next to them.

All of which means that when you're looking at a pointer to some Cell and want to figure out its Zone, you'll need to do something different based on whether it's a JSObject vs JSString. And that is derived from a bit that is guaranteed to be zero for JSObjects and one for nursery JSStrings -- unless we mess up and collide that bit with some other information because we messed up endianness.

And if so, you'll get the type wrong, and so look in the wrong place for the Zone* and thereby get the wrong Zone pointer value.

Comment 41

•

6 years ago

I'm not seeing that Servo crash in central and I think it may be fixed by https://github.com/servo/servo/pull/20339 but I haven't had the time to attempt a backport of that to esr60 yet.  (central is just crashing after actually popping a window with "pipe error", oh for bug 1428509 to land something useful...)

The Marking-inl.h static asserts made my head hurt.  I should note that this is my first serious mozdev work in almost 10 years, and wow did the JS interpreter change in that time!  :P  I'm not sure what the proper solution there is and I don't have a little endian box set up yet to ensure that whatever I do doesn't break LE.

Pulsebot

Comment 42

•

6 years ago

Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c14789dbc570
MOZ_LITTLE_ENDIAN is 0/1, not undef/1, r=tcampbell

Cosmin Sabou [:CosminS]

Comment 43

•

6 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox66: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla66

Simon McVittie

Comment 44

•

6 years ago

I'm looking at backporting this fix to Spidermonkey 60 (aka mozjs60) in Debian, but I'm confused about the status of the patches. Which patch(es) would be desirable to apply to backport this fix? Attachment #9008245 [details] [diff]? Attachment #9008260 [details] [diff]? Attachment #9016789 [details] [diff]? Some combination of those?

Debian has all combinations of 32- and 64-bit LE and BE architectures available, so I can test via the Spidermonkey test suite on any or all combinations.

For a version usable in Debian we can't just #error on 32-bit BE, but the vast majority of the Spidermonkey tests already pass on our 32-bit BE machines (mips and powerpc), which I think indicates that the LE code paths were OK for 32-bit BE before this bug was fixed, so it seems we only need two code paths:

little-endian || 32-bit
big-endian && 64-bit

Is there a separate bug open for making this work on 32-bit BE, or is this bug the right place to continue discussion?

Comment 45

•

6 years ago

(Needinfo-ing myself to look at next week.)

This is the right place for 32-bit BE discussion. Probably can fix and get these things uplifted to ESR60. This code was a gnarly mess and was re-written in Bug 1479900 but that cannot be backported to ESR60.

Flags: needinfo?(tcampbell)

Simon McVittie

Comment 46

•

6 years ago

Thanks! Please let me know if there's something you'd like tested on an assortment of architectures.

andrew.c.morrow

Comment 47

•

6 years ago

MongoDB is in the process of upgrading our bundled third party libraries, including MozJS. We currently bundle ESR-45 and are attempting to upgrade to ESR-60. We appear to be encountering this bug on our s390x builds, where we are observing crashes like the following:

[js_test:application_message] 2019-01-25T02:24:12.277+0000 2019-01-25T02:24:12.269+0000 F - [js] Invalid access at address: 0
[js_test:application_message] 2019-01-25T02:24:12.287+0000 2019-01-25T02:24:12.281+0000 F - [js] Got signal: 11 (Segmentation fault).
[js_test:application_message] 2019-01-25T02:24:12.287+0000 mongo(mongo::printStackTrace(std::basic_ostream<char, std::char_traits<char> >&)+0x42) [0x2aa0b783242]
[js_test:application_message] 2019-01-25T02:24:12.287+0000 mongo(+0x198235E) [0x2aa0b78235e]
[js_test:application_message] 2019-01-25T02:24:12.287+0000 mongo(+0x19829E2) [0x2aa0b7829e2]
[js_test:application_message] 2019-01-25T02:24:12.287+0000 ??? [0x3ffb71fda98]
[js_test:application_message] 2019-01-25T02:24:12.287+0000 mongo(js::TenuringTracer::moveToTenured(JSString*)+0xC2) [0x2aa0ae0a1ba]
[js_test:application_message] 2019-01-25T02:24:12.287+0000 mongo(void js::TenuringTracer::traverse<JSString>(JSString**)+0x54) [0x2aa0ae0a364]
[js_test:application_message] 2019-01-25T02:24:12.287+0000 mongo(js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::CellPtrEdge>::trace(js::gc::StoreBuffer*, js::TenuringTracer&)+0x36) [0x2aa0ae2228e]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(js::Nursery::doCollection(JS::gcreason::Reason, js::gc::TenureCountCache&)+0x232) [0x2aa0ae0f462]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(js::Nursery::collect(JS::gcreason::Reason)+0x136) [0x2aa0ae0fa06]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(js::gc::GCRuntime::minorGC(JS::gcreason::Reason, js::gcstats::PhaseKind)+0xAC) [0x2aa0adc38ac]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason)+0x9A) [0x2aa0adda632]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason)+0x4CC) [0x2aa0addacb4]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason)+0x54) [0x2aa0addaf64]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(JSRuntime::destroyRuntime()+0x1AE) [0x2aa0ac1b3d6]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(js::DestroyContext(JSContext*)+0x2CA) [0x2aa0ab9943a]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(mongo::mozjs::MozJSImplScope::~MozJSImplScope()+0x15A0) [0x2aa0a6a6428]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(mongo::mozjs::MozJSImplScope::~MozJSImplScope()+0x1A) [0x2aa0a6a6592]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(mongo::mozjs::MozJSProxyScope::implThread(void*)+0x344) [0x2aa0a6e5814]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(nspr::Thread::ThreadRoutine(void*)+0x42) [0x2aa0a6df7c2]
[js_test:application_message] 2019-01-25T02:24:12.288+0000 mongo(+0x1AE9D1C) [0x2aa0b8e9d1c]

Like the above commenter, we are a little puzzled about the state of things after reading through the history in this bug. We would very much appreciate a unified patch (or clear guidance on the correct sequence of patches already linked here) that we can cherry-pick into ESR 60 and test. In our case we do not need a 32-bit BE fix.

Along with s390x, we will also be testing on aarch64, ppc64le, and x86_64.

Simon McVittie

Comment 48

•

6 years ago

Here is an attempt at some backports to mozjs60, including untested fixes for 32-bit big-endian: https://salsa.debian.org/gnome-team/mozjs60/merge_requests/1. Note that with those patches, several unit tests still fail on s390x (I think TypedArray is broken there, perhaps with a single root cause); that should perhaps be addressed as a separate bug.

Julien Cristau [:jcristau]

Comment 49

•

6 years ago

Attached patch 64-bit and 32-bit BE fix for ESR 60 — Details — Splinter Review

This is the patch we are shipping. Please note that this patch has NOT been tested on 32-bit BE yet, as we are still in the process of bringing up Rust on ppc32. We aren't just building mozjs but the entire Firefox 60 ESR browser.

That should be done this week, if we're lucky, and then I can begin ensuring that this patch should work. Functionally, it is definitely correct on 64-bit BE.

Comment 50

•

6 years ago

FWIW https://people.debian.org/~jcristau/mozjs60-mips.log is a log from building SM 60 on mips (32-bit BE) with the patch from comment 49. jstests don't look too unhappy. (ion is disabled)

I'll try and do the same on s390x (64-bit BE) soon.

Julien Cristau [:jcristau]

Comment 51

•

6 years ago

https://people.debian.org/~jcristau/mozjs60-s390x.log is my s390x build log.

The number of unexpected failures goes from 28+k to 65. \o/

TEST-UNEXPECTED-FAIL | non262/extensions/clone-transferables.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/extensions/sharedtypedarray.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/extensions/clone-errors.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/extensions/typedarray.js | (args: "") [0.2 s]
TEST-UNEXPECTED-FAIL | non262/TypedArray/slice-detached.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/TypedArray/Tconstructor-fromTypedArray-byteLength.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/TypedArray/set-same-buffer-different-source-target-types.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/TypedArray/iterator-next-with-detached.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/TypedArray/subarray.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/TypedArray/sort_byteoffset.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/TypedArray/indexOf-and-lastIndexOf.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | non262/ArrayBuffer/CloneArrayBuffer.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArray/prototype/subarray/speciesctor-get-species-custom-ctor-invocation.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArray/prototype/copyWithin/bit-precision.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArray/prototype/set/typedarray-arg-set-values-same-buffer-same-type-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArray/prototype/set/typedarray-arg-set-values-diff-buffer-same-type-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArray/prototype/set/typedarray-arg-set-values-diff-buffer-other-type-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArray/prototype/byteLength/return-bytelength.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wait/was-woken.js | (args: "") [0.5 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wait/no-spurious-wakeup.js | (args: "") [0.5 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wait/did-timeout.js | (args: "") | (TIMEOUT) [600.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wait/nan-timeout.js | (args: "") [0.5 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wait/negative-timeout.js | (args: "") | (TIMEOUT) [600.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wake/wake-one.js | (args: "") [0.1 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wake/wake-zero.js | (args: "") [0.1 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wake/wake-two.js | (args: "") [0.1 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wake/wake-nan.js | (args: "") [0.7 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/Atomics/wake/wake-negative.js | (args: "") [0.5 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-toindex-byteoffset-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-excessive-offset-throws.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-defined-offset-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-returns-new-instance-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-excessive-length-throws.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-defined-length.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-returns-new-instance.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-defined-length-and-offset-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-toindex-byteoffset.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-bufferbyteoffset-throws-from-modulo-element-size.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-excessive-offset-throws-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-defined-length-and-offset.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-bufferbyteoffset-throws-from-modulo-element-size-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-defined-length-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-byteoffset-throws-from-modulo-element-size.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-byteoffset-throws-from-modulo-element-size-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-defined-offset.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/buffer-arg-excessive-length-throws-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Float64Array/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Float64Array/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Uint16Array/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Uint16Array/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/internals/Get/indexed-value-sab.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Uint8Array/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Uint8Array/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Int32Array/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Int32Array/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Float32Array/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Float32Array/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Int16Array/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Int16Array/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Int8Array/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Int8Array/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Uint8ClampedArray/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Uint8ClampedArray/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Uint32Array/BYTES_PER_ELEMENT.js | (args: "") [0.0 s]
TEST-UNEXPECTED-FAIL | test262/built-ins/TypedArrays/Uint32Array/prototype/BYTES_PER_ELEMENT.js | (args: "") [0.1 s]

Julien Cristau [:jcristau]

Comment 52

•

6 years ago

Fixing the TypedArrays issue over in bug 1543659.

Updated

•

5 years ago

Attachment #9008245 - Attachment is obsolete: true

Updated

•

5 years ago

Attachment #9016789 - Attachment is obsolete: true

Flags: needinfo?(tcampbell)