636879 - Assertion failure: (uint8*)ic.funGuard.executableAddress() + ic.joinPointOffset == returnAddress

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Unpack, chdir and run js -j -m -a main.js

The attached testcase (shell testcase, unpack archive, change into directory and run "js -j -m -a main.js") causes the following assertion on TM tip:

Assertion failure: (uint8*)ic.funGuard.executableAddress() + ic.joinPointOffset == returnAddress, at ./methodjit/MethodJIT.cpp:1074

Locking on recommendation of dvander.

Comment 1

[Jason Orendorff [:jorendorff]]

Reduced to a single script:

var m = {A: "Object.defineProperty(Array.prototype, 'q', " +
            "{enumerable: true, get: function() { x; }}); " +
            "try { x; } catch(e) { } Array.prototype.r = null;",
         B: '',
         C: "options('tracejit');"};
var ieval = eval;
var arr = [["A"], ["B"], [], ["B", "B", "C"]];
for (var j = 0; j < arr.length; j++) {
    var files = arr[j];
    for (var i in files) {
        try {
            var desc = Object.getOwnPropertyDescriptor(m, files[i]);
            ieval(desc.value);
        } catch (e) {
        }
    }
}

Comment 2

[Jason Orendorff [:jorendorff]]

I managed to squeeze out a little more of the craziness. Debugging.

HOTLOOP = 8;
var p = {q: ""};
var obj = Object.create(p);
for (var t = 0; t < HOTLOOP - 1; t++)
    obj[t] = "";
obj[HOTLOOP - 1] = "options('tracejit');";
var ieval = eval;
for (var i in obj)
    ieval(obj[i]);
print("OK");  // crashes after this executes

Comment 3

[Jason Orendorff [:jorendorff]]

The tracejit starts recording the `for (var i in obj)` loop. The loop calls eval indirectly.

This re-enters the methodjit, which causes trace recording to stop. Then, in methodjit-compiled eval code, we call options(). This clears cx->traceJitEnabled. It does nothing else significant; we are not currently recording and no tracejit code is on the stack. If I change Options to just clear that one flag and return true, the bug still occurs. Comment out that line and the bug doesn't happen.

The methodjit returns to EvalKernel, which returns to the interpreter. At this point in js::Interpret, leaveOnSafePoint is false. Should it be true?

Comment 4

[Jason Orendorff [:jorendorff]]

In case the question at the end of comment 3 does not point to the bug, here's what happens after that...

We continue in the interpreter: JSOP_POP, JSOP_MOREITER, JSOP_IFNE. CASE(JSOP_IFNE) contains a BRANCH(), which we execute. The BRANCH includes a call to MONITOR_BRANCH.

Here we test cx->traceJitEnabled. If it were set, we would call MonitorLoopEdge, which would effectively do nothing and return MONITOR_NOT_RECORDING, and everything would be fine. But it is cleared, so we MONITOR_BRANCH_METHODJIT() instead. This calls back into methodjit code (JaegerShotAtSafePoint). This seems to finish OK; once it returns back to js::Interpret, we jump to leave_on_safe_point and exit.

Then we're back in RunTracer. It sees that we have no extra frames and we're at a safe point, so it returns to the toplevel methodjit code, which crashes after a few instructions:

001C1081  call        js::mjit::stubs::InvokeTracer (0F1380Ah)
001C1086  test        eax,eax                         ;; eax is 001C04B6  
001C1088  je          001C04B6
001C108E  mov         ebx,dword ptr [esp+1Ch]
001C1092  jmp         eax

001C04B6  cmp         dword ptr ds:[4602C0h],0
001C04BD  jne         001C0E6F
001C04C3  mov         edi,dword ptr [ebx+30h]
001C04C6  cmp         dword ptr [edi+4],13F1888h      ;; crashes, edi is 0
001C04CD  jne         001C0E93

Comment 5

[Brendan Eich [:brendan]]

Is this a shell-only bug or could something like options() happen in web content?

/be

Comment 6

[Jeff Walden [:Waldo]]

window._options is all I'm aware of, but it's gone.  An extension could toggle in response to something in content, but this seems far-fetched.  If you're thinking to unhide, makes sense to me.

Total strawman for the future: perhaps JIT options should be part of the script at compile time and not dynamically changeable?

Comment 7

[Jason Orendorff [:jorendorff]]

The patch I put in bug 637385 helped me step through this.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

001C04C3  mov         edi,dword ptr [ebx+30h]
001C04C6  cmp         dword ptr [edi+4],13F1888h      ;; crashes, edi is 0

This appears to be a guard on local->clasp == js_IteratorClass. Smells like the stack being unbalanced somewhere. I'm trying to determine how this relates to the options-flip.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thanks to Jason's explanation I think I can see what's going on. The call stack looks like:

> 0: <jit code>
> 1: JaegerShotAtSafePoint()
> 2: Interpret()
> 3: RunTracer()

When we return from frame 1, we instantly leave the interpreter through leaveAtSafePoiint, then return back to RunTracer. The current JSStackFrame, though, has actually been finished. So we start executing a frame at a totally wrong place.

I think the bug here is just that cx->regs->pc hasn't been updated, but also we must signal that ScriptEpilogue has been run by setting the finishedInInterpreter flag on the frame.

Bill, does that make sense?

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Maybe we could kill two birds with one stone, let FrameIsFinished() peek for the finishedInInterpreter flag, and set that flag after leaveOnSafePoint.

This means case #5 of the big comment in HandleFinishedFrame is slightly mistaken: here we have a situation where the method JIT ran, but we weren't able to tell that it ran, so we treat it as an interpreter frame.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Fixes bug 636889 too. This is extremely hard to hit but it's a real bug, i.e. not related to options(). I can't say how security sensitive it is, both cases are only sg:dos but re-running completed code is pretty of scary. We should fix this for 4.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 515808 [details] [diff] [review]
fix

I have to admit that I don't really understood this code very well. It might be good to check with Luke as well.

We might want to rename the finishedInInterpreter flag to something like finishedScriptEpilogue instead.

Comment 14

[Christian Holler (:decoder)]

(In reply to comment #11)

> Fixes bug 636889 too. This is extremely hard to hit but it's a real bug, i.e.
> not related to options(). 

Is it possible to derive a test case that does not use options()? I'd be curious to analyze the impact of this, but I didn't manage to remove it in the test case.

Maybe I can check how security sensitive it is then.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

These things are pretty hard to test case; did bug 636889 flip options? You have to start recording a loop from the mjit, then abort, then fail to record that loop again and instead run it in the mjit.

Bill: Another way to think about this bug is that at MONITOR_BRANCH_METHODJIT(), we should have a LEAVE_AT_SAFEPOINT or something, because FinishExcessFrames() is supposed to be calling JaegerShotAtSafePoint, so whatever introduced that probably introduced this bug.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 515808 [details] [diff] [review]
fix

r+ with irc nits (explain that FrameIsFinished could just be finishedInInterpreter(), but that we pop the frame directly as an optimization, perhaps rename FrameIsFinished to reflect this distinction).

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/4d16463b0655

Comment 18

[Andreas Gal :gal]

http://hg.mozilla.org/mozilla-central/rev/4d16463b0655