1264948 - Crash [@ js::TypeSet::addType] with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 10f66b316457 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --ion-eager --ion-offthread-compile=off):

loadFile(`
  T = TypedObject
  ObjectStruct = new T.StructType({f: T.Object})
  var o = new ObjectStruct
  function testGC(p) {
    for (; i < 5; i++) 
        whatever.push;
  }
  testGC(o)
  function writeObject() 
    o.f = v
    writeObject({function() { } })
  for (var i ; i < 5 ; ++i) 
    try {} catch (StringStruct) {}
`);
function loadFile(lfVarx) {
  oomTest(Function(lfVarx));
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::TypeSet::addType (this=this@entry=0x0, type=..., alloc=0x7ffff69a87c0) at js/src/vm/TypeInference.cpp:525
#0  js::TypeSet::addType (this=this@entry=0x0, type=..., alloc=0x7ffff69a87c0) at js/src/vm/TypeInference.cpp:525
#1  0x000000000077f7a7 in PropertyTypeIncludes (alloc=..., value=value@entry=0x7ffff69b4780, implicitType=implicitType@entry=js::jit::MIRType_Null, property=...) at js/src/jit/MIR.cpp:5797
#2  0x00000000007817ee in CanWriteProperty (implicitType=js::jit::MIRType_Null, value=0x7ffff69b4780, property=..., constraints=0x7ffff69b3128, alloc=...) at js/src/jit/MIR.cpp:5920
#3  js::jit::PropertyWriteNeedsTypeBarrier (alloc=..., constraints=0x7ffff69b3128, current=0x7ffff69b3b98, pobj=pobj@entry=0x7fffffffa678, name=name@entry=0x7ffff7e009b8, pvalue=pvalue@entry=0x7fffffffa670, canModify=canModify@entry=true, implicitType=implicitType@entry=js::jit::MIRType_Null) at js/src/jit/MIR.cpp:5957
#4  0x00000000006daf00 in js::jit::IonBuilder::storeReferenceTypedObjectValue (this=this@entry=0x7ffff69b31c0, typedObj=typedObj@entry=0x7ffff69b4350, byteOffset=..., type=type@entry=js::ReferenceTypeDescr::TYPE_OBJECT, value=value@entry=0x7ffff69b4780, name=name@entry=0x7ffff7e009b8) at js/src/jit/IonBuilder.cpp:14044
#5  0x00000000006dbc46 in js::jit::IonBuilder::setPropTryReferencePropOfTypedObject (this=this@entry=0x7ffff69b31c0, emitted=emitted@entry=0x7fffffffa870, obj=obj@entry=0x7ffff69b4350, fieldOffset=0, value=value@entry=0x7ffff69b4780, fieldPrediction=..., name=name@entry=0x7ffff7e009b8) at js/src/jit/IonBuilder.cpp:12436
#6  0x00000000006dbf52 in js::jit::IonBuilder::setPropTryTypedObject (this=this@entry=0x7ffff69b31c0, emitted=emitted@entry=0x7fffffffa870, obj=0x7ffff69b4350, name=name@entry=0x7ffff7e009b8, value=0x7ffff69b4780) at js/src/jit/IonBuilder.cpp:12404
#7  0x00000000006fd09c in js::jit::IonBuilder::jsop_setprop (this=this@entry=0x7ffff69b31c0, name=0x7ffff7e009b8) at js/src/jit/IonBuilder.cpp:12213
#8  0x00000000006f5c41 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff69b31c0, op=op@entry=JSOP_SETPROP) at js/src/jit/IonBuilder.cpp:2033
#9  0x00000000006f6678 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff69b31c0) at js/src/jit/IonBuilder.cpp:1523
#10 0x00000000006f6eb5 in js::jit::IonBuilder::build (this=this@entry=0x7ffff69b31c0) at js/src/jit/IonBuilder.cpp:918
#11 0x00000000006aa1d9 in js::jit::IonCompile (cx=cx@entry=0x7ffff6908800, script=script@entry=0x7ffff7e744a0, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::Normal) at js/src/jit/Ion.cpp:2160
#12 0x00000000006aac5c in js::jit::Compile (cx=cx@entry=0x7ffff6908800, script=..., script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2392
#13 0x00000000006aae8e in js::jit::CanEnter (cx=cx@entry=0x7ffff6908800, state=...) at js/src/jit/Ion.cpp:2484
#14 0x0000000000a8fb69 in js::RunScript (cx=cx@entry=0x7ffff6908800, state=...) at js/src/vm/Interpreter.cpp:402
#15 0x0000000000a8fdf9 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6908800, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:498
#16 0x0000000000a9008b in InternalCall (cx=cx@entry=0x7ffff6908800, args=...) at js/src/vm/Interpreter.cpp:525
#17 0x0000000000a9019a in js::CallFromStack (cx=cx@entry=0x7ffff6908800, args=...) at js/src/vm/Interpreter.cpp:531
#18 0x000000000061773d in js::jit::DoCallFallback (cx=0x7ffff6908800, frame=0x7fffffffb388, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffb320, res=...) at js/src/jit/BaselineIC.cpp:6116
#19 0x00007ffff7ff1a1f in ?? ()
[...]
rax	0x0	0
rbx	0x0	0
rcx	0x1	1
rdx	0x7ffff69a87c0	140737330710464
rsi	0x7	7
rdi	0x0	0
rbp	0x7fffffffa500	140737488332032
rsp	0x7fffffffa4c0	140737488331968
r8	0x7ffff7e009b8	140737352042936
r9	0x7fffffffa670	140737488332400
r10	0x5	5
r11	0x7ffff69a87c0	140737330710464
r12	0x7ffff69b4780	140737330759552
r13	0x7ffff69b3020	140737330753568
r14	0x7	7
r15	0x7ffff69b4780	140737330759552
rip	0xb709d2 <js::TypeSet::addType(js::TypeSet::Type, js::LifoAlloc*)+18>
=> 0xb709d2 <js::TypeSet::addType(js::TypeSet::Type, js::LifoAlloc*)+18>:	mov    (%rdi),%eax
   0xb709d4 <js::TypeSet::addType(js::TypeSet::Type, js::LifoAlloc*)+20>:	mov    %rsi,-0x40(%rbp)

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160229051912" and the hash "8986592ec95420af9ef332aeb5b471a7396dbb7f".
The "bad" changeset has the timestamp "20160229052113" and the hash "1da938156283fc4fec76261e41efbd5abd894e24".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8986592ec95420af9ef332aeb5b471a7396dbb7f&tochange=1da938156283fc4fec76261e41efbd5abd894e24

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Could be related to the bugs mentioned in the regression window in comment 1, ni? nbp.

Comment 3

[Nicolas B. Pierron [:nbp]]

I don't think this is related to any of the patches I made, I can reproduce this issue before any of these patches.  I have no clue which the bisect stopped there.

Any how, I was wondering about the allocation failures in TI, and the issue seems to be that we are using the LifoAlloc instead of the TempAllocator.  So even if we have enough ballast space in practice the oomTest function causes us to fail.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Ignore OOM in PropertyTypeIncludes.

This is likely a false positive cause by the simulated OOM of the LifoAlloc,
due to the fact that the TypeSet code use a LifoAlloc instead of the
TempAllocator.  These allocation are more than likely covered by the
previous ballast space reserve because they the only surrounding loops are
bounded by the number of objects in a TypeSet.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8743230 [details] [diff] [review]
Ignore OOM in PropertyTypeIncludes.

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

From IRC,
<jonco> nbp: your current patch + ballast assertion would be fine
I think Jon is much more fit to this review...

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Make it possible to use a placement-new with a fallible TempAllocator.

This modification is made to remove 2 explicit calls to ensureBallast, which
is already called by the fallible allocator.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Assert that when do not allocate new chunks when using infallible allocation.

This patch adds a new flag to the LifoAlloc structure, which is used to assert
when we attempt to allocate a new chunk for the LifoAlloc.  This ensure that we
assert (in debug builds) if we attempt to allocate beyong the reserved space of
the ballast.

Changes:
 - Add a flag to assert if there is enough ballas space.
 - Do not emulate OOM if a fallible allocator is used under the
   AutoAssertBallast(alloc, true), to handle Bug 1264948 issue.
 - Turn off the assertEnoughBallast assertion when making fallible
   allocations.
 - Add a EnsureBallast class to reserve ballast space, and turn on the
   assertion to ensure that we have enough ballast space.
 - For the moment, turn on the assertEnoughBallast assertion when making
   infallible allocations.  We should later convert this into an assertion /
   static analysis, such that we can ensure that EnsureBallast classes are
   wrapping all infallible allocations.

The current patch is not yet passing the test suite, because of the lack of
EnsureBallast calls.  So we would have to make many additional patches to
cover all the failures, and we should probably request some fuzzing when
these are fixed.

The good news is that we are currently calling a function which does a
MOZ_RELEASE_ASSERT on OOM, so none of these are security issues.

Comment 8

[Jon Coppeard (:jonco)]

I like the idea of asserting if we attempt to allocate beyond the ballast.  I have a couple of questions though:

1. Is the ballast size always a multiple of chunk size, so will this assertion always catch allocation beyond the ballast?

2. Do we reserve ballast again after a fallible allocation?  I couldn't see where this happens.  If not then what stops a fallible allocation (with size controlled by user input) from using all the ballast?

In general I think it would be better to use a completely separate allocator for fallible allocations, but I appreciate that may not be possible.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Jon Coppeard (:jonco) from comment #8)
> 1. Is the ballast size always a multiple of chunk size, so will this
> assertion always catch allocation beyond the ballast?

AFAIK, this is only used in the JitAllocPolicy, and the chunk size is twice larger than the ballast size, but I do not think there is any requirement here.  The only detail is that id we used a ballast space larger than the chunk size, we might end-up with larger chunks overall.

This assertion catches allocations beyond the remaining free space, it is not aware of the ballast size, but assumes that ensureUnusedApproximate reserved enough free-space for the ballast.

> 2. Do we reserve ballast again after a fallible allocation?  I couldn't see
> where this happens.  If not then what stops a fallible allocation (with size
> controlled by user input) from using all the ballast?

Yes [1], we do that in JitAllocPolicy, not in the the alloc function of the LifoAlloc.  This way, the alloc calls from TypeSet code should consume the ballast space or assert.  The only way to prevent the assertion then is to use

  AutoAssertBallast ballastNoAssert(alloc, false);
  
and check the returned value of the allocations.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/JitAllocPolicy.h#46

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8746146 [details] [diff] [review]
Assert that when do not allocate new chunks when using infallible allocation.

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

From IRC comments it sounds like it would be possible to make the check for allocation beyond the end of ballast more exact, but that it would be more complicated and this change is already finding issues as is.

Comment 11

[Fuzzing Team]

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

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8746146 [details] [diff] [review]
Assert that when do not allocate new chunks when using infallible allocation.

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

I do not see the reason for the "TempAllocator::EnsureBallast" class. The TempAllocator always overrides the "assertEnoughBallast_" value during allocate/allocateInfallible.
And otherwise the class isn't doing anything except for a indirection to use "ensureBallast();".

::: js/src/ds/LifoAlloc.h
@@ +162,5 @@
>      size_t      defaultChunkSize_;
>      size_t      curSize_;
>      size_t      peakSize_;
> +#if defined(DEBUG) || defined(JS_OOM_BREAKPOINT)
> +    bool        assertEnoughBallast_;

A lot of conditionals with "assertEnoughBallast" were hard to read.
Can we rename that to "assertNoBallastAlloc"

@@ +283,5 @@
>          return allocImpl(n);
>      }
>  
>      MOZ_ALWAYS_INLINE
> +    void* allocInfallible(size_t n) {

Nice this is again one function.

@@ +311,5 @@
>              latest = latestBefore;
>          return true;
>      }
>  
> +    class MOZ_STACK_CLASS AutoAssertBallast {

s/MOZ_STACK_CLASS/MOZ_RAII and also add
- MOZ_GUARD_OBJECT_NOTIFIER_PARAM
- MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
- MOZ_GUARD_OBJECT_NOTIFIER_INIT

https://developer.mozilla.org/en-US/docs/Mozilla/RAII_classes

@@ +317,5 @@
> +        LifoAlloc* lifoAlloc_;
> +        bool prevAssertEnoughBallast_;
> +
> +      public:
> +        AutoAssertBallast(LifoAlloc* lifoAlloc, bool enough) {

It was quite confusing to see AutoAssertBallast(alloc, false) and to know this suppresses asserts.
Can we do one of the following:

1)
- Can we use an enum instead of boolean?
AutoAssertBallast(alloc, AutoAssertBallast::NoAssert)
- And also add a default param to AutoAssertBallast::Assert
AutoAssertBallast(alloc) // would by default assert.

OR

2)
- Introduce AutoAssertBallast and AutoNoAssertBallast

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 8746139 [details] [diff] [review]
Make it possible to use a placement-new with a fallible TempAllocator.

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

I don't see the need to add this code for just two exceptions... ?

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #13)
> Comment on attachment 8746139 [details] [diff] [review]
> Make it possible to use a placement-new with a fallible TempAllocator.
> 
> Review of attachment 8746139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see the need to add this code for just two exceptions... ?

No, the up-coming patch is also making use of that.

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #12)
> Review of attachment 8746146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I do not see the reason for the "TempAllocator::EnsureBallast" class. The
> TempAllocator always overrides the "assertEnoughBallast_" value during
> allocate/allocateInfallible.
> And otherwise the class isn't doing anything except for a indirection to use
> "ensureBallast();".

The reason is that this tie the use of the ensureBallast function with the creation of the assertion scope.  Thus, if you are using the ballast.reserve() function, then you necessary assert that there is ballast space.

Note, that, as noted earlier in this bug, the problem comes from functions which are using the ballast space with fallible allocations wrapped by infallible functions.  As these fallible allocations are not using the JitAllocatorPolicy, they do not get their allocation function wrapped by an AutoAssertBallast scope.  This EnsureBallast class is made to enforce that.

Comment 16

[Hannes Verschore [:h4writer]]

Comment on attachment 8746146 [details] [diff] [review]
Assert that when do not allocate new chunks when using infallible allocation.

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

Thanks for the explanation and reasoning on IRC. It took me a while to understand how this works and how this would help.
Please ignore the I don't see no need for "TempAllocator::EnsureBallast".
Can you post an updated patch with the other changes I requested in this review and previous review?

::: js/src/jit/JitAllocPolicy.h
@@ +71,5 @@
>      {
>          return &lifoScope_.alloc();
>      }
>  
> +    class MOZ_STACK_CLASS EnsureBallast

https://developer.mozilla.org/en-US/docs/Mozilla/RAII_classes

@@ +81,5 @@
> +        EnsureBallast(TempAllocator& alloc)
> +            : assertBallast(alloc.lifoAlloc(), true),
> +            alloc_(alloc)
> +        {
> +        }

please style format correctly

Comment 17

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Register if the LifoAlloc is supposed to be infallible or not.

This patch adds a new flag to the LifoAlloc structure, which is used to assert
when we attempt to allocate a new chunk for the LifoAlloc.  This ensure that we
assert (in debug builds) if we attempt to allocate beyong the reserved space of
the ballast.

This is a nicer fix than the previous patch, as this does not require adding
an EnsureBallast class.  On the other hand, I did the following:

 - The LifoAllocScope restore the fact that we expect the LifoAlloc to be
   fallible. (we do not use any LifoAllocScope within IonMonkey)
 - Add a function to flag the LifoAlloc as an infallible allocator by
   default. This implies that any fallible allocation should be under an
   AutoFallibleScope, as done in the TempAllocator fallible allocation
   functions.

This solves the current issue as the LifoAlloc is marked as being infallible
even for TI fallibles allocations, thus no new chunk can be allocated even
under TI.  Only fallible allocations are allowed to fail with the simulated
OOM.

This also catches more OOM errors (~30) than the previous patch, as the
LifoAlloc is now considered infallible by default, and not only under scopes
which are calling ensureBallast.

Another good news, is that this patch can land after all the fixes are made,
which implies that we can fix the OOM as they appear, and not in one
strike.

Comment 18

[Jon Coppeard (:jonco)]

Comment on attachment 8754362 [details] [diff] [review]
part 1 - Register if the LifoAlloc is supposed to be infallible or not.

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

This looks good to me.

Comment 19

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure that we have enough ballast space IonMonkey compilation.

Comment 20

[Nicolas B. Pierron [:nbp]]

Attachment: Weaken infallible allocator assertion for jsapi-tests.

Comment 21

[Nicolas B. Pierron [:nbp]]

Attachment: Disable infallible allocator assertion for iregexp.

Comment 22

[Dan Gohman [:sunfish]]

Comment on attachment 8754479 [details] [diff] [review]
Weaken infallible allocator assertion for jsapi-tests.

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

Sounds right; it's a range analysis test, not a fallible allocator test.

Comment 24

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8746139 [details] [diff] [review]
Make it possible to use a placement-new with a fallible TempAllocator.

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

Now, that I uploaded the patch which is fixing most other issues, and added 4 new uses of this fallible() function.
Do you think there is still not enough uses, or should I explicit ensureBallast call in every other locations?

