rusty_v8
rusty_v8 copied to clipboard
denoland
[WIP] Bindings for `{Dis,}allowJavascriptExecutionScope`
I've got the raw functionality working, but I'm not sure at all about the API and the lifetimes/generics. For the time being I've managed to patch something that works for the ValueDeserializer use case, but I'm not sure at all that it's any good.
Closes #833.
This is not ready for merging – the generics probably need changes, and I didn't add tests to scope.rs because they'd probably have to be reworked after the changes – but it is ready for review.
Looking good so far. By the look of it, it seems possible to create a HandleScope inside a Disallow scope. Is that intentional? What about creating a Disallow scope inside a Disallow scope? And what about creating an Allow scope inside another Allow scope?
It should be possible to create a TryCatch inside an Allow scope, and I guess it might be useful to have a TryCatch inside a Disallow scope created with ThrowOnFailure, to prevent JS code from being run but not throw an exception. It would not make sense to create an Allow scope inside another Allow scope, but I guess a Disallow inside a Disallow of a different type might make sense.
The main reason why I'm allowing creating a HandleScope inside a Disallow scope, and not requiring an Allow scope to have a Disallow parent in the type system is because ValueDeserializerImpl::read_host_object takes a HandleScope that in the C++ side is a child of a Disallow scope, and it must be possible to create a child Allow scope in the implementation. But I guess changing the passed type might simplify things?
Are there any blockers for this?
Rebasing and a thorough review from @piscisaureus I guess. But I haven't looked at this PR in over a year, and scopes are complicated.
Rebased. Since this PR is one of the blockers for denoland/deno#12067, which is lately gaining prominence due to Deno KV, it would be good to put some more work into this. However, I don't fully understand the details of how scopes are meant to work together, in terms of creating scopes from other scopes and as/deref, and my understanding of those details has certainly not grown in the year that this PR has been open. So a lot of help from @piscisaureus would probably be needed before this PR can be landed.
Closing because it's very old. Please open a new PR if you want to continue this.
I last rebased this PR two months ago. The single merge conflict is trivial, and after resolving it, the tests pass. Is that too old, really?
The issue this PR fixes is a main blocker for structured cloning and serializing web APIs, which I would have assumed is of high relevance to the Deno company lately because of Deno KV. But recently I haven't seen much interest from Deno in getting this PR landed, even though at this point the ball was on Deno's court, see https://github.com/denoland/rusty_v8/pull/862#issuecomment-1529785212.
At this point, I don't have the motivation to reopen this. If anyone does, feel free to take this code, and I would appreciate it if you would ping me in the resulting PR.
Hey @andreubotella, sorry for the miscommunication. I'm still very interested in landing this PR - sorry I hadn't picked it up earlier, it slipped in my backlog.
The only thing Bert wanted before merging this one was an update to deref_types test from src/scope.rs. I will prioritize getting it done this week.
@andreubotella I added more test cases in deref_types() test. I'll wait a couple days for Bert to take a look and then land the PR.
EDIT: I just need to update the comment in scope.rs and we're good to merge provided the CI passes. Will circle back tomorrow.