1331444 - Crash [@ JSObject::getClass] or Crash [@ js::CloseIterator] Assertion failure: isObject(), at dist/include/js/Value.h:642

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

symbols = [Symbol];
for (comparator of[, ])
  for (a of symbols)
    for (;;) 
      expect;



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000007c5739 in JSObject::getClass (this=<optimized out>) at js/src/jsobj.h:127
#0  0x00000000007c5739 in JSObject::getClass (this=<optimized out>) at js/src/jsobj.h:127
#1  JSObject::getOpsGetProperty (this=0xe) at js/src/jsobj.h:139
#2  js::GetProperty (vp=..., id=..., receiver=..., obj=..., cx=0x7ffff695f000) at js/src/vm/NativeObject.h:1508
#3  js::GetProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=0x7ffff695f000) at js/src/jsobj.h:848
#4  js::GetProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=0x7ffff695f000) at js/src/jsobj.h:864
#5  js::IteratorCloseForException (cx=cx@entry=0x7ffff695f000, obj=obj@entry=...) at js/src/jsiter.cpp:1254
#6  0x00000000005f2c9c in js::jit::CloseLiveIteratorIon (tn=<optimized out>, frame=..., cx=0x7ffff695f000) at js/src/jit/JitFrames.cpp:368
#7  js::jit::HandleExceptionIon (overrecursed=0x7fffffffcb0f, rfe=0x7fffffffd0e8, frame=..., cx=0x7ffff695f000) at js/src/jit/JitFrames.cpp:449
#8  js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:853
#9  0x00007ffff7e3b15b in ?? ()
#10 0x0000000000000000 in ?? ()
rax	0xe	14
rbx	0x7ffff695f000	140737330409472
rcx	0x7fffffffc9c0	140737488341440
rdx	0x7fffffffc9a0	140737488341408
rsi	0x7ffff695f000	140737330409472
rdi	0x7fffffffc9d0	140737488341456
rbp	0x7fffffffca80	140737488341632
rsp	0x7fffffffc950	140737488341328
r8	0x7fffffffc980	140737488341376
r9	0x7fffffffd2a8	140737488343720
r10	0x7fffffffc990	140737488341392
r11	0x0	0
r12	0x7fffffffc970	140737488341360
r13	0x7fffffffcbd0	140737488341968
r14	0x7fffffffc9b0	140737488341424
r15	0x7ffff69e7a38	140737330969144
rip	0x7c5739 <js::IteratorCloseForException(JSContext*, JS::Handle<JSObject*>)+313>
=> 0x7c5739 <js::IteratorCloseForException(JSContext*, JS::Handle<JSObject*>)+313>:	mov    (%rax),%rax
   0x7c573c <js::IteratorCloseForException(JSContext*, JS::Handle<JSObject*>)+316>:	mov    (%rax),%rax

Comment 1

[Christian Holler (:decoder)]

This bug seems to crash in multiple ways, marking fuzzblocker.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Teach Ion about observable stack slots.

This is unfortunate. The bug is that the on-stack iterator object was being
optimized out by Ion since it's only used by the exception unwinder to call
IteratorClose.

I had to teach MResumePoint about stack slots that are observable due to the
exception unwinder and I'm afraid it might regress Ion compilation times
(haven't benchmarked). Jan, what do you think?

Comment 3

[Jan de Mooij [:jandem]]

Is it possible to use the IonBuilder::processIterators mechanism here somehow? I think it was added a long time ago to solve a similar problem with for-in loops, bug 759213.

Also, when we have a try block, we have some code to make EliminateDeadResumePointOperands and phi elimination play nice. Maybe we could do something similar here for now? See MIRGraph::hasTryBlock.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Is it possible to use the IonBuilder::processIterators mechanism here
> somehow? I think it was added a long time ago to solve a similar problem
> with for-in loops, bug 759213.

Maybe we could add a no-op JSOP to communicate to IonBuilder what the iterator slot is that we have to keep alive? Then we could just append it to iterators_, similar to what we do for JSOP_ITER.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Keep iterators alive in Ion in for-of loops for IteratorClose due to exceptions.

Simply appending the SSA value of JSOP_CALLITER to iterators_ seems to work.
Thanks for the suggestion.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20170114094823" and the hash "5ce3882eec21be3a70e4afc050959ca2f76bfa76".
The "bad" changeset has the timestamp "20170114144423" and the hash "142dbb4bffc04efdb358df11293079d00af693fc".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5ce3882eec21be3a70e4afc050959ca2f76bfa76&tochange=142dbb4bffc04efdb358df11293079d00af693fc

Guessing this is related to bug 1147371.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8828144 [details] [diff] [review]
Keep iterators alive in Ion in for-of loops for IteratorClose due to exceptions.

Review of attachment 8828144 [details] [diff] [review]:
-----------------------------------------------------------------

Glad it works.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Keep iterators alive in Ion in for-of loops for IteratorClose due to exceptions.

I ran into some assertions where some of the iterator values were phis, and
iterators_ is a vector of MInstructions. Hannes suggested just changing it to
MDefinitions, seems to work.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8828624 [details] [diff] [review]
Keep iterators alive in Ion in for-of loops for IteratorClose due to exceptions.

Review of attachment 8828624 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ +865,5 @@
>  {
>      // Find phis that must directly hold an iterator live.
>      Vector<MPhi*, 0, SystemAllocPolicy> worklist;
>      for (size_t i = 0; i < iterators_.length(); i++) {
> +        MDefinition* def = iterators_[i];

Not sure if it's strictly necessary but, just to be safe, we could add:

if (def->isPhi()) {
    if (!worklist.append(def->toPhi()))
        return abort(AbortReason::Alloc); 
}

Comment 10

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20170113151323" and the hash "0431a0ab907d53646d359ad10507efe7f89dd487".
The "bad" changeset has the timestamp "20170114194423" and the hash "18ab7878c67ba22b2f98bcc7a1e94b826061dd19".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0431a0ab907d53646d359ad10507efe7f89dd487&tochange=18ab7878c67ba22b2f98bcc7a1e94b826061dd19

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/287d743f3b70
Keep iterators alive in Ion in for-of loops for IteratorClose due to exceptions. (r=jandem)

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/287d743f3b70