Comment 25

[Hannes Verschore [:h4writer]]

Comment on attachment 8754362 [details] [diff] [review]
part 1 - Register if the LifoAlloc is supposed to be infallible or not.

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

Just a small question, no obligations: can we call it "fallibleScope"? (instead of inInfallibleScope)
1) inIn => is quite annoying
2) the default behaviour is fallible. lets make that the default wording too...

Comment 26

[Hannes Verschore [:h4writer]]

Comment on attachment 8754478 [details] [diff] [review]
Ensure that we have enough ballast space IonMonkey compilation.

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

::: js/src/jit/Lowering.cpp
@@ +475,5 @@
>              LStackArgT* stack = new(alloc()) LStackArgT(argslot, arg->type(), useRegisterOrConstant(arg));
>              add(stack);
>          }
> +
> +        if (!alloc().ensureBallast())

Do:
return false;

@@ +491,5 @@
>      MOZ_ASSERT(call->getFunction()->type() == MIRType::Object);
>  
> +    // In case of oom, skip the rest of the allocations.
> +    if (!lowerCallArguments(call))
> +        return;

and do:

if (!lowerCallArguments(call)) {
    gen->abort("OOM: LIRGenerator::visitCall");
    return;
}

::: js/src/jit/Lowering.h
@@ +56,5 @@
>      void lowerShiftOp(JSOp op, MShiftInstruction* ins);
>      void lowerBinaryV(JSOp op, MBinaryInstruction* ins);
>      void definePhis();
>  
> +    bool lowerCallArguments(MCall* call);

MOZ_MUST_USE ?

::: js/src/jit/MacroAssembler.cpp
@@ +2442,5 @@
>      MoveOperand to(*this, arg);
>      if (from == to)
>          return;
>  
>      if (!enoughMemory_)

if (oom())

Comment 27

[Hannes Verschore [:h4writer]]

Comment on attachment 8746139 [details] [diff] [review]
Make it possible to use a placement-new with a fallible TempAllocator.

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

I don't see the need to add this code for just two exceptions... ?

Comment 28

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #27)
> Comment on attachment 8746139 [details] [diff] [review]
> Make it possible to use a placement-new with a fallible TempAllocator.
> 
> Review of attachment 8746139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see the need to add this code for just two exceptions... ?

s/^.*$/Okay/

Comment 29

[Nicolas B. Pierron [:nbp]]

I am setting the leave-open keyword, as this bug contains multiple patches, including some which are fixing OOM checks, and others which are adding assertions to raise issue about missing OOM checks.

I will flag each commit as being checked-in as we go.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b52620a1f87
Make it possible to use a placement-new with a fallible TempAllocator. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c9c54ef08f
Ensure that we have enough ballast space IonMonkey compilation. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cac3360b8c7
Weaken infallible allocator assertion for jsapi-tests. r=sunfish

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b52620a1f87
https://hg.mozilla.org/mozilla-central/rev/73c9c54ef08f
https://hg.mozilla.org/mozilla-central/rev/8cac3360b8c7

Comment 32

[Nicolas B. Pierron [:nbp]]

Attachment: bug1264948.patch

Requesting feedback from fuzzers on this patch, which increases the number of issues coming from IonBuilder LifoAlloc usage, and as such might produce a large number of fuzz-blockers.

This patch aggregates all the patches which are not landed yet.

Comment 33

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at js/src/ds/LifoAlloc.cpp:105
Build version: mozilla-central revision 92e0c73391e7+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize
Runtime options: --fuzzing-safe --thread-count=2 --disable-oom-functions --disable-oom-functions --baseline-eager --ion-eager

Testcase:

var N = 70 * 1000;
var x = build("&&")();
function build(operation) {
    var a = [];
    for (var i = 1; i != N - 1; ++i) a.push("f()");
    return new Function(a.join(operation));
}

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000c5e33f in js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7f2a851b8040, n=n@entry=1048576) at js/src/ds/LifoAlloc.cpp:105
#1  0x000000000087646b in js::LifoAlloc::allocImpl (this=0x7f2a851b8040, n=1048576) at js/src/ds/LifoAlloc.h:225
#2  0x0000000000b7392e in js::LifoAlloc::alloc (n=1048576, this=0x7f2a851b8040) at js/src/ds/LifoAlloc.h:285
#3  js::LifoAlloc::newArrayUninitialized<js::TemporaryTypeSet> (count=<optimized out>, this=<optimized out>) at js/src/ds/LifoAlloc.h:362
#4  js::TypeScript::FreezeTypeSets (constraints=0x7f2a848b9138, script=0x7f2a859bc640, pThisTypes=pThisTypes@entry=0x7f2a848b9390, pArgTypes=pArgTypes@entry=0x7f2a848b9398, pBytecodeTypes=pBytecodeTypes@entry=0x7f2a848b93a0) at js/src/vm/TypeInference.cpp:1113
#5  0x000000000063e902 in js::jit::IonBuilder::init (this=0x7f2a848b91d0) at js/src/jit/IonBuilder.cpp:769
#6  0x000000000067c788 in js::jit::IonBuilder::build (this=0x7f2a848b91d0) at js/src/jit/IonBuilder.cpp:800
#7  0x000000000068f1f4 in js::jit::IonCompile (cx=cx@entry=0x7f2a8ba1a400, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2198
#8  0x000000000068fc01 in js::jit::Compile (cx=cx@entry=0x7f2a8ba1a400, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2430
#9  0x000000000068fe06 in js::jit::CanEnter (cx=cx@entry=0x7f2a8ba1a400, state=...) at js/src/jit/Ion.cpp:2522
#10 0x0000000000a8ba19 in js::RunScript (cx=cx@entry=0x7f2a8ba1a400, state=...) at js/src/vm/Interpreter.cpp:374
#11 0x0000000000a8bd28 in js::InternalCallOrConstruct (cx=cx@entry=0x7f2a8ba1a400, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:470
#12 0x0000000000a8bf76 in InternalCall (cx=cx@entry=0x7f2a8ba1a400, args=...) at js/src/vm/Interpreter.cpp:497
#13 0x0000000000a8c09a in js::CallFromStack (cx=cx@entry=0x7f2a8ba1a400, args=...) at js/src/vm/Interpreter.cpp:503
#14 0x0000000000deaad9 in js::jit::DoCallFallback (cx=0x7f2a8ba1a400, frame=0x7ffe40cd1618, stub_=<optimized out>, argc=<optimized out>, vp=0x7ffe40cd15b0, res=...) at js/src/jit/BaselineIC.cpp:5979
#15 0x00007f2a8cfb4f2f in ?? ()
[...]
#26 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x200000	2097152
rcx	0x7f2a8bd9ca2d	139820711660077
rdx	0x0	0
rsi	0x7f2a8c06b770	139820714604400
rdi	0x7f2a8c06a540	139820714599744
rbp	0x7ffe40cd0b70	140729985600368
rsp	0x7ffe40cd0ab0	140729985600176
r8	0x7f2a8c06b770	139820714604400
r9	0x7f2a8d15c740	139820732368704
r10	0x0	0
r11	0x0	0
r12	0x7f2a848b9000	139820589092864
r13	0x7f2a851b8040	139820598526016
r14	0x100000	1048576
r15	0x0	0
rip	0xc5e33f <js::LifoAlloc::getOrCreateChunk(unsigned long)+847>
=> 0xc5e33f <js::LifoAlloc::getOrCreateChunk(unsigned long)+847>:	movl   $0x0,0x0
   0xc5e34a <js::LifoAlloc::getOrCreateChunk(unsigned long)+858>:	ud2

Comment 34

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at js/src/ds/LifoAlloc.cpp:105
Build version: mozilla-central revision 92e0c73391e7+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize
Runtime options: --fuzzing-safe --thread-count=2 --disable-oom-functions --disable-oom-functions --baseline-eager --ion-eager

Testcase:

var dbg = new g.Debugger(this);
if (typeof evalInFrame === 'function') {
    let x00, x01, x02, x03, x04, x05, x06, x07, x08, x09, x0a, x0b, x0c, x0d, x0e, x0f,
        xa0, xa1, xa2, xa3, xa4, xa5, xa6, xa7, xa8, xa9, xaa, xab, xac, xad, xae, xaf,
        xd0, xd1, xd2, xd3, xd4, xd5, xd6, xd7, xd8, xd9, xda, f, xdc, xdd, xde, xdf,
        xe0, xe1, xe2, xe3, xe4, xe5, xe6, xe7, xe8, xe9, xea, xeb, xec, xed, xee, xef,
        xf0, xf1, xf2, xf3, xf4, xf5, xf6, xf7, xf8, xf9, xfa, xfb, xfc, xfd, xfe, xff;
}

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000c5e33f in js::LifoAlloc::getOrCreateChunk (this=this@entry=0x7f3bb02fc840, n=n@entry=224) at js/src/ds/LifoAlloc.cpp:105
#1  0x00000000005ac933 in js::LifoAlloc::allocImpl (n=224, this=0x7f3bb02fc840) at js/src/ds/LifoAlloc.h:225
#2  js::LifoAlloc::allocInfallible (this=0x7f3bb02fc840, n=224) at js/src/ds/LifoAlloc.h:291
#3  0x000000000075762a in js::jit::TempAllocator::allocateInfallible (bytes=224, this=0x7f3bafda5020, this@entry=0x0) at js/src/jit/JitAllocPolicy.h:43
#4  js::jit::TempObject::operator new (alloc=..., nbytes=224) at js/src/jit/JitAllocPolicy.h:161
#5  js::jit::MPhi::New (alloc=..., resultType=js::jit::MIRType::Value) at js/src/jit/MIR.h:7514
#6  0x00000000007551cb in js::jit::MBasicBlock::addPredecessorPopN (this=0x7f3bafddf098, alloc=..., pred=0x7f3bafc16510, popped=popped@entry=0) at js/src/jit/MIRGraph.cpp:1198
#7  0x000000000075589c in js::jit::MBasicBlock::addPredecessor (this=<optimized out>, alloc=..., pred=<optimized out>) at js/src/jit/MIRGraph.cpp:1167
#8  0x0000000000657882 in js::jit::IonBuilder::processIfEnd (this=0x7f3bafda51d0, state=...) at js/src/jit/IonBuilder.cpp:2297
#9  0x00000000006664d1 in js::jit::IonBuilder::processCfgStack (this=this@entry=0x7f3bafda51d0) at js/src/jit/IonBuilder.cpp:2211
#10 0x000000000067bf01 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7f3bafda51d0) at js/src/jit/IonBuilder.cpp:1465
#11 0x000000000067cef5 in js::jit::IonBuilder::build (this=0x7f3bafda51d0) at js/src/jit/IonBuilder.cpp:918
#12 0x000000000068f1f4 in js::jit::IonCompile (cx=cx@entry=0x7f3bb301a400, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2198
#13 0x000000000068fc01 in js::jit::Compile (cx=cx@entry=0x7f3bb301a400, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2430
#14 0x000000000068fe06 in js::jit::CanEnter (cx=cx@entry=0x7f3bb301a400, state=...) at js/src/jit/Ion.cpp:2522
#15 0x0000000000a8ba19 in js::RunScript (cx=cx@entry=0x7f3bb301a400, state=...) at js/src/vm/Interpreter.cpp:374
#16 0x0000000000a8e2cb in js::ExecuteKernel (cx=cx@entry=0x7f3bb301a400, script=..., script@entry=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x7ffc637b3868) at js/src/vm/Interpreter.cpp:676
#17 0x0000000000a8e8a8 in js::Execute (cx=cx@entry=0x7f3bb301a400, script=..., script@entry=..., scopeChainArg=..., rval=rval@entry=0x7ffc637b3868) at js/src/vm/Interpreter.cpp:709
#18 0x00000000008acb2c in ExecuteScript (cx=cx@entry=0x7f3bb301a400, scope=scope@entry=..., script=script@entry=..., rval=rval@entry=0x7ffc637b3868) at js/src/jsapi.cpp:4427
#19 0x00000000008acd79 in JS_ExecuteScript (cx=cx@entry=0x7f3bb301a400, scriptArg=scriptArg@entry=..., rval=rval@entry=...) at js/src/jsapi.cpp:4453
#20 0x0000000000443b80 in runOffThreadScript (cx=cx@entry=0x7f3bb301a400, argc=<optimized out>, vp=0x7ffc637b3868) at js/src/shell/js.cpp:3928
#21 0x0000000000a949a1 in js::CallJSNative (cx=cx@entry=0x7f3bb301a400, native=0x443a60 <runOffThreadScript(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#22 0x0000000000a8bc29 in js::InternalCallOrConstruct (cx=cx@entry=0x7f3bb301a400, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:452
#23 0x0000000000a8bf76 in InternalCall (cx=cx@entry=0x7f3bb301a400, args=...) at js/src/vm/Interpreter.cpp:497
#24 0x0000000000a8c0ce in js::Call (cx=cx@entry=0x7f3bb301a400, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:516
#25 0x00000000009cc44b in js::Wrapper::call (this=this@entry=0x1c928a0 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7f3bb301a400, proxy=..., proxy@entry=..., args=...) at js/src/proxy/Wrapper.cpp:165
#26 0x00000000009b5da3 in js::CrossCompartmentWrapper::call (this=0x1c928a0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f3bb301a400, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:309
#27 0x00000000009b0803 in js::Proxy::call (cx=cx@entry=0x7f3bb301a400, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:399
#28 0x00000000009b0908 in js::proxy_Call (cx=cx@entry=0x7f3bb301a400, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:691
#29 0x0000000000a949a1 in js::CallJSNative (cx=cx@entry=0x7f3bb301a400, native=0x9b0870 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#30 0x0000000000a8be27 in js::InternalCallOrConstruct (cx=cx@entry=0x7f3bb301a400, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:440
#31 0x0000000000a8bf76 in InternalCall (cx=cx@entry=0x7f3bb301a400, args=...) at js/src/vm/Interpreter.cpp:497
#32 0x0000000000a8c09a in js::CallFromStack (cx=cx@entry=0x7f3bb301a400, args=...) at js/src/vm/Interpreter.cpp:503
#33 0x0000000000deaad9 in js::jit::DoCallFallback (cx=0x7f3bb301a400, frame=0x7ffc637b3f38, stub_=<optimized out>, argc=<optimized out>, vp=0x7ffc637b3ef8, res=...) at js/src/jit/BaselineIC.cpp:5979
#34 0x00007f3bb454ff2f in ?? ()
#35 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x8000	32768
rcx	0x7f3bb3337a2d	139894386293293
rdx	0x0	0
rsi	0x7f3bb3606770	139894389237616
rdi	0x7f3bb3605540	139894389232960
rbp	0x7ffc637b2c80	140721977502848
rsp	0x7ffc637b2bc0	140721977502656
r8	0x7f3bb3606770	139894389237616
r9	0x7f3bb46f7740	139894407001920
r10	0x0	0
r11	0x0	0
r12	0x7f3bafc2e000	139894328582144
r13	0x7f3bb02fc840	139894335719488
r14	0xe0	224
r15	0x0	0
rip	0xc5e33f <js::LifoAlloc::getOrCreateChunk(unsigned long)+847>
=> 0xc5e33f <js::LifoAlloc::getOrCreateChunk(unsigned long)+847>:	movl   $0x0,0x0
   0xc5e34a <js::LifoAlloc::getOrCreateChunk(unsigned long)+858>:	ud2

