Boa: Altering vital properties of builtins

[54k1]

Describe the bug
Some internal properties (which must not be allowed to set by the user) are being set.

Ex:

>> Object.prototype = 12
12
>> Object.prototype
>> new Object()
thread 'main' panicked at 'assertion failed: prototype.is_null() || prototype.is_object()', boa/src/builtins/object/mod.rs:433:9

Expected behavior
Such things must not alter the contents of a property. For example, in the case of Object.prototype, the spec tells the property must be NOT (writable, enumerable, configurable). But it looks like the property is being set using the insert_field method where the attributes of the property are (WRITABLE, ENUMERABLE, CONFIGURABLE).

[54k1]

I think one solution is to add a new method on objects: insert_internal_property (or any other name) which will insert a new property with attributes (READONLY, NON_ENUMERABLE, PERMANENT). Then we must replace the calls to insert_field at various places by insert_internal_property.

@HalidOdat could you share your thoughts about this?

[HalidOdat]

I think one solution is to add a new method on objects: insert_internal_property (or any other name) which will insert a new property with attributes (READONLY, NON_ENUMERABLE, PERMANENT). Then we must replace the calls to insert_field at various places by insert_internal_property.

@HalidOdat could you share your thoughts about this?

Hmmm. I would rename insert_propertyto insert and add insert_property with the following signiture:

#[inline]
pub(crate) fn insert_property<K>(&mut self, key: K value: V, attribute: Attribute) -> Option<Property>
where
    Key: Into<PropertyKey>,
    V: Into<Value>,
{
    self.insert(key.into(), Property::data_descriptor(value.into(), attribute))
}

This way forces us to change all calls to add the attributes:

object.insert_property("length", 10, Attribute::READONLY | Attribute::NON_ENUMERABLE | Attribute::PERMANENT);

And we remove the insert_field

What do you think @54k1 ?

[54k1]

pub(crate) fn insert_property<K>(&mut self, key: K value: V, attribute: Attribute) -> Option<Property>

Actually I do not understand why insert_property must return an Option.

This way forces us to change all calls to add the attributes:

Yes, this would be better. However, I think it'll also be handy to have a function to insert properties whose attribute is (READONLY, NON_ENUMERABLE, PERMANENT) since there are many properties (>=68) with those attributes. This way code can be simpler at call sites in those cases

object.insert_internal_property("length", 1);

@HalidOdat what do you think about this?

[HalidOdat]

Actually I do not understand why insert_property must return an Option.

If there already was a property with with the same key then a the old property is returned and the new property is put inplace, other wise one is returned this is what HasHMap::insert() does.

However, I think it'll also be handy to have a function to insert properties whose attribute is (READONLY, NON_ENUMERABLE, PERMANENT) since there are many properties (>=68) with those attributes. This way code can be simpler at call sites in those cases

We have a Attribute::DEFAULT with (READONLY, NON_ENUMERABLE, PERMANENT):

object.insert_property("length", 10, Attribute::DEFAULT);

Having the property be explicit, as the spec does, is nice. If we have multiple places we can factor it out:

let attribute = Attribute::READONLY | Attribute::NON_ENUMERABLE | Attribute::PERMANENT;
object.insert_property("length1", 1, attribute);
object.insert_property("length2", 2, attribute);
object.insert_property("length3", 3, attribute);
object.insert_property("length4", 4, attribute);
object.insert_property("length5", 5, attribute);
object.insert_property("length6", 6, attribute);

To me the name insert_internal_property is not descriptive of what it does.

[54k1]

We have a Attribute::DEFAULT with (READONLY, NON_ENUMERABLE, PERMANENT):

I should have taken a closer look before itself. Sorry about that.

We can then have the insert_property with the signature as suggested by you. I can get started with this.

[54k1]

Actually, there's one more method which is used to insert properties: insert_field. In case we want all property inserts to take an explicit attribute as argument, then we must remove it.

[HalidOdat]

Actually, there's one more method which is used to insert properties: insert_field. In case we want all property inserts to take an explicit attribute as argument, then we must remove it.

Yes. the insert_field should be removed.

[54k1]

I was just looking at this again. I think the current insert_property is named appropriately.
For inserting new properties(by specifying Value and Attribute), I think insert_field is better suited than plain insert.
We can have a new insert_field with the signature you had suggested previously.

#[inline]
pub(crate) fn insert_field<K>(&mut self, key: K value: V, attribute: Attribute) -> Option<Property>
where
    K: Into<PropertyKey>,
    V: Into<Value>,
{
    self.insert(key.into(), Property::data_descriptor(value.into(), attribute))
}

Sorry for bringing this naming issue up and dragging this a bit too much :sweat_smile:.

[HalidOdat]

Hmmm. I think it's fine to call it insert the only thing that can be inserted is a property descriptor.
The property descriptor are of two types data descriptor and accessor, so calling it insert_property (by "property" we mean a normal property like "length" not a property descriptor) is fine, in the future we when we will support acessors we will have a insert_accessor.