Last Comment Bug 743119 - IonMonkey: Crash [@ js::Interpret] with infinite recursion
: IonMonkey: Crash [@ js::Interpret] with infinite recursion
Status: RESOLVED FIXED
[jsbugmon:update,ignore]
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Linux
: -- major (vote)
: ---
Assigned To: David Anderson [:dvander]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-04-05 18:27 PDT by Christian Holler (:decoder)
Modified: 2012-05-15 15:16 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (694 bytes, patch)
2012-05-14 18:49 PDT, David Anderson [:dvander]
sstangl: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-04-05 18:27:16 PDT
The following testcase crashes on ionmonkey revision a9a18824b4c1 (run with --ion -n):


function printStatus (msg) {}
try {
evaluate("\
test();\
function test() {\
  var fThis;\
  function f() {\
    new printStatus( fThis, 'isFinite( new String(\"Infinity\") )', false, isFinite(new f('Infinity')) );\
  }\
  f()();\
}\
");
} catch(exc0) {}
evaluate("test();");
Comment 1 Christian Holler (:decoder) 2012-04-05 18:28:44 PDT
Crash stack:

Program received signal SIGSEGV, Segmentation fault.
0x0813efd1 in js::Interpret (cx=0x870de10, entryFrame=0xf7b10090, interpMode=js::JSINTERP_BAILOUT) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsinterp.cpp:1297
1297    {
(gdb) bt 8
#0  0x0813efd1 in js::Interpret (cx=0x870de10, entryFrame=0xf7b10090, interpMode=js::JSINTERP_BAILOUT) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsinterp.cpp:1297
#1  0x0846ea85 in js::ion::ThunkToInterpreter (vp=0xff3fe568) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/Bailouts.cpp:476
#2  0x00414885 in ?? ()
#3  0x08390f65 in EnterIon (cx=0x870de10, fp=0xf7b10040, jitcode=0x4148d8) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/Ion.cpp:974
#4  0x083911aa in js::ion::Cannon (cx=0x870de10, fp=0xf7b10040) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/Ion.cpp:1002
#5  0x08149821 in js::Interpret (cx=0x870de10, entryFrame=0xf7b0ffa0, interpMode=js::JSINTERP_BAILOUT) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/jsinterp.cpp:2785
#6  0x0846ea85 in js::ion::ThunkToInterpreter (vp=0xff3fee58) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/Bailouts.cpp:476
#7  0x00414885 in ?? ()
(More stack frames follow...)

Repeats always with those 5 frames.
Comment 2 Christian Holler (:decoder) 2012-04-11 17:13:32 PDT
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 030178aae6e4).
Comment 3 Christian Holler (:decoder) 2012-04-18 05:28:03 PDT
Bisection points to this revision as the fix:

The first good revision is:
changeset:   92561:c1aa75c192b8
user:        Kannan Vijayan
date:        Wed Apr 11 17:24:51 2012 -0400
summary:     Bug 744535 - Invalidate/recompile scripts on change to ThisTypeSet

Kannan, is this likely a fix for this bug so we can close it?
Comment 4 Kannan Vijayan [:djvj] 2012-04-21 21:01:44 PDT
Not sure about that.  It seemed like Bug 744535 was a perf bug, not correctness, but I could be wrong.  I would think that failing to freeze ThisTypeSet shouldn't lead to infinite loops, just a whole lot of expensive deoptimizations when guards fail.

bhackett should be able to answer this more definitively.
Comment 5 Brian Hackett (:bhackett) 2012-04-22 06:16:27 PDT
I think there is a JS_CHECK_RECURSION needed somewhere in that call stack, probably in ion::Cannon to avoid blowing the C stack.  The test itself has an infinite recursion.
Comment 6 David Anderson [:dvander] 2012-05-14 18:49:55 PDT
Created attachment 623902 [details] [diff] [review]
fix

Although this bug no longer reproduces, I encountered this a few days ago and we definitely need a JS_CHECK_RECURSION in EnterIon.
Comment 7 Sean Stangl [:sstangl] 2012-05-15 14:51:11 PDT
Comment on attachment 623902 [details] [diff] [review]
fix

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

::: js/src/ion/Ion.cpp
@@ +945,5 @@
>  
>  static bool
>  EnterIon(JSContext *cx, StackFrame *fp, void *jitcode)
>  {
> +    JS_CHECK_RECURSION(cx, return true);

Needs to return false.
Comment 8 David Anderson [:dvander] 2012-05-15 15:16:19 PDT
Of course, I managed to leave a bug in a one-line trivial patch ;)

http://hg.mozilla.org/projects/ionmonkey/rev/29b9a3fd01e4

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