94693 - Enhancement - way of making standard library readonly/permanent

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[petew]

A really useful enhancement to SpiderMonkey would be to allow the creation of 
the core JavaScript libraries in a readonly/permanent way. In some environments 
such as server-side JavaScript this can prevent the classes being created again 
for each page served by a process in case the previous pages modified the 
libraries.

I believe Rhino has this facility - although I have never used Rhino!
Pete

Comment 1

[Brendan Eich [:brendan]]

I'll take this unless someone wants it -- it's an old RFE on SpiderMonkey that
Rhino satisfied long ago with object sealing.

/be

Comment 2

[Brendan Eich [:brendan]]

Bumping, but not far.

/be

Comment 3

[Brendan Eich [:brendan]]

Per the mozilla 1.0 manifesto, this is so Future.

/be

Comment 4

[Brendan Eich [:brendan]]

shaver points out that by making JSClass.resolve fail, one can seal objects of
that class against property gets and sets that might otherwise create an ad hoc
property.  He mentions that one must be willing to delegate to prototypes rather
than failing, if that's the right thing.  Standard classes do use delegation and
lazy (resolve-driven) reflection, so some care will be required to use this
approach, but it seems promising to me -- it beats having some kind of random
logic based on a "sealed" flag that must be stored somewhere in each object.

Comments?

/be

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We could probably meet API users halfway, by providing JSSealedObjectResolve and
JSSealedDelegatingObjectResolve for them to just plug in, avoiding error-prone
and tedious recoding of such methods.

That doesn't really solve the standard classes issue, but it would provide a
basic mechanism for us to use underneath a
JS_InitStandardClassesSealed/JS_ResolveStandardClassSealed
(SealedStandardClass(es)?) API growth.  More thought required for that piece,
clearly: some standard classes resolve, I think.

Comment 6

[Brendan Eich [:brendan]]

I'm trying to make js1.5 align with mozilla1.0, and I don't think this lack is a
1.5-stopper -- it's clearly not a mozilla1.0-stopper.  We can always do a 1.5.1
release if there's a hue and cry soon after 1.5 is shipped.

/be

Comment 7

[Brendan Eich [:brendan]]

Patch in a second.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix

I want this patch for 1.4a, because it enables a fix for bug 129496 based on
sealing a cloneable prototype object at compile time for each object literal. 
At runtime, unless an ad-hoc property not in the literal is added, only
lightweight JSObjects with slots are needed -- they all share the prototype's
scope.

And even if one of these lightweight objects mutates at runtime via the
addition of an ad-hoc property, the scope it gets as its own doesn't need to
copy a bunch of properties -- it just needs to extend the runtime's
scope-property tree from the proto-scope's lastProp, and possibly get a hash
table if populous enough.

/be

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 117135 [details] [diff] [review]
proposed fix

You should use JS_ConvertArguments in Seal instead of your explicit calls, if
only because it promulgates that righteous style.

Should SealObjectGraph not call SealObjectGraph rather than SealObject for its
slots?	I would expect

js> o = {p:{p:{p:{}}}};
js> seal(o);
js> o.p.p.p.f = 0;

to fail.

Do we want an API for unsealing, as well?  I would think so.

Comment 10

[Brendan Eich [:brendan]]

Attachment: better fix, thanks shaver

No bloaty JS_SealObject and JS_SealObjectGraph, just the former with a JSBool
deep third param; ditto for JS_UnsealObject.  And don't forget to check
SCOPE_IS_SEALED in js_SetProperty, and beware the property cache!

/be

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 117165 [details] [diff] [review]
better fix, thanks shaver

r=shaver, but you gotta run the JS suite and browse a little before you hit the
tree.  M'kay?

Comment 12

[Brendan Eich [:brendan]]

Fixed.  js/tests all good, mozilla -P test looks great (I'm reading /. about 1.3
and following links).

/be

Comment 13

[Phil Schwartau]

I ran the JS testsuite in both the debug and optimized JS shell on
WinNT4.0, Linux RH7.2, and Mac OSX, and found no test regressions.

Provisionally marking Verified FIXED. But the example Mike gave 
above does not fail. Is that correct?

> Should SealObjectGraph not call SealObjectGraph rather than SealObject
> for its slots? I would expect

> js> o = {p:{p:{p:{}}}};
> js> seal(o);
> js> o.p.p.p.f = 0;
> 
> to fail.


I get no failure:

js> o = {p:{p:{p:{}}}};
[object Object]
js> seal(o);
js> o.p.p.p.f = 0;
0
js> o.p.p.p.f;
0

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

No, my example was bogus: seal needs to be called as "seal(o, true)" in order to
seal the entire graph.  Sorry.

Comment 15

[Phil Schwartau]

Still not failing for me. Am I doing something else wrong?

js> o = {p:{p:{p:{}}}};
[object Object]
js> seal(o, true);
js> o.p.p.p.f = 0;
0
js> o.p.p.p.f;
0

Comment 16

[Phil Schwartau]

This is how it seems to be working:

js> o = {p:{p:{p:{}}}};
[object Object]
js> o.toSource()
({p:{p:{p:{}}}})
js> seal(o, true);


1. Can't modify any existing propery in the graph.
   No error is given (even in strict mode):

js> o.p = 'Hello';
Hello
js> o.toSource()
({p:{p:{p:{}}}})
js> o.p.p = 'Hello';
Hello
js> o.toSource()
({p:{p:{p:{}}}})
js> o.p.p.p = 'Hello';
Hello
js> o.toSource()
({p:{p:{p:{}}}})


2. Can't add a property to the existing graph until you reach the end:
js> o.f = 'Hello'
13: Error: o is read-only
js> o.p.f = 'Hello'
14: Error: o.p is read-only
js> o.p.p.f = 'Hello'
15: Error: o.p.p is read-only
js> o.p.p.p.f = 'Hello'
Hello
js> o.toSource()
({p:{p:{p:{f:"Hello"}}}})


So properties cannot be added to |o|, |o.p|, |o.p.p|,
but they can be added to the lowest element, |o.p.p.p|.

Is that the way it should work?

Comment 17

[Brendan Eich [:brendan]]

Argh, there's a path through js_SetProperty that doesn't test SCOPE_IS_SEALED:
the case where the property is not inherited from a prototype, and does not exist!

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: close the hole in js_SetProperty

I moved the readonly check down and used a downward goto to share it, but that
goto will disappear some day when the "XXXbe ecma violation"-commented-code is
removed.

/be

Comment 19

[rogerl (gone)]

Comment on attachment 117493 [details] [diff] [review]
close the hole in js_SetProperty

r=rogerl

Comment 20

[Brendan Eich [:brendan]]

I checked in with r=rogerl, but I'm pretty sure shaver will r= shortly.  I need
to do more work on jsobj.c and wanted to disentangle patches.

/be

Comment 21

[Phil Schwartau]

Reopening, because I think there is something wrong with this fix.
Please correct me if I'm wrong:

js> var o = {};
js> seal(o, true);
js> function f() {}
4: Error: [object global] is read-only

I wouldn't expect the global object to be sealed by sealing |o|.
We also seem to be missing '[object global]' in this variation:

js> var o = {};
js> seal(o, true);
js> var x = 1;
4: Error:  is read-only


Another question: is seal() supposed to have any effect on non-object
variables? What I'm seeing is that seal(x) does not have any effect,
whereas seal(x, true) does:

js> var x = 1;
js> seal(x);
js> x = 2;
2
js> x
2

COMPARE: here |x| is made read-only by seal(x, true)

js> var x = 1;
js> seal(x, true);
js> x = 2;
2
js> x
1

This may simply be another side effect of the previous problem,
since |x| is a property of the global object, i.e. |this.x|

Comment 22

[Brendan Eich [:brendan]]

seal(o, true) follows __parent__ up to the global object, sealing it too.  In
the JS shell, you can't prevent that, because while you can null o.__proto__
before calling seal, you can't set o.__parent__ (it's readonly).

JS_SealObject is for embeddings that want to create a global object, init its
standard objects, and then seal the whole graph for sharing among N threads. 
The test harness in the shell is of limited use, although I suppose I could add
something to force the parent slot to null.

/be

Comment 23

[Phil Schwartau]

Brendan: thanks. 

There still remains a gap in the error message, however. 
Could that be fixed without too much trouble?

js> var o = {};
js> seal(o, true);
js> var x = 1;
4: Error:     is read-only
          ^^^


I also have a question about the behavior of unseal(o, true).
The following conforms to my expectations:

js> var o = {p:{p:{p:{}}}};
js> seal(o, true);
js> unseal(o, true);
js> var x = 1;
js> x
1

That is, |unseal(o, true)| successfully undid |seal(o, true)|,
and so we were able to define |x|. However, this breaks down
if we interpose a call to |unseal(o)|:

