627728 - TM: record_JSOP_IN can guard on wrong condition

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

During recording record_JSOP_IN calls obj->lookupProperty() to get the condition to guard the js_HasNamedProperty invocation on trace. Since obj->lookupProperty can point to a proxy during the recording time that can return a different result from what is expected leading to a wrong guard. The following test demonstrates the problem:

function f() {
    var obj = {x: 1};
    var proxy_was_called = false;
    var proxy = Proxy.create({has: function() {
	if (proxy_was_called)
	    return true;
	proxy_was_called = true;
	return false;
    }});
    
    var array = [obj, obj, obj, obj, obj, obj, obj, proxy, obj, obj];
    
    var s = 0;
    for (var i = 0; i != 10; ++i) {
	if ("x" in array[i])
	    ++s;
    }

    assertEq(s, 9);
}

f();

With jit enabled the last assert is violated as due to the use of lookupProperty the presence of the jit is not transparent.

I close the bug since I do not know if there are implications that are more serious than just bad semantics of user scripts.

Comment 1

[Igor Bukanov]

It seems that TraceRecorder::test_property_cache has the same issue with non-transparent jit as it also calls js_LookupPropertyWithFlags that may run proxied code via proxy on the prototype.

Comment 2

[Igor Bukanov]

There are quite a few comments and code like 

// js_FindPropertyHelper can reenter the interpreter and kill |this|
if (!localtm.recorder)
    return ARECORD_ABORTED;

I suppose all such places can be used to detect the jit recording and lead to jit evaluation that is different from non-jit one.

Another observation is that with a proxy coming from a different compartment we may end up with running arbitrary scripts under the recording and continue the recording after that finishes. Presumably this is safe.

Comment 3

[David Mandelin [:dmandelin]]

For now, just looks like it would cause incorrect output with Proxy objects.

Comment 4

[Jason Orendorff [:jorendorff]]

Opening up, per comment 3.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit. FWIW, when I change the AssertEq to line to just print s, I get 9 with Interp, JM, and JM+TI.