Add the V8 Inspector API #57

iddm · 2023-09-04T17:21:32Z

Adds the V8 Inspector wrapper and the Rust-idiomatic code to interact
with one from C++.
Also provides a WebSocket server for easier establishing the
remote debugging sessions.

src/v8/inspector/mod.rs

src/v8/inspector/server.rs

v8_c_api/src/v8_c_api.cpp

MeirShpilraien

Thanks @vityafx great feature that I believe will be widely used.
I added comments on the review itself and I have some general comments as well:

I think it would be nice to summerise the new API on this PR on the top comment with explanation on each API, why it was created and how it should be used. I believe it would have made the review part easier and would probably spare some of the questions I asked.
I believe we should consider the API safety property. I understand that if we bind the inspector lifetime to the inspector it will be very hard for the user to use it. But I still believe we should come up with a safer API that will not allow using the inspector without been entered to the isolate and if possible make sure its the right isolate. I already suggested having an inspector guard but I would like to know if you maybe have other ideas.
I am not follow why, in general, we need to use Mutex and Arc/Rc on this PR. Can you please explain why it is needed? (maybe cover it on the top comment so we will have it documented?)
I would avoid saving pointers to isolate/context as much as possible (if possible). And if it is mandatory to save them, I would avoid exposing them back for safety reasons.
I believe it will be easier for user to get an API that except closures (using generics) instead of boxes, we can use boxes internally where we need to.

v8_c_api/src/v8_c_api.cpp

src/v8/inspector/server.rs

MeirShpilraien

👍 few last comments and some few comments that was left open.

Adds the V8 Inspector wrapper and the Rust-idiomatic code to interact with one from C++. Also provides with a WebSocket server for easier establishing of the remote debugging sessions.

Makes it is safer to use the dispatch_protocol_message and schedule_pause_on_next_statement methods of the Inspector API. The only requirement for these methods is that the strings passed do not contain NUL symbols in Rust, and in C++ those must contain a valid JSON object (stringified).

This allows the data to leak from Rust, moving the responsibility of managing the objects to C++.

Makes the isolate stored instead.

This removes all the assumptions for the users to make about what it needs for correct operation.

Helps with the correct usage of the APIs.

This moves some of the functionality of the Inspector struct to the new InspectorGuard struct, objects of which may only be created when the isolate scope passed is for the same isolate this Inspector object was created with. This is done to make sure that the APIs which rely on the isolate and the corresponding HandleScope are correctly used.

MeirShpilraien

👍 Great work

iddm requested a review from MeirShpilraien September 4, 2023 17:21

iddm self-assigned this Sep 4, 2023

iddm force-pushed the add-inspector-api branch from a1995ee to 89d8581 Compare September 4, 2023 17:21

iddm changed the title ~~Draft: Add inspector api~~ Add the V8 Inspector API Sep 5, 2023

iddm force-pushed the add-inspector-api branch from de26841 to bb5a23f Compare September 6, 2023 10:20