Closed Bug 724938 Opened 12 years ago Closed 12 years ago

IonMonkey: Crash [@ js::HeapPtr<js::ion::IonCode, unsigned int>::operator]

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: crash, testcase)

Crash Data

Attachments

(2 files)

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()
Attached patch part 1Splinter Review
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.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #595173 - Flags: review?(christopher.leary)
Attached patch part 2Splinter Review
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.
Attachment #595266 - Flags: review?(jdemooij)
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?
Attachment #595266 - Flags: review?(jdemooij) → review+
Attachment #595173 - Flags: review?(christopher.leary) → review+
You need to log in before you can comment on or make changes to this bug.