Comment 35

[Christian Holler (:decoder)]

The last two comments went to the wrong bug, please ignore them.

Comment 36

[Christian Holler (:decoder)]

Oh, in fact they didn't, this is the right bug. I need more coffee. Carry on ;D

Comment 37

[Nicolas B. Pierron [:nbp]]

Attachment: IonBuilder::init, reserve ballast space after freezing type sets.

The Type inference code properly handles OOM, but it can exhaust the ballast
space.  So we have to flag the section as fallible (as we check the error
code), and reserve extra ballast space.

Comment 38

[Nicolas B. Pierron [:nbp]]

Attachment: MBasicBlock::addPredecessor, check for OOMs when allocating Phi nodes.

When we have a lot of Phi nodes, the loop of addPrdecessor can exhaust the
ballast space.  Using fallible allocation of MPhi solves this issue.

Comment 39

[Hannes Verschore [:h4writer]]

Comment on attachment 8760383 [details] [diff] [review]
MBasicBlock::addPredecessor, check for OOMs when allocating Phi nodes.

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

::: js/src/jit/MIRGraph.cpp
@@ +1197,4 @@
>                  else
> +                    phi = MPhi::New(alloc.fallible());
> +                if (!phi)
> +                    return false;

This doesn't make sense to me?

MPhi::New itself is not fallible. Similar to every MDefinition::New by default is not fallible.
The issue here is that unbound looping and creating MPhi::New is fallible.

Instead of this patch, we should actually call "ensureBallast" in the loop

Comment 40

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #39)
> ::: js/src/jit/MIRGraph.cpp
> @@ +1197,4 @@
> >                  else
> > +                    phi = MPhi::New(alloc.fallible());
> > +                if (!phi)
> > +                    return false;
> 
> This doesn't make sense to me?
> 
> MPhi::New itself is not fallible. Similar to every MDefinition::New by
> default is not fallible.

