Reapply "Add initial Rust JSG integration with error support"#5645
Reapply "Add initial Rust JSG integration with error support"#5645
Conversation
2637a14 to
9839191
Compare
|
The generated output of |
4c119db to
b6bc021
Compare
b6bc021 to
e43bc5c
Compare
e43bc5c to
7935905
Compare
|
Can you expand the PR description a bit more to explain what the issue being fixed is. Without it, it's a bit difficult to review the PR to determine if it and the test adequately address the issue. |
Added a description to the pull-request @jasnell. Let me know if you have any questions. |
7962941 to
fc684e3
Compare
guybedford
left a comment
There was a problem hiding this comment.
Just to understand the mental model properly here - how does the realm get disposed for normal isolate disposal from C++?
The destructor of Isolate::Impl which also owns the isolate destroys it. |
fc684e3 to
783a83a
Compare
183d6a9 to
5661b0e
Compare
|
hey @jasnell @guybedford would you mind reviewing this pull request? |
|
I'll let @guybedford and @mikea do the necessary sign off on this. Nothing problematic was apparent on an initial review, however. |
Yes this was clear from the change, but what is not clear to me is how Rust is informed about Realm disposal, and how that happens? Specifically, when you remove the current disposal function here, what is it that calls that now? Or does Rust not need to be informed? |
guybedford
left a comment
There was a problem hiding this comment.
Talked this through with Yagiz and was able to get a better picture, the main question I still had after the discussion was just how we know we definitely have a lock for Rust's realm disposal drop call, and he had some good checks for that as well.
5661b0e to
3c55736
Compare
3c55736 to
693b5e7
Compare
|
Upon recommendation from @guybedford, I've added a debug assertion to Realm::drop |
This reverts commit 66f5143.
Replaces the ownership mechanism triggering LSAN leak error. Added a test to ensure we don't regress in the future.
Realm object (a Rust-created structure) is stored in v8's embedder data via context_set_realm(). When LSAN run leak checks, it cannot trace through v8's internal embedder data to find references to this memory. From LSAN's perspective, the Realm looks like a leaked allocation because:
This false memory leak was easily reproducible through a static TestFixture (mimicking fuzzer pattern) some random work followed by calling
__lsan_do_recovarable_leak_check.To solve this, we either had to ignore the object from LSAN or stored the owned reference in
Isolate::Impland store the reference in the context object using embedder data.One commit fixes the memory leak and reproduces it, and the last commit converts the Realm data to be owned by the
Isolate::Impl