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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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

[Jesse Ruderman]

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.

Comment 2

[Brian Hackett [Laid off!]]

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

Comment 3

[Luke Wagner [:luke]]

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?

Comment 4

[Brian Hackett [Laid off!]]

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.

Comment 5

[Luke Wagner [:luke]]

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

Comment 6

[Brian Hackett [Laid off!]]

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.

Comment 7

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/9f2be90f07d3

Comment 8

[Daniel C]

Missing mozilla-central comment?