js> var o = {p:{p:{p:{}}}};
js> seal(o, true);
js> unseal(o);
js> unseal(o, true);
js> var x = 1;
7: Error:  is read-only

I know this is a minor point, but that isn't right, is it?
Shouldn't |unseal(o, true)| be able to perform its original
function, even after |unseal(o)| has been called?

Comment 24

[Phil Schwartau]

Brendan and I spoke about possibly opening a new bug on the 
questions above, or simply reopening this one. I think I'll
reopen, since the context is here -

Comment 25

[Brendan Eich [:brendan]]

I don't think unseal (JS_UnsealObject) with true passed for the deep parameter
should walk past an already-unsealed object.  Doing so requires a separate depth
first search mark bit, and a cleanup pass to clear all such bits that were set,
or equivalent schemes.  I'd rather just note that JS_UnsealObject with deep=true
does not unseal kids of an object that it reaches in zero or more steps from its
obj argument if that object is already unsealed.

Patch for the "gap" in the error message coming soon.

/be

Comment 26

[Brendan Eich [:brendan]]

Attachment: fix for gappy error message

JSOP_BINDNAME lacked JOF_SET, and it was the only op with JOF_ASSIGNING but not
JOF_SET.  So JOF_SET and JOF_ASSIGNING are one and the same now.  I am going to
check in this minimized change right now, r=me.  No one else understands the
decompiler, anyway.

/be

Comment 27

[Brendan Eich [:brendan]]

I also tweaked jsscope.c to make ReportReadOnlyScope name only the object, by
its toString result, instead of the property name, which is not itself
"read-only" -- the object in which a script is attempting to bind the name is
read-only, and the name does not identify any property, read-write or read-only
or write-only, in that object!

/be

Comment 28

[Phil Schwartau]

The latest check-in fixes the error message for the global object,
but has eliminated this same message for sub-objects, e.g. |o|. 

Recall the behavior we had in Comment #16:

1. Can't modify any existing property ...
   No error is given (even in strict mode):

js> o.p = 'Hello';
Hello
js> o.toSource()
({p:{p:{p:{}}}})

2. Can't add a new property ...
   A "read-only" error is given:

js> o.f = 'Hello'
13: Error: o is read-only
js> o.toSource()
({p:{p:{p:{}}}})



-----------------------  WHAT WE HAVE NOW  -----------------------
js> o = {p:{p:{p:{}}}};
[object Object]
js>  o.toSource()
({p:{p:{p:{}}}})
js> seal(o, true);

js> o.p = 'Hello'                         (modify existing property)
Hello               <<<------------------ As before, no error given 
js>  o.toSource()
({p:{p:{p:{}}}})

js> o.f = 'Hello'                         (add a new property)
Hello               <<<------------------ Read-only error no longer given!
js>  o.toSource()
({p:{p:{p:{}}}})


js> var x = 1;                            (add new properties to global object)
13: Error: [object global] is read-only   <<<--- Error correctly given   
js> function f() {}
14: Error: [object global] is read-only   <<<--- Error correctly given


So, from Comment #16, behavior 1 is intact; behavior 2 has been lost. 
Is is right to only report the read-only error for the global object,
and not for the object |o| itself, as was formerly done?

The same question applies for |seal(o)| as well as |seal(o, true)|.

Comment 29

[Phil Schwartau]

This new behavior may be already explained in Comment #27, but I just
wanted to get clarification. From a user's point of view, it's odd to
get the error for one object, but not the other -

Comment 30

[petew]

I originally raised this enhancement request, although you may be using it to 
answer a slightly different problem. I'm confused by the issue with the global 
object getting locked because an object contained within that object gets 
locked. The example in comment #21.

