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');

(In reply to comment #0) > Two objects with the same shape can have different prototype chains. That's the root of all evil. /be

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.

Never give up, never surrender. I'm fixing this in the patch for bug 497789, it'll be up today. /be

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');

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

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

Said dependency is bug 550391.

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.

(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 <sayrer@gmail.com> 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

With tests (both of them :-|). /be

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

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

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