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

Uncaught TypeError when calling method on global object #1987

Closed
lupd opened this issue Mar 29, 2022 · 2 comments
Closed

Uncaught TypeError when calling method on global object #1987

lupd opened this issue Mar 29, 2022 · 2 comments
Assignees
Labels
bug Something isn't working execution Issues or PRs related to code execution
Milestone

Comments

@lupd
Copy link
Contributor

lupd commented Mar 29, 2022

Calling a method of the global object (using this/ globalThis) results in a TypeError.

To Reproduce
This JavaScript code reproduces the issue:

this.hasOwnProperty("length")

Expected behavior
true is printed

Actual behavior
Uncaught "TypeError": "not a callable function" is printed.

Additional context
At least one test in the test suite requires the expected behavior above.

@lupd lupd added the bug Something isn't working label Mar 29, 2022
@Razican Razican added this to the v0.16.0 milestone Aug 22, 2022
@Razican
Copy link
Member

Razican commented Aug 22, 2022

It turns out that we have two bugs related to the global object:

  • When we initialize builtins, they are added to the Global environment record as bindings, not as properties of the global object, which means that this and globalThis are empty objects ({}).
  • The global object has no prototype. This is required by the spec, and as far as I can tell, it should probably be the Object prototype, since hasOwnProperty() and others are part of the Object prototype.

The first issue is relatively easy to fix: we can just insert properties in the global object when we initialize builtins. In theory, we should not create bindings for these, and the global environment should check if a variable is part of the global object or one of the global bindings, but removing the insertion into the global bindings breaks around 50k tests.

The second issue is trickier, since when the global object gets constructed, the Object built-in doesn't exist, so we cannot easily assign its prototype to it. I will publish a PR that solves the first issue, but the second one will need some extra thought.

@jedel1043 jedel1043 added the execution Issues or PRs related to code execution label Aug 22, 2022
@jedel1043 jedel1043 moved this to In Progress in Boa pre-v1 Aug 22, 2022
@raskad
Copy link
Member

raskad commented Aug 23, 2022

  • When we initialize builtins, they are added to the Global environment record as bindings, not as properties of the global object, which means that this and globalThis are empty objects ({}).
  • The global object has no prototype. This is required by the spec, and as far as I can tell, it should probably be the Object prototype, since hasOwnProperty() and others are part of the Object prototype.

The way the global object currently works is, that it uses special internal object methods defiend in GLOBAL_INTERNAL_METHODS in boa_engine/src/object/internal_methods/global.rs. That way it can use the global environment bindings as properties. The implementation currently completeley ignores the objects prototype. The best example is global_has_property where only the global property map is checked while the spec suggests that the object's prototype should be checked if no property is found on the object itself.

I think the best way to fix this issue is to add a global_prototype field on the Realm (same principle as global_property_map and global_extensible). Then the internal methods for the global object must be adjusted to use this global prototype in the same way that ordinary internal methods use the objects prototype. Hopefully this can be implemented without impacting the performance of those methods.
Additionally the internal methods __get_prototype_of__, __set_prototype_of__ and __own_property_keys__ must be implemented for global object as they are currently taken from the ordinary methods.

While the spec currently does not define that the prototype of the global object should be Object.prototype, there is an open issue that attempts to make sure that it is somewhere in the prototype chain tc39/ecma262#1967.

@raskad raskad self-assigned this Sep 12, 2022
bors bot pushed a commit that referenced this issue Sep 18, 2022
This Pull Request closes #894.

It changes the following:

- Adds the `encodeURI()`, `decodeURI()`, `encodeURIComponent()` and `decodeURIComponent()` functions
- Passes all the tests except for those depending on #1987 or on the comment below.

Things to discuss:
 - I'm unable to find in the spec information regarding the only failing tests, which relate to [this](https://github.com/tc39/test262/blob/f1870753fa616261193b99f79d996889921b3404/test/built-ins/encodeURI/S15.1.3.3_A1.1_T2.js):
   > If string.charAt(k) in [0xDC00 - 0xDFFF], throw URIError

Let me know your thoughts :)


Co-authored-by: raskad <[email protected]>
@bors bors bot closed this as completed in db8a299 Sep 19, 2022
Repository owner moved this from In Progress to Done in Boa pre-v1 Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants