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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: decoder, Assigned: dvander)
References
Details
(Keywords: crash, testcase)
Crash Data
Attachments
(2 files)
5.67 KB,
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
23.36 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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()
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #595173 -
Flags: review?(christopher.leary) → review+
Assignee | ||
Comment 4•12 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/87e804b03e58 http://hg.mozilla.org/projects/ionmonkey/rev/de33951b455d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•