inspector: fix null request_queue dereference in InspectorIo

[alichtman]

request_queue_ is only assigned on the IO thread and may still be null when Start() checks Expired(). Return nullptr there and guard the destructor, which runs on that failure path.

[nodejs-github-bot]

Review requested:

• @nodejs/inspector

[github-actions]

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

• the guide for first-time contributors
• the contribution and automation policies
• the pull request guide
• the AI use policy
• the Code of Conduct

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.28%. Comparing base (20b3a69) to head (05eeb2e).
⚠️ Report is 37 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66201      +/-   ##
==========================================
- Coverage   90.30%   90.28%   -0.02%     
==========================================
  Files         790      789       -1     
  Lines      272044   272878     +834     
  Branches    51934    52108     +174     
==========================================
+ Hits       245671   246375     +704     
- Misses      16875    16968      +93     
- Partials     9498     9535      +37     

Files with missing lines	Coverage Δ
src/inspector_io.cc	91.80% <100.00%> (ø)

... and 59 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[legendecas]

node/src/inspector_io.cc

Line 271 in 3cd2d6e

thread_start_condition_.Wait(scoped_lock);

This waits on the InspectorIo::ThreadMain to signal that the InspectorIo has started and initialized request_queue_. I'm not convinced that the condition described is a valid condition.

[alichtman]

Hey! Thanks for taking a look :)

I could totally be wrong, and this is my first node contribution.

I'm looking at a VS Code crash (unfortunately I can't share the entire minidump bc it happened in a production env), but here's an excerpt that I can share:

Electron Framework UUID in minidump:
4C4C44FD-5555-3144-A1F3-6189BEFDD732

UUID of Electron 39.8.8 binary used for symbolication:
4C4C44FD-5555-3144-A1F3-6189BEFDD732

It's an ARM64 machine.

(lldb) thread select 1
(lldb) bt
* thread #1, stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: libsystem_pthread.dylib
    frame #1: Electron Framework`uv_mutex_lock + 12
    frame #2: Electron Framework`node::inspector::InspectorIo::Start(...) + 260
    frame #3: Electron Framework`node::inspector::Agent::StartIoThread() + 284
    frame #4: Electron Framework`node::Environment::RunAndClearInterrupts() + 104

(lldb) register read pc lr x0 x21
      pc = 0x0000000186de2408
      lr = 0x000000011c0569d4  Electron Framework`uv_mutex_lock + 12
      x0 = 0x0000000000000008
     x21 = 0x0000000000000000

(lldb) disassemble --start-address 0x11c5471fc --count 7
Electron Framework`node::inspector::InspectorIo::Start:
    0x11c5471fc <+248>: ldr x21, [x19, #0x10]
    0x11c547200 <+252>: add x0, x21, #0x8
    0x11c547204 <+256>: bl  0x11c0569c8 ; uv_mutex_lock
    0x11c547208 <+260>: ldr x22, [x21]
    0x11c54720c <+264>: add x0, x21, #0x8
    0x11c547210 <+268>: bl  0x11c056a14 ; uv_mutex_unlock
    0x11c547214 <+272>: cbz x22, 0x11c547238

Looking at 0x11c547200, it seems like x0 has a value of 0x0 before adding 0x8, which is what causes the register to be x0 = 0x0000000000000008 at the time of the crash. Frames 2-3 of the stack are:

    frame #1: Electron Framework`uv_mutex_lock + 12
    frame #2: Electron Framework`node::inspector::InspectorIo::Start(...) + 260

Am I misinterpreting this? Is there any serious downside to merging this? I think the worst case outcome is that the null check is unnecessary, and we lose a fraction of a millisecond on an extra instruction. (But I might be missing something.)

[legendecas]

Is there any serious downside to merging this?

Merging a patch without understanding the problem accumulates tech debt on the project.

Looking at the crash backtrace, I don't think this patch fixes it.

The problem seems to me that the thread_start_condition_.Wait is missing a predicate and it didn't handle spurious wakeups well. A proper fix should be:

InspectorIo::InspectorIo(std::shared_ptr<MainThreadHandle> main_thread,
                         const std::string& path,
                         std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
                         const InspectPublishUid& inspect_publish_uid)
    : main_thread_(main_thread),
      host_port_(host_port),
      inspect_publish_uid_(inspect_publish_uid),
      thread_(),
      script_name_(path),
      id_(GenerateID()) {
  Mutex::ScopedLock scoped_lock(thread_start_lock_);
  CHECK_EQ(uv_thread_create(&thread_, InspectorIo::ThreadMain, this), 0);
- thread_start_condition_.Wait(scoped_lock);
+ while (request_queue_ == nullptr)
+    thread_start_condition_.Wait(scoped_lock);
}

Would you like to apply the fix?

[alichtman]

Thanks for the help :) updated!

[alichtman]

Fixed format-cpp lint