Skip to content

feat: custom Node domains support in Inspector#1904

Open
littledivy wants to merge 16 commits intomainfrom
crdtp
Open

feat: custom Node domains support in Inspector#1904
littledivy wants to merge 16 commits intomainfrom
crdtp

Conversation

@littledivy
Copy link
Member

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.

@kajukitli
Copy link

suggestion: the std::string_view in string_util.h could be replaced with const String& to stay consistent with the protocol's type aliases and avoid the C++ <string_view> dependency:

-  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 CharactersLatin1 and CharactersUTF16)

v8 build works fine with this change, couldn't fully test locally due to bindgen needing clang 19+

bartlomieju and others added 2 commits March 7, 2026 16:01
…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>
Copy link

@kajukitli kajukitli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a substantial PR — adding crdtp bindings and custom inspector protocol domains

overall architecture looks reasonable:

  • crdtp.rs provides Rust bindings to V8's Chrome DevTools Protocol Transport
  • crdtp_binding.cc is the C++ glue layer
  • deno_inspector/ contains the PDL domain definitions

a few observations:

  1. memory safety concerns — there's a lot of raw pointer handling and unsafe blocks. the FrontendChannel implementation with the custom CxxVTable is especially tricky. would be good to have thorough tests for memory leaks/use-after-free scenarios.

  2. string_util.cc — i left a comment before about using const String& instead of std::string_view to stay consistent with the protocol's type aliases. was that considered?

  3. test coverage — the test_api.rs additions look comprehensive for the happy path. any negative test cases (malformed CBOR, invalid method names, etc.)?

  4. 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

bartlomieju and others added 10 commits March 7, 2026 16:27
- 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>
Copy link

@kajukitli kajukitli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm — thanks for addressing all the feedback

changes since last review:

  • std::string_viewconst String& in string_util.h/cc ✅
  • memory leak fixed: DomainDispatcher now properly freed via C++ destructor callback (crdtp__DomainDispatcher__BASE__Drop) ✅
  • double-serialization fixed in Serializable::to_bytes()
  • added DropTracker test 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>
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.

3 participants