If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: shu)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla40
x86_64
Linux
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39+ fixed, firefox40 fixed)

Details

(Whiteboard: [jsbugmon:], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 2

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

Updated

3 years ago
Attachment #8587064 - Flags: review?(jdemooij)
(Assignee)

Comment 3

3 years ago
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.
Flags: needinfo?(nicolas.b.pierron)
Blocks: 1143194
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Created attachment 8587353 [details] [diff] [review]
Initialize the MachineState with safe-bad pointers.

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]:
Crashes.
status-firefox39: --- → affected
tracking-firefox39: --- → ?
tracking-firefox40: --- → ?

Updated

3 years ago
Attachment #8587064 - Flags: review?(jdemooij) → review+

Updated

3 years ago
Attachment #8587353 - Flags: review?(jdemooij) → review+
Tracking for 39+ because, well, crashes.
tracking-firefox39: ? → +
tracking-firefox40: ? → +
https://hg.mozilla.org/integration/mozilla-inbound/rev/1298c13f4181

(leave-open for shu's patch)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/1298c13f4181
(Reporter)

Updated

3 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:]
(Reporter)

Comment 9

3 years ago
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
https://hg.mozilla.org/mozilla-central/rev/5e450b2dbade
Attachment #8587353 - Flags: checkin+
Shu, do you need somebody else to checkin the patch?
Flags: needinfo?(shu)
(Assignee)

Comment 12

3 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 3 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?
status-firefox40: affected → fixed
tracking-firefox40: + → ---
Flags: needinfo?(shu)
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 14

3 years ago
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
https://hg.mozilla.org/releases/mozilla-aurora/rev/a5578b2096e4
https://hg.mozilla.org/releases/mozilla-aurora/rev/692922cc239c
status-firefox39: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.