544161 - global object may change on trace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Luke Wagner [:luke]]

From bug 543149 comment 18 and 19, it is possible to have two function objects with the same JSFunction* and different global objects.  In some cases we skip the parent check when this can still happen.  Talking to Brendan, we don't think this can happen in content (since a content function is always parented by its static scope, and will not be cloned even when accessed from another global), however, it might happen in chrome where we do cloning to avoid re-compiling XUL scripts for each XUL window.  Brendan was able to simulate this using js/shell extensions:

var g1 = this;
var g2 = evalcx('');
function m() {
    return x;
}
g2.m = clone(m, g2);
var x = 42;
g2.x = 99;
var a = [g1, g1, g1, g1, g2];
var b = [];
for (var i = 0; i < a.length; i++)
    b[b.length] = a[i].m();
print(uneval(b));
print(g2.m());

checkStats({
    recorderStarted: 1,
    recorderAborted: 0,
    traceCompleted: 1
});

produces this output:

[42, 42, 42, 42, 42]
99

Comment 1

[Luke Wagner [:luke]]

See also bug 543149 comment 20.

Comment 2

[Brendan Eich [:brendan]]

Insidious: TR::guardPropertyCacheHit does not guard on the shape of g1, because it is the global object for this recorder. This may be the whole cause of the bug right here. Jason, what do you think?

/be

Comment 3

[Brendan Eich [:brendan]]

Not the whole bug, of course -- if another object happened to have the same shape, then the missing shape guard would be satisfied even if that object had a different global (parent slot).

We could guard on global object identity rather than shape in the case where the base object of the callee property reference is the global object. That would fix the bug, but would it be too strict a check? It matches the check done to trigger a trace: global object identity.

The alternative would be to make an object's shape depend on its parent's shape, but that's ugly because it breaks property tree sharing. It is tantamount to a property tree per global.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Anyone see other holes? I mentioned CALLELEM in the other bug.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

With fixes to problems Jason mentioned over IRC.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 425349 [details] [diff] [review]
proposed fix, v2

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>     // For any hit that goes up the scope and/or proto chains, we will need to
>     // guard on the shape of the object containing the property.
>     if (PCVCAP_TAG(entry->vcap) >= 1) {
>         JS_ASSERT(OBJ_SHAPE(obj2) == vshape);
>+        if (obj2 == globalObj)
>+            RETURN_STOP("hitting the global object via a prototype chain");

I don't remember mentioning the bug you're fixing with these two lines (which is really a separate issue from the rest) but boy howdy is that broken without the patch. Please add this as a trace-test:

var b = Object.create(this);
for (var i = 0; i < 9; i++)
    assertEq(b.i, i);

r=me with a bullet.

Comment 7

[Brendan Eich [:brendan]]

We should log our IRC exhanges via bugzilla ;-).

Thanks for the test, I added it:

http://hg.mozilla.org/tracemonkey/rev/23245d41bd32

/be

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/23245d41bd32