Last Comment Bug 724938 - IonMonkey: Crash [@ js::HeapPtr<js::ion::IonCode, unsigned int>::operator]
: IonMonkey: Crash [@ js::HeapPtr<js::ion::IonCode, unsigned int>::operator]
Status: RESOLVED FIXED
: crash, 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-02-07 08:16 PST by Christian Holler (:decoder)
Modified: 2012-02-09 12:12 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 (5.67 KB, patch)
2012-02-07 14:15 PST, David Anderson [:dvander]
cdleary: review+
Details | Diff | Splinter Review
part 2 (23.36 KB, patch)
2012-02-07 17:53 PST, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-02-07 08:16:54 PST
The following testcase crashes on ionmonkey revision c34398f961e7 (run with --ion -n -m), tested on 64 bit:


gczeal(2);
Date.prototype.dateFormat=function(character) + String.escape() + "' + "
String.escape=function (val, size, ch) {
    result = new String(val)
        ch + result
}
var date = new Date;
for (i=0; ; )
  date.dateFormat()
Comment 1 David Anderson [:dvander] 2012-02-07 14:15:12 PST
Created attachment 595173 [details] [diff] [review]
part 1

It looks like we continued to run code in the interpreter after having invalidated a frame. This caused some pc-snooping stuff to fail. The fix is to just look for invalidated frames in GetPcScript, but I also refactored IonFrameIterator a little to make this code nicer.

There is still one more problem. If the invalidation was caused by GC, then script->hasAnalysis() will return false, and we won't be able to backwards-sniff the pc from the safepoint. This needs an additional, more invasive fix that I'll do in the next patch.
Comment 2 David Anderson [:dvander] 2012-02-07 17:53:18 PST
Created attachment 595266 [details] [diff] [review]
part 2

This changes snapshots to always use the actual pc, and to call GetNextPc() before interpreting. This way we don't have to rely on script->analysis() to recover the correct pc for TI. For some reason... I'm not a big fan of this patch, so if you have other ideas let me know.
Comment 3 Jan de Mooij [:jandem] (PTO until July 31) 2012-02-08 01:22:24 PST
Comment on attachment 595266 [details] [diff] [review]
part 2

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

(In reply to David Anderson [:dvander] from comment #2)
> Created attachment 595266 [details] [diff] [review]
> part 2
> 
> I'm not a big fan of this
> patch, so if you have other ideas let me know.

I think this is the right approach, at least I can't think of a better fix.

::: js/src/ion/IonBuilder.cpp
@@ -89,5 @@
> -static inline jsbytecode *
> -GetNextPc(jsbytecode *pc)
> -{
> -    return pc + js_CodeSpec[JSOp(*pc)].length;
> -}

Pre-existing nit: I just noticed that IonFrames.cpp has a copy of this function, can you remove that one too?

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