Closed Bug 757428 Opened 12 years ago Closed 12 years ago

IonMonkey: Assertion failure: Crossing fingers: Unable to read snapshot slot., at ../ion/IonFrameIterator.h:249 or Crash [@ js::ion::InlineFrameIterator::more]

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Assigned: dvander)

References

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

The following testcase asserts on ionmonkey revision d80602d38aa8 (run with --ion -n -m --ion-eager):


function f(o) {
    var prop = "arguments";
    f[prop] = f[prop];
}
for(var i=0; i > -10; i-- ) {
    f(f / 16);
}
Crash trace:


Program received signal SIGSEGV, Segmentation fault.
0x0000000000645f6b in js::ion::InlineFrameIterator::more (this=0x3cc3) at ../ion/IonFrameIterator.h:274
274             return frame_ && framesRead_ < start_.frameCount();
(gdb) bt
#0  0x0000000000645f6b in js::ion::InlineFrameIterator::more (this=0x3cc3) at ../ion/IonFrameIterator.h:274
#1  0x0000000000000001 in ?? ()
#2  0x0000000000011d30 in ?? ()
#3  0x00007fffffffb3d0 in ?? ()
#4  0x00007fffffffb3f0 in ?? ()
#5  0x0000000000000001 in ?? ()
#6  0x0000000000011d30 in ?? ()
#7  0x00007fffffffb590 in ?? ()
#8  0x0000000000649bbc in js::ion::InlineFrameIterator::forEachCanonicalActualArg<PutArg> (this=0x7ffff0910940, op=..., start=32767, count=4294950640) at ../ion/IonFrameIterator-inl.h:84
Backtrace stopped: frame did not save the PC
(gdb) x /i $pc
=> 0x645f6b <js::ion::InlineFrameIterator::more() const+17>:    mov    (%rax),%rax
(gdb) info reg rax
rax            0x3cc3   15555
Not security critical. Potentially observable JIT behavior is all. The assert is here to see if this case comes up in real world code.
Group: core-security
Attached patch fixSplinter Review
Just disable the assert for now. I can construct a test case to show the behavior difference but it's pretty difficult, and requires --ion-eager. So we'll see how this flies in the real world.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #626274 - Flags: review?(nicolas.b.pierron)
Comment on attachment 626274 [details] [diff] [review]
fix

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

::: js/src/ion/IonFrameIterator.h
@@ -250,1 @@
>          return UndefinedValue();

Honestly, I would prefer to have a poison which can be observed and kill the engine where this is critical, and be translated to naive thing when we can break the backward compatibility.

My point is that the crash is hard to identify, and a poison can be trace to the only source producing it and thus help us to detect this case in the wild.

Sadly I think this kind of bug will be extremelly present in the wild because we don't save the fpu registers, and thus some arguments may still be lived in fpu registers during an oolCallVM.
Attachment #626274 - Flags: review?(nicolas.b.pierron)
What? We absolutely don't want to crash here. We cannot do that. I don't understand why you think this will be "extremely present" at all - what evidence is there of that? The rarity of this unspecified language feature, and the difficulty to even reproduce an observable difference in the optimizing compiler - says otherwise. The test case in this bug actually works. And, if it does happen in the wild - we have multiple strategies for fixing it.

We should just take a risk here and see if we can break an obscure edge case of this junk language feature.
To add to that, even though this test case works, and it is the easiest way to hit this error, it's also the most unlikely usage (why would you access f.arguments inside f, rather than |arguments|?) It also requires a function using this feature to be hot enough to Ion compile, which means we could mitigate it by marking |f| uncompilable as soon as we see this access.
Attached patch fix 2Splinter Review
Okay, sorry if I flew off the handle - I just think any amount of time spent on f.arguments at this point is pretty much wasted.

Talked with Nicolas in person - the concern about debugging a possible regression in the wild is what motivated the assert in the first place, but I don't think it's that serious. This patch is a compromise. stderr spew if it does happen, and disables Ion-compilation if we see f.arguments use.
Attachment #626285 - Flags: review?(nicolas.b.pierron)
Comment on attachment 626285 [details] [diff] [review]
fix 2

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

::: js/src/jsfun.cpp
@@ +112,5 @@
>  
> +#ifdef JS_ION
> +        // If this script hasn't been compiled yet, make sure it will never
> +        // be compiled. |f.arguments| is tricky to deal with so we mitigate
> +        // any possibility of regression.

tricky = we cannot guarantee to recover everything because we are not spilling everything on ool VM calls.
Attachment #626285 - Flags: review?(nicolas.b.pierron) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/038b237f7dad
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.