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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: decoder, Assigned: dvander)

Tracking

(Blocks: 2 bugs, {crash, testcase})

Other Branch
x86_64
Linux
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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()
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.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #595173 - Flags: review?(christopher.leary)
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.
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+
http://hg.mozilla.org/projects/ionmonkey/rev/87e804b03e58
http://hg.mozilla.org/projects/ionmonkey/rev/de33951b455d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.