764148 - TI: TypeObject properties incorrectly shadowed if prototype custom setter.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

TI assumes that all sets to a property should shadow the prototype chain. This is nearly always the case, except in the case where there is an inherited custom setter from somewhere above in the prototype chain.

Comment 1

[Eric Faust [:efaust]]

The problem is that the place that does the marking is a common function (TypeObject::getProperty()) for both the creation paths (where optimally we would do such a check) *and* every access path (as you should do the shadow check every time, and remarking the property as an own property hurts nothing) since the access code just adds it if it isn't there and carries on.

TypeObject::getProperty() takes a parameter (bool assign), which decides whether we should shadow the current property that we have either just added or looked up (Properties are maintained as a HashSet in the TypeObject, the insertion function of which returns the already present value, if it was there) in a straight line:

    if (assign)
        types->setOwnProperty(cx, false);

The trouble is, if the property set is on a property that has an unshadowed custom getter above on the prototype chain, then it's *not* an own property. It's a propagated property with the setter as described. In testing, removing these two lines, while having obvious completely incorrect implications for most sets, does cause proper property percolation for inherited custom setters, so it's just a matter of improving on the conditions under which we setOwnProperty().

Here's where things get sticky:

Keeping track of this data about whether there's a setter above us is unfortunate because any static flag kept on the property which is set at first property access would need to be maintained in the case that the property was deleted from the prototype. This means either calculating it on /every/ property lookup if the property is a set (which seems prohibitively slow), or, perhaps even worse, adding a percolating constraint when a property with a setter is removed that runs down the prototype tree and unsets all the flags. This would be great, except it involves adding pointers down the prototype chain, which will lead to enormous memory blowup (clearly prohibitive, given the current memory expense of TI).

It has also been suggested that we keep a side table of setters, and then do a side lookup in reasonable runtime. Since setters are relatively rare, this seems like it may be a possible solution. A complication comes in the form of the fact that it's hard to know how to key such a table. There are many objects in a given prototype chain (Especially in the DOM, for example) and having to search up the prototype chain for which prototype is in the table, if any, is not really any better than just searching the prototype chain on every access (Note that we can't keep track of which prototype has it after creation for the same reasons listed above).

With all that said, I'm not really sure what direction to push this. All the alternatives seem to have prohibitive red flags on them. Have I missed something? Are there other suggestions?

Comment 2

[Brian Hackett [Laid off!]]

I think it will work to just change the test to:

if (assign && 'this->proto and its prototypes does not *currently* have a custom setter for id')
    types->setOwnProperty(cx, false);

Every time the VM tries to assign to the id it will go through getProperty() to do the update, whether the type object already has that property or not.  So if the setter is later deleted from the prototype then the next time the id is assigned to the property will be marked as 'own'.  This safely abstracts the state of the heap, as the object will not get its 'own' property until an assignment occurs when the prototype does not have a custom setter, even if assignments occurred in the past when the setter was still installed.

Comment 3

[Eric Faust [:efaust]]

This was my first instinct as well, but that seems to add a lot of expense to getProperty in the assign case, since it now has to do the whole prototype walk.

If we think this is a necessary evil, then that's certainly the easiest fix.

Comment 4

[Brian Hackett [Laid off!]]

Er, yeah, there should be a !types->isOwnProperty(false) in that test too before doing the lookup.  Then in the normal case where there is no prototype setter we will only need to do the test for that setter once (insignificant cost) and in the case where there is a prototype setter we will need to do the test every time the VM assigns to that property.  That adds some cost but not that much, these property writes are already crazy expensive when performed from the VM.

Comment 5

[Eric Faust [:efaust]]

What do we want to do about property hooks? Is it correct to only do the check in the case when the prototype chain is clean of them? The problem is, the simplest way to check for the shape is to call obj->lookupGeneric(), but this can call propertyLookup hooks, which will not have the desired behavior in terms of return value and out parameters. Another option is to just call obj->nativeLookup(), but this does not call the resolve hooks, which may evaluate to a setter.

Comment 6

[Eric Faust [:efaust]]

Attachment: FIX

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 634218 [details] [diff] [review]
FIX

Review of attachment 634218 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinferinlines.h
@@ +1284,5 @@
>  
> +    if (assign && !types->isOwnProperty(false)) {
> +        bool foundSetter = false;
> +
> +        JSObject *protoWalk = proto;

This needs a comment to explain what's going on.  Try this:

'assign' is set if id is being directly assigned to on an object of this type.  In most cases this will add a data property on the object and require the 'own' flag on the property types to be set, but if there is a setter on the object's prototype chain then that setter will be invoked instead and no property will appear on the base object.

@@ +1295,5 @@
> +            const Shape *shape = protoWalk->nativeLookup(cx, id);
> +
> +            foundSetter = shape &&
> +                          !shape->hasDefaultSetter() &&
> +                          shape->hasSetterValue();

I don't think the hasSetterValue() bit is necessary, and will cause setter ops which are StrictPropertyOps to be excluded.  Not sure if that is relevant for any of the cases you're interested in.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60fedf46af8e