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>

Author: codemonkeychris

src/Reactor/Docking/Native/DockFloatingWindow.cs

@@ -542,9 +542,11 @@ void RemoveLocal(DockableContent pane)
             // Capture the source XamlRoot BEFORE RemoveLocal might close
             // ownWindow (single-tab case → its only pane is the one we're
             // tearing off, panes goes to empty, ownWindow.Close()).
-            // Accessing it through a closed window's NativeWindow would NRE.
-            XamlRoot? sourceXamlRoot = null;
-            try { sourceXamlRoot = ownWindow.NativeWindow?.Content?.XamlRoot; } catch { }
+            // ownWindow was just retrieved from holder[0] inside this
+            // synchronous handler, so its Window/Content/XamlRoot are
+            // alive; the `?.` chain handles the not-yet-mounted edge
+            // case where Content is null.
+            var sourceXamlRoot = ownWindow.NativeWindow?.Content?.XamlRoot;
 
             // Whether this tear-off will leave ownWindow with no remaining
             // panes (→ RemoveLocal closes it). Drives the Z-order strategy
@@ -564,18 +566,19 @@ void RemoveLocal(DockableContent pane)
             var initialTopLeft = (
                 (double)(req.CursorScreenPhys.X - req.PressOffsetPhys.X) / req.SourceScale,
                 (double)(req.CursorScreenPhys.Y - req.PressOffsetPhys.Y) / req.SourceScale);
-            ReactorWindow draggedWindow;
-            try
-            {
-                draggedWindow = DockFloatingWindow.Open(
-                    pane,
-                    manager: manager,
-                    opacity: 0.5,
-                    noActivate: true,
-                    ignorePointerInput: true,
-                    initialPosition: initialTopLeft);
-            }
-            catch { return null; }
+            // DockFloatingWindow.Open is a normal-code-flow call: its
+            // failure modes (WinUI window-creation refusal, pre-condition
+            // violations) are bugs we want to surface, not swallow. The
+            // layout mutation (RemoveLocal) and session.Begin happen
+            // AFTER Open returns, so a throw here leaves source state
+            // intact.
+            var draggedWindow = DockFloatingWindow.Open(
+                pane,
+                manager: manager,
+                opacity: 0.5,
+                noActivate: true,
+                ignorePointerInput: true,
+                initialPosition: initialTopLeft);
             // CRITICAL: stop ownWindow from intercepting pointer events
             // before the preview opens. ownWindow has foreground focus
             // (user just clicked a tab) and sits at the top of Z-order;
@@ -594,13 +597,39 @@ void RemoveLocal(DockableContent pane)
             //     drag events; restored in confirm/cancel below.
             if (willClose)
             {
-                try { ownWindow.AppWindow.Hide(); }
-                catch { /* window may already be tearing down */ }
+                // ReactorWindow.Hide() wraps _appWindow.Hide() with the
+                // _disposed guard + teardown-reentry COMException catch
+                // (the bare ownWindow.AppWindow.Hide() bypass below
+                // didn't have either). The previous outer catch was
+                // hiding any throw path; using the wrapper makes the
+                // behavior explicit and the catch redundant.
+                ownWindow.Hide();
             }
             else
             {
-                try { ownWindow.SetIgnorePointerInput(true); }
-                catch { /* window may already be tearing down */ }
+                // Multi-tab path: source window stays visible with its
+                // remaining tabs. We mark it click-through so the host
+                // overlays see the drag's pointer events.
+                //
+                // SetIgnorePointerInput requires WS_EX_LAYERED on the
+                // underlying HWND (OS only honors transparent on layered
+                // windows). A regular floating window opens with full
+                // opacity → not layered → SetIgnorePointerInput(true)
+                // would throw InvalidOperationException. Nudge opacity
+                // to 0.9999 first to install WS_EX_LAYERED — the alpha
+                // byte rounds to 255 so the user sees no visual change.
+                // RestoreSourcePointerInput (below) reverses both.
+                //
+                // The previous code wrapped this whole block in a bare
+                // catch and so the SetIgnorePointerInput call was being
+                // silently swallowed by the InvalidOperationException
+                // every multi-tab tear-off — the source window was NEVER
+                // actually marked click-through. Pointer-pass-through
+                // here is required behavior per §2.6, so the fix is to
+                // make the call succeed, not to keep swallowing it.
+                if (ownWindow.Spec.Opacity >= 1.0)
+                    ownWindow.SetOpacity(0.9999);
+                ownWindow.SetIgnorePointerInput(true);
             }
             RemoveLocal(pane);
 
@@ -617,24 +646,30 @@ void RestoreSourcePointerInput()
             {
                 // Multi-tab case only — undo the SetIgnorePointerInput
                 // toggle so the user can interact with the source window's
-                // remaining tabs after the drag ends. No-op when the
-                // source closed.
+                // remaining tabs after the drag ends, and restore full
+                // opacity (paired with the 0.9999 nudge above that
+                // installed WS_EX_LAYERED). Both calls are no-op on a
+                // disposed window, so the genuine external race (user
+                // closed the source window mid-drag) is safe — no catch
+                // needed.
                 if (capturedSource is null) return;
-                try { capturedSource.SetIgnorePointerInput(false); }
-                catch { /* window may have been closed already */ }
+                capturedSource.SetIgnorePointerInput(false);
+                capturedSource.SetOpacity(1.0);
             }
             Action confirm = () =>
             {
                 var target = DockTabTearOff.TryConfirmHoveredTargetFor(manager);
                 if (target is not null)
                 {
                     RestoreSourcePointerInput();
+                    // ReactorWindow.Close is idempotent (no-op after
+                    // _disposed) and internally narrows the teardown
+                    // COMException set, so calling it once per branch —
+                    // enqueued OR sync fallback, never both — is safe
+                    // without an outer catch.
                     var dq = global::Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread();
-                    bool ok = dq is not null && dq.TryEnqueue(() =>
-                    {
-                        try { capturedDragged.Close(); } catch { }
-                    });
-                    if (!ok) { try { capturedDragged.Close(); } catch { } }
+                    bool ok = dq is not null && dq.TryEnqueue(capturedDragged.Close);
+                    if (!ok) capturedDragged.Close();
                     return;
                 }
                 // No target hit — drop-outside semantics: strip drag styles,
@@ -652,9 +687,10 @@ void RestoreSourcePointerInput()
 
             // If we couldn't capture the source XamlRoot (rare race), fall
             // back to the dragged window's once it's mounted. Worst case
-            // RasterizationScale is 1.0 default.
-            var xr = sourceXamlRoot;
-            try { xr ??= capturedDragged.NativeWindow?.Content?.XamlRoot; } catch { }
+            // RasterizationScale is 1.0 default. capturedDragged was
+            // opened synchronously above, so its NativeWindow chain is
+            // live; the `?.` chain handles the not-yet-mounted Content.
+            var xr = sourceXamlRoot ?? capturedDragged.NativeWindow?.Content?.XamlRoot;
 
             return new DockTabTearOff.TearOffActive
             {
@@ -669,14 +705,19 @@ void RestoreSourcePointerInput()
 
         static void RestoreWindowFromDrag(ReactorWindow w)
         {
-            try
-            {
-                w.SetIgnorePointerInput(false);
-                w.SetOpacity(1.0);
-                w.SetNoActivate(false);
-                w.Activate();
-            }
-            catch { /* window may have been closed already */ }
+            // All four ReactorWindow mutators are no-op on _disposed and
+            // internally narrow the teardown COMException set, so a
+            // genuine external close mid-drag (e.g. user closed the
+            // dragged preview through some other path) is safe without
+            // an outer catch. The drop-outside contract says this window
+            // stays open at the cursor's release position; if it's
+            // actually closed by the time we get here, that's a bug we
+            // want to surface rather than silently strip styles on a
+            // dead handle.
+            w.SetIgnorePointerInput(false);
+            w.SetOpacity(1.0);
+            w.SetNoActivate(false);
+            w.Activate();
         }
 
         var chrome = DockFloatingWindow.BuildChrome(

src/Reactor/Docking/Native/DockHostNativeComponent.cs

@@ -475,23 +475,19 @@ void LogOp(
                 (double)(req.CursorScreenPhys.X - req.PressOffsetPhys.X) / req.SourceScale,
                 (double)(req.CursorScreenPhys.Y - req.PressOffsetPhys.Y) / req.SourceScale);
 
-            ReactorWindow floatingWindow;
-            try
-            {
-                floatingWindow = DockFloatingWindow.Open(
-                    pane,
-                    manager: manager,
-                    opacity: 0.5,
-                    noActivate: true,
-                    ignorePointerInput: true,
-                    initialPosition: initialTopLeft);
-            }
-            catch
-            {
-                // Failure → no mutation should be visible. We didn't
-                // StoreOverride yet so the layout is still intact.
-                return null;
-            }
+            // DockFloatingWindow.Open is a normal-code-flow call: its
+            // failure modes are bugs (WinUI window-creation refusal,
+            // pre-condition violations) that we want to surface, not
+            // swallow. The layout mutation (StoreOverride) is deferred
+            // until after Open returns so a throw here leaves
+            // effectiveLayout intact.
+            var floatingWindow = DockFloatingWindow.Open(
+                pane,
+                manager: manager,
+                opacity: 0.5,
+                noActivate: true,
+                ignorePointerInput: true,
+                initialPosition: initialTopLeft);
 
             StoreOverride(afterRemove);
             manager.OnContentFloated?.Invoke(new DockContentFloatedEventArgs { Content = pane });
@@ -576,30 +572,25 @@ void FinalizeImmediateDrop(ReactorWindow floatingWindow, DockableContent pane, b
                 // breaks WinUI's dispatcher (see the spike doc's
                 // "render-time cleanup" gotcha). One queued tick is
                 // enough to let the host's pending re-render flush.
+                // ReactorWindow.Close is idempotent (no-op after _disposed)
+                // and internally swallows the COMException teardown-reentry
+                // set, so calling it once per branch — enqueued OR sync
+                // fallback, never both — is safe without an outer catch.
                 var dq = global::Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread();
-                bool enqueued = dq is not null && dq.TryEnqueue(() =>
-                {
-                    try { floatingWindow.Close(); }
-                    catch { /* window may already be tearing down */ }
-                });
-                if (!enqueued)
-                {
-                    try { floatingWindow.Close(); } catch { }
-                }
+                bool enqueued = dq is not null && dq.TryEnqueue(floatingWindow.Close);
+                if (!enqueued) floatingWindow.Close();
                 return;
             }
 
             // No target hit (drop-outside / cancel). Strip the
             // drag-only window styles so the floating window becomes a
-            // real, interactive floating pane.
-            try
-            {
-                floatingWindow.SetIgnorePointerInput(false);
-                floatingWindow.SetOpacity(1.0);
-                floatingWindow.SetNoActivate(false);
-                floatingWindow.Activate();
-            }
-            catch { }
+            // real, interactive floating pane. All four ReactorWindow
+            // mutators are no-op on _disposed and internally narrow the
+            // teardown COMException set, so no outer catch is needed.
+            floatingWindow.SetIgnorePointerInput(false);
+            floatingWindow.SetOpacity(1.0);
+            floatingWindow.SetNoActivate(false);
+            floatingWindow.Activate();
 
             session?.End();
             setDragActive(false);

src/Reactor/Docking/Native/DockTabTearOff.cs

@@ -547,18 +547,22 @@ private static void PositionFloating()
         var cursorPhys = DockTabTearOff.TryGetCursorPhys();
         var x = cursorPhys.X - active.OffsetPhys.X;
         var y = cursorPhys.Y - active.OffsetPhys.Y;
-        try
-        {
-            // Bypass ReactorWindow.SetPosition's DIP→physical roundtrip
-            // (which uses the window's _dpi — unreliable for a fresh
-            // NoActivate window). Drive AppWindow.Move with absolute
-            // screen-physical coords directly. Same coordinate system
-            // as GetCursorPos returns, so the cursor and the window
-            // top-left stay locked together exactly.
-            active.FloatingWindow.AppWindow.Move(
-                new global::Windows.Graphics.PointInt32(x, y));
-        }
-        catch { /* window may have been closed externally */ }
+        // Bypass ReactorWindow.SetPosition's DIP→physical roundtrip
+        // (which uses the window's _dpi — unreliable for a fresh
+        // NoActivate window). Drive AppWindow.Move with absolute
+        // screen-physical coords directly. Same coordinate system
+        // as GetCursorPos returns, so the cursor and the window
+        // top-left stay locked together exactly.
+        //
+        // Ordering guarantee: every path that closes the floating window
+        // (FinalizeInternal, ForceCancel, host-unmount UseEffect cleanup)
+        // calls Stop() FIRST, which detaches the Tick handler and clears
+        // s_active. PositionFloating is only invoked from OnTick (timer
+        // disabled after Stop) and synchronously from Start (window is
+        // fresh). No path leaves a live tracker pointing at a closed
+        // window, so AppWindow.Move always targets a live AppWindow.
+        active.FloatingWindow.AppWindow.Move(
+            new global::Windows.Graphics.PointInt32(x, y));
     }
 
     private static void FinalizeInternal(bool commit)