feat: custom Node domains support in Inspector#1904
Open
littledivy wants to merge 16 commits intomainfrom
Open
feat: custom Node domains support in Inspector#1904littledivy wants to merge 16 commits intomainfrom
littledivy wants to merge 16 commits intomainfrom
Conversation
|
suggestion: the - static const uint8_t* CharactersUTF8(const std::string_view s);
- static size_t CharacterCount(const std::string_view s);
+ static const uint8_t* CharactersUTF8(const String& s);
+ static size_t CharacterCount(const String& s);(same for v8 build works fine with this change, couldn't fully test locally due to bindgen needing clang 19+ |
…bindings - Add Rust wrappers for crdtp::CreateResponse and crdtp::CreateNotification - Add DomainDispatcher C++/Rust bindings allowing Rust code to implement domain-specific protocol handlers and wire them to UberDispatcher - Apply reviewer feedback: replace std::string_view with const String& in string_util.h/cc for consistency with protocol type aliases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kajukitli
reviewed
Mar 7, 2026
kajukitli
left a comment
kajukitli
left a comment
There was a problem hiding this comment.
this is a substantial PR — adding crdtp bindings and custom inspector protocol domains
overall architecture looks reasonable:
crdtp.rsprovides Rust bindings to V8's Chrome DevTools Protocol Transportcrdtp_binding.ccis the C++ glue layerdeno_inspector/contains the PDL domain definitions
a few observations:
-
memory safety concerns — there's a lot of raw pointer handling and
unsafeblocks. theFrontendChannelimplementation with the custom CxxVTable is especially tricky. would be good to have thorough tests for memory leaks/use-after-free scenarios. -
string_util.cc— i left a comment before about usingconst String&instead ofstd::string_viewto stay consistent with the protocol's type aliases. was that considered? -
test coverage — the
test_api.rsadditions look comprehensive for the happy path. any negative test cases (malformed CBOR, invalid method names, etc.)? -
CI status — last i checked CI was failing due to OOM building V8. has that been resolved?
not blocking, but would like clarity on #2 and #4 before approving
- Fix memory leak: DomainDispatcher Rust data is now properly freed via C++ destructor callback (crdtp__DomainDispatcher__BASE__Drop) instead of being leaked with Box::leak - Fix double-serialization: Serializable::to_bytes() now uses a single AppendSerialized call instead of two separate size+copy calls - Fix copyright year: crdtp_binding.cc 2026 -> 2024 - Rename _imp -> imp (underscore prefix means unused in Rust convention) - Add FrontendChannel size assertion to catch vtable size mismatches - Document create_notification panic on interior null bytes - Remove unnecessary RefCell usage in test structs (dispatch takes &mut self) - Add response delivery assertion in crdtp_domain_dispatcher_wire test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add unsafe blocks inside unsafe fn from_raw (Rust 2024 edition requirement) - Make into_raw methods pub(crate) to avoid private-interfaces warnings - Make UberDispatcher::channel private (only used internally) - Remove unused crdtp__FrontendChannel__BASE__SIZE and crdtp__vec_u8__data - Remove size assertion that depended on deleted SIZE function Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix DispatchResult::run() leaking the wrapper (was using mem::forget instead of letting Drop clean up) - Remove unused UberDispatcher::as_raw() method - Add compile-time static_assert for FrontendChannel BASE size Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Malformed/truncated/empty CBOR input to Dispatchable - Missing required fields (no method, no id) - Invalid JSON and CBOR for conversion functions - Dispatch to unregistered domains - DomainDispatcher returning error responses - Multiple domain routing and isolation - All DispatchResponse error type codes (JSON-RPC conventions) - Error notifications via create_error_notification - Drop cleanup verification (C++ destructor -> Rust Drop callback) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove unnecessary `mut` on channel variable - Use `let _ =` for intentionally ignored Box::into_raw return value - Remove dead UberDispatcher::channel() wrapper method Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split json.unwrap() into a separate let binding before passing to String::from_utf8_lossy to avoid temporary lifetime issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The upstream crdtp Notification class stores `const char* method_` as a raw pointer without copying. When called from Rust, the CString holding the method name is dropped before AppendSerialized is called, causing a use-after-free detected by ASAN. Replace with OwnedNotification that stores a std::string copy of the method name, reproducing the same CBOR serialization logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pub(crate) still triggers private_interfaces lint since the return types (RawSerializable, DispatchResponseWrapper) are module-private. These methods are only used within crdtp.rs, so make them fully private. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
crdtp Dispatchable requires an integer id property for all messages (MESSAGE_MUST_HAVE_INTEGER_ID_PROPERTY). A message without id is not ok(), not a valid notification as previously assumed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kajukitli
approved these changes
Mar 7, 2026
kajukitli
left a comment
kajukitli
left a comment
There was a problem hiding this comment.
lgtm — thanks for addressing all the feedback
changes since last review:
std::string_view→const String&in string_util.h/cc ✅- memory leak fixed:
DomainDispatchernow properly freed via C++ destructor callback (crdtp__DomainDispatcher__BASE__Drop) ✅ - double-serialization fixed in
Serializable::to_bytes()✅ - added
DropTrackertest to verify cleanup ✅ - removed dead code (
UberDispatcher::as_raw, unused SIZE function) ✅
CI is running now — assuming it passes, this is good to merge
UberDispatcher sends an error response for unregistered methods when run() is called, so there are 3 responses (2 success + 1 error), not 2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch lets us extend the Inspector protocol with new domains defined in
src/deno_inspector/node_protocol.pdl. It also adds bindings to crdtp for sending notifcations and responses to protocol clients.