Skip to content

Bug: breadcrumb-webview-post-executes-under-upgrade-lifetime-lock #500

Description

@drmoisan
  • Work Mode: full-bug

Summary

BreadcrumbCoordinatorUpgradeLifetime.TryRunCurrent invokes the caller's action while holding its
_sync monitor, so a WebView2 post runs under two nested locks (lifetime._sync then hub._sync)
and reaches an out-of-process SDK call from inside both. Because lock is re-entrant, the lock does
not deliver the atomicity it appears to: a re-entrant call on the same thread can mutate _current
between the currency check and the completion of the action, which is the exact invariant
TryRunCurrent exists to enforce.

Environment

  • OS/version: Windows 11 Pro 10.0.26200
  • Python version: n/a (C# / .NET Framework 4.8.1 WinForms VSTO add-in with Microsoft WebView2)
  • Command/flags used: n/a - reached through the QuickFiler ItemViewer breadcrumb selector
  • Data source or fixture: any breadcrumb suggestion population that issues a render/selector post

Steps to Reproduce

This is a concurrency and re-entrancy defect established by code inspection rather than a
deterministic user-facing repro. No existing test reproduces it, and constructing one requires a
re-entrant STA message pump, which repository unit-test policy prohibits.

  1. Populate breadcrumb suggestions so BreadcrumbBridgeCoordinator.PostRenderAndSelectorAsync runs.
  2. Observe that _messenger.PostJson executes inside BreadcrumbCoordinatorUpgradeLifetime._sync.
  3. In production the messenger is BreadcrumbMessengerHub, whose PostJson takes its own _sync
    and, still holding it, calls PostToSurface, reaching the WebView2 SDK.

Expected Behavior

The currency check and the guarded action should be atomic with respect to lease invalidation, and
no out-of-process SDK call should be made while a lock is held. Locks should cover state mutation
only, with the action invoked outside them, re-checking currency as needed.

Actual Behavior

The action runs inside the lock. Because Monitor is re-entrant, a re-entrant BeginPopulation,
Invalidate, or TryDispose on the same thread acquires lifetime._sync successfully and mutates
_current mid-action, defeating the guarantee. Separately, a re-entrant Attach/Detach during the
hub's broadcast would throw InvalidOperationException because the hub holds _sync across its
foreach.

Logs / Screenshots

  • Attached minimal logs or screenshot
  • Snippet: n/a - no exception is raised on the current wiring; the defect is a silently unenforced
    invariant.

Impact / Severity

  • Blocker
  • High
  • Medium
  • Low

Rationale: no deadlock is demonstrable on current code and the lock ordering is consistent, so this
is a latent correctness hazard rather than an active failure. It is recorded at Medium because an STA
COM call can pump messages and re-enter managed code, which is precisely the condition that voids the
intended atomicity.

Source

From: docs/features/potential/2026-08-08-breadcrumb-webview-post-executes-under-upgrade-lifetime-lock.md

Activity

  1. added a commit that references this issue on Aug 28, 2026
  2. drmoisan commented on Sep 1, 2026

    @drmoisan
    OwnerAuthor

    Closing as completed. Delivered under breadcrumb-coordinator-hub-defects-501, merged via PR #659 (merge commit 4cb709db), an ancestor of origin/main. The delivering spec (docs/features/active/breadcrumb-coordinator-hub-defects-501/spec.md) states "Issue: #501 (also closes #462, #500, #502)."

    Confirmed directly against origin/main:QuickFiler/Viewers/BreadcrumbCoordinatorUpgradeLifetime.cs: TryRunCurrent now captures the currency verdict inside lock (_sync), releases the lock, and only then invokes action() outside it. The method's own doc comment cites this issue explicitly: "Issue #500 (I-500.1): the action is invoked with _sync RELEASED, so no foreign or out-of-process call is ever made under the lifetime's monitor." The hub half (BreadcrumbMessengerHub.PostJson) received the matching fix in the same commit — attachments are snapshotted under _sync, then broadcast outside the lock.

    All four invariants this issue drove (AC-04 through AC-07, I-500.1 through I-500.4) are checked [x] in the spec, including a regression test (AC-17).

    Verified during an admission check against the bugs-638-644-647 parallel run (2026-09-01). No branch or PR remains to ship; this was left open only as bookkeeping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions