Fix native Windows adapter broken after detach

[xusheng6]

Summary

• DebugActiveProcessStop must be called from the same thread that called DebugActiveProcess. Previously it was called from the main thread in Detach(), causing ERROR_ACCESS_DENIED and crashing the target process.
• Moved DebugActiveProcessStop (and breakpoint cleanup) into DebugLoop() on the debug thread, handling both the stopped-at-breakpoint and target-running cases.

Fixes #1041

Test plan

• Attach to a process with the native Windows adapter
• Detach from the process
• Verify the process continues running
• Verify the adapter can be used again

🤖 Generated with Claude Code

[plafosse]

Detach() can now report success even when the actual detach fails, which leaves the adapter state inconsistent and drops all tracked breakpoints. The old code returned false immediately on DebugActiveProcessStop failure in public/debugger/core/adapters/windowsnativeadapter.cpp:252, but this PR moves that call into DebugLoop() and only logs a warning before breaking out of the loop (around public/debugger/core/adapters/windowsnativeadapter.cpp:564 and the new post-loop cleanup block after public/debugger/core/adapters/windowsnativeadapter.cpp:589). Detach() still joins the thread and proceeds as if detach succeeded, so any non-thread-affinity failure in DebugActiveProcessStop becomes a silent false success. Worse, the cleanup clears m_breakpoints before attempting the detach, so on failure the process can remain attached while the adapter has already discarded its breakpoint state.

I did not find a second issue from the patch alone. The main fix direction looks right, but I would not merge it without preserving detach failure propagation back to Detach().

[xusheng6]

Detach() can now report success even when the actual detach fails, which leaves the adapter state inconsistent and drops all tracked breakpoints. The old code returned false immediately on DebugActiveProcessStop failure in public/debugger/core/adapters/windowsnativeadapter.cpp:252, but this PR moves that call into DebugLoop() and only logs a warning before breaking out of the loop (around public/debugger/core/adapters/windowsnativeadapter.cpp:564 and the new post-loop cleanup block after public/debugger/core/adapters/windowsnativeadapter.cpp:589). Detach() still joins the thread and proceeds as if detach succeeded, so any non-thread-affinity failure in DebugActiveProcessStop becomes a silent false success. Worse, the cleanup clears m_breakpoints before attempting the detach, so on failure the process can remain attached while the adapter has already discarded its breakpoint state.

I did not find a second issue from the patch alone. The main fix direction looks right, but I would not merge it without preserving detach failure propagation back to Detach().

I see your concern here but there is really not much we can do if the DebugActiveProcessStop call failed. The breakpoints have to be removed before a detach is attempted, and while in theory we could try to add them back, but that is going quite far to account for a scenario that is probably very rare. For reference, the TitanEngine (which is used by x64dbg) also does not check if DebugActiveProcessStop succeeds: https://github.com/x64dbg/TitanEngine/blob/x64dbg/TitanEngine/TitanEngine.Debugger.DebugLoop.cpp#L114

I made two changes to the PR:

1. Make sure hardware breakpoints are also properly removed before detach -- previously it only removes the software breakpoints
2. Force terminate the process if the detach attempt failed, making sure a clean state of the debugger

What do you think of it?

[plafosse]

I've tried to understand this code but its complex state management and its pretty tricky. Relying on Codex but I've tried to understand the concerns and I think this addresses them well. A slight rearchitecture of how the "detach on debug thread" would solve the concerns but it needs to be less one shot fire and forget and more of a handshake. Current problems:

1. Detach() can now return success even when the actual detach fails on the debug thread. Before this PR, Detach() returned false immediately if DebugActiveProcessStop failed in public/debugger/core/adapters/windowsnativeadapter.cpp:252. After the change, that call moves into DebugLoop() and only logs a warning before exiting. Detach() still joins the thread and continues cleanup as if detach succeeded. That hides real detach failures from the controller and can leave the process attached while the adapter believes it is detached.
2. The new flow clears all tracked breakpoints before attempting DebugActiveProcessStop. If the detach then fails, the adapter has already discarded m_breakpoints, so it no longer has an accurate view of the breakpoints still planted in the debuggee. That makes recovery from a failed detach much worse than before.

Here's a suggestion on how to fix:

1. Detach() sets m_shouldStop = true, wakes the debug thread, and then waits for a detach result from that thread.
2. The debug thread performs:
   • resume/continue if needed
   • DebugActiveProcessStop(m_processId)
   • only if that succeeds, remove breakpoints and clear m_breakpoints
   • store a bool m_detachSucceeded and notify a condition variable
3. Detach() joins the thread and returns that stored result.

In other words, move both:

• DebugActiveProcessStop
• destructive state cleanup

behind a success check on the debug thread.

Pseudo-flow:

  bool WindowsNativeAdapter::Detach()
  {
      if (!m_activelyDebugging)
          return true;

      {
          std::lock_guard lock(m_detachMutex);
          m_shouldStop = true;
          m_detachCompleted = false;
          m_detachSucceeded = false;
      }
      m_debugCondition.notify_one();

      {
          std::unique_lock lock(m_detachMutex);
          m_detachCondition.wait(lock, [&] { return m_detachCompleted; });
      }

      if (m_debugThread.joinable())
          m_debugThread.join();

      if (!m_detachSucceeded)
          return false;

      // close handles / final local cleanup
      ...
      return true;
  }

And in DebugLoop():

  if (m_shouldStop)
  {
      bool ok = DebugActiveProcessStop(m_processId);
      if (ok)
      {
          std::lock_guard bpLock(m_breakpointsMutex);
          for (auto& bp : m_breakpoints)
              if (bp.isActive)
                  RemoveBreakpointInternal(bp.address);
          m_breakpoints.clear();
      }

      {
          std::lock_guard lock(m_detachMutex);
          m_detachSucceeded = ok;
          m_detachCompleted = true;
      }
      m_detachCondition.notify_one();
      break;
  }