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

RESOLVED FIXED in Firefox 50

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks 2 bugs, {assertion, testcase})

Trunk
mozilla50
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 affected, firefox49 affected, firefox-esr45 affected, firefox50 fixed)

Details

(Whiteboard: [jsbugmon:update,ignore], crash signature)

Attachments

(14 attachments, 3 obsolete attachments)

3.08 KB, patch
h4writer
: review+
nbp
: checkin+
Details | Diff | Splinter Review
9.00 KB, patch
h4writer
: review+
jonco
: review+
nbp
: checkin+
Details | Diff | Splinter Review
23.70 KB, patch
h4writer
: review+
nbp
: checkin+
Details | Diff | Splinter Review
2.34 KB, patch
sunfish
: review+
nbp
: checkin+
Details | Diff | Splinter Review
1.24 KB, patch
bhackett
: review+
nbp
: checkin+
Details | Diff | Splinter Review
1.71 KB, patch
h4writer
: review+
nbp
: checkin+
Details | Diff | Splinter Review
2.15 KB, patch
h4writer
: review+
nbp
: checkin+
Details | Diff | Splinter Review
2.01 KB, patch
h4writer
: review+
nbp
: checkin+
Details | Diff | Splinter Review
1.17 KB, patch
h4writer
: review+
nbp
: checkin+
Details | Diff | Splinter Review
1020 bytes, patch
h4writer
: review+
nbp
: checkin+
Details | Diff | Splinter Review
966 bytes, patch
nbp
: review+
Details | Diff | Splinter Review
35.03 KB, text/plain
Details
6.45 KB, text/plain
Details
2.28 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
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)

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

3 years ago
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
Could be related to the bugs mentioned in the regression window in comment 1, ni? nbp.
Flags: needinfo?(nicolas.b.pierron)
Assignee

Comment 3

3 years ago
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.
Assignee

Comment 4

3 years ago
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.
Assignee

Updated

3 years ago
Flags: needinfo?(nicolas.b.pierron)
Attachment #8743230 - Flags: review?(bbouvier)
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...
Attachment #8743230 - Flags: review?(bbouvier)
Assignee

Comment 6

3 years ago
This modification is made to remove 2 explicit calls to ensureBallast, which
is already called by the fallible allocator.
Attachment #8746139 - Flags: review?(hv1989)
Assignee

Comment 7

3 years ago
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.
Attachment #8746146 - Flags: review?(jcoppeard)
Attachment #8746146 - Flags: review?(hv1989)
Assignee

Updated

3 years ago
Attachment #8743230 - Attachment is obsolete: true
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.
Flags: needinfo?(nicolas.b.pierron)
Assignee

Comment 9

3 years ago
(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
Flags: needinfo?(nicolas.b.pierron)
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.
Attachment #8746146 - Flags: review?(jcoppeard) → review+

Updated

3 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]

Comment 11

3 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 77cead2cd203).
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
Attachment #8746146 - Flags: review?(hv1989)
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... ?
Attachment #8746139 - Flags: review?(hv1989)
Assignee

Comment 14

3 years ago
(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.
Assignee

Comment 15

3 years ago
(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 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
Assignee

Comment 17

3 years ago
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.
Attachment #8754362 - Flags: review?(jcoppeard)
Attachment #8754362 - Flags: review?(hv1989)
Assignee

Updated

3 years ago
Attachment #8746146 - Attachment is obsolete: true
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.
Attachment #8754362 - Flags: review?(jcoppeard) → review+
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.
Attachment #8754479 - Flags: review?(sunfish) → review+
Attachment #8754480 - Flags: review?(bhackett1024) → review+
Assignee

Updated

3 years ago
Duplicate of this bug: 1269756
Assignee

Comment 24

3 years ago
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?
Attachment #8746139 - Flags: review?(hv1989)
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...
Attachment #8754362 - Flags: review?(hv1989) → review+
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())
Attachment #8754478 - Flags: review?(hv1989) → review+
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... ?
Attachment #8746139 - Flags: review?(hv1989) → review+
(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/
Assignee

Comment 29

3 years ago
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.
Keywords: leave-open

Comment 30

3 years ago
Pushed by npierron@mozilla.com:
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
Assignee

Updated

3 years ago
Attachment #8746139 - Flags: checkin+
Assignee

Updated

3 years ago
Attachment #8754478 - Flags: checkin+
Assignee

Updated

3 years ago
Attachment #8754479 - Flags: checkin+
Assignee

Comment 32

3 years ago
Posted patch bug1264948.patch (obsolete) — Splinter Review
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.
Attachment #8759160 - Flags: feedback?(gary)
Attachment #8759160 - Flags: feedback?(choller)
Reporter

Comment 33

3 years ago
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
Reporter

Comment 34

3 years ago
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
Reporter

Comment 35

3 years ago
The last two comments went to the wrong bug, please ignore them.
Reporter

Comment 36

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

Comment 37

3 years ago
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.
Attachment #8760382 - Flags: review?(hv1989)
Assignee

Comment 38

3 years ago
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.
Attachment #8760383 - Flags: review?(hv1989)
Attachment #8760382 - Flags: review?(hv1989) → review+
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
Attachment #8760383 - Flags: review?(hv1989)
Assignee

Comment 40

3 years ago
(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.
Assignee

Comment 41

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

(re-asking for review after comment 40)
Attachment #8760383 - Flags: review?(hv1989)
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+
Attachment #8760383 - Flags: review?(hv1989) → review+
Assignee

Comment 46

3 years ago
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

3 years ago
Pushed by npierron@mozilla.com:
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
Reporter

Comment 49

3 years ago
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 :)
Flags: needinfo?(nicolas.b.pierron)
Attachment #8759160 - Flags: feedback?(choller)
Reporter

Comment 50

3 years ago
Cleared wrong flag, restoring.
Flags: needinfo?(nicolas.b.pierron)
Attachment #8762068 - Flags: review?(hv1989) → review+
Attachment #8762069 - Flags: review?(hv1989) → review+
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
Attachment #8762070 - Flags: review?(hv1989) → review+
Attachment #8759160 - Flags: feedback?(gary)

Comment 52

3 years ago
Pushed by npierron@mozilla.com:
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
Assignee

Comment 54

3 years ago
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])
Flags: needinfo?(nicolas.b.pierron)
Attachment #8762933 - Flags: review+
Depends on: 1280329
Assignee

Comment 56

3 years ago
(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
Flags: needinfo?(nicolas.b.pierron)

Comment 57

3 years ago
Pushed by npierron@mozilla.com:
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
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.
Attachment #8762933 - Flags: feedback?(gary) → feedback-
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.
Assignee

Updated

3 years ago
Attachment #8754362 - Attachment description: Assert when we allocate new chunks using an infallible allocator. → part 1 - Register if the LifoAlloc is supposed to be infallible or not.
Attachment #8754362 - Flags: checkin+
Assignee

Updated

3 years ago
Attachment #8754480 - Flags: checkin+
Attachment #8760382 - Flags: checkin+
Assignee

Updated

3 years ago
Attachment #8760383 - Flags: checkin+
Attachment #8762068 - Flags: checkin+
Assignee

Updated

3 years ago
Attachment #8762069 - Flags: checkin+
Attachment #8762070 - Flags: checkin+
Assignee

Updated

3 years ago
Attachment #8759160 - Attachment is obsolete: true
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?
Attachment #8763846 - Flags: review?(hv1989) → review+
Assignee

Comment 64

3 years ago
(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

3 years ago
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6ff2e767e3
Check for OOM when linking all break keywords of switch statements. r=h4writer
Assignee

Updated

3 years ago
Keywords: leave-open

Comment 68

3 years ago
Pushed by npierron@mozilla.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/977e5fd31b3d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee

Updated

3 years ago
Depends on: 1285237
Assignee

Updated

3 years ago
Depends on: 1285217
Assignee

Updated

3 years ago
Depends on: 1285218
Depends on: 1286462
Assignee

Updated

3 years ago
Depends on: 1287086
Depends on: 1287416
Depends on: 1289025
Assignee

Updated

3 years ago
Depends on: 1289184
Depends on: 1289926
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
Assignee

Updated

3 years ago
Depends on: 1292666
Depends on: 1298139
Assignee

Updated

3 years ago
Depends on: 1296667
Assignee

Updated

3 years ago
Depends on: 1299007
Assignee

Updated

3 years ago
Depends on: 1287411
Assignee

Updated

3 years ago
Depends on: 1313869
Assignee: nobody → nicolas.b.pierron
You need to log in before you can comment on or make changes to this bug.