"Adding" property cache entries are unsound

RESOLVED FIXED in mozilla1.9.3a3

Status

()

P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: brendan)

Tracking

Other Branch
mozilla1.9.3a3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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');
(Assignee)

Comment 1

9 years ago
(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
(Reporter)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
Never give up, never surrender.

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

/be
(Reporter)

Updated

9 years ago
Blocks: 503080
(Reporter)

Comment 4

9 years ago
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');
(Assignee)

Comment 5

9 years ago
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
(Assignee)

Comment 6

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

/be
Said dependency is bug 550391.
(Reporter)

Comment 8

9 years ago
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.
(Assignee)

Comment 9

9 years ago
(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
(Assignee)

Comment 10

9 years ago
Created attachment 431580 [details] [diff] [review]
patch (depends on patch for bug 497789)

With tests (both of them :-|).

/be
Attachment #431580 - Flags: review?(jorendorff)
(Reporter)

Updated

9 years ago
Attachment #431580 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 12

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

/be
(Assignee)

Comment 13

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.