Last Comment Bug 715662 - browser-only methodjit crash (triggered by CSS transform tests)
: browser-only methodjit crash (triggered by CSS transform tests)
Status: VERIFIED FIXED
[sg:critical]
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla10
Assigned To: Brian Hackett (:bhackett)
:
:
Mentors:
http://aryeh.name/tmp/gecko-crash.html
Depends on:
Blocks: 716900
  Show dependency treegraph
 
Reported: 2012-01-05 13:51 PST by Jason Orendorff [:jorendorff]
Modified: 2012-12-10 07:23 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
unaffected
unaffected


Attachments
patch (822 bytes, patch)
2012-01-05 17:58 PST, Brian Hackett (:bhackett)
dvander: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-01-05 13:51:23 PST
Found by Aryeh Gregor, see URL. I can't get it to happen in the shell.



<!doctype html>
<script>
var p = [[0]];
function f(y) {
    var a, b, c, d;
    for (var k = 0; k < 2; k++) {
	var z = 0 + p[0][0] - y;
	if (a === undefined || z < a)
	    a = z;
	if (c === undefined || z > c)
	    c = z;
    }
    throw 0;
}

var arr = [1, 1, 1, 1, 1, 1, 1, 0.5];
for (var i = 0; i < 5; i++) {
    for (var j = 0; j < 8; j++) {
        try {
            f(arr[j]);
        } catch (exc) {
            console.log("fail");
        }
    }
}
</script>
Comment 1 Jason Orendorff [:jorendorff] 2012-01-05 14:00:50 PST
It crashes computing `z > c`.

c should be undefined, but the value on the stack is
  ObjectValue(*(JSObject *) NULL).

#0  0x0000000103f3701c in js::HeapPtr<js::Shape, unsigned long>::operator js::Shape* (this=0x0) at Barrier.h:231
#1  0x0000000103fa5d48 in JSObject::lastProperty (this=0x0) at jsobj.h:515
#2  0x0000000104005378 in JSObject::getClass (this=0x0) at jsscope.h:1116
#3  0x0000000103de984b in JSObject::defaultValue (this=0x0, cx=0x113faaf10, hint=JSTYPE_NUMBER, vp=0x10ae2e150) at jsobjinlines.h:125
#4  0x00000001040d507d in ToPrimitive (cx=0x113faaf10, preferredType=JSTYPE_NUMBER, vp=0x10ae2e150) at jsobjinlines.h:1443
#5  0x00000001040e10e5 in js::mjit::stubs::GreaterThan (f=@0x7fff5fbf6fc0) at StubCalls.cpp:808
#6  0x0000000117c93a53 in ?? ()
#7  0x00000001040d0f6f in js::mjit::EnterMethodJIT (cx=0x113faaf10, fp=0x10ae2e030, code=0x117c94c17, stackLimit=0x10b20e000, partial=true) at MethodJIT.cpp:1051
#8  0x00000001040d13b5 in CheckStackAndEnterMethodJIT (cx=0x113faaf10, fp=0x10ae2e030, code=0x117c94c17, partial=true) at MethodJIT.cpp:1109
#9  0x00000001040d1424 in js::mjit::JaegerShotAtSafePoint (cx=0x113faaf10, safePoint=0x117c94c17, partial=true) at MethodJIT.cpp:1127
#10 0x0000000103e841f4 in js::Interpret (cx=0x113faaf10, entryFrame=0x10ae2e030, interpMode=js::JSINTERP_NORMAL) at jsinterp.cpp:1847
#11 0x0000000103ea5811 in js::RunScript (cx=0x113faaf10, script=0x10b40f350, fp=0x10ae2e030) at jsinterp.cpp:478
#12 0x0000000103ea5c1d in js::ExecuteKernel (cx=0x113faaf10, script=0x10b40f350, scopeChain=@0x10b411060, thisv=@0x7fff5fbfae30, type=js::EXECUTE_GLOBAL, evalInFrame=0x0, result=0x0) at jsinterp.cpp:714
#13 0x0000000103ea6075 in js::Execute (cx=0x113faaf10, script=0x10b40f350, scopeChainArg=@0x10b411060, rval=0x0) at jsinterp.cpp:755
#14 0x0000000103d5f078 in EvaluateUCScriptForPrincipalsCommon (cx=0x113faaf10, obj=0x10b411060, principals=0x10d707118, originPrincipals=0x0, chars=0x114324408, length=434, filename=0x11427b158 "file:///Users/jorendorff/dev/gecko-crash/gecko-crash.html", lineno=2, rval=0x0, compileVersion=JSVERSION_DEFAULT) at jsapi.cpp:5321

(gdb) p rval.data.debugView
$1 = {
  payload47 = 0, 
  tag = JSVAL_TAG_OBJECT
}
Comment 2 Brian Hackett (:bhackett) 2012-01-05 17:58:15 PST
Created attachment 586296 [details] [diff] [review]
patch

At control flow join points, if a variable is known to be a double and is being held in a FP register then it isn't required to be synced to memory at the join.  Its type tag, however, was being marked as synced, and in the example after some later copies between the variable z and a (whose type is unknown) we forgot that z was a double but still treated its type as synced, causing later codegen to be incorrect.  The fix marks both the type and payload as unsynced.
Comment 3 Brian Hackett (:bhackett) 2012-01-05 19:15:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3cea75e5ba
Comment 4 Brian Hackett (:bhackett) 2012-01-05 19:17:44 PST
Comment on attachment 586296 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Potential crash in jitcode on a torn value (security risk).
Risk to taking this patch (and alternatives if risky): Should be low risk, affects code generation in a corner case, but should bake in nightlies for a few days at least.
Comment 5 Jason Orendorff [:jorendorff] 2012-01-06 12:33:48 PST
This needs a test.
Comment 6 Jason Orendorff [:jorendorff] 2012-01-06 13:30:33 PST
OK, here's the first half of a shell test. If you run this with -m -n -a, it triggers the exact situation being patched here: fe->data.unsync() executes, and reg.isReg() is false.

However that is not enough, because the test case then passes. Is there an assertion we could add to a later stage of the JIT that would detect this class of bug? If not, how can I make it cause some observable runtime effect?

function f() {
    var a = [1, 0.5, 1];
    var b;
    for (var i = 0; i < a.length; i++) {
	var z = -a[i];
	if (b === undefined || z < b)
	    b = z;
    }
}

f();
Comment 7 Ian Melven :imelven 2012-01-08 16:30:37 PST
Marking fixed for edmorley.

https://hg.mozilla.org/mozilla-central/rev/6c3cea75e5ba
Comment 8 Alex Keybl [:akeybl] 2012-01-09 12:38:27 PST
Looking for a security rating prior to deciding on whether or not to take based upon that analysis. If not, there's no need to track until it's verified that this is a significant crasher or recent regression.
Comment 9 Brian Hackett (:bhackett) 2012-01-10 13:15:23 PST
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> However that is not enough, because the test case then passes. Is there an
> assertion we could add to a later stage of the JIT that would detect this
> class of bug? If not, how can I make it cause some observable runtime effect?

There really isn't a way for the JIT to assert the correctness of its information here, as the basic problem is that the JIT has incorrect information about which parts of the frame are coherent in the generated machine code, and that correctness is not a property that can be asserted.
Comment 10 Alex Keybl [:akeybl] 2012-01-10 15:08:26 PST
Comment on attachment 586296 [details] [diff] [review]
patch

[Triage Comment]
Approving for aurora and beta since this is externally found, sg-crit and deemed low-risk.

Note You need to log in before you can comment on or make changes to this bug.