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

RESOLVED FIXED in Firefox 53

Status

()

--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Unassigned)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla53
x86_64
Linux
assertion, crash, jsbugmon, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 fixed)

Details

(Whiteboard: [fuzzblocker] [jsbugmon:update], crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

2 years ago
Flags: needinfo?(shu)
(Reporter)

Comment 1

2 years ago
This bug seems to crash in multiple ways, marking fuzzblocker.
Crash Signature: [@ JSObject::getClass] → [@ JSObject::getClass] [@ js::CloseIterator]
Summary: Crash [@ JSObject::getClass] or Assertion failure: isObject(), at dist/include/js/Value.h:642 → Crash [@ JSObject::getClass] or Crash [@ js::CloseIterator] Assertion failure: isObject(), at dist/include/js/Value.h:642
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][fuzzblocker]

Updated

2 years ago
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected

Comment 2

2 years ago
Created attachment 8827733 [details] [diff] [review]
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?
Attachment #8827733 - Flags: review?(jdemooij)

Updated

2 years ago
Flags: needinfo?(shu)
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.
Flags: needinfo?(shu)
(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

2 years ago
Created attachment 8828144 [details] [diff] [review]
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.
Attachment #8828144 - Flags: review?(jdemooij)

Updated

2 years ago
Attachment #8827733 - Attachment is obsolete: true
Attachment #8827733 - Flags: review?(jdemooij)

Updated

2 years ago
Flags: needinfo?(shu)
=== 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.
Blocks: 1147371
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.
Attachment #8828144 - Flags: review?(jdemooij) → review+

Comment 8

2 years ago
Created attachment 8828624 [details] [diff] [review]
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.
Attachment #8828624 - Flags: review?(jdemooij)

Updated

2 years ago
Attachment #8828144 - Attachment is obsolete: true
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); 
}
Attachment #8828624 - Flags: review?(jdemooij) → review+

Updated

2 years ago
Crash Signature: [@ JSObject::getClass] [@ js::CloseIterator] → [@ JSObject::getClass] [@ js::CloseIterator]
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]

Comment 10

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

2 years ago
Pushed by shu@rfrn.org:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/287d743f3b70
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

2 years ago
See Also: → bug 1333946
You need to log in before you can comment on or make changes to this bug.