503860 - "Adding" property cache entries are unsound

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

The interpreter skips a property lookup when an "adding" property cache entry is found. But shape isn't strong enough to guarantee this lookup is unnecessary. Two objects with the same shape can have different prototype chains. We need the lookup to find read-only properties, properties in sealed scopes, and setters along the prototype chain. Any of those would mean no new property should be added.

An example with a setter:

var s = 'FAIL';
var a = {y: 1};

function B(){}
B.prototype.__defineSetter__('x', function setx(val) { s = 'ok'; });
var b = new B;
b.y = 1;

var arr = [a, b];  // same shape
for (var obj in arr)
    obj.x = 2;  // should call b's setter but doesn't
assertEq(s, 'ok');

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> Two objects with the same shape can have different prototype chains.

That's the root of all evil.

/be

Comment 2

[Jason Orendorff [:jorendorff]]

The only fix I can think of is:

  - give up on having property cache entries of this type; and
  - in the JIT, emit guards to check the shape of every object on the
    prototype chain

Total capitulation. A clever solution for bug 497789 would likely help here too.

Comment 3

[Brendan Eich [:brendan]]

Never give up, never surrender.

I'm fixing this in the patch for bug 497789, it'll be up today.

/be

Comment 4

[Jason Orendorff [:jorendorff]]

The patch in bug 497789 doesn't fix this, because two objects with the same shape can still have different prototype chains. Here's a better test than the one in the description (that one is busted):

var s = 'FAIL';
var p1 = {}, x1 = Object.create(p1);
var p2 = {}, x2 = Object.create(p2);
p2.__defineSetter__("x", function () { s = 'ok'; });

var arr = [x1, x2];  // same shape
for each (var obj in arr)
    obj.x = 2;  // should call p2's setter but doesn't
assertEq(s, 'ok');

Comment 5

[Brendan Eich [:brendan]]

The rt->protoHazardShape idiocy protects INITPROP but not SETPROP -- how'd we lose that protection? IIRC it was needed by the original bug motivating rt->pHS. Will investigate and fix. Suspect this has nothing to do with bug 497789.

/be

Comment 6

[Brendan Eich [:brendan]]

(BTW, rt->protoHazardShape must die -- see bug 550389 and watch for a dependency of it bz is filing.)

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

Said dependency is bug 550391.

Comment 8

[Jason Orendorff [:jorendorff]]

The correctness of our SETPROP code (with and without the tracing jit) depends on the Adding guarantee, which I'll quote in full:

> If at time t0 the object x has shape s,
> and rt->protoHazardShape is z,
> and x does not inherit a JSPROP_SHARED or JSPROP_READONLY property with name n
>   from any prototype,
> and at time t1 an object y has shape s and rt->protoHazardShape is z,
> and no shape-regenerating GC occurred,
> then y does not inherit a JSPROP_SHARED or JSPROP_READONLY property named n
>   from any prototype.

But get this, I added this at the end:

> (Informally: adding a shared or readonly property to a prototype changes
> rt->protoHazardShape.)

The informal description is what we actually implement. The formal description is sufficient to make our SETPROP code correct. But they do not describe the same thing! The informal description is weaker. :(

This is related to bug 497789 in that there's an unattractive change we could make that would fix both: make shape cover prototype identity.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> But get this, I added this at the end:
> 
> > (Informally: adding a shared or readonly property to a prototype changes
> > rt->protoHazardShape.)
> 
> The informal description is what we actually implement. The formal description
> is sufficient to make our SETPROP code correct. But they do not describe the
> same thing! The informal description is weaker. :(

The patch for bug 492355 added protoHazardShape checking to SETPROP as well as to INITPROP. But that check was removed later, apparently due to a merge error:

$ hg annotate -r30279 jsinterp.cpp | grep protoHazardShape
28179:                 vshape = cx->runtime->protoHazardShape;
28179:                         PCVCAP_SHAPE(entry->vcap) == rt->protoHazardShape) {
28179:                     PCVCAP_SHAPE(entry->vcap) == rt->protoHazardShape) {
$ hg annotate -r30280 jsinterp.cpp | grep protoHazardShape
28179:                 vshape = cx->runtime->protoHazardShape;
28179:                     PCVCAP_SHAPE(entry->vcap) == rt->protoHazardShape) {
$ hg diff -r30280^ -r30280 jsinterp.cpp|grep protoHazardShape
-                        PCVCAP_SHAPE(entry->vcap) == rt->protoHazardShape) {
$ hg log -r30280
changeset:   30280:85a00250da71
parent:      30279:345cddad4769
parent:      30229:c4b5d3e7a8fa
user:        Robert Sayre <[e-mail]>
date:        Mon Jul 13 18:19:51 2009 -0400
summary:     Merge tracemonkey to mozilla-central.

Sorry I didn't notice this, and that we didn't have a test that caught it(!). We should have, due to bug 452189, the original bug on the propcache "adding" unsoundness, whose patch regressed Txul, leading to bug 492355, which added protoHazardShape.

Leaving out a test is asking for a merge error (totally likely over time) -- pride goeth before a fall.

> This is related to bug 497789 in that there's an unattractive change we could
> make that would fix both: make shape cover prototype identity.

If we do that, then we should collapse scopes onto sprops and make them thread-local. This is what JSC does with its Structures.

I'm still looking for a way to separate shape from prototype identity, though.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: patch (depends on patch for bug 497789)

With tests (both of them :-|).

/be

Comment 11

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/49e51ab798cf
http://hg.mozilla.org/tracemonkey/rev/08b5d4ff62f5

/be

Comment 12

[Brendan Eich [:brendan]]

Note: until the patch for but 497789 lands (today, ulp) the new test js1_5/Regress/regress-503860.js will fail.

/be

Comment 13

[Brendan Eich [:brendan]]

http://hg.mozilla.org/mozilla-central/rev/550c5bd4fbae

and then

http://hg.mozilla.org/mozilla-central/rev/08b5d4ff62f5

after

http://hg.mozilla.org/mozilla-central/rev/49e51ab798cf

/be