TypeInference: JM: crash when calling function defined in other script

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jandem, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-jaegermonkey][fixed-in-tracemonkey])

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Suppose we have a file /tmp/a.js:
--
load("/tmp/b.js");
x = "";
f(10);
assertEq(x, 10);
--

And a file /tmp/b.js:
--
f = function(s) {
    x = s;
}
--
Running ./js -m /tmp/a.js crashes:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x000fb47e in JSStackFrame::pc (this=0x1000030, cx=0x70b2f0, next=0x0) at jsinterp.cpp:134
134	    if (next->flags_ & JSFRAME_HAS_PREVPC)
(gdb) bt
#0  0x000fb47e in JSStackFrame::pc (this=0x1000030, cx=0x70b2f0, next=0x0) at jsinterp.cpp:134
#1  0x002f2b35 in js::mjit::Recompiler::recompile (this=0xbfffec40) at ../methodjit/Retcon.cpp:309


This crash is very common when using multiple scripts/files like this.

Comment 1

8 years ago
The first bad revision is:
changeset:   0cd7e38f0b39
user:        Brian Hackett
date:        Fri Oct 29 08:05:55 2010 -0700
summary:     [INFER] Javascript type inference, bug 557407.
fp->pc() can break when Execute is called without a prev frame and we try to get the pc for frames in a previous segment, as we don't locate the suspended regs with the right pc.  Luke, should this fix go into TM?

http://hg.mozilla.org/projects/jaegermonkey/rev/a0812f46f7ba
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-jaegermonkey

Comment 3

8 years ago
I don't really think the patch fixes the source of the bug; the patch changes the optimized path for the case where 'this' is the top-most stack frame.  The crash in comment 0 seems to happen when the optimized path doesn't hit, hence the bug would seem to be that computeNextFrame is returning null, not that the optimized path doesn't hit.  That is, we should be able to remove the optimized path and have everything work (albeit more slowly).

Perhaps reopen for further investigation?
I hadn't really thought of the JSFrameRegs testing as being an optimized path, more the case of getting the pc from the active or suspended regs when there is no next frame to compute it from.

The recompilation triggering the assert is caused when a.js loads b.js, analyzing its code.  The stack has two segments, one for the frames in a.js and one for the single newly pushed frame in b.js.  When Execute gets called it gets passed a NULL prev frame and pushes a (second) global frame.  That new frame has a NULL prev, has the HAS_PREVPC flag set but prevpc is garbage.  The pc which fp->pc should return for the topmost frame in the a.js segment is stored in the suspended regs of that a.js segment.  What the patch fixes is that previously we would only look at the suspended regs for the current segment, not the one containing the frame we're interested in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 5

8 years ago
Created attachment 517236 [details] [diff] [review]
sameish fix

(In reply to comment #4)
> I hadn't really thought of the JSFrameRegs testing as being an optimized path,
> more the case of getting the pc from the active or suspended regs when there is
> no next frame to compute it from.

Ah, now that I understand the bug I see what you mean; findNextFrame has no chance of finding a next frame if there is no next frame.  With that understanding, I think there are a couple of simplifications we can make to the code, attached.  Also, you are right, this should land on TM.
Attachment #517236 - Flags: review?(bhackett1024)
Comment on attachment 517236 [details] [diff] [review]
sameish fix

Looks good.  If you push to TM, I'll clobber the JM patch when merging back.
Attachment #517236 - Flags: review?(bhackett1024) → review+

Comment 7

8 years ago
http://hg.mozilla.org/tracemonkey/rev/9f2be90f07d3
Whiteboard: fixed-in-jaegermonkey → [fixed-in-jaegermonkey][fixed-in-tracemonkey]
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Comment 8

8 years ago
Missing mozilla-central comment?
You need to log in before you can comment on or make changes to this bug.