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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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] - inactive, e-mail if emergency]

Attachment: 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] - inactive, e-mail if emergency]

Attachment: 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]]

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?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/projects/ionmonkey/rev/87e804b03e58
http://hg.mozilla.org/projects/ionmonkey/rev/de33951b455d