Closed Bug 1149510 Opened 9 years ago Closed 9 years ago

Crash [@ js::jit::SnapshotIterator::allocationValue] with Debugger


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox39 + fixed
firefox40 --- fixed


(Reporter: decoder, Assigned: shu)



(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:])

Crash Data


(2 files)

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

g = newGlobal();
g.parent = this
function check(val) {}
function getTestContent() {
  yield "hello";
  yield 2+3;
  yield 12;
  doubleref.ref2 = obj;
evaluate("for (var obj of new getTestContent) check(obj);");


Program received signal SIGSEGV, Segmentation fault.
js::jit::SnapshotIterator::allocationValue (this=this@entry=0x7fffffffa280, alloc=..., rm=rm@entry=js::jit::SnapshotIterator::RM_Normal) at js/src/jit/JitFrames.cpp:1940
#0  js::jit::SnapshotIterator::allocationValue (this=this@entry=0x7fffffffa280, alloc=..., rm=rm@entry=js::jit::SnapshotIterator::RM_Normal) at js/src/jit/JitFrames.cpp:1940
#1  0x000000000087632c in js::jit::SnapshotIterator::read (this=0x7fffffffa280) at js/src/jit/JitFrameIterator.h:542
#2  0x0000000000848491 in InitFromBailout (ionScript=<optimized out>, excInfo=<optimized out>, callPC=<synthetic pointer>, nextCallee=..., startFrameFormals=..., builder=..., invalidate=true, iter=..., script=..., fun=..., callerPC=0x7ffff6926d17 ":", cx=0x7ffff691b4e0, caller=...) at js/src/jit/BaselineBailouts.cpp:912
#3  js::jit::BailoutIonToBaseline (cx=cx@entry=0x7ffff691b4e0, activation=<optimized out>, iter=..., invalidate=invalidate@entry=true, bailoutInfo=bailoutInfo@entry=0x7fffffffa600, excInfo=excInfo@entry=0x7fffffffaac0) at js/src/jit/BaselineBailouts.cpp:1524
#4  0x000000000084b2fe in js::jit::ExceptionHandlerBailout (cx=cx@entry=0x7ffff691b4e0, frame=..., rfe=rfe@entry=0x7fffffffb4b8, excInfo=..., overrecursed=overrecursed@entry=0x7fffffffa9d0) at js/src/jit/Bailouts.cpp:210
#5  0x00000000008bcc0b in HandleExceptionIon (overrecursed=0x7fffffffa9d0, rfe=0x7fffffffb4b8, frame=..., cx=0x7ffff691b4e0) at js/src/jit/JitFrames.cpp:427
#6  js::jit::HandleException (rfe=0x7fffffffb4b8) at js/src/jit/JitFrames.cpp:784
#7  0x00007ffff7fe8608 in ?? ()
#25 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffff9e30	140737488330288
rcx	0x8aedd8	9104856
rdx	0xffffffffffc11e88	-4120952
rsi	0x7fffffff9e30	140737488330288
rdi	0x6	6
rbp	0x7fffffff9e20	140737488330272
rsp	0x7fffffff9de0	140737488330208
r8	0x7ffff5042412	140737304077330
r9	0xfffc7ffff525a1a0	-985162600570464
r10	0x0	0
r11	0x0	0
r12	0x7fffffffa280	140737488331392
r13	0x7fffffffa120	140737488331040
r14	0x7fffffffa280	140737488331392
r15	0x7ffff691b4e0	140737330132192
rip	0x8aedff <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+559>
=> 0x8aedff <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+559>:	mov    (%rax),%rax
   0x8aee02 <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+562>:	jmpq   0x8aec80 <js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod)+176>
Flags: needinfo?(nicolas.b.pierron)
I am investigating this issue, but so far I am stuck and I am trying to generate more spew and add more release assertions to figure out what is happening here.

It seems that we have a Safepoint with no live registers, while the snapshots expects to read from a register.=
Attachment #8587064 - Flags: review?(jdemooij)
This is a regression from bug 1143194.

The problem is that for-of trynotes have a stackDepth of 2, which is supposed to mean that there are always 2 values live on the stack (the iterator and the result object) the in-place debug mode can read them out.

But consider the disassembly of an empty for-of loop:

  loc     op
  -----   --
  00000:  newarray 0
  00004:  dup
  00005:  symbol 0
  00007:  callelem
  00008:  swap
  00009:  call 0
  00012:  undefined
  00013:  goto 30 (+17)
  00018:  loophead
  00019:  dup
  00020:  getprop "value"
  00025:  setlocal 0
  00029:  pop
  00030:  loopentry 129
  00032:  pop                        <-- old result object popped
  00033:  dup
  00034:  dup
  00035:  callprop "next"            <-- new result object pushed
                                         fuzz test triggers bailout here
  00040:  swap
  00041:  call 0
  00044:  dup
  00045:  getprop "done"
  00050:  ifeq 18 (-32)
  00055:  popn 2
  00058:  retrval

  Exception table:
  kind      stack    start      end
   for-of       2       18       55

The problem is that 00041 above is what triggers the in-place bailout. Since it's the call that's throwing, no result has been pushed and the snapshot cannot read out the result, *even though stackDepth is 2*.

I think the right fix to just ignore the result object. The only reason we read out live stack values for for-in and for-of loops is to close the iterator, so I think we can just ignore the result object.
Flags: needinfo?(nicolas.b.pierron)
Blocks: 1143194
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
This patch does not fix this issue, but prevent us from using pointers which
are uninitialized.
Attachment #8587353 - Flags: review?(jdemooij)
[Tracking Requested - why for this release]:
Attachment #8587064 - Flags: review?(jdemooij) → review+
Attachment #8587353 - Flags: review?(jdemooij) → review+
Tracking for 39+ because, well, crashes.
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Attachment #8587353 - Flags: checkin+
Shu, do you need somebody else to checkin the patch?
Flags: needinfo?(shu)
(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> Shu, do you need somebody else to checkin the patch?

It's been checked in and merged to central already, see comment 10. Clearing leave-open.
Closed: 9 years ago
Flags: needinfo?(shu)
Keywords: leave-open
Resolution: --- → FIXED
This seems to affect 39 as well. 
Shu or nbp, can you request uplift to aurora? 
Are you sure that this affects 39 and will be applicable?
Flags: needinfo?(shu)
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8587064 [details] [diff] [review]
Don't try to read the result object when doing in-place debug mode bailout in a for-of loop.

Approval Request Comment
[Feature/regressing bug #]: 1143194
[User impact if declined]: Rare crashes when using debugger.
[Describe test coverage new/current, TreeHerder]: On m-c
[Risks and why]: Low, no user-visible changes, bug fix.
[String/UUID change made/needed]: None.
Flags: needinfo?(shu)
Attachment #8587064 - Flags: approval-mozilla-aurora?
Comment on attachment 8587064 [details] [diff] [review]
Don't try to read the result object when doing in-place debug mode bailout in a for-of loop.

Approving for uplift to aurora as it has had time on m-c and seems low risk.
Attachment #8587064 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → shu
Flags: needinfo?(nicolas.b.pierron)
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.