1138984 - Crash [@ js::Nursery::moveToTenured] or Assertion failure: !templateObj->as<NativeObject>().numDynamicSlots(), at jit/MacroAssembler.cpp:1018 with --unboxed-objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 0b3c520002ad (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --enable-arm-simulator --disable-debug, run with --fuzzing-safe --ion-eager --arm-sim-icache-checks --unboxed-objects --thread-count=2):

var gTestcases = new Array();
var gTc = 0;
function AddTestCase() {
  new TestCase();
}
function TestCase(e) {
  this.expect = e;
  this.type = 'shell';
  gTestcases[gTc++] = this;
};
function reportCompare () {
  new TestCase();
}
new TestCase('/(?:)/');
new TestCase('/.+/');
reportCompare();
AddRegExpCases();
AddRegExpCases();
function AddRegExpCases() {
  AddTestCase();
  AddTestCase();
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::Nursery::moveToTenured(js::gc::MinorCollectionTracer*, JSObject*) () at js/src/jsgc.h:155
#0  js::Nursery::moveToTenured(js::gc::MinorCollectionTracer*, JSObject*) () at js/src/jsgc.h:155
#1  0x08160f10 in js::Nursery::collectToFixedPoint(js::gc::MinorCollectionTracer*, js::Nursery::TenureCountCache&) () at js/src/gc/Nursery.cpp:626
#2  0x0816151f in js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, js::Vector<js::ObjectGroup*, 0u, js::SystemAllocPolicy>*) () at js/src/gc/Nursery.cpp:855
#3  0x084abd6f in js::gc::GCRuntime::minorGCImpl(JS::gcreason::Reason, js::Vector<js::ObjectGroup*, 0u, js::SystemAllocPolicy>*) () at js/src/jsgc.cpp:6437
#4  0x084c3429 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) () at js/src/gc/GCRuntime.h:618
#5  0x084c38f0 in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) () at js/src/jsgc.cpp:6228
#6  0x084c3cda in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) () at js/src/jsgc.cpp:6289
#7  0x084c3e76 in js::DestroyContext(JSContext*, js::DestroyContextMode) () at js/src/jscntxt.cpp:185
#8  0x084c3f87 in JS_DestroyContext(JSContext*) () at js/src/jsapi.cpp:576
#9  0x08058f65 in main () at js/src/shell/js.cpp:5649
eax	0x933eee0	154398432
ebx	0x9330398	154338200
ecx	0x1e	30
edx	0x9452cf0	155528432
esi	0x9363ce0	154549472
edi	0x6f747541	1869903169
ebp	0xf5b441f0	4122231280
esp	0xffffc680	4294952576
eip	0x81600a2 <js::Nursery::moveToTenured(js::gc::MinorCollectionTracer*, JSObject*)+162>
=> 0x81600a2 <_ZN2js7Nursery13moveToTenuredEPNS_2gc21MinorCollectionTracerEP8JSObject+162>:	cmpb   $0x0,-0xb68fb8(%ebx,%edi,1)
   0x81600aa <_ZN2js7Nursery13moveToTenuredEPNS_2gc21MinorCollectionTracerEP8JSObject+170>:	mov    %edi,0x18(%esp)


Testcase is intermittent.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

GC intermittent involving unboxed objects...

Comment 2

[Brian Hackett [Laid off!]]

I can't reproduce this.  Do you have a stack for the assertion failure?

Comment 3

[Brian Hackett [Laid off!]]

Attachment: potential patch

Hmm, so the patch in bug 1133369 removed this old and unnecessary restriction that the definite properties analysis not add definite properties which are dynamic slots.  That change was producing crashes on b2g which I couldn't reproduce, so I removed it, but after bug 1133369 we do have the possibility of creating 'new' objects with dynamic slots, when using unboxed objects.  And looking at the assertion stack trace (which decoder sent me via IRC) we are asserting when allocating a 'new' object in Ion.

If I again remove the crufty check then the assertion is pretty easy to get a testcase for.  The problem is easy to fix, as MacroAssembler::newGCThing has some outdated behavior where it refuses to allocate objects with dynamic slots, even though allocateObject() can handle such cases.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8573302 [details] [diff] [review]
potential patch

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

Nice.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: revised

Actually, after some further testing the earlier patch is not complete.  There is another place where CreateNewObject assumes the new object doesn't have dynamic slots, and when the new object has dynamic slots the MacroAssembler methods do not really allow for a VM call that allocates the object and jitcode which fills it in, as CreateNewObject wants.  This patch fixes these issues, cleans up the MacroAssembler handling of slots initialization, and is green on try.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8574393 [details] [diff] [review]
revised

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

::: js/src/jit/CodeGenerator.cpp
@@ +4802,1 @@
>                                     StoreRegisterTo(objReg));

Pre-existing but I think calling NewGCObject and rejoining before initGCThing is unnecessarily complicated. It seems simpler to just do an OOL call to a generic CreateThis function and rejoin at the end, as we do everywhere else. Doesn't need to happen in this bug though, just noticing.

Comment 7

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/437bf0c48c67

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/437bf0c48c67