781248 - IonMonkey: Assertion failure: !allocated(), at ../gc/Heap.h:482 or Crash [@ js::gc::Chunk::allocateArena]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on ionmonkey revision 15db586f9a12 (run with --ion -n):


function newArrayTest()
{
  var a = [];
  for (var i = 0; i < 10; newArrayTest ++)
    a[i] = new Array(0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,
       0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9);
}
assertEq(newArrayTest(), "0,0,0,0,0,0,0,0,0,0");

Comment 1

[Christian Holler (:decoder)]

Opt64 crash:

==23202== Invalid read of size 8
==23202==    at 0x46FB3E: js::gc::Chunk::allocateArena(JSCompartment*, js::gc::AllocKind) (jsgc.cpp:742)
==23202==    by 0x47A76E: js::gc::ArenaLists::refillFreeList(JSContext*, js::gc::AllocKind) (jsgc.cpp:1595)
==23202==    by 0x437788: js::NewDenseAllocatedArray(JSContext*, unsigned int, JSObject*) (jsgcinlines.h:419)
==23202==    by 0x72E43F: js::ion::NewInitArray(JSContext*, unsigned int, js::types::TypeObject*) (VMFunctions.cpp:236)
==23202==    by 0x4033769: ???
==23202==    by 0x7FEFFEF17: ???
==23202==    by 0x7FEFFEF17: ???
==23202==    by 0x1: ???
==23202==    by 0xAB00DF: ???
==23202==    by 0xC98D0D7: ???
==23202==    by 0x40342A1: ???
==23202==    by 0x1FF: ???
==23202==  Address 0xfff8800000000009 is not stack'd, malloc'd or (recently) free'd

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Bug is that we accidentally take an inline allocation path, but the template object requires non-inline slots. So we end up painting over unallocated memory.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 650398 [details] [diff] [review]
fix

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

Sounds good.  I don't know exactly the details of the template Object allocation policy but if we are supposed to only allocate under a specific boundary, then this patch might remove some other memory corruption.  Would be nice to find the reason of this bound check and to add a reference/comment above it.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

It's okay to allocate over the boundary, but we don't have a great facility for allocating dynamic slots, thus the callVM if we are over the max inline slots.

https://hg.mozilla.org/projects/ionmonkey/rev/b07af1efa3dd

Comment 5

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 6

[Hannes Verschore [:h4writer]]

Bug #742829 introduced this inline path. This was to equalize the allocation path with the one used in JM.

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Proper fix

This should be the proper fix for this bug. Only use the lazily allocating path when only one argument is provided.

Comment 8

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision ed05182dac42).

Comment 9

[Hannes Verschore [:h4writer]]

Revert b07af1efa3dd:
https://hg.mozilla.org/projects/ionmonkey/rev/b087c794dd80

Fix
https://hg.mozilla.org/projects/ionmonkey/rev/98004d8529b5

Comment 10

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thanks for catching this, Hannes!