Last Comment Bug 757428 - IonMonkey: Assertion failure: Crossing fingers: Unable to read snapshot slot., at ../ion/IonFrameIterator.h:249 or Crash [@ js::ion::InlineFrameIterator::more]
: IonMonkey: Assertion failure: Crossing fingers: Unable to read snapshot slot....
Status: RESOLVED FIXED
[jsbugmon:update]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-05-22 07:18 PDT by Christian Holler (:decoder)
Modified: 2013-02-07 05:19 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (766 bytes, patch)
2012-05-22 18:01 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
fix 2 (2.88 KB, patch)
2012-05-22 19:05 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-05-22 07:18:30 PDT
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);
}
Comment 1 Christian Holler (:decoder) 2012-05-22 07:19:50 PDT
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
Comment 2 David Anderson [:dvander] 2012-05-22 12:07:32 PDT
Not security critical. Potentially observable JIT behavior is all. The assert is here to see if this case comes up in real world code.
Comment 3 David Anderson [:dvander] 2012-05-22 18:01:48 PDT
Created attachment 626274 [details] [diff] [review]
fix

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.
Comment 4 Nicolas B. Pierron [:nbp] 2012-05-22 18:16:19 PDT
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.
Comment 5 David Anderson [:dvander] 2012-05-22 18:42:13 PDT
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.
Comment 6 David Anderson [:dvander] 2012-05-22 18:45:23 PDT
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.
Comment 7 David Anderson [:dvander] 2012-05-22 19:05:36 PDT
Created attachment 626285 [details] [diff] [review]
fix 2

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.
Comment 8 Nicolas B. Pierron [:nbp] 2012-05-22 22:29:42 PDT
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.
Comment 9 David Anderson [:dvander] 2012-05-22 23:16:49 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/038b237f7dad
Comment 10 Christian Holler (:decoder) 2013-02-07 05:19:33 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397

Note You need to log in before you can comment on or make changes to this bug.