It is, when called with alloc.fallible().  The Koenig lookup will make us select the newly add MPhi::New, as well as the "TempObject::operator new" which uses a fallible allocator.

I am also changing other MDefinition::New to be either infallible ::New(alloc), or fallible ::New(alloc.fallible()), as part of Bug 1278303 part 2.  The idea being that we can syntactically check for fallible allocation from the call site.

Note, *some* MFoo::New(alloc) are fallible.  These MFoo are doing things such as allocating unbounded arrays.  In such case we still have to check the returned pointer against nullptr.

My goal, as part of  Bug 1278303 would be to remove definition of ::New(alloc) when the allocation is fallible, thus using the type system to enforce use of ::New(alloc.fallible()), thus letting the developers & reviewers know that the result must be checked.

> The issue here is that unbound looping and creating MPhi::New is fallible.
> 
> Instead of this patch, we should actually call "ensureBallast" in the loop

As we use the fallible version of the allocator, the TempAllocator::allocate function is used, and ensureBallast is called after making the fallible allocation.

Comment 41

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8760383 [details] [diff] [review]
MBasicBlock::addPredecessor, check for OOMs when allocating Phi nodes.

(re-asking for review after comment 40)

Comment 42

[Hannes Verschore [:h4writer]]

Comment on attachment 8760383 [details] [diff] [review]
MBasicBlock::addPredecessor, check for OOMs when allocating Phi nodes.

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

Seems like I wasn't up to date with the newest plan on how to tackle this. Thanks for explaining. r+

Comment 43

[Nicolas B. Pierron [:nbp]]

Attachment: MBasicBlock::inherit, check for OOMs when allocating Phi nodes.

Comment 44

[Nicolas B. Pierron [:nbp]]

Attachment: IonBuilder::inlineArray, check for OOMs when creating array elements without resume points.

Comment 45

[Nicolas B. Pierron [:nbp]]

Attachment: IonBuilder::addOsrValueTypeBarrier, check for OOMs when unboxing OSR values.

Comment 46

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8754362 [details] [diff] [review]
part 1 - Register if the LifoAlloc is supposed to be infallible or not.

I will split this patch in 2 parts, by only moving the assertion into a second patch, because I need the AutoFallibleScope structure which is added in the patch for landing other fixes attached to this bug.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e54f664175
part 1 - Register if the LifoAlloc is supposed to be infallible or not. r=jonco,h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9f6e6c7d42
Disable infallible allocator assertion for iregexp. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf8fc5c0f34
IonBuilder::init, reserve ballast space after freezing type sets. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ee3d056695
MBasicBlock::addPredecessor, check for OOMs when allocating Phi nodes. r=h4writer

Comment 48

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1264948 and bug 1278303 for Static failures in Ion.cpp and undefined oomTest in bug1269756.js:

bug 1264948:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b44d6438cf58c12c198161654ab0048b7c3bb29
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77e4ad223d4a46b6fa85c45347ce96891206b4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d72428c4f435d1dabcdcdb0a4b64f4c21f9a3fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2527718052423d722383ca11059332a1cb42ac

bug 1278303:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4b01b4e40764d1468575bd1e5e65cccaec0461
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c1a37d417ef991f2f0649da657e9796f242289
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb6b1e8a6eeef6685cc82120f7e38efb8c78423
https://hg.mozilla.org/integration/mozilla-inbound/rev/a071095ecfdc80533cd602309516d9a048a31255

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=09359e9505b1fbc25003046fa5ed213bb5b5863b

Comment 49

[Christian Holler (:decoder)]

Comment on attachment 8759160 [details] [diff] [review]
bug1264948.patch

Clearing the old feedback? here. I tested this patch, found several issues and :nbp fixed all of them. If there's a new patch we should test, please let us know :)

Comment 50

[Christian Holler (:decoder)]

Cleared wrong flag, restoring.

Comment 51

[Hannes Verschore [:h4writer]]

Comment on attachment 8762070 [details] [diff] [review]
IonBuilder::addOsrValueTypeBarrier, check for OOMs when unboxing OSR values.

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

