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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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
g.eval("Debugger(parent).onExceptionUnwind=(function(){})")
function check(val) {}
function getTestContent() {
  yield "hello";
  yield 2+3;
  yield 12;
  doubleref.ref2 = obj;
}
evaluate("for (var obj of new getTestContent) check(obj);");



Backtrace:

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>

Comment 1

[Nicolas B. Pierron [:nbp]]

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.=

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Don't try to read the result object when doing in-place debug mode bailout in a for-of loop.

Comment 3

[Shu-yu Guo [:shu]]

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
  -----   --
  main:
  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 iterator.next() 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.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Initialize the MachineState with safe-bad pointers.

This patch does not fix this issue, but prevent us from using pointers which
are uninitialized.

Comment 5

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:
Crashes.

Comment 6

[jen strickland :jenstrickland]

Tracking for 39+ because, well, crashes.

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1298c13f4181

(leave-open for shu's patch)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1298c13f4181

Comment 9

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5e450b2dbade

Comment 11

[Nicolas B. Pierron [:nbp]]

Shu, do you need somebody else to checkin the patch?

Comment 12

[Shu-yu Guo [: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.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 14

[Shu-yu Guo [:shu]]

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.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5578b2096e4
https://hg.mozilla.org/releases/mozilla-aurora/rev/692922cc239c