When I embed SpiderMonkey - I want to lock the standard classes - not the 
object to which they are attached. I should still be able to create other 
global objects otherwise I'm a little confused how I'd write any code (maybe by 
placing an even more global object above global in the scope chain?

Anyway - it would seem that if I wanted to seal the global object I'd:
   seal(globalObject,true)
If I want to seal 'var o' then I'd call 
   seal(o,true);

For seal to go up a level and seal from the parent downwards seems a little 
convoluted and doesn't provide a mechanism for sealing an object unless I first 
create a child of that object.
  var o = {a:1};
  seal (o.a,true);

It is entirely possible that I'm missing the point here - but my original aim 
was to seal the standard classes (and my proprietrary embedded classes) so that 
I didn't have to recreate them each time I run a script. I still want those 
scripts to be able to create their own global variables. 

Is this an issue where unless you seal global:
   seal(String,true)
   String = function() {...}
allows the String class to be redefined? That may not be an issue. I may not 
mind if within a script the user chooses to replace the String class as long as 
my verbatim copy remains uncorrupted. When I start the next script I will be 
creating a new virgin global object and can attach the original pristing String 
class to it.

Comment 31

[Brendan Eich [:brendan]]

Phil, the difference is between declarations (var and function) vs. assignments.
 Assigning to a readonly property, per ECMA, silently fails to set the value,
but the result of the assignment expression is the right-hand side value (even
though it did not get stored in the left-hand side).  If you switch to
version(120) in the shell, or use JavaScript1.2, you'll get a pre-ECMA error for
assignments, as for declarations.

petew: you may want JS_SealObject to skip the parent (and proto, I suppose)
slots and mark only "downward tree arcs", but who is to say that a property
might not link through a backward or up-tree arc to some other part of the live
object graph?  JS does not require tree-like property linkage, even if you
ignore the parent slot.

I reckoned that you *do* want to preclude String from being redefined, so you do
want to seal an entire object graph newly created, starting from global object 
and reaching all reachable objects.  But if you would prefer a mode where the
parent (and proto? skipping proto would defeat the thread-safety immutability
and share-ability among N different trust domains) slot is skipped, please file
an RFE, or ask Phil to file it -- I'll fix it quickly.

/be

Comment 32

[Phil Schwartau]

> Assigning to a readonly property, per ECMA, silently fails to set the value

Sorry for any confusion. My question wasn't about setting the existing
read-only property |o.p|, but about creating the new property |o.f|. 

Previously, we got an error for that (see Comment #16, number 2.)
With the latest checkin, we don't - except for the global object.


EXAMPLE (without using the |var| keyword). Note the property |f|
is nonexistent before sealing, both in |o| and in global scope:

js> o = {p:{p:{p:{}}}};
[object Object]
js> seal(o, true); <<<--- seals up the __parent__ chain to global object

js> f=1;
10: Error: [object global] is read-only

js> o.f=1;
1   <<<---- no error message

We also get no error message if we try to define |f| on the
global object in this manner:

js> this.f = 1;
1   <<<---- no error message


I suppose one might argue, "You don't get an error message for
|o.f = 1| for the same reason you don't get one for |this.f = 1|."

But I believe that according to ECMA, |this.f = 1| and |f = 1|
are syntactically equivalent. If one errors, so should the other,
and so should |o.f = 1|.

My main concern is not ECMA-correctness, however. I just find
it confusing as a user to get a read-only error under certain 
conditions, but not under others which seem to be the same.

Comment 33

[Phil Schwartau]

Per discussion with Brendan, reopening to see if the error reporting
can be made more consistent -

Comment 34

[Brendan Eich [:brendan]]

I'd like to fix this by making the sealed-scope error cases all real "read only"
errors, not ECMA "silent failure to store the assigned value" no-ops.

/be

Comment 35

[Brendan Eich [:brendan]]

Attachment: proposed fix

Sealing objects is an ECMA extension, and per ECMA-262 Edition 3 Chapter 16, we
are allowed to throw exceptions due to erroneous use of extensions, such as
when you try to set a new or existing, non-readonly property in a sealed
object.

Note that when you set a readonly property in a sealed object, we follow ECMA
and make the attempt appear to succeed, by the result of the assignment
expression, but the attempt does not actually store the value in the readonly
property.  Only for read-write existent, and for non-existent, property sets in
sealed objects, do we throw the "read only" error-as-exception.

/be

Comment 36

[rogerl (gone)]

Comment on attachment 120138 [details] [diff] [review]
proposed fix

r=rogerl. [I don't understand the decompiler] Isn't passing NULL as the
fallback string potentially going to cause the error to go unreported if the
decompiler should fail to decompile for a reason other than a system fault?
How comes the '{' gets to live on the label line, isn't that against the rules?

Comment 37

[Brendan Eich [:brendan]]

rogerl: the label must apply to the block, because of the str block-local (or we
need a labeled block immediately within the then-block, which seems worse). 
This is new territory, stylistically speaking.  I'm open to suggestions, but I
don't think the labeled brace is all that bad.

As for the null fallback, note the last line of js_DecompileValueGenerator.  It
copes with a null fallback by using js_ValueToString.  If that fails, id was an
int and we are very low on memory, so we'll drop the error-as-exception in
js_SetProperty and unwind.

/be

Comment 38

[Phil Schwartau]

In testing the latest patch, I get results that seem the reverse of
the goals stated in Comment #35. That is, setting the existing property
generates the error, but trying to create a new property does not:


js> o = {p:{p:{p:{}}}};
[object Object]

js> seal(o, true);

js> o.p = 1;
10: Error: o.p is read-only

js> o.f = 1;
1


Also, we still have an inconsistency in error reporting for the
global object, on assignments that are syntactically equivalent:

js> f = 1;
19: Error: [object global] is read-only

js> this.f = 1;
1   <<<---- no error message

Comment 39

[Brendan Eich [:brendan]]

Attachment: the right fix for all cases (new or existing property)

Phil, can you give this a try?	I'm hacking on my home PC, without MSVC
installed.

/be

Comment 40

[Brendan Eich [:brendan]]

Comment on attachment 120214 [details] [diff] [review]
the right fix for all cases (new or existing property)

The ugly goto and labeled block will go away when the "XXXbe ECMA violation:
..." bug 90596 is fixed.  Then, the only  "read only" error you'll get is when
trying to set a new or existent property that's directly in the object (not in
one of its prototypes) that has its own sealed scope.

/be

Comment 41

[Phil Schwartau]

The latest patch works very well on seal(o, true).
I have a few questions farther down on seal(o).

js> o = {p:{p:{p:{}}}};
[object Object]

js> seal(o, true);

js> o.p = 1;
13: Error: o.p is read-only
js> o.f = 1;
14: Error: o.f is read-only

js> f = 1;
17: Error: [object global] is read-only
js> this.f = 1;
18: Error: this.f is read-only


Verifying that nothing changed in |o| or |this| after |o| was sealed:

js> o.toSource();
({p:{p:{p:{}}}})
js> this.toSource();
({o:{p:{p:{p:{}}}}})


I have also tried more elaborate examples (and will continue to do so).
Everything looks great. Furthermore, the patch passes the JS testsuite
on WinNT in the optimized JS shell: no test regressions are observed.


However, I want to clarify the behavior of seal(o) if the second parameter
is omitted:

js> o = {p:{p:{p:{}}}};
[object Object]

js> seal(o);

js> o.p.p.p.f = 0;
0
js> o.toSource()
({p:{p:{p:{f:0}}}})


Note we were able to add the new property |f| to |o.p.p.p|. Is that right?
The example comes from Mike's question in Comment #9 above:

---
Should SealObjectGraph not call SealObjectGraph rather than SealObject
for its slots?	I would expect

js> o = {p:{p:{p:{}}}};
js> seal(o);
js> o.p.p.p.f = 0;

to fail.
---


The way things work with the latest patch, we only get blocked
when we try to change |o.p|. We ARE able to change |o.p.p.p|, etc. :


js> o.p.p.p = 0;
0
js> o.toSource()
({p:{p:{p:0}}})

js> o.p.p = 0;
0
js> o.toSource()
({p:{p:0}})

js> o.p = 0;
25: Error: o.p is read-only


Is that the proper behavior of seal(o) without the |true| parameter?

Comment 42

[Phil Schwartau]

Oops - answering my own question. I guess that is the correct behavior
for seal(o), as Mike explained to me in Comment #14  -

Comment 43

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 44

[Phil Schwartau]

Marking Verified FIXED from the results above. One final note:

The following shows that in the JS shell, |seal(o1, true)| not only
propagates up the __parent__ chain to |this|, but also back down
again to any other object |o2|:

js> o1 = {p:{p:{p:{}}}};
[object Object]
js> o2 = {p:{p:{p:{}}}};
[object Object]

js> seal(o1, true);            <--- seal |o1| with |true| parameter

js> o2.p = 1;                  <---- now |o2| is read-only
14: Error: o2.p is read-only  
js> o2.f = 1;
15: Error: o2.f is read-only


That seems correct to me, at least in the JS shell. That's because
the |true| parameter is propagating up to the global object along
with the seal. Thus we have this chain of events: 

           seal(o1, true) ---> seal(this, true)

And as we've seen above, |seal(this, true)| should seal every object
property of |this|, which will include |o2|.

NOTE: the behavior in the JS shell will not necessarily pertain to
other embeddings of the JS Engine. See Brendan's Comment #22 -

Comment 45

[Matthias Versen [:Matti]]

Comment on attachment 117493 [details] [diff] [review]
close the hole in js_SetProperty

clearing obsolete review request