GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat(inspector): attach Chrome DevTools to Web Worker isolates by edusperoni · Pull Request #1973 · NativeScript/android Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds worker thread debugging support to the Android V8 inspector. The socket message handler contract shifts from string return to boolean-with-response, worker targets are registered and routed via session identifiers, a new per-worker inspector client integrates with the worker's event loop, and inspector lifecycle is wired into worker bootstrap/teardown with console message forwarding. ChangesWorker Thread Debugging Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp (1)
514-524: 🩺 Stability & Availability | 🏗️ Heavy liftAvoid socket writes while holding
workerTargetsMutex_.Line 514+ / Line 542+ / Line 571+ / Line 585+ call
SendToFrontend(...)while the registry mutex is held. If socket send blocks, worker register/unregister/routing can stall behind that lock. Snapshot payloads under lock, then send after unlocking.Also applies to: 542-559, 571-577, 585-605
🤖 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 514 - 524, You're holding workerTargetsMutex_ while performing socket writes via SendToFrontend/JsonDump (e.g., in the branch handling missing sessionId and other branches that currently call SendToFrontend while locked), which can block other registry operations; instead, inside the critical section (while holding workerTargetsMutex_) construct and copy the JSON payload/string you intend to send and then release the lock, and only after unlocking call SendToFrontend(JsonDump(...)) or SendToFrontend(copiedPayload). Apply this pattern for all occurrences that call SendToFrontend while workerTargetsMutex_ is held (search for SendToFrontend usages near workerTargets_ handling: the missing-session branch, and the other locations noted in the comment).
🤖 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.
Inline comments:
In `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp`:
- Around line 814-821: GetInstance currently does an atomic load->new->store
which can race and leak if two threads concurrently see null; change
JsV8InspectorClient::GetInstance to perform a race-free singleton init by using
either std::call_once with a std::once_flag or an atomic compare_exchange loop
on the static instance so only one caller constructs the object and others reuse
it; keep the creation using Runtime::GetRuntime(0)->GetIsolate() and ensure
GetInstanceIfCreated remains unchanged, and verify callers like
Java_com_tns_AndroidJsV8Inspector_handleMessageOnSocketThread use the fixed
GetInstance to avoid double construction.
In `@test-app/runtime/src/main/cpp/WorkerInspectorClient.cpp`:
- Around line 205-239: The pauseTerminated_ flag is subject to a data race
because it is written from NotifyTerminating() (called from another thread)
while being read/modified in runMessageLoopOnPause() and
quitMessageLoopOnPause(); change the pauseTerminated_ member in
WorkerInspectorClient to std::atomic<bool> and update all accesses to use atomic
operations with appropriate memory ordering (e.g., load(memory_order_acquire)
when reading in runMessageLoopOnPause and store(memory_order_release) when
setting in NotifyTerminating and quitMessageLoopOnPause) so reads/writes are
synchronized across threads.
---
Nitpick comments:
In `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp`:
- Around line 514-524: You're holding workerTargetsMutex_ while performing
socket writes via SendToFrontend/JsonDump (e.g., in the branch handling missing
sessionId and other branches that currently call SendToFrontend while locked),
which can block other registry operations; instead, inside the critical section
(while holding workerTargetsMutex_) construct and copy the JSON payload/string
you intend to send and then release the lock, and only after unlocking call
SendToFrontend(JsonDump(...)) or SendToFrontend(copiedPayload). Apply this
pattern for all occurrences that call SendToFrontend while workerTargetsMutex_
is held (search for SendToFrontend usages near workerTargets_ handling: the
missing-session branch, and the other locations noted in the comment).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a62cec75-6809-44d1-af66-cbcd3dff76e0
📒 Files selected for processing (9)
test-app/app/src/main/java/com/tns/AndroidJsV8Inspector.javatest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/JsV8InspectorClient.cpptest-app/runtime/src/main/cpp/JsV8InspectorClient.htest-app/runtime/src/main/cpp/WorkerInspectorClient.cpptest-app/runtime/src/main/cpp/WorkerInspectorClient.htest-app/runtime/src/main/cpp/WorkerWrapper.cpptest-app/runtime/src/main/cpp/WorkerWrapper.htest-app/runtime/src/main/cpp/com_tns_AndroidJsV8Inspector.cpp
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+205
to
+239
| void WorkerInspectorClient::runMessageLoopOnPause(int contextGroupId) { | ||
| if (runningPauseLoop_.load(std::memory_order_acquire) || dying_) { | ||
| return; | ||
| } | ||
| runningPauseLoop_.store(true, std::memory_order_release); | ||
| pauseTerminated_ = false; | ||
|
|
||
| while (!pauseTerminated_ && !dying_) { | ||
| std::string message = this->PopMessage(); | ||
| bool shouldWait = message.empty(); | ||
| if (!shouldWait) { | ||
| this->DispatchOne(message); | ||
| } | ||
|
|
||
| while (v8::platform::PumpMessageLoop(Runtime::platform, isolate_)) { | ||
| } | ||
|
|
||
| if (shouldWait && !pauseTerminated_ && !dying_) { | ||
| std::unique_lock<std::mutex> lock(messageArrivedMutex_); | ||
| messageArrived_.wait_for(lock, std::chrono::milliseconds(1)); | ||
| } | ||
| } | ||
|
|
||
| runningPauseLoop_.store(false, std::memory_order_release); | ||
| } | ||
|
|
||
| void WorkerInspectorClient::quitMessageLoopOnPause() { | ||
| pauseTerminated_ = true; | ||
| } | ||
|
|
||
| void WorkerInspectorClient::NotifyTerminating() { | ||
| dying_ = true; | ||
| pauseTerminated_ = true; | ||
| messageArrived_.notify_all(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Data race on pauseTerminated_ flag.
pauseTerminated_ is written from a different thread in NotifyTerminating() (called via WorkerWrapper::Terminate() from the parent thread) while it may be read concurrently in runMessageLoopOnPause() on the worker thread. This needs to be an std::atomic<bool> with appropriate memory ordering.
🔒 Proposed fix: make pauseTerminated_ atomic
In the header, change:
- bool pauseTerminated_ = false; + std::atomic<bool> pauseTerminated_{false};
Then update usages with appropriate memory ordering:
void WorkerInspectorClient::runMessageLoopOnPause(int contextGroupId) {
if (runningPauseLoop_.load(std::memory_order_acquire) || dying_) {
return;
}
runningPauseLoop_.store(true, std::memory_order_release);
- pauseTerminated_ = false;
+ pauseTerminated_.store(false, std::memory_order_relaxed);
- while (!pauseTerminated_ && !dying_) {
+ while (!pauseTerminated_.load(std::memory_order_acquire) && !dying_) {
// ...
- if (shouldWait && !pauseTerminated_ && !dying_) {
+ if (shouldWait && !pauseTerminated_.load(std::memory_order_acquire) && !dying_) {
// ...
}
}
// ...
}
void WorkerInspectorClient::quitMessageLoopOnPause() {
- pauseTerminated_ = true;
+ pauseTerminated_.store(true, std::memory_order_release);
}
void WorkerInspectorClient::NotifyTerminating() {
dying_ = true;
- pauseTerminated_ = true;
+ pauseTerminated_.store(true, std::memory_order_release);
messageArrived_.notify_all();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void WorkerInspectorClient::runMessageLoopOnPause(int contextGroupId) { | |
| if (runningPauseLoop_.load(std::memory_order_acquire) || dying_) { | |
| return; | |
| } | |
| runningPauseLoop_.store(true, std::memory_order_release); | |
| pauseTerminated_ = false; | |
| while (!pauseTerminated_ && !dying_) { | |
| std::string message = this->PopMessage(); | |
| bool shouldWait = message.empty(); | |
| if (!shouldWait) { | |
| this->DispatchOne(message); | |
| } | |
| while (v8::platform::PumpMessageLoop(Runtime::platform, isolate_)) { | |
| } | |
| if (shouldWait && !pauseTerminated_ && !dying_) { | |
| std::unique_lock<std::mutex> lock(messageArrivedMutex_); | |
| messageArrived_.wait_for(lock, std::chrono::milliseconds(1)); | |
| } | |
| } | |
| runningPauseLoop_.store(false, std::memory_order_release); | |
| } | |
| void WorkerInspectorClient::quitMessageLoopOnPause() { | |
| pauseTerminated_ = true; | |
| } | |
| void WorkerInspectorClient::NotifyTerminating() { | |
| dying_ = true; | |
| pauseTerminated_ = true; | |
| messageArrived_.notify_all(); | |
| } | |
| void WorkerInspectorClient::runMessageLoopOnPause(int contextGroupId) { | |
| if (runningPauseLoop_.load(std::memory_order_acquire) || dying_) { | |
| return; | |
| } | |
| runningPauseLoop_.store(true, std::memory_order_release); | |
| pauseTerminated_.store(false, std::memory_order_relaxed); | |
| while (!pauseTerminated_.load(std::memory_order_acquire) && !dying_) { | |
| std::string message = this->PopMessage(); | |
| bool shouldWait = message.empty(); | |
| if (!shouldWait) { | |
| this->DispatchOne(message); | |
| } | |
| while (v8::platform::PumpMessageLoop(Runtime::platform, isolate_)) { | |
| } | |
| if (shouldWait && !pauseTerminated_.load(std::memory_order_acquire) && !dying_) { | |
| std::unique_lock<std::mutex> lock(messageArrivedMutex_); | |
| messageArrived_.wait_for(lock, std::chrono::milliseconds(1)); | |
| } | |
| } | |
| runningPauseLoop_.store(false, std::memory_order_release); | |
| } | |
| void WorkerInspectorClient::quitMessageLoopOnPause() { | |
| pauseTerminated_.store(true, std::memory_order_release); | |
| } | |
| void WorkerInspectorClient::NotifyTerminating() { | |
| dying_ = true; | |
| pauseTerminated_.store(true, std::memory_order_release); | |
| messageArrived_.notify_all(); | |
| } |
🤖 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/WorkerInspectorClient.cpp` around lines 205 -
239, The pauseTerminated_ flag is subject to a data race because it is written
from NotifyTerminating() (called from another thread) while being read/modified
in runMessageLoopOnPause() and quitMessageLoopOnPause(); change the
pauseTerminated_ member in WorkerInspectorClient to std::atomic<bool> and update
all accesses to use atomic operations with appropriate memory ordering (e.g.,
load(memory_order_acquire) when reading in runMessageLoopOnPause and
store(memory_order_release) when setting in NotifyTerminating and
quitMessageLoopOnPause) so reads/writes are synchronized across threads.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
NathanWalker
approved these changes
Jun 16, 2026
Uh oh!
There was an error while loading. Please reload this page.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
None yet
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 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.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.