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

Allow Proxy target and handler to be Proxy too #1947

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

Conversation

svaarala
Copy link
Owner

Up to now Duktape has intentionally rejected a Proxy as either a Proxy target or a Proxy handler object; both cases cause more potential for native recursion and are relatively rare in practice. ES2015+ does not place such a limitation on Proxies. This pull removes the current limitation and allows Proxies as both target and handler.

Tasks:

  • Remove Proxy check for target/handler in Proxy creation
  • Proxy chain support for getprop
  • Proxy chain support for putprop
  • Proxy chain support for delprop
  • Proxy chain support for hasprop
  • TBD: enumeration etc
  • Testcase coverage
  • Releases entry

This is currently work in progress, to be merged after 2.3.

@svaarala svaarala force-pushed the proxy-chain-support branch from 8471522 to 9d9fd80 Compare July 22, 2018 19:29
@fatcerberus
Copy link
Contributor

I’m actually a bit surprised they allow a Proxy for the handler. Proxy-of-Proxy makes some sense, but a Proxy’d handler doesn’t seem as useful.

@svaarala
Copy link
Owner Author

I think it's just allowed by default: it's not explicitly rejected even though it may not be very useful in practice.

@fatcerberus
Copy link
Contributor

Yeah, makes sense. It just seems like something that might want to be rejected explicitly simply to avoid all the potential edge cases it creates. But thinking about it, I suppose that the same pitfalls apply to recursive proxies so disallowing a Proxy as handler doesn’t really buy you anything in that regard.

@svaarala
Copy link
Owner Author

In practice the main difficulties for Duktape would be:

  • Additional paths for native recursion. Since native stack depth is controlled via the native callstack limit, it's problematic that this particular form of recursion might happen outside of that control (i.e. without function calls and thus not controlled by the limit). However, it should be possible to do the Proxy chain resolution without native recursion (even tail recursion) so this is solvable, and the pull currently does so.
  • Potential for Proxy target/handler loops. These would be possible in principle, as far as the data structures are concerned, but because all Proxy instances are created with the target/handler being an already created object, it's not possible to create loops in practice. So this is also a non-issue.
  • Chains of billions of Proxies might cause performance issues and be problematic for the execution timeout mechanism, and such chains could be created from plain Ecmascript code too. So maybe long Proxy chains should invoke the timeout check from time to time. There are similar potential issues for effectively bypassing the execution timeout with other script-accessible features too (e.g. huge array joins) so it wouldn't be the only such potential around.

There may be more corner cases here and there though.

@svaarala svaarala force-pushed the proxy-chain-support branch from 9d9fd80 to 7a5fa23 Compare August 6, 2018 22:31
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.

2 participants