203013 - Any modification of sealed object should throw exception?

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Igor Bukanov]

Rhino supports a notion of a sealed object where properties can not be added /
deleted from it. It does not make the object fully immune since the existing
properties can be still modified. On the other hand Rhino also has an option to
initialize standard objects as sealed ones but then it also makes all properties
of the standard objects read only.

In my opinion the later behavior is more natural and meets expectations that
sealed == immune. Thus I suggest to make sealed == immune to hold always.

Comment 1

[Igor Bukanov]

Attachment: sealed == immune

Patch makes sealed objects to behave as if their properties are read only.

Comment 2

[Igor Bukanov]

I commited the patch.

Comment 3

[Phil Schwartau]

Checkin verified -

Comment 4

[Igor Bukanov]

Any modification of sealed object should throw exception?

I reopen this since it seems that the notion 

sealed object -> all properties read only 

is less useful then the following 

sealed object -> any attempt to modify the object should fail with exception.

In this way one would have guaranty that if a script that operates with sealed
objects runs without exceptions then 

1. the sealed objects are not modified
2. the script would run exactly in the same way if objects are not sealed

Currently only 1 is satisfied since with read-only properties sealed objects are
immune to modifications indeed, but the item 2 is not addressed as changes to
read-only properties are silently ignored. 

If one has a script with

Object.prototype.hasOwnProperty = myHasOwnproperty

and later to save memory one decided to share sealed standard objects between
different script invocations then the fact that it is not possible to do this
with the above script would not be visible immediately. 

Moreover, read-only properties ignore changes on whole prototype chain so if
Object.prototype is sealed with the current semantic then

x = { hasOwnProperty: 1 }

would not set hasOwnProperty in x since Object.prototype.hasOwnProperty is read
only. In this example it makes perfect sense to have sealed shared standard
library, but the current rules prevent it.

On the other hand if sealed would mean that any modification to the object
should fail without affecting prototype chain semantics, in the first case one
would get an exception telling what is wrong and the second case would continue
to work in the same way.

Comment 5

[Igor Bukanov]

New title

Comment 6

[Igor Bukanov]

Attachment: sealed == any modification throws exception

The patch also adds -sealedlib options to Rhino shell so after 
java -jar js.jar -sealedlib 
I have:

Rhino 1.5 release 5 0000 00 00
js> Math.sin = 1
js: Cannot modify a property of a sealed object: sin.
js>  Object.prototype.hasOwnProperty = 1
js: Cannot modify a property of a sealed object: hasOwnProperty.
js>  x = { hasOwnProperty: 1}
[object Object]
js> x.hasOwnProperty 
1

Comment 7

[Igor Bukanov]

I committed the change in sealed semantics.

Comment 8

[Phil Schwartau]

I have a question. I do see what Igor reported above:

----------------  WITHOUT -sealedlib OPTION  ----------------
[ ] java org.mozilla.javascript.tools.shell.Main
Rhino 1.5 release 5 0000 00 00
js> Math.sin = 1;
1
js> Math.sin
1

------------------  WITH -sealedlib OPTION  -----------------
[ ] java org.mozilla.javascript.tools.shell.Main -sealedlib
Rhino 1.5 release 5 0000 00 00
js> Math.sin = 1;
js: Cannot modify a property of a sealed object: sin.


However, I am able to modify |eval|, for example:

js> eval = 1;
1
js> eval;
1
js> eval('a');
js: uncaught JavaScript exception: TypeError: 1 is not a function.


That isn't the expected behavior under -sealedlib, is it?
Shouldn't |eval| become ReadOnly then, just like |Math.sin|?

Comment 9

[Phil Schwartau]

Compare the current SpiderMonkey shell:

---------------------  WITHOUT SEALING  ---------------------
js> eval = 1;
1
js> eval;
1

-----------------------  WITH SEALING  ----------------------
js> seal(this);
js> eval = 1;
7: Error: eval is read-only

Comment 10

[Igor Bukanov]

The option -sealedlib in the current form seals only the standard library
objects themselves, it does not affect the global scope object and its
properties. Otherwise no new properties can be added to the global object. So
even with -sealedlib one can write Math = Object even if eval.length = 2 gives
an exception.

This is confusing, but then it is for another bug report. 

Comment 11

[Igor Bukanov]

Attachment: seal function in Rhino shell

The patch adds seal function to Rhino shell with to match SM.

Comment 12

[Igor Bukanov]

I committed the above patch to add seal so now in Rhino one can also have:

js> seal(this)
js> eval = 1
js: "<stdin>", line 2: Cannot modify a property of a sealed object: eval.

Comment 13

[Phil Schwartau]

Looks good:

js> seal(this);
js> eval = 1;
js: "<stdin>", line 4: Cannot modify a property of a sealed object: eval.


But I still have some questions:

1. If we have the seal() function, do we also want to have
   the optional second parameter, as in SpiderMonkey?

   seal(obj, true) ---> seals the entire object graph,
                        including the __parent__ chain 

   See bug 94693 comment 10, bug 94693 comment 22, etc.


2. The help message needs updating, to mention the -sealedlib option:

   [ ] java org.mozilla.javascript.tools.shell.Main -XXX
   Didn't understand "-XXX".
   Valid arguments are:
       -w
       -version 100|110|120|130|140|150
       -opt [-1|0-9]
       -f script-filename
       -e script-source


3. Should |seal(this)| prevent the user from doing |this = 1| ? 
   Note: the latter is currently causing a Rhino crash, independently
   of any sealing issues. I have filed bug 206658 -

Comment 14

[Igor Bukanov]

Targeting as resolved against 1.5R5