The following testcase crashes on mozilla-central revision 10ea10d9993c (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads): "use strict"; for (var j = 0; j < 34; ++j) { for (var k = 0; k < 30; ++k) { try { (function (x) { (x ? 0 : ([y] = [])); })(); } catch (e) {} } } Backtrace: #0 0x00000000007461b6 in js::jit::RInstructionResults::operator[] (index=0, this=0x0) at js/src/jit/JitFrames.cpp:1720 #1 js::jit::SnapshotIterator::fromInstructionResult (this=0x7fffde16dc20, index=0) at js/src/jit/JitFrames.cpp:2222 #2 0x00000000007462c2 in js::jit::SnapshotIterator::allocationValue (this=this@entry=0x7fffde16dc20, alloc=..., rm=rm@entry=js::jit::SnapshotIterator::RM_Normal) at js/src/jit/JitFrames.cpp:1947 #3 0x000000000074ad09 in js::jit::SnapshotIterator::read (this=0x7fffde16dc20) at js/src/jit/JitFrameIterator.h:540 #4 js::jit::CloseLiveIteratorIon (tn=<optimized out>, frame=..., cx=0x7f5886075000) at js/src/jit/JitFrames.cpp:351 #5 js::jit::HandleExceptionIon (overrecursed=0x7fffde16da5f, rfe=0x7fffde16e0b8, frame=..., cx=0x7f5886075000) at js/src/jit/JitFrames.cpp:449 #6 js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:873 /snip For detailed crash information, see attachment. Setting s-s because a recently-fixed similar bug 1342049 was marked s-s.

autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/df2d145e4c40 user: Tom Schuster date: Wed Apr 05 20:35:01 2017 +0200 summary: Bug 1353170 - Implement scalar replacement for NewArrayIterator. r=jandem Tom, is bug 1353170 a likely regressor?

Yes, I think I know what's going on. CloseLiveIteratorIon doesn't expect the iterator object to be a recover instruction, i.e. eliminated. I am relatively sure this could be triggered before if somebody took the time to write their own Iterator that would be eliminated by scalar replacement.

This fixes the crash and seems to correctly recover the object. I am just not sure if the way I constructed MaybeReadFallback makes sense.

Comment on attachment 8855930 [details] [diff] [review] POC Review of attachment 8855930 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense, but I'm curious about the v.isObject check. Also, if the iterator closing is not observable for the array iterator, maybe we could assert in debug builds and just return? ::: js/src/jit/JitFrames.cpp @@ +349,5 @@ > si.skip(); > > + MaybeReadFallback recover(cx, cx->activation()->asJit(), &frame.frame(), MaybeReadFallback::Fallback_DoNothing); > + Value v = si.maybeRead(recover); > + if (!v.isObject()) When is v.isObject() false?

Comment on attachment 8855930 [details] [diff] [review] POC Review of attachment 8855930 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/JitFrames.cpp @@ +349,5 @@ > si.skip(); > > + MaybeReadFallback recover(cx, cx->activation()->asJit(), &frame.frame(), MaybeReadFallback::Fallback_DoNothing); > + Value v = si.maybeRead(recover); > + if (!v.isObject()) Actually it shouldn't be possible. canRecoverResults is always true when we have a context.

What should we assert for array iterator? A user could probably add their own "return" method to the array iterator prototype anyway. Oh and what fallback behavior should we use: Fallback_DoNothing or Invalidate?

Is this just a null deref? We could unhide it then.

I'm guessing Jan might be able to answer comment 7...

(In reply to Andrew McCreight [:mccr8] from comment #8) > Is this just a null deref? We could unhide it then. Yes, this is always a null-deref because the instructionResult_ is initialized to null[1,4,5] and the RInstructionResults[2] given as argument to the operator[] [3] would always be null, as it is not initialized. (In reply to Jan de Mooij [:jandem] from comment #5) > > + MaybeReadFallback recover(cx, cx->activation()->asJit(), &frame.frame(), MaybeReadFallback::Fallback_DoNothing); > > + Value v = si.maybeRead(recover); > > + if (!v.isObject()) > > When is v.isObject() false? At least not from the recover instruction mechanism, In case of a failure[5], we abort with an un-handled oom[6]. [1] http://searchfox.org/mozilla-central/source/js/src/jit/JitFrames.cpp#1743,1752 [2] http://searchfox.org/mozilla-central/source/js/src/jit/JitFrames.cpp#2219-2224 [3] http://searchfox.org/mozilla-central/source/js/src/jit/JitFrames.cpp#1718 [4] http://searchfox.org/mozilla-central/source/js/src/jit/JitFrames.cpp#2165 [5] http://searchfox.org/mozilla-central/source/js/src/jit/JitFrames.cpp#2193 [6] http://searchfox.org/mozilla-central/source/js/src/jit/JitFrames.cpp#1981-1982

Okay let's get this finished. Is this patch okay like this or should I change Fallback_DoNothing to Fallback_Invalidate?

Fallback_DoNothing should be fine as we are under HandleException, which implies that we are not going to resume the execution of the frame from which we recover the instruction, thus we are not going to diverge from what the execution should have been. Fallback_Invalidate is used to prevent the diverging executions (i-e getting 2 different allocations for what is supposed to be the same object) by invalidating the code, thus forcing a bailout with the same information as the one recovered while being on an inner function (a different frame).

Comment on attachment 8861118 [details] [diff] [review] Fix Review of attachment 8861118 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay.

Oh, if this is not a sec-high/crit, can we add a testcase?

Not critical, I landed with a testcase and we should unrestrict this bug. https://hg.mozilla.org/integration/mozilla-inbound/rev/34f2d83ee773