Closed Bug 503860 Opened 11 years ago Closed 10 years ago

"Adding" property cache entries are unsound

Categories

(Core :: JavaScript Engine, defect, P1)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: jorendorff, Assigned: brendan)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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
Depends on: 497789
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
Blocks: 503080
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
Assignee: general → brendan
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9.3a3
(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
Attachment #431580 - Flags: review?(jorendorff)
Attachment #431580 - Flags: review?(jorendorff) → review+
Note: until the patch for but 497789 lands (today, ulp) the new test js1_5/Regress/regress-503860.js will fail.

/be
You need to log in before you can comment on or make changes to this bug.