Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attributes created by JSON.parse cannot be modified #434

Closed
tomasol opened this issue May 31, 2020 · 16 comments · Fixed by #504
Closed

Attributes created by JSON.parse cannot be modified #434

tomasol opened this issue May 31, 2020 · 16 comments · Fixed by #504
Assignees
Labels
bug Something isn't working E-Easy Easy good first issue Good for newcomers
Milestone

Comments

@tomasol
Copy link

tomasol commented May 31, 2020

JSON.parse seems to create object whose attributes cannot be modified (but can be deleted).

Steps To Reproduce
Checkout current master (87aea64)
start cli: ./target/debug/boa
Enter

var a = JSON.parse('{"x":0}');
a.x=2;
a.x

The value a.x is still 0 instead of expected 2.

Build environment (please complete the following information):

  • OS: Ubuntu
  • Version: 20.04
  • Target triple: x86_64-unknown-linux-gnu
  • Rustc version: 1.43.1 (8d69840ab 2020-05-04)
@tomasol tomasol added the bug Something isn't working label May 31, 2020
@HalidOdat
Copy link
Member

The bug is here.
when we inset the fields of the object, we don't set it as writable, configurable and enumerable:

Property::default().value(Value::from(json.clone())).writable(true).configurable(true).enumerable(true),

Doing this should fix it :)

@HalidOdat HalidOdat added this to the v0.9.0 milestone May 31, 2020
@Razican Razican added E-Easy Easy good first issue Good for newcomers labels Jun 1, 2020
@n14little
Copy link
Contributor

I can take this.

@n14little
Copy link
Contributor

Looks like this was partially addressed in #410 . I'm trying to figure out a good way to test the enumerable portion.

@n14little
Copy link
Contributor

Assuming propertyIsEnumerable is implemented, I will go ahead with that unless I hear or think of a better idea.

@Razican
Copy link
Member

Razican commented Jun 2, 2020

I can take this.

Go ahead! :)

@n14little
Copy link
Contributor

n14little commented Jun 3, 2020

@HalidOdat -- I'm trying to test that the properties are enumerable by using the propertyIsEnumerable function which I have rough implementation written here. However, it doesn't seem like the Object.prototype methods are getting added to the new objects created in Value::from_json based on

let mut new_obj = Object::default();

and
pub fn default() -> Self {
let mut object = Self {
kind: ObjectKind::Ordinary,
internal_slots: FxHashMap::default(),
properties: FxHashMap::default(),
sym_properties: FxHashMap::default(),
state: None,
func: None,
};
object.set_internal_slot("extensible", Value::from(true));
object
}

What would be the best way to grab the Object.prototype so that it can be added to those objects?

@HalidOdat
Copy link
Member

HalidOdat commented Jun 3, 2020

What would be the best way to grab the Object.prototype so that it can be added to those objects?

Hmmm. For this we need to take in an Interpreter (context) in Value::from_json, and get the prototype of Object and construct the object, it seems Value::from_json does not do that. I'll create an issue for this.

This issue can be easily tested with:

var a = JSON.parse('{"x":0}');
a.x=2;
a.x

and asserting that a.x == 2

@n14little
Copy link
Contributor

@HalidOdat doesn’t asserting that the value changes only assert that it is writeable? As the bug was written, it is technically no longer a bug. However, the properties are not enumerated so I figured I’d just use this issue for that problem.

@HalidOdat
Copy link
Member

@HalidOdat doesn’t asserting that the value changes only assert that it is writeable? As the bug was written, it is technically no longer a bug. However, the properties are not enumerated so I figured I’d just use this issue for that problem.

I see. Even if we implement propertyIsEnumerable we can't use it if the object does not have a prototype instance so we first need to fix #452

@n14little
Copy link
Contributor

I see. Even if we implement propertyIsEnumerable we can't use it if the object does not have a prototype instance so we first need to fix #452

Agreed.

@n14little
Copy link
Contributor

@HalidOdat @Razican -- As was mentioned earlier in this thread, I'm using propertyIsEnumerable to try to test enumerability on the properties for objects and arrays in Value::from_json. In the Array::create function, I'm trying to create the array prototype using the object prototype based on the fourth bullet here: https://tc39.es/ecma262/#sec-properties-of-the-array-prototype-object. I do this by calling Value::new_object with the global object; however, it seems that the object prototype data is being lost. I've pasted the contents of the function below with a few print statements (and the output below that) that show what leads me to believe that the prototype data is being lost:

/// Returns a new empty object
    pub fn new_object(global: Option<&Value>) -> Self {
        let _timer = BoaProfiler::global().start_event("new_object", "value");
        if let Some(global) = global {
            let object_prototype = global.get_field("Object").get_field(PROTOTYPE);

            println!("object prototype: {:?}", object_prototype);
            let object = Object::create(object_prototype);
            println!("new object: {:?}", object);
            Self::object(object)
        } else {
            Self::object(Object::default())
        }
    }
object prototype: Value(Object(GcCell { value: {
	kind: Ordinary
	state: None
	func: None
	properties: {
		constructor
		hasOwnProperty
		propertyIsEnumerable
		toString
	 }
} }))
new object: {
	kind: Ordinary
	state: None
	func: None
	properties: {
	 }
}

Are my expectations on what should be happening off here? I've pushed my branch up if that would be easier than interpreting what I've stated above. You can find it here

@HalidOdat
Copy link
Member

HalidOdat commented Jun 12, 2020

I do this by calling Value::new_object with the global object; however, it seems that the object prototype data is being lost. I've pasted the contents of the function below with a few print statements (and the output below that) that show what leads me to believe that the prototype data is being lost

Were not losing the object prototype but were not displaying it, the debug printer Debug trait was implemented by hand (no idea why, we should almost always let the compiler generate it for us) https://github.com/boa-dev/boa/blob/master/boa/src/builtins/object/mod.rs#L66-L79 we should delete it and add a Debug in Object derive here https://github.com/boa-dev/boa/blob/master/boa/src/builtins/object/mod.rs#L50

The prototype property prototype is stored in properties (as any property, like 'x', 'foo'), but prototype instance __proto__ is stored in internal_slots (this are properties which can not be accessed through javascript) v8 and spidermonkey allows accessing it

Hope this helps :)

@n14little
Copy link
Contributor

It helps explain why I'm not seeing it in the print statements 😄; however, I'm still not seeing the propertyIsEnumerable function on an array.

@HalidOdat
Copy link
Member

HalidOdat commented Jun 16, 2020

I'm still not seeing the propertyIsEnumerable function on an array.

Hmmm. I think I know why this is happening because the order of the builtins initialization is not right! here Object should be declared before Array.

@n14little
Copy link
Contributor

here Object should be declared before Array.

This doesn't quite seem to do the trick. Everything is added to that global object all at once after everything has been initialized. Thus, when Array::init(global) is called, the object has not been put on the global since all of that happens here. I tried to initialize each thing and then immediately add it to the global object, but I was then met with

thread 'main' panicked at 'GcCell<T> already mutably borrowed', <::std::macros::panic macros>:5:6

@HalidOdat
Copy link
Member

#502 should fix this bug :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working E-Easy Easy good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants