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::CloseIterator] or Assertion failure: isObject(), at ../../../dist/include/js/Value.h:1237

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla49
x86_64
Linux
assertion, crash, regression, testcase
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox49 fixed)

Details

(Whiteboard: [jsbugmon:], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

2 years ago
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.

Updated

2 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]

Comment 2

2 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision e1ef2be156de).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]

Updated

2 years ago
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]

Comment 3

2 years ago
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.
Jan, do you think comment 1 and comment 3 make sense?
Flags: needinfo?(jdemooij)
(Assignee)

Comment 5

2 years ago
Created attachment 8702251 [details] [diff] [review]
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8702251 - Flags: review?(hv1989)
(Assignee)

Comment 6

2 years ago
(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 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());
Attachment #8702251 - Flags: review?(hv1989) → review+
Jan, what's next here?
Flags: needinfo?(jdemooij)

Comment 9

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c992892782cb
(Assignee)

Comment 10

a year ago
(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.
(Assignee)

Updated

a year ago
Flags: needinfo?(jdemooij)

Comment 11

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

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c992892782cb
https://hg.mozilla.org/mozilla-central/rev/d4eaaba47a5f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.