feat(inspector): serve source maps to DevTools via Network.loadNetwor…#1966
feat(inspector): serve source maps to DevTools via Network.loadNetwor…#1966edusperoni wants to merge 0 commit into
Conversation
📝 WalkthroughWalkthroughThis PR introduces DevTools source map fetching by adding a socket-thread fast-path handler that intercepts inspector protocol calls to serve source map resources directly. Messages from Java pass through a native handler; if a source map request is recognized, it is streamed back immediately; otherwise, existing queuing continues. The handler also rewrites outgoing sourceMapURL fields to nsruntime:// form and tracks nested pause state to avoid spurious re-pauses. ChangesDevTools Source Map Fetching via Socket Thread
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test-app/runtime/src/main/cpp/com_tns_AndroidJsV8Inspector.cpp (1)
32-44: 💤 Low valueAdd null check for
jMessageto prevent potential crash.If
jMessageis null (e.g., due to an unexpected null payload),ArgConverter::jstringToStringmay crash. While unlikely given the call site, a defensive check is warranted for JNI boundary code.🛡️ Suggested defensive check
JNIEXPORT extern "C" jstring Java_com_tns_AndroidJsV8Inspector_handleMessageOnSocketThread(JNIEnv* env, jobject instance, jstring jMessage) { try { + if (jMessage == nullptr) { + return nullptr; + } std::string message = ArgConverter::jstringToString(jMessage); std::string response = JsV8InspectorClient::GetInstance()->handleMessageOnSocketThread(message);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/com_tns_AndroidJsV8Inspector.cpp` around lines 32 - 44, In Java_com_tns_AndroidJsV8Inspector_handleMessageOnSocketThread, add a defensive null check for the JNI jstring parameter (jMessage) before calling ArgConverter::jstringToString to avoid a potential crash; if jMessage is null, return nullptr (or an appropriate empty jstring) immediately and skip calling ArgConverter::jstringToString and JsV8InspectorClient::GetInstance()->handleMessageOnSocketThread, keeping the existing try/catch behavior intact.test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp (1)
233-251: 💤 Low valueConsider clearing
resourceStreams_on disconnect to prevent resource accumulation.If DevTools disconnects without calling
IO.closefor open streams, theresourceStreams_map retains the data until explicitly closed by a future session. While the strings self-manage their memory, clearing streams on disconnect prevents unbounded accumulation across multiple debug sessions.🧹 Suggested cleanup
void JsV8InspectorClient::disconnect() { if (connection_ == nullptr) { return; } + { + std::lock_guard<std::mutex> lock(resourceStreamsMutex_); + resourceStreams_.clear(); + } + v8::Locker locker(isolate_); Isolate::Scope isolate_scope(isolate_);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp` around lines 233 - 251, The disconnect path in JsV8InspectorClient::disconnect should clear the resourceStreams_ container to avoid accumulating unused stream state across sessions; update JsV8InspectorClient::disconnect to iterate/clear resourceStreams_ (and close/release any associated stream handles if necessary) before resetting session_/connection_ (e.g., just before or after session_.reset()) and set resourceStreams_ to empty so subsequent sessions start fresh. Ensure you reference and clear the member resourceStreams_ in the disconnect implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test-app/runtime/src/main/cpp/com_tns_AndroidJsV8Inspector.cpp`:
- Around line 32-44: In
Java_com_tns_AndroidJsV8Inspector_handleMessageOnSocketThread, add a defensive
null check for the JNI jstring parameter (jMessage) before calling
ArgConverter::jstringToString to avoid a potential crash; if jMessage is null,
return nullptr (or an appropriate empty jstring) immediately and skip calling
ArgConverter::jstringToString and
JsV8InspectorClient::GetInstance()->handleMessageOnSocketThread, keeping the
existing try/catch behavior intact.
In `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp`:
- Around line 233-251: The disconnect path in JsV8InspectorClient::disconnect
should clear the resourceStreams_ container to avoid accumulating unused stream
state across sessions; update JsV8InspectorClient::disconnect to iterate/clear
resourceStreams_ (and close/release any associated stream handles if necessary)
before resetting session_/connection_ (e.g., just before or after
session_.reset()) and set resourceStreams_ to empty so subsequent sessions start
fresh. Ensure you reference and clear the member resourceStreams_ in the
disconnect implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec98d23b-29a9-46de-a0f0-0099ecb21cd0
📒 Files selected for processing (5)
test-app/app/src/main/java/com/tns/AndroidJsV8Inspector.javatest-app/runtime/src/main/cpp/JsV8InspectorClient.cpptest-app/runtime/src/main/cpp/JsV8InspectorClient.htest-app/runtime/src/main/cpp/com_tns_AndroidJsV8Inspector.cpptest-app/runtime/src/main/cpp/third_party/json.hpp
27f577f to
9b45990
Compare
…kResource
Chrome DevTools no longer fetches external source maps itself when debugging remote targets: it issues Network.loadNetworkResource to the target and reads the result back through IO.read/IO.close. None of these embedder-side CDP domains are implemented by V8's inspector, so external source maps failed and apps had to fall back to bloated inline-source-map builds.
Ports NativeScript/ios#385 and NativeScript/ios#378 to Android.
Refs: nodejs/node#58077
Summary by CodeRabbit