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

Should ScriptableObject.sealObject also prevent modifications of parent and prototype #1085

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Nov 12, 2021

Please consider this PR first as question, if I (mis)understand documentation or if it is really a bug

My use case is, that I have a shared scope and I want to prevent that scope from any modifications.

So I thougt, it would be a good idea to call scope.sealObject()

I noticed, that no normal property is writeable - but I can change the __parent__ and __proto__ of such sealed objects.
(This is also what I try to fix in this PR)

I planned also to implement sealObject in NativeJavaObject, so that you can seal them too, to prevent from further modification. For example block put operations and especially __parent__ and __proto__ property (I'm aware that I can use UnmodifiableMap and so on, but this does not prevent modification of the special props)

Then I found in the documentation of ScriptableObject:

It is possible to change the value of an existing property

This seems not to be true, at least what I have found in my tests. I dig further into documentation and found #676 and https://262.ecma-international.org/11.0/#sec-object.seal which confirms that existing properties should be modifiable.

So there IS a difference between java ScriptableObject.sealObject and js Object.seal (the later one does not call the first one and objects sealed with Object.seal throw only an EcmaError in strict mode, while objects sealed with ScriptableObject.sealObject throw an EvaluatorException)

Maybe someone can explain, what's the intention of sealObject

NativeObject.init(scope, sealed);
BaseFunction.init(scope, sealed);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When these two were swapped, the setPrototype some lines below is not needed

@@ -692,6 +693,7 @@ public Scriptable getParentScope() {
/** Sets the parent (enclosing) scope of the object. */
@Override
public void setParentScope(Scriptable m) {
checkNotSealed("__parent__", 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would prevent modification of SpecialRefs (TODO: Should I make 2 constants SpecialRef.PARENT / SpecialRef.PROTO)?

@p-bakker
Copy link
Collaborator

I think the JavaScript Object.seal and ScriptableObject.sealObject aren't necessarily to do the same thing.

JavaScript's Object.seal probably got added long after ScriptableObject.sealObject. Also, besides Object.seal there's also the more restrictive Object.freeze

To my knowledge, the purpose of ScriptableObject.sealObject is to prevent modifications on shared scopes, in which case I would say that the parent and proto properties should also not be writable.

Then again: no experience here with shared scopes 🙂

Changing the behavior would be a potentially breaking change imho, so something to postpone for v2.0.0?

@p-bakker p-bakker added the Triage Issues yet to be triaged label Jan 28, 2022
@rPraml
Copy link
Contributor Author

rPraml commented May 5, 2022

I've updated this PR.
My use case is still to protect a shared scope from any modification withScriptableObject.sealObject (also __parent__ and __proto__) This is what this PR would fix.

Nevertheless, it would be great if someone could explain the difference and intention of ScriptableObject.sealObject and Object.seal

@p-bakker
Copy link
Collaborator

p-bakker commented May 9, 2022

I think there's no one around anymore to detail the difference between ScriptableObject.sealObject and Object.seal, but I did find this in old release notes: https://p-bakker.github.io/rhino/releases/new_in_rhino_1.5r5.html#seal-and-changes-in-semantic-of-sealed-objects and https://p-bakker.github.io/rhino/releases/new_in_rhino_1.5r5.html#api-for-context-sealing

Just guessing here, but it looks like ScriptableObject.sealObject was added before Object.seal existed and that they ought to behave similarly?

Object.seal(...) however allows changing the values of existing properties, which is not desirable when 'sealing' scopes using ScriptableObject.sealObject(...) to make the scope sharable. so in this respect, ScriptableObject.sealObject(...) is more similar in behavior to Object.freeze(...) (which is like Object.freeze, but also prevents adding new properties).

Note that in browsers, Object.seal(...) makes the proto property readonly, as modifying it could introduce new properties, which Object.seal(...) is to prevent.

Also note that __parent__ is deprecated and non-existing in most JS engines, but as long at its still there in Rhino, I think when sealing (or freezing) an object it should be treated similarly as proto.

So, in summary:

  • I think I agree that ScriptableObject.sealObject(...) should also prevent modification of the proto/parent property
  • ScriptableObject.sealObject(...) behavior is more like Object.freeze than Object.seal
  • Then I found in the documentation of [ScriptableObject](https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/ScriptableObject.java#L1942): It is possible to change the value of an existing property I think this is an anomaly in the JavaDocs, since that behavior seems to have been changes in 1.5r5

So maybe the entire ScriptableObject.sealObject(...) implementation should be rewired to use the Object.freeze implementation, as it seems that are both trying to achieve the same thing

@p-bakker p-bakker added Community input needed Issues requiring community input and removed Triage Issues yet to be triaged labels May 9, 2022
@rPraml
Copy link
Contributor Author

rPraml commented May 13, 2022

So maybe the entire ScriptableObject.sealObject(...) implementation should be rewired to use the Object.freeze implementation, as it seems that are both trying to achieve the same thing

Full ack!

But one must note that Object.seal currently only works in strict mode. (AFAIR! I didn't check, if it prevents write access in non strict mode).

@p-bakker
Copy link
Collaborator

p-bakker commented May 13, 2022

Full ack!

Do you mean 'full acknowledgement', as in agreed?

But one must note that Object.seal currently only works in strict mode. (AFAIR! I didn't check, if it prevents write access in non strict mode).

I'm not aware of that. If that is indeed the case, a case should be created about that I think, cause I dont think there is one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community input needed Issues requiring community input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants