Take OffScreenBrowser's thread out of WinForms' application lifetime (BL-16668) - #8177
Take OffScreenBrowser's thread out of WinForms' application lifetime (BL-16668)#8177andrew-polk wants to merge 4 commits into
Conversation
…D an epub (BL-16668) Every existing CreateArtifactsCommandTests case requests a single artifact, which is why CI stayed green through the bug the harvester hit in production. The harvester always passes --bloomdOutputPath/--bloomDigitalOutputPath *and* --epubOutputPath in one run, and only that combination exposes BL-16668: building the bloomdigital creates and then disposes PublishHelper's off-screen browser, whose dedicated thread is running the only WinForms message loop a CLI process has. Ending that loop makes WinForms raise Application.ApplicationExit, ApplicationContainer disposes itself in response, and the epub step -- which runs after -- then dies with ObjectDisposedException resolving ProjectContext.BookServer, so createArtifacts returns EpubException. This test fails on the current code with exactly the production stack trace, and is the shared repro for the two candidate fixes on their separate branches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…plication.Run (BL-16668) Application.Run registers the loop it starts as an *application* message loop. So when the off-screen browser's dedicated thread ended its loop, WinForms concluded the application was exiting -- correct in the GUI, badly wrong in a CLI process where ours is the only such loop -- and raised Application.ApplicationExit. ApplicationContainer acted on that and disposed itself, taking down the parent scope of the ProjectContext that `Bloom.exe createArtifacts` was still using. The epub step, which runs right after the bloompub step releases the browser, then died with ObjectDisposedException, so the harvester failed every book with epubs enabled and uploaded no artifacts at all (BH-7836). Rather than teach one more listener to distrust the event, stop telling the lie: pump with a bare GetMessage/TranslateMessage/DispatchMessage loop and end it with PostQuitMessage from the teardown we already post onto that thread. That dispatches everything the thread actually needs -- the WindowsFormsSynchronizationContext's marshaling control and the WebView2's windows and async completions -- while staying invisible to WinForms' application-lifetime bookkeeping. This removes the whole bug class: no non-GUI entry point (harvester, bulk upload, a future CLI verb) can now be told the application is exiting merely because a worker finished with a browser. ReleaseBrowser's resource-saving intent is untouched -- we still tear the browser and its thread down per batch of page checks. Since Application.Run also disposed the thread's windows as its loop ended, the pump now disposes the synchronization context (and its marshaling control) itself, so we don't leave a Control to be finalized on a dead thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two documentation fixes from Devin's review of the previous commit, no behavior change. The note on ctv.Dispose() claimed "Application.Run disposed this thread's windows as its loop ended". That conflates two things: WinForms' loop teardown destroys the thread's window HANDLES, but it does not Dispose non-Form controls -- the browser has always been disposed explicitly from our own Dispose(). Reworded so it no longer implies prior coverage that never existed. (The previous commit's message carries the same overstatement; this is the correction.) Also recorded the deliberate other half of the trade-off: because WinForms no longer knows about this loop, Application.Exit() no longer ends it either, so a GUI shutdown with a browser still alive leaves this thread pumping until the process goes away. That is harmless -- the thread is IsBackground so it cannot hold the process open, and the CoreWebView2 process dies with its host -- but it is a real consequence of leaving the bookkeeping and deserves to be written down rather than rediscovered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude Opus 5 (1M context) from Andrew Polk's machine during preflight. Consulted Devin on 2026-08-07, twice: first up to commit The second pass raised 1 Bug and 3 Investigate flags, each now its own thread:
Also 3 Informational items, not mirrored. Two of them — a comment that overstated what CI: the only GitHub check is |
…BL-16668) FatalExceptionHandler asked Application.MessageLoop to choose between a graceful ProgramExit.Exit() and a bare Environment.Exit(1). But that flag is per-THREAD: it answers "does the thread this exception arrived on have a WinForms message loop", not "is there a running GUI to shut down". For anything surfacing on a worker thread the answer was no even with Bloom running normally, so we took the hard-kill path -- which does not run `finally` blocks, and therefore skips the one in Main that releases Bloom's single-instance token. The user's Bloom could then refuse to start again, which is the exact failure ProgramExit exists to prevent. Previously the off-screen browser's thread hid this, by accident: it called Application.Run, so MessageLoop was true there. Moving it to a private pump exposed the pre-existing flaw rather than creating it -- any other worker thread was always at risk. So ask the question directly: Program.MainMessageLoopIsRunning is true only while the Application.Run in RunBloom is pumping. This is correct for every thread, not just the browser's. Found by Devin during preflight review of the previous commits; the choice to fix it here rather than defer was Andrew's. Also recorded, at Andrew's request, the decision to accept the WinForms ThreadContext that each off-screen browser now leaves behind: self-limiting (the table is keyed by reused native thread ids) and only truly removable by keeping one browser thread alive for the process, which would undo ReleaseBrowser's intent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude Opus 5 (1M context) from Andrew Polk's machine during preflight. Consulted Devin a third time on 2026-08-07, up to commit Re-review clean — no new findings, and the bug is confirmed fixed. Devin marks its earlier bug resolved, citing the switch to Thread status — all four now resolved with a documented outcome:
Five Informational items this pass, none mirrored. One is new — a note that the practical effect of the Full C# suite at this commit, run locally: 3049 passed, 12 skipped, 1 failed — the one failure is an opt-in Reading App Builder test failing its own environment precondition on this machine, unrelated to this branch. Note that nothing server-side builds or tests this branch (the |
1 participant
Bloom.exe createArtifactswas dying withObjectDisposedExceptionon the epub step, so theharvester failed every book with epubs enabled and uploaded no artifacts at all — not even
the .bloompub it had already built successfully. Reported downstream as BH-7836.
What was happening
Application.Runregisters the loop it starts as an application message loop.OffScreenBrowser(introduced in
48d8e2588cfor BL-12614 / BL-13120) runs one on its own dedicated STA thread. TheCLI path never calls
Application.Runitself — it waits withThread.Sleep+Application.DoEvents— so that browser thread's loop is the only application message loop in the process. Therefore:
createArtifactsbuilds the .bloompub first;BloomPubMakerdoesusing (var helper = new PublishHelper()).PublishHelper.ReleaseBrowser()→OffScreenBrowser.Dispose()→ApplicationContext.ExitThread().Application.ApplicationExit.ApplicationContainer.OnApplicationExitdisposes the container — the parent ofProjectContext._scope.s_projectContext.BookServer→ObjectDisposedException.In the GUI this never fired, because the main thread's loop is still running. It only bites a
non-GUI entry point.
The fix
Rather than teach one more listener to distrust the event, stop telling the lie: pump the browser
thread with a bare
GetMessage/TranslateMessage/DispatchMessageloop and end it withPostQuitMessagefrom the teardown we already post onto that thread. That dispatches everythingthe thread actually needs — the
WindowsFormsSynchronizationContext's marshaling control and theWebView2's windows and async completions — while staying invisible to WinForms'
application-lifetime bookkeeping.
This removes the whole bug class: no non-GUI entry point (harvester, bulk upload, a future CLI
verb) can now be told the application is exiting merely because a worker finished with a browser.
ReleaseBrowser's resource-saving intent is untouched — the browser and its thread are still torndown per batch of page checks. Since
Application.Runalso disposed the thread's windows as itsloop ended, the pump now disposes the synchronization context itself, so we don't leave a
Controlto be finalized on a dead thread.
Regression test
Every existing
CreateArtifactsCommandTestscase requested a single artifact, which is exactly whyCI stayed green through this. The harvester always passes
--bloomdOutputPath/--bloomDigitalOutputPathand--epubOutputPathin one run, and only that combination exposesthe bug — with one artifact, nothing runs after the premature disposal. The new test asks for both
and fails on unmodified
masterwith the production stack trace, frame for frame:The test commit is deliberately byte-identical to the one on
BL-16668-harvester-exit-guard, whichcarries the minimal alternative fix (guarding
OnApplicationExitonProgram.RunningHarvesterMode)for comparison.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16668
Devin review
This change is