::: js/src/jit/IonBuilder.cpp
@@ +1289,5 @@
>  {
>      MInstruction*& def = *def_;
>      MBasicBlock* osrBlock = def->block();
> +    if (!alloc().ensureBallast())
> +        return false;

Can you put the ensureBallast in the for loop instead?

https://dxr.mozilla.org/mozilla-central/rev/ddb6b30149221f00eb5dd180530e9033093d4c2b/js/src/jit/IonBuilder.cpp#1404

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f3f077a1ec
part 1 - Register if the LifoAlloc is supposed to be infallible or not. r=jonco,h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceb7ec4fd334
Disable infallible allocator assertion for iregexp. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dfdec5ed99b
IonBuilder::init, reserve ballast space after freezing type sets. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e99a0d76e62
IonBuilder::init, fixup
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c123367921
MBasicBlock::addPredecessor, check for OOMs when allocating Phi nodes. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/46d50a87b3b5
MBasicBlock::inherit, check for OOMs when allocating Phi nodes. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/831077a22f58
IonBuilder::inlineArray, check for OOMs when creating array elements without resume points. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f37f6cde59
IonBuilder::addOsrValueTypeBarrier, check for OOMs when unboxing OSR values. r=h4writer

Comment 53

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Assert when we allocate new chunks using an infallible allocator.

Comment 54

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8762933 [details] [diff] [review]
part 2 - Assert when we allocate new chunks using an infallible allocator.

(already reviewed as part of attachment 8754362 [details] [diff] [review])

Comment 55

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for 'warning treated as error' build failure in JitAllocPolicy.h at least on Windows XP:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc371921454fce21f7a34952d555bb6ede873e3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1cbd40927beeb38a6917918e76a1e681ab6301c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8db1d19b779e6a32be61847ca909f7b74afc87f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ab2788d6bfab238c638a9ea21f98b798971060
https://hg.mozilla.org/integration/mozilla-inbound/rev/727cb8756008ac717ce0b04fdc0b9ca760d3c61d
https://hg.mozilla.org/integration/mozilla-inbound/rev/36147a153b1bbf803567e2ad210746ed0ce00713
https://hg.mozilla.org/integration/mozilla-inbound/rev/57aca488acc1a2e2e53accb1e65ebcd19fa4bfab
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1ff601b5614e582425b2ab4360d9a68f8c5008

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=87f37f6cde598e58168536e9907c00f035cc375c
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30184208&repo=mozilla-inbound
jit/JitAllocPolicy.h(157): error C2220: warning treated as error - no 'object' file generated

Comment 56

[Nicolas B. Pierron [:nbp]]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #55)
> Backed out for 'warning treated as error' build failure in JitAllocPolicy.h
> at least on Windows XP:

The problem comes from:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b52620a1f87
Make it possible to use a placement-new with a fallible TempAllocator. r=h4writer

Strangely, this patch is already landed and this code path is already used as well.  So I have no clue why the MSVC starts complaining about it today.  Anyhow, it seems that the proper way to define fallible allocators is to use "nothrow()" instead of "noexcept".

https://treeherder.mozilla.org/#/jobs?repo=try&revision=255b87a9f791bac7a0dc898dfed532135c1c3e71

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e07ea79523b
Compily with Windows lack of support for noexcept keyword. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b5c2c00f20a
part 1 - Register if the LifoAlloc is supposed to be infallible or not. r=jonco,h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d9acf640f5
Disable infallible allocator assertion for iregexp. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/42b04c4bae8f
IonBuilder::init, reserve ballast space after freezing type sets. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/88336c73abae
MBasicBlock::addPredecessor, check for OOMs when allocating Phi nodes. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1a20de30f9
MBasicBlock::inherit, check for OOMs when allocating Phi nodes. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a5f51c271b
IonBuilder::inlineArray, check for OOMs when creating array elements without resume points. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5f00905c50
IonBuilder::addOsrValueTypeBarrier, check for OOMs when unboxing OSR values. r=h4writer

Comment 58

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: testcase that is possibly exposed by patch in comment 53

The patch in bug 1264948 comment 53 seems to cause the following symptom to show up:

$ ./js-dbg-64-dm-clang-darwin-1264948-c53-563e5fb3ca66-629faa5d9254 --fuzzing-safe --ion-eager testcase.js
Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at /Users/skywalker/trees/mozilla-inbound/js/src/ds/LifoAlloc.cpp:105
Segmentation fault: 11

Tested on mozilla-inbound rev 629faa5d9254.

Comment 59

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack for testcase in comment 58

Comment 60

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Also, I didn't seem to notice a large number of fuzzblockers for this patch onwards. If try is green for the upcoming fix, I'd think we can probably land the patch containing the assertion.

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e07ea79523b
https://hg.mozilla.org/mozilla-central/rev/4b5c2c00f20a
https://hg.mozilla.org/mozilla-central/rev/02d9acf640f5
https://hg.mozilla.org/mozilla-central/rev/42b04c4bae8f
https://hg.mozilla.org/mozilla-central/rev/88336c73abae
https://hg.mozilla.org/mozilla-central/rev/8b1a20de30f9
https://hg.mozilla.org/mozilla-central/rev/a7a5f51c271b
https://hg.mozilla.org/mozilla-central/rev/ab5f00905c50

Comment 62

[Nicolas B. Pierron [:nbp]]

Attachment: Check for OOM when linking all break keywords of switch statements.

Comment 63

[Hannes Verschore [:h4writer]]

Comment on attachment 8763846 [details] [diff] [review]
Check for OOM when linking all break keywords of switch statements.

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

::: js/src/jit/MIR.h
@@ +2912,5 @@
>  
>    public:
>      INSTRUCTION_HEADER(Goto)
>      static MGoto* New(TempAllocator& alloc, MBasicBlock* target);
> +    static MGoto* New(TempAllocator::Fallible alloc, MBasicBlock* target);

I thought those were auto created? Why not here?

Comment 64

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #63)
> ::: js/src/jit/MIR.h
> >      static MGoto* New(TempAllocator& alloc, MBasicBlock* target);
> > +    static MGoto* New(TempAllocator::Fallible alloc, MBasicBlock* target);
> 
> I thought those were auto created? Why not here?

Currently the MGoto::New function has an assertion which ensure that we do not create a MGoto with a non-existing successor.  On the other hand, this assertion does not exists in the MGoto::NewAsmJS version of it, thus we cannot move this assertion to the constructor.  Thus, we cannot use the TRIVIAL_NEW_WRAPPER either.

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6ff2e767e3
Check for OOM when linking all break keywords of switch statements. r=h4writer

Comment 66

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7f6ff2e767e3

Comment 67

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4146909cad30c1ee3edf3f35f69c1e86ce471a5

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/977e5fd31b3d
part 2 - Assert when we allocate new chunks using an infallible allocator. r=jonco,h4writer

Comment 69

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

https://hg.mozilla.org/mozilla-central/rev/977e5fd31b3d

Comment 70

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'js::TypeSet::addType':
 - nightly(version 50):1 crash from 2016-06-06.
 - aurora (version 49):7 crashes from 2016-06-07.
 - beta   (version 48):156 crashes from 2016-06-06.
 - release(version 47):145 crashes from 2016-05-31.
 - esr    (version 45):6 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       1       0       0       0       0
 - aurora        0       5       0       1       0       1       0
 - beta         15      15      26      35      18      20      24
 - release      21      26      34      22      15       9      10
 - esr           1       0       2       1       0       2       0

Affected platforms: Windows, Linux