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

Extract the Object.seal/freeze/isSealed/isFrozen logic, fixing a few bugs in the process #922

Merged
merged 4 commits into from
Jun 7, 2021

Conversation

p-bakker
Copy link
Collaborator

@p-bakker p-bakker commented Jun 3, 2021

For #243 the logic to freeze an object needs to be accessible from Java code

This PR extracts the internal logic of Object.seal/freeze/isSealed/isFrozen to the abstract object operations testIntegrityLevel & setIntegrityLevel as defined by EcmaScript, as on that level the logic needs to be reused in Tagged Template Literals

In the process, the following issues were encountered and fixed:

  • Three of the aforementioned 4 methods of Object were not taking Symbols into account
  • getOwnPropertyDescriptor for length property of array didn't properly represent its actual state (due to using a cached attributes value that wasn't being kept up to date)
  • proper handling of attempt to update length property on frozen array: again due to stale cached attributes value
  • IdScriptableObject overwrote .defineOwnProperty(Context cx, Object key, ScriptableObject desc) instead of .defineOwnProperty(Context cx, Object key, ScriptableObject desc, boolean checkValid), causing it's logic to never be triggers when Rhino code calls the later overload directly

bugs in the process

- all operations now properly take Symbols into account
- getOwnPropertyDescriptor for length property of array properly
represents state
- proper handling of attempt to update length property on frozen array
Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

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

I have a few minor-ish things I'd love to have you look at, but otherwise thanks for moving this forward.

@p-bakker
Copy link
Collaborator Author

p-bakker commented Jun 7, 2021

From my POV this branch is now ready to merge

@gbrail
Copy link
Collaborator

gbrail commented Jun 7, 2021

LGTM for me too. Thanks!

@gbrail gbrail merged commit 1e4a000 into mozilla:master Jun 7, 2021
@p-bakker p-bakker added this to the Release 1.7.14 milestone Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants