1197769 - Crash [@ js::CloseIterator] or Assertion failure: isObject(), at ../../../dist/include/js/Value.h:1237

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

test();
function test() {
  var a = [""];
  var i = 0;
  for each (let e in a) {
      if (i == 10)
        for each (g in [1]) {}
      throw test() 
  }
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::CloseIterator (cx=cx@entry=0x7ffff6907000, obj=obj@entry=...) at js/src/jsobj.h:122
#0  js::CloseIterator (cx=cx@entry=0x7ffff6907000, obj=obj@entry=...) at js/src/jsobj.h:122
#1  0x00000000008ed2d9 in js::UnwindIteratorForException (cx=cx@entry=0x7ffff6907000, obj=obj@entry=...) at js/src/jsiter.cpp:1133
#2  0x0000000000788825 in CloseLiveIteratorIon (stackSlot=<optimized out>, frame=..., cx=0x7ffff6907000) at js/src/jit/JitFrames.cpp:407
#3  HandleExceptionIon (overrecursed=0x7fffffefdb80, rfe=0x7fffffefe170, frame=..., cx=0x7ffff6907000) at js/src/jit/JitFrames.cpp:487
#4  js::jit::HandleException (rfe=0x7fffffefe170) at js/src/jit/JitFrames.cpp:841
#5  0x00007ffff7fe815d in ?? ()
#6  0x00007ffff6907018 in ?? ()
#7  0x00007fffffefe170 in ?? ()
#8  0x0000000000000000 in ?? ()
rax	0xfff9000000000000	-1970324836974592
rbx	0x7ffff6907000	140737330049024
rcx	0xf	15
rdx	0xfffc7ffff43a0280	-985162616012160
rsi	0x7fffffefdc10	140737487297552
rdi	0x7ffff6907000	140737330049024
rbp	0x7fffffefdc10	140737487297552
rsp	0x7fffffefdad8	140737487297240
r8	0x7ffff6981a48	140737330551368
r9	0x729a4e80	1922715264
r10	0x7ffff6981a20	140737330551328
r11	0x7ffff6907068	140737330049128
r12	0x1	1
r13	0x6	6
r14	0x7fffffefdca0	140737487297696
r15	0x7ffff6907018	140737330049048
rip	0x8ed193 <js::CloseIterator(JSContext*, JS::Handle<JSObject*>)+3>
=> 0x8ed193 <js::CloseIterator(JSContext*, JS::Handle<JSObject*>)+3>:	mov    (%rcx),%rax
   0x8ed196 <js::CloseIterator(JSContext*, JS::Handle<JSObject*>)+6>:	mov    (%rax),%rdx


Crash seems to be always a null-deref, so not marking s-s.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/310b3af47e93
user:        Jan de Mooij
date:        Fri Mar 20 13:45:35 2015 +0100
summary:     Bug 1142669 part 2 - Lower the script inlining size limit if off-thread compilation is not available. r=h4writer

changeset:   https://hg.mozilla.org/mozilla-central/rev/157929ef51b3
user:        Jan de Mooij
date:        Fri Mar 20 13:45:36 2015 +0100
summary:     Bug 1142669 part 3 - Limit the total inlined bytecode size to avoid excessive inlining. r=h4writer

changeset:   https://hg.mozilla.org/mozilla-central/rev/f7299a88c59c
user:        Jan de Mooij
date:        Thu Mar 19 15:10:07 2015 +0100
summary:     Bug 1142669 part 4 - Fix some inlining issues and inline scripts with loops. r=h4writer

changeset:   https://hg.mozilla.org/mozilla-central/rev/4d744eeea51c
user:        Mike Shal
date:        Tue Mar 17 15:29:07 2015 -0400
summary:     Bug 1137000 - Enable SDK building on nightlies; r=glandium

changeset:   https://hg.mozilla.org/mozilla-central/rev/847f9159b3e9
user:        Jan de Mooij
date:        Fri Mar 20 14:14:06 2015 +0100
summary:     Bug 1142669 followup - Move OffThreadCompilationAvailable definition outside namespace block. r=red CLOSED TREE

This iteration took 0.849 seconds to run.

Comment 2

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision e1ef2be156de).

Comment 3

[Fuzzing Team]

JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a0ccab2a6e28
user:        Morgan Phillips
date:        Thu Oct 22 20:05:37 2015 -0700
summary:     Bug 1192329 - Change JS shell to default to the standard version of JS (not 1.7+) 1/2; r=jorendorff

This iteration took 221.138 seconds to run.

Comment 4

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

Jan, do you think comment 1 and comment 3 make sense?

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

Ugh, good catch.

We can inline functions with iterators now but IonBuilder::jsop_iter added the iterator to the inner builder's iterator_ list (which is never used).

This patch adds an assert and fixes jsop_iter to add to the outermost builder's list, so it's processed by processIterators.

I really want to use different classes for the outer and inline builders, to avoid these issues, but that can wait for now.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #4)
> Jan, do you think comment 1 and comment 3 make sense?

Comment 1 makes sense. Comment 3 is a red herring; it still reproduced after I changed the for-each-in loop to a plain for-in loop.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8702251 [details] [diff] [review]
Patch

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

Like suggested in comment, it would make sense to have separate classes for top build / inlined builds

::: js/src/jit/IonBuilder.cpp
@@ +1090,5 @@
>  
>      // Discard unreferenced & pre-allocated resume points.
>      replaceMaybeFallbackFunctionGetter(nullptr);
>  
> +    MOZ_ASSERT(iterators_.empty(), "Iterators should be added to outer builder");

Can we assert this in destructor of IonBuilder when we have an inlined builder?

MOZ_ASSERT_IF(inlineCallInfo_, iterators_.empty());

or

MOZ_ASSERT_IF(callerBuilder_, iterators_.empty());

Comment 8

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

Jan, what's next here?

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c992892782cb

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #8)
> Jan, what's next here?

Oops, I forgot to push this. Done.

(In reply to Hannes Verschore [:h4writer] from comment #7)
> Can we assert this in destructor of IonBuilder when we have an inlined
> builder?

I added the assert to buildInline() so it matches build() (build calls processIterators and buildInline asserts there are no iterators).

I'll file a followup bug to split IonBuilder in inner/outer.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4eaaba47a5f

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c992892782cb
https://hg.mozilla.org/mozilla-central/rev/d4eaaba47a5f