Skip to content

Commit 321b0f8

Browse files
codemonkeychrisclaude
codemonkeychris
and
claude
authored
fix(045): remove broad catch{} swallows in tear-off pipeline (#420) (#421)
* fix(045): remove broad catch{} swallows in tear-off pipeline (#420) PR #418 (Spec 045 §2.6 VS-style immediate tab tear-off) landed with 14 broad catch{} blocks across the tear-off code paths. Most were dead-defensive: the ReactorWindow public mutators (Close, Hide, Activate, SetOpacity, SetNoActivate, SetIgnorePointerInput) are all no-op on _disposed and internally narrow the teardown-COMException set, so an outer catch swallowed nothing. Changes per issue #420 inventory: DockTabTearOff.cs (1 site) • PositionFloating tracker-tick catch removed. Audited Stop-before-Close ordering: every path that closes the floating window calls Stop() first (detaches Tick handler, clears s_active), so AppWindow.Move always targets a live AppWindow. DockHostNativeComponent.cs (4 sites → 0) • BeginImmediateTearOff: DockFloatingWindow.Open catch removed. Open failure is a bug we want to surface; layout mutation is deferred until after Open returns. • FinalizeImmediateDrop: two floatingWindow.Close catches removed. ReactorWindow.Close is idempotent; called once per branch. • FinalizeImmediateDrop drop-outside: strip-drag-styles catch removed. All four mutators are disposal-safe. DockFloatingWindow.cs (9 sites → 0, plus 1 latent bug fix) • BeginFloatingTearOff source-XamlRoot read: catch removed. • BeginFloatingTearOff Open call: catch removed. • Single-tab path: ownWindow.AppWindow.Hide() → ownWindow.Hide(). The bare AppWindow.Hide() bypassed ReactorWindow.Hide's _disposed guard and teardown-COMException catch; the wrapper has both. • Multi-tab path SetIgnorePointerInput(true) — latent bug fix. The call requires WS_EX_LAYERED. A regular floating window opens with Opacity=1.0 (not layered), so SetIgnorePointerInput(true) was throwing InvalidOperationException every multi-tab tear-off, silently swallowed — the source window was never actually marked click-through despite the comment claiming it was. Now nudge SetOpacity(0.9999) first to install WS_EX_LAYERED (alpha rounds to 255 → visually identical). • RestoreSourcePointerInput catch removed; also restores SetOpacity(1.0) to undo the layering nudge. • Two capturedDragged.Close catches (enqueued + sync fallback) removed. • Fallback xr ??= capturedDragged.NativeWindow?.Content?.XamlRoot catch removed (window was just opened synchronously). • RestoreWindowFromDrag four-setter catch removed (all disposal-safe). Three explicitly out-of-scope catches in DockTabTearOff.cs are preserved per the issue: • L191 TabView.CapturePointer (best-effort, can fail on stale pointer) • L256 TabView.ReleasePointerCaptures (no captures → no-op) • L502 active.CancelDrop in Tracker.ForceCancel (must not crash next drag) Verified: dotnet test Reactor.Tests on ARM64 — 9038 passed, 0 failed. The selftest fixtures (T07/T08 host tear-off, T11-T14 floating tear-off) check Spec.Opacity >= 0.99 on restore; the 0.9999 layering nudge plus the explicit SetOpacity(1.0) restore satisfy those assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(045): narrow XamlRoot-read catch to ERROR_INVALID_OPERATION_ID T14 (NativeDockingTearOff_T14_FloatingTearOff_StaleTrackerForceCleaned) regressed when the catch at BeginFloatingTearOff L547 was removed in the previous commit. The catch wasn't dead defensiveness — it was guarding a cross-render teardown race that T14 explicitly exercises: 1. First tear-off (single-tab path) → RemoveLocal sees willEmpty=true → holder[0]?.Close() → source `floating` window disposes 2. `windowHolder` array still holds a strong reference to the disposed ReactorWindow across the test's `await Harness.Render()` 3. Second press fires on the same TabView (still alive — WinUI hasn't synchronously torn down the visual tree) 4. BeginFloatingTearOff runs the closure from the floating window's last render; `holder[0]` returns the disposed ReactorWindow 5. `ReactorWindow.NativeWindow` is a raw field accessor (no _disposed guard) → `.Content` is a WinRT projection call into the disconnected COM proxy → throws COMException with HResult 0x800710DD (HRESULT_FROM_WIN32(ERROR_INVALID_OPERATION_ID=4317), surface text "The operation identifier is not valid.") Empirically verified the HResult ↔ message mapping with `[System.ComponentModel.Win32Exception]::new(0x800710DD).Message`; my prior commit comment incorrectly cited 0x80070C70 (NET.ACC database error, unrelated). Fix per the issue author's L547 prescription: > Narrow to COMException and treat throw as identical to null (continue > with the `xr` fallback at L657). Added `HResults.ERROR_INVALID_OPERATION_ID = 0x800710DD` to the central constants file and use it in a `when` filter — anything else here is a real bug we want to surface. The `xr ??= capturedDragged.NativeWindow?. Content?.XamlRoot` fallback below picks up the freshly-opened dragged preview's XamlRoot when the source is dead. Why not a deeper structural fix (null windowHolder[0] on Closed, detach press hook on TabView unload)? Either would change T14's expectation — the test explicitly requires the second tear-off to SUCCEED on the orphaned TabView with a fresh preview window. Per the test's stuck-state-recovery contract, the correct behavior is to continue gracefully when the source is dead, not to refuse the drag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 005ecae commit 321b0f8

4 files changed

Lines changed: 151 additions & 85 deletions

File tree

‎src/Reactor/Core/Diagnostics/HResults.cs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,17 @@ internal static class HResults
3838
/// <summary>Access is denied.</summary>
3939
public const int ERROR_ACCESS_DENIED = unchecked((int)0x80070005);
4040

41+
/// <summary>
42+
/// HRESULT_FROM_WIN32(ERROR_INVALID_OPERATION_ID=4317) — "The operation
43+
/// identifier is not valid." Thrown by the WinUI <see cref="Microsoft.UI.Xaml.Window"/>
44+
/// projection when properties are accessed on a Window whose underlying
45+
/// COM proxy has been disconnected by <c>Close</c> → <c>Dispose</c>.
46+
/// Distinct from the standard teardown set (RPC_E_DISCONNECTED / E_HANDLE
47+
/// / CO_E_OBJNOTCONNECTED / RPC_E_SERVERFAULT) — surfaces specifically
48+
/// when reading <c>Window.Content</c> across a Close boundary.
49+
/// </summary>
50+
public const int ERROR_INVALID_OPERATION_ID = unchecked((int)0x800710DD);
51+
4152
/// <summary>
4253
/// True if <paramref name="hr"/> is one of the WinUI / COM teardown-reentry
4354
/// HRESULTs — the standard "your proxy is gone, your handle is gone, your

‎src/Reactor/Docking/Native/DockFloatingWindow.cs‎

Lines changed: 98 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Runtime.CompilerServices;
2+
using System.Runtime.InteropServices;
23
using Microsoft.UI.Reactor.Core;
34
using Microsoft.UI.Reactor.Hosting;
45
using Microsoft.UI.Xaml;
@@ -542,9 +543,29 @@ void RemoveLocal(DockableContent pane)
542543
// Capture the source XamlRoot BEFORE RemoveLocal might close
543544
// ownWindow (single-tab case → its only pane is the one we're
544545
// tearing off, panes goes to empty, ownWindow.Close()).
545-
// Accessing it through a closed window's NativeWindow would NRE.
546+
//
547+
// Cross-render teardown race exercised by selftest T14
548+
// (stuck-state recovery: defensive ForceCancel + retry on a
549+
// TabView whose host Window was closed by the prior tear-off).
550+
// `holder[0]` survives across renders, so the second
551+
// BeginFloatingTearOff invocation reads `ownWindow` as a
552+
// disposed `ReactorWindow`. `NativeWindow` is a raw field
553+
// accessor (no `_disposed` guard) → `.Content` is a WinRT
554+
// projection call into the disconnected COM proxy → throws
555+
// COMException with HResult 0x800710DD
556+
// (HRESULT_FROM_WIN32(ERROR_INVALID_OPERATION_ID), surface
557+
// text "The operation identifier is not valid.").
558+
//
559+
// Narrow to exactly that HResult. Any other COM failure here
560+
// is a real bug we want to surface. The `xr ??= ...` fallback
561+
// below picks up the freshly-opened dragged preview's
562+
// XamlRoot when sourceXamlRoot stays null.
546563
XamlRoot? sourceXamlRoot = null;
547-
try { sourceXamlRoot = ownWindow.NativeWindow?.Content?.XamlRoot; } catch { }
564+
try { sourceXamlRoot = ownWindow.NativeWindow?.Content?.XamlRoot; }
565+
catch (COMException ex) when (ex.HResult == Core.Diagnostics.HResults.ERROR_INVALID_OPERATION_ID)
566+
{
567+
// Source window closed by prior tear-off — fall through to fallback.
568+
}
548569

549570
// Whether this tear-off will leave ownWindow with no remaining
550571
// panes (→ RemoveLocal closes it). Drives the Z-order strategy
@@ -564,18 +585,19 @@ void RemoveLocal(DockableContent pane)
564585
var initialTopLeft = (
565586
(double)(req.CursorScreenPhys.X - req.PressOffsetPhys.X) / req.SourceScale,
566587
(double)(req.CursorScreenPhys.Y - req.PressOffsetPhys.Y) / req.SourceScale);
567-
ReactorWindow draggedWindow;
568-
try
569-
{
570-
draggedWindow = DockFloatingWindow.Open(
571-
pane,
572-
manager: manager,
573-
opacity: 0.5,
574-
noActivate: true,
575-
ignorePointerInput: true,
576-
initialPosition: initialTopLeft);
577-
}
578-
catch { return null; }
588+
// DockFloatingWindow.Open is a normal-code-flow call: its
589+
// failure modes (WinUI window-creation refusal, pre-condition
590+
// violations) are bugs we want to surface, not swallow. The
591+
// layout mutation (RemoveLocal) and session.Begin happen
592+
// AFTER Open returns, so a throw here leaves source state
593+
// intact.
594+
var draggedWindow = DockFloatingWindow.Open(
595+
pane,
596+
manager: manager,
597+
opacity: 0.5,
598+
noActivate: true,
599+
ignorePointerInput: true,
600+
initialPosition: initialTopLeft);
579601
// CRITICAL: stop ownWindow from intercepting pointer events
580602
// before the preview opens. ownWindow has foreground focus
581603
// (user just clicked a tab) and sits at the top of Z-order;
@@ -594,13 +616,39 @@ void RemoveLocal(DockableContent pane)
594616
// drag events; restored in confirm/cancel below.
595617
if (willClose)
596618
{
597-
try { ownWindow.AppWindow.Hide(); }
598-
catch { /* window may already be tearing down */ }
619+
// ReactorWindow.Hide() wraps _appWindow.Hide() with the
620+
// _disposed guard + teardown-reentry COMException catch
621+
// (the bare ownWindow.AppWindow.Hide() bypass below
622+
// didn't have either). The previous outer catch was
623+
// hiding any throw path; using the wrapper makes the
624+
// behavior explicit and the catch redundant.
625+
ownWindow.Hide();
599626
}
600627
else
601628
{
602-
try { ownWindow.SetIgnorePointerInput(true); }
603-
catch { /* window may already be tearing down */ }
629+
// Multi-tab path: source window stays visible with its
630+
// remaining tabs. We mark it click-through so the host
631+
// overlays see the drag's pointer events.
632+
//
633+
// SetIgnorePointerInput requires WS_EX_LAYERED on the
634+
// underlying HWND (OS only honors transparent on layered
635+
// windows). A regular floating window opens with full
636+
// opacity → not layered → SetIgnorePointerInput(true)
637+
// would throw InvalidOperationException. Nudge opacity
638+
// to 0.9999 first to install WS_EX_LAYERED — the alpha
639+
// byte rounds to 255 so the user sees no visual change.
640+
// RestoreSourcePointerInput (below) reverses both.
641+
//
642+
// The previous code wrapped this whole block in a bare
643+
// catch and so the SetIgnorePointerInput call was being
644+
// silently swallowed by the InvalidOperationException
645+
// every multi-tab tear-off — the source window was NEVER
646+
// actually marked click-through. Pointer-pass-through
647+
// here is required behavior per §2.6, so the fix is to
648+
// make the call succeed, not to keep swallowing it.
649+
if (ownWindow.Spec.Opacity >= 1.0)
650+
ownWindow.SetOpacity(0.9999);
651+
ownWindow.SetIgnorePointerInput(true);
604652
}
605653
RemoveLocal(pane);
606654

@@ -617,24 +665,30 @@ void RestoreSourcePointerInput()
617665
{
618666
// Multi-tab case only — undo the SetIgnorePointerInput
619667
// toggle so the user can interact with the source window's
620-
// remaining tabs after the drag ends. No-op when the
621-
// source closed.
668+
// remaining tabs after the drag ends, and restore full
669+
// opacity (paired with the 0.9999 nudge above that
670+
// installed WS_EX_LAYERED). Both calls are no-op on a
671+
// disposed window, so the genuine external race (user
672+
// closed the source window mid-drag) is safe — no catch
673+
// needed.
622674
if (capturedSource is null) return;
623-
try { capturedSource.SetIgnorePointerInput(false); }
624-
catch { /* window may have been closed already */ }
675+
capturedSource.SetIgnorePointerInput(false);
676+
capturedSource.SetOpacity(1.0);
625677
}
626678
Action confirm = () =>
627679
{
628680
var target = DockTabTearOff.TryConfirmHoveredTargetFor(manager);
629681
if (target is not null)
630682
{
631683
RestoreSourcePointerInput();
684+
// ReactorWindow.Close is idempotent (no-op after
685+
// _disposed) and internally narrows the teardown
686+
// COMException set, so calling it once per branch —
687+
// enqueued OR sync fallback, never both — is safe
688+
// without an outer catch.
632689
var dq = global::Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread();
633-
bool ok = dq is not null && dq.TryEnqueue(() =>
634-
{
635-
try { capturedDragged.Close(); } catch { }
636-
});
637-
if (!ok) { try { capturedDragged.Close(); } catch { } }
690+
bool ok = dq is not null && dq.TryEnqueue(capturedDragged.Close);
691+
if (!ok) capturedDragged.Close();
638692
return;
639693
}
640694
// No target hit — drop-outside semantics: strip drag styles,
@@ -652,9 +706,10 @@ void RestoreSourcePointerInput()
652706

653707
// If we couldn't capture the source XamlRoot (rare race), fall
654708
// back to the dragged window's once it's mounted. Worst case
655-
// RasterizationScale is 1.0 default.
656-
var xr = sourceXamlRoot;
657-
try { xr ??= capturedDragged.NativeWindow?.Content?.XamlRoot; } catch { }
709+
// RasterizationScale is 1.0 default. capturedDragged was
710+
// opened synchronously above, so its NativeWindow chain is
711+
// live; the `?.` chain handles the not-yet-mounted Content.
712+
var xr = sourceXamlRoot ?? capturedDragged.NativeWindow?.Content?.XamlRoot;
658713

659714
return new DockTabTearOff.TearOffActive
660715
{
@@ -669,14 +724,19 @@ void RestoreSourcePointerInput()
669724

670725
static void RestoreWindowFromDrag(ReactorWindow w)
671726
{
672-
try
673-
{
674-
w.SetIgnorePointerInput(false);
675-
w.SetOpacity(1.0);
676-
w.SetNoActivate(false);
677-
w.Activate();
678-
}
679-
catch { /* window may have been closed already */ }
727+
// All four ReactorWindow mutators are no-op on _disposed and
728+
// internally narrow the teardown COMException set, so a
729+
// genuine external close mid-drag (e.g. user closed the
730+
// dragged preview through some other path) is safe without
731+
// an outer catch. The drop-outside contract says this window
732+
// stays open at the cursor's release position; if it's
733+
// actually closed by the time we get here, that's a bug we
734+
// want to surface rather than silently strip styles on a
735+
// dead handle.
736+
w.SetIgnorePointerInput(false);
737+
w.SetOpacity(1.0);
738+
w.SetNoActivate(false);
739+
w.Activate();
680740
}
681741

682742
var chrome = DockFloatingWindow.BuildChrome(

‎src/Reactor/Docking/Native/DockHostNativeComponent.cs‎

Lines changed: 26 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -475,23 +475,19 @@ void LogOp(
475475
(double)(req.CursorScreenPhys.X - req.PressOffsetPhys.X) / req.SourceScale,
476476
(double)(req.CursorScreenPhys.Y - req.PressOffsetPhys.Y) / req.SourceScale);
477477

478-
ReactorWindow floatingWindow;
479-
try
480-
{
481-
floatingWindow = DockFloatingWindow.Open(
482-
pane,
483-
manager: manager,
484-
opacity: 0.5,
485-
noActivate: true,
486-
ignorePointerInput: true,
487-
initialPosition: initialTopLeft);
488-
}
489-
catch
490-
{
491-
// Failure → no mutation should be visible. We didn't
492-
// StoreOverride yet so the layout is still intact.
493-
return null;
494-
}
478+
// DockFloatingWindow.Open is a normal-code-flow call: its
479+
// failure modes are bugs (WinUI window-creation refusal,
480+
// pre-condition violations) that we want to surface, not
481+
// swallow. The layout mutation (StoreOverride) is deferred
482+
// until after Open returns so a throw here leaves
483+
// effectiveLayout intact.
484+
var floatingWindow = DockFloatingWindow.Open(
485+
pane,
486+
manager: manager,
487+
opacity: 0.5,
488+
noActivate: true,
489+
ignorePointerInput: true,
490+
initialPosition: initialTopLeft);
495491

496492
StoreOverride(afterRemove);
497493
manager.OnContentFloated?.Invoke(new DockContentFloatedEventArgs { Content = pane });
@@ -576,30 +572,25 @@ void FinalizeImmediateDrop(ReactorWindow floatingWindow, DockableContent pane, b
576572
// breaks WinUI's dispatcher (see the spike doc's
577573
// "render-time cleanup" gotcha). One queued tick is
578574
// enough to let the host's pending re-render flush.
575+
// ReactorWindow.Close is idempotent (no-op after _disposed)
576+
// and internally swallows the COMException teardown-reentry
577+
// set, so calling it once per branch — enqueued OR sync
578+
// fallback, never both — is safe without an outer catch.
579579
var dq = global::Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread();
580-
bool enqueued = dq is not null && dq.TryEnqueue(() =>
581-
{
582-
try { floatingWindow.Close(); }
583-
catch { /* window may already be tearing down */ }
584-
});
585-
if (!enqueued)
586-
{
587-
try { floatingWindow.Close(); } catch { }
588-
}
580+
bool enqueued = dq is not null && dq.TryEnqueue(floatingWindow.Close);
581+
if (!enqueued) floatingWindow.Close();
589582
return;
590583
}
591584

592585
// No target hit (drop-outside / cancel). Strip the
593586
// drag-only window styles so the floating window becomes a
594-
// real, interactive floating pane.
595-
try
596-
{
597-
floatingWindow.SetIgnorePointerInput(false);
598-
floatingWindow.SetOpacity(1.0);
599-
floatingWindow.SetNoActivate(false);
600-
floatingWindow.Activate();
601-
}
602-
catch { }
587+
// real, interactive floating pane. All four ReactorWindow
588+
// mutators are no-op on _disposed and internally narrow the
589+
// teardown COMException set, so no outer catch is needed.
590+
floatingWindow.SetIgnorePointerInput(false);
591+
floatingWindow.SetOpacity(1.0);
592+
floatingWindow.SetNoActivate(false);
593+
floatingWindow.Activate();
603594

604595
session?.End();
605596
setDragActive(false);

‎src/Reactor/Docking/Native/DockTabTearOff.cs‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -547,18 +547,22 @@ private static void PositionFloating()
547547
var cursorPhys = DockTabTearOff.TryGetCursorPhys();
548548
var x = cursorPhys.X - active.OffsetPhys.X;
549549
var y = cursorPhys.Y - active.OffsetPhys.Y;
550-
try
551-
{
552-
// Bypass ReactorWindow.SetPosition's DIP→physical roundtrip
553-
// (which uses the window's _dpi — unreliable for a fresh
554-
// NoActivate window). Drive AppWindow.Move with absolute
555-
// screen-physical coords directly. Same coordinate system
556-
// as GetCursorPos returns, so the cursor and the window
557-
// top-left stay locked together exactly.
558-
active.FloatingWindow.AppWindow.Move(
559-
new global::Windows.Graphics.PointInt32(x, y));
560-
}
561-
catch { /* window may have been closed externally */ }
550+
// Bypass ReactorWindow.SetPosition's DIP→physical roundtrip
551+
// (which uses the window's _dpi — unreliable for a fresh
552+
// NoActivate window). Drive AppWindow.Move with absolute
553+
// screen-physical coords directly. Same coordinate system
554+
// as GetCursorPos returns, so the cursor and the window
555+
// top-left stay locked together exactly.
556+
//
557+
// Ordering guarantee: every path that closes the floating window
558+
// (FinalizeInternal, ForceCancel, host-unmount UseEffect cleanup)
559+
// calls Stop() FIRST, which detaches the Tick handler and clears
560+
// s_active. PositionFloating is only invoked from OnTick (timer
561+
// disabled after Stop) and synchronously from Start (window is
562+
// fresh). No path leaves a live tracker pointing at a closed
563+
// window, so AppWindow.Move always targets a live AppWindow.
564+
active.FloatingWindow.AppWindow.Move(
565+
new global::Windows.Graphics.PointInt32(x, y));
562566
}
563567

564568
private static void FinalizeInternal(bool commit)

0 commit comments

Comments
 (0)