Closed Bug 621526 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Unassigned)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

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.
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
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-jaegermonkey
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 → ---
Attached patch sameish fixSplinter Review
(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+
http://hg.mozilla.org/tracemonkey/rev/9f2be90f07d3
Whiteboard: fixed-in-jaegermonkey → [fixed-in-jaegermonkey][fixed-in-tracemonkey]
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Missing mozilla-central comment?
You need to log in before you can comment on or make changes to this bug.