555246 - TM: Wrong answer for 'this' in function code when entry frame was produced by a call across globals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

var cx = evalcx("");
evalcx("function f() {var x; for (var i=0;i<9;i++) x=this; return x;}", cx);
var f = cx.f;
assertEq(f(), this);

This test fails only when the tracing JIT is enabled. f() should return the caller's global. getThis() incorrectly assumes that the caller has the same global as the callee, so on trace f() returns cx.

Marking security-sensitive out of an abundance of caution.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

This seems like a bug in the interpreter to me. In particular, for a given variable |v| anywhere, |v()| and |v.call(null)| should be equivalent. This isn't the case here. Brendan, this seems to break the comment above js_ComputeGlobalThis in jsinterp.cpp, which sub-engine is correct?

Comment 2

[Jason Orendorff [:jorendorff]]

Taking this. I will proceed under the assumption that the comment above js_ComputeGlobalThis is correct and this is a bug in the interpreter.

Comment 3

[Jason Orendorff [:jorendorff]]

Let me see if I understand what the desired behavior is.

1. In top-level script code, `this` is chain->thisObject(), where chain is the second parameter to js_Execute, so we have:
  this === window
  eval("this", obj) === obj

Otherwise we are in a function.  Note that in the cases below, it doesn't matter whether the callee is interpreted or native.

2. If we were called via the JSAPI or from Function.prototype.{call,apply}, then use the provided "this" object, if any; else use GetGlobalForObject(callee).

Otherwise we were called from script, via a syntactic CallExpression.

3. If the call was syntactically `obj.f()`, with a MemberExpression to the left of the arguments, then `this` is obj.

Otherwise we were called via an unqualified `f()` CallExpression.

4. If `f` was found on a With object due to an enclosing with-block, then
`this` should be the wrapped object (caller determines this):

    with (obj) { f(); }  // `this` should be obj

5. If `f` is a lexical variable, a DeclEnv binding, or a global property, then `this` should be GetGlobalForObject(f)->thisObject().  (callee determines this)

    var cx = evalcx("");
    hasOwnProperty2 = cx.Object.prototype.hasOwnProperty;
    var blah = 2;
    hasOwnProperty2("blah") === false;   // 'this' is cx

6. Otherwise `f` is a property of a non-censored object S on the scope chain other than the global. (You can definitely get this to happen via the JSAPI; I'm not sure you can make it happen with pure JS, even with shell extensions.) In that case, `this` is S.  (caller determines this)

Is that right?

Comment 4

[Jason Orendorff [:jorendorff]]

I missed a case:

1a. In direct-eval code in a function, `this` is the same as in the enclosing function.

Comment 5

[Brendan Eich [:brendan]]

Don't forget ES5 Function.prototype.bind (although maybe it is covered by apply, which is used to implement it in JS today).

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Other omissions:

Case 2 requires a ->thisObject call just like cases 1 and 5.

Case 3 neglects the special cases that arise due to PRIMITIVE_THIS. (But those are pretty straightforward. The main thing to watch out for is the case where ComputeThis has to call js_PrimitiveToObject.)

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

This applies on top of mrbkap's patch in bug 515496. It passes the tests, but I need to look at the performance impact. I expect no change.

Also, I should split out the actual bug fix, which I think is a one-line change.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v2

Sunspider claims this is about 2% worse on crypto-sha1 and regexp-dna. The measured difference is <1msec in both cases. If it's really worse, it is due to the extra conditional branch in JSOP_CALLNAME. Bug 556277 will more than make up for it.

Comment 9

[Brendan Eich [:brendan]]

Invert the sense of that ?: test to see whether your non-PGO'ed build improves in perf? The likely case with JSOP_CALLNAME is a call to a global function, I bet. In any event, it would be great to have shark or similar confirmation.

/be

Comment 10

[Jason Orendorff [:jorendorff]]

It's noise. Flipping ?: had no effect, but running again with --runs=30 made it go away.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 436321 [details] [diff] [review]
v2

Thanks,

/be

Comment 12

[Jason Orendorff [:jorendorff]]

Whoa, this is failing tests like crazy. I must not have tested it.

The four-line comment I added, explaining in detail why it's OK to store a non-null value even though the interpreter is going to store JSVAL_NULL, is basically correct, but nonetheless we assert on leaving trace that the value in the native slot is actually NULL. That assertion is of course failing.

I don't want to lose that assertion coverage. Instead I'm going to give up on fixing the whole bug here and just fix the CALLGVAR half of it, which is easy. Bug 556277 will fix the rest.

Smaller patch coming.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v3

Here is the half-fix.

CALLGVAR is fixed; CALLNAME can still put the wrong 'this' value on the stack.

The patch adds a test for each and marks the CALLNAME test as failing.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 436478 [details] [diff] [review]
v3

Onward to the super-huge omg i can't wait fix ;-).

/be

Comment 15

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/e4364736e170

Comment 16

[Robert Sayre]

these reftests are failing:

js1_8_5/regress/regress-555246-0.js:7: evalcx is not defined item 1

Comment 17

[Robert Sayre]

(In reply to comment #16)
> these reftests are failing:
> 
> js1_8_5/regress/regress-555246-0.js:7: evalcx is not defined item 1

I fixed these up. no evalcx in the browser.

Comment 18

[Jesse Ruderman]

rev e4364736e170 has landed on mozilla-central as part of today's merge.

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e4364736e170