From 20b9b445368d44b27832b553eb3087ff0186e228 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Thu, 2 Jul 2026 18:13:00 -0500 Subject: [PATCH 1/2] Rework BookProcessor to use OffScreenBrowser (BL-16430) Allows us to finally retire RunJavascriptWithStringResult_Sync_Dangerous. --- src/BloomExe/Book/BookProcessor.cs | 155 +++++++----------- src/BloomExe/IBrowser.cs | 5 - src/BloomExe/Publish/OffScreenBrowser.cs | 131 ++++++++++++--- src/BloomExe/WebView2Browser.cs | 66 ++++---- src/BloomExe/web/controllers/ExternalApi.cs | 49 +++--- src/BloomTests/Book/BookProcessorTests.cs | 173 ++++++++++++++++++++ 6 files changed, 400 insertions(+), 179 deletions(-) create mode 100644 src/BloomTests/Book/BookProcessorTests.cs diff --git a/src/BloomExe/Book/BookProcessor.cs b/src/BloomExe/Book/BookProcessor.cs index 77d954de3879..57346972f2df 100644 --- a/src/BloomExe/Book/BookProcessor.cs +++ b/src/BloomExe/Book/BookProcessor.cs @@ -2,9 +2,9 @@ using System.Diagnostics; using System.Linq; using System.Threading; -using System.Windows.Forms; using Bloom.Api; using Bloom.Edit; +using Bloom.Publish; using Bloom.ToPalaso; using SIL.Progress; @@ -39,7 +39,9 @@ public static class BookProcessor /// end is skipped, and nothing is persisted. The caller (external/process-book) surfaces this /// as an error so BloomBridge can re-run, rather than leaving a half-processed book on disk. /// - /// Must be called on the UI thread: it creates and pumps a WebView2 browser. + /// Can run on any thread (and the caller runs it on a background thread so the UI stays responsive): + /// the WebView2 it drives lives on the OffScreenBrowser's own dedicated thread, and this method just + /// blocks on it. The book, however, must not be touched by another thread while we process it. /// /// When is true, simple two-pane origami pages with a /// single illustration in the first pane and a single text block in the second pane have their @@ -51,11 +53,6 @@ public static class BookProcessor /// public static int ProcessBook(Book book, bool fitImageTextSplits = false) { - Debug.Assert( - Program.RunningOnUiThread || Program.RunningUnitTests, - "BookProcessor.ProcessBook must run on the UI thread (it drives a WebView2)." - ); - // 1. Structural "make it right" pass. Besides migrations, this ensures stylesheet links // (and, when we Save below, the actual CSS files) that BloomBridge's raw HTML may // be missing. See BookStorage.EnsureHasLinksToStylesheets. @@ -85,57 +82,48 @@ public static int ProcessBook(Book book, bool fitImageTextSplits = false) var pageIndex = 0; - // Create a FRESH WebView2 control per page, but share ONE CoreWebView2Environment across them. + // Process every page with an off-screen browser that lives on its own dedicated thread, so we can + // drive it with blocking calls that never pump the main UI message loop. // - // Fresh control per page: we must NOT reuse a single control. Each editing page is a full + // Fresh renderer per page: we must NOT reuse a single browser control. Each editing page is a full // live-edit page (it opens the edit WebSocket channel and fires editView/* API calls on load); // that residual state wedges the next top-level navigation in the same control, which hangs or - // crashes Bloom on the 2nd page. A fresh control gives each page a clean renderer, torn down on - // dispose, so every page behaves like the always-working first one. + // crashes Bloom on the 2nd page. StartFreshBrowser() gives each page a clean renderer, so every + // page behaves like the always-working first one. // - // Shared environment: left to itself, each control would also create its OWN environment — a new - // browser process and a cold HTTP cache — costing ~300ms+ per page. Sharing one environment - // (one process, one user-data folder, one cache) across the batch removes that per-page cost and - // warms the cache so later pages navigate faster. Measured ~31s -> ~18s for a 22-page book - // (per-page init ~315ms -> ~100ms, nav ~940ms -> ~550ms after page 1). - WebView2Browser.BeginSharedEnvironmentBatch(); - try + // Shared environment: OffScreenBrowser keeps ONE CoreWebView2Environment (one browser process, + // user-data folder, and HTTP cache) alive across those fresh renderers, so we don't pay + // environment creation per page and later pages navigate against a warm cache. (Measured, with the + // previous shared-environment approach, ~31s -> ~18s for a 22-page book.) + using (var browser = new OffScreenBrowser()) { - foreach (var page in pages) + try { - pageIndex++; - Browser browser = null; - try - { - browser = BrowserMaker.MakeBrowser(); - // Force the control's window handle into existence. The WebView2's CoreWebView2 - // initialization (kicked off unawaited in the browser constructor) can only complete - // once the control has a realized HWND; until it does, _readyToNavigate never becomes - // true and navigation just times out. HtmlThumbNailer does the same thing for its - // off-screen browser. (We never add this browser to a Form, so nothing else would - // create the handle for us.) - browser.CreateControl(); + // If realizing the off-screen browser pulled Bloom to the front, put the previous window + // back so we keep processing quietly in the background. + RestoreForeground(priorForeground); - // If realizing the off-screen browser pulled Bloom to the front, put the - // previous window back so we keep processing quietly in the background. - RestoreForeground(priorForeground); + foreach (var page in pages) + { + pageIndex++; + if (pageIndex > 1) + { + // Fresh renderer for this page (see above); the shared environment stays warm. + browser.StartFreshBrowser(); + RestoreForeground(priorForeground); + } ProcessOnePage(book, browser, page, fitImageTextSplits); Log($"page {pageIndex}/{pages.Count} [{page.Id}] done"); } - finally - { - browser?.Dispose(); - } } - } - finally - { - WebView2Browser.EndSharedEnvironmentBatch(); - // Final safety net: make sure we leave the foreground where we found it, even if a - // page threw partway through the batch. - RestoreForeground(priorForeground); + finally + { + // Final safety net: make sure we leave the foreground where we found it, even if a + // page threw partway through the batch. + RestoreForeground(priorForeground); + } } // 3. One full save now that every page's in-memory DOM has been updated. @@ -175,7 +163,7 @@ private static void Log(string message) private static void ProcessOnePage( Book book, - Browser browser, + OffScreenBrowser browser, IPage page, bool fitImageTextSplits ) @@ -197,31 +185,15 @@ bool fitImageTextSplits // Frame == "Editing View is updating single displayed page": serves the page as-is. // (Unlike JustCheckingPage, it does not swap videos for placeholder images.) // - // We deliberately do NOT use NavigateAndWaitTillDone here. That waits for the WebView2 - // NavigationCompleted event (the window 'load' event), which is unreliable for a full - // editing page loaded off-screen: the editing bundle opens the edit WebSocket channel and - // fires editView/* API calls on load, and we have observed document.readyState getting - // stuck at "interactive" (load never firing) even though bootstrap()/SetupElements() has - // fully run and there are no pending sub-resources. Waiting for 'load' just burns the whole - // timeout. Instead we fire the navigation and then poll for __bloomEditablePageReady, which - // is the signal we actually care about (the load-time DOM fix-ups are in place). - // - // Navigate() blocks internally until the control is ready to navigate, so we pre-wait here. - // With the shared environment the heavy browser-process/cache warm-up is paid once for the - // batch, so after the first page this mostly waits on the control's handle/CoreWebView2 - // initialization. - var readyTimer = Stopwatch.StartNew(); - while (!browser.IsReadyToNavigate) - { - if (readyTimer.ElapsedMilliseconds >= kReadyTimeoutMs) - throw new ApplicationException( - $"process-book: timed out waiting for the browser to become ready to navigate on page {page.Id}." - ); - Application.DoEvents(); - Thread.Sleep(5); - } - - browser.Navigate(dom, source: InMemoryHtmlFileSource.Frame); + // We navigate WITHOUT waiting for the 'load' event, which is unreliable for a full editing page + // loaded off-screen: the editing bundle opens the edit WebSocket channel and fires editView/* API + // calls on load, and we have observed document.readyState getting stuck at "interactive" (load + // never firing) even though bootstrap()/SetupElements() has fully run and there are no pending + // sub-resources. Waiting for 'load' would just burn the whole timeout. Instead we fire the + // navigation and then poll for __bloomEditablePageReady, which is the signal we actually care + // about (the load-time DOM fix-ups are in place). The browser is already ready to navigate (the + // OffScreenBrowser blocked until it was) so there is no ready-wait to do here. + browser.NavigateWithoutWaitingForLoad(dom, InMemoryHtmlFileSource.Frame); // Wait until bootstrap()/SetupElements() has actually run (signaled by // __bloomEditablePageReady), not merely until the bundle's exports exist, so the load-time @@ -230,7 +202,7 @@ bool fitImageTextSplits browser, "(window.__bloomEditablePageReady && window.editablePageBundle) ? 'ready' : ''", "the editing bundle to initialize", - page + page.Id ); // Ask the bundle to gather the (now browser-processed) page content. It stashes the @@ -239,9 +211,7 @@ bool fitImageTextSplits // The boolean tells the bundle whether to auto-fit simple image/text origami splits before // capturing (see fitImageOverTextSplits in bloomEditing.ts). // Fire-and-forget: capture is asynchronous (it stashes onto window.__bloomExternalPageContent - // when it finishes) and we poll for that below, so there is no result to wait for here. The - // sync RunJavascriptWithStringResult_Sync_Dangerous would only block until the script's - // synchronous kickoff returned, which buys us nothing. + // when it finishes) and we poll for that below, so there is no result to wait for here. browser.RunJavascriptFireAndForget( $"window.editablePageBundle.captureContentForExternalProcessing({(fitImageTextSplits ? "true" : "false")})" ); @@ -250,7 +220,7 @@ bool fitImageTextSplits browser, "window.__bloomExternalPageContent || ''", "the page content to be captured", - page + page.Id ); if (pageContent.StartsWith("ERROR:", StringComparison.Ordinal)) @@ -268,35 +238,36 @@ bool fitImageTextSplits } /// - /// Polls a javascript expression (which evaluates to a non-empty string when "ready") until - /// it is non-empty or we time out, returning the result. + /// Polls a javascript expression (which evaluates to a non-empty string when "ready") against the + /// given off-screen browser until it is non-empty or we time out, returning the result. /// - /// We deliberately keep this "poll a window global" approach for the off-screen processor - /// rather than the async editView/pageContent API callback the live editor uses: that callback - /// pattern needs the live EditingModel and edit WebSocket channel, which a throwaway off-screen - /// browser doesn't have, and the per-page loop here wants a deterministic in-line result. We are - /// aware RunJavascriptWithStringResult_Sync_Dangerous is the same family of call implicated in - /// past page-content deadlocks (BL-13120 etc.); moving this to an async/callback design is - /// deferred. That method already pumps the message loop while it waits for each script to - /// return, so we do NOT pump again between polls (just sleep briefly). + /// We deliberately keep this "poll a window global" approach for the off-screen processor rather than + /// the async editView/pageContent API callback the live editor uses: that callback pattern needs the + /// live EditingModel and edit WebSocket channel, which a throwaway off-screen browser doesn't have, + /// and the per-page loop here wants a deterministic in-line result. Each poll blocks the calling + /// thread while the browser's OWN thread runs the script, so we never pump the main UI message loop + /// between polls (just sleep briefly) — this is what replaced the old + /// RunJavascriptWithStringResult_Sync_Dangerous, which pumped the main loop and risked the reentrancy + /// behind past page-content deadlocks (BL-13120 etc.). /// - private static string WaitForJavascriptResult( - Browser browser, + internal static string WaitForJavascriptResult( + OffScreenBrowser browser, string script, string whatWeAreWaitingFor, - IPage page + string pageId, + int timeoutMs = kReadyTimeoutMs ) { var timer = Stopwatch.StartNew(); - while (timer.ElapsedMilliseconds < kReadyTimeoutMs) + while (timer.ElapsedMilliseconds < timeoutMs) { - var result = browser.RunJavascriptWithStringResult_Sync_Dangerous(script); + var result = browser.RunJavascript(script); if (!string.IsNullOrEmpty(result)) return result; Thread.Sleep(20); } throw new ApplicationException( - $"process-book: timed out waiting for {whatWeAreWaitingFor} on page {page.Id}." + $"process-book: timed out waiting for {whatWeAreWaitingFor} on page {pageId}." ); } } diff --git a/src/BloomExe/IBrowser.cs b/src/BloomExe/IBrowser.cs index fd25e58fdf3c..c284c304b2f5 100644 --- a/src/BloomExe/IBrowser.cs +++ b/src/BloomExe/IBrowser.cs @@ -324,11 +324,6 @@ public async void OnCopyPageHtml(object sender, EventArgs e) PortableClipboard.SetText(html); } - [Obsolete( - "This method is dangerous because it has to loop Application.DoEvents(). RunJavaScriptAsync() is preferred." - )] - public abstract string RunJavascriptWithStringResult_Sync_Dangerous(string script); - public abstract Task GetStringFromJavascriptAsync(string script); public abstract Task GetObjectFromJavascriptAsync(string script); public abstract Task RunJavascriptAsync(string script); diff --git a/src/BloomExe/Publish/OffScreenBrowser.cs b/src/BloomExe/Publish/OffScreenBrowser.cs index 1db0f91d5a1a..a8d53433a54e 100644 --- a/src/BloomExe/Publish/OffScreenBrowser.cs +++ b/src/BloomExe/Publish/OffScreenBrowser.cs @@ -5,6 +5,7 @@ using System.Windows.Forms; using Bloom.Api; using Bloom.Book; +using Microsoft.Web.WebView2.Core; namespace Bloom.Publish { @@ -18,6 +19,14 @@ namespace Bloom.Publish /// nothing here is specific to that; it suits any task that needs to ask a real browser questions about a /// document off-screen. /// + /// One instance can manage a SERIES of inner browsers over its lifetime, all sharing a single + /// CoreWebView2Environment: call to discard the current browser and + /// continue with a clean one (a fresh renderer with no residual page state) while keeping that + /// environment — so the browser process, user-data folder, and HTTP cache stay warm across the series. + /// The environment is created lazily with the first browser and belongs to this instance alone (it is + /// deliberately not static/shared between instances, so instances on different threads never contend). + /// A caller that only needs one browser simply never calls StartFreshBrowser. + /// /// How it stays safe: the browser is owned by a private, dedicated STA thread with its own Windows Forms /// message loop, and THAT thread — not the caller — services the browser's callbacks. So a caller can /// simply block for a result. It never has to pump the MAIN UI message loop the way @@ -35,6 +44,11 @@ public sealed class OffScreenBrowser : IDisposable private readonly Thread _thread; private WebView2Browser _browser; + // The one CoreWebView2Environment (browser process + user-data folder + HTTP cache) shared across + // every inner browser this instance creates. Captured from the first browser and reused for each + // fresh browser (see StartFreshBrowser), so we don't pay environment creation each time. + private CoreWebView2Environment _environment; + // The message loop we run on the dedicated thread; ExitThread() on it ends the loop at Dispose. private ApplicationContext _appContext; @@ -88,16 +102,9 @@ private void ThreadMain() SynchronizationContext.SetSynchronizationContext(ctx); _ctx = ctx; - // The constructor kicks off CoreWebView2 initialization unawaited; it completes via the message - // loop we start below. - _browser = new WebView2Browser(); - // Realize the HWND now; CoreWebView2 initialization can only complete once the control has a - // window handle, and nothing else (no parent Form) will create it for us. Same trick as - // BookProcessor's off-screen browser. - _browser.CreateControl(); - - // Poll for readiness once the loop is pumping, then signal the constructor. - ctx.Post(_ => WaitUntilReadyThenSignal(), null); + // Create the browser and wait for it to become ready once the loop is pumping (its async + // CoreWebView2 initialization completes via that loop), then signal the constructor. + ctx.Post(_ => InitializeAsync(), null); _appContext = new ApplicationContext(); Application.Run(_appContext); // pump until the context's loop is ended in Dispose @@ -109,19 +116,14 @@ private void ThreadMain() } } - private async void WaitUntilReadyThenSignal() + private async void InitializeAsync() { try { - var timer = Stopwatch.StartNew(); - while (!_browser.IsReadyToNavigate) - { - if (timer.ElapsedMilliseconds > kInitTimeoutMs) - throw new ApplicationException( - "Timed out initializing the off-screen WebView2." - ); - await Task.Delay(20); - } + await CreateInnerBrowserAndWaitReadyAsync(); + // Capture the environment the first browser created, so each later fresh browser can reuse it + // (see StartFreshBrowser) instead of paying environment creation again. + _environment = _browser.CoreEnvironment; } catch (Exception e) { @@ -133,14 +135,60 @@ private async void WaitUntilReadyThenSignal() } } + // Creates the inner browser — reusing our shared environment once we have captured one — and waits + // until it is ready to navigate. Runs on the dedicated thread. + private async Task CreateInnerBrowserAndWaitReadyAsync() + { + _browser = + _environment == null + ? new WebView2Browser() + : WebView2Browser.CreateWithInjectedEnvironment(_environment); + // Realize the HWND now; CoreWebView2 initialization can only complete once the control has a + // window handle, and nothing else (no parent Form) will create it for us. Same trick as + // BookProcessor's off-screen browser. + _browser.CreateControl(); + + var timer = Stopwatch.StartNew(); + while (!_browser.IsReadyToNavigate) + { + if (timer.ElapsedMilliseconds > kInitTimeoutMs) + throw new ApplicationException( + "Timed out initializing the off-screen WebView2." + ); + await Task.Delay(20); + } + } + /// /// Navigates the browser to the given DOM (served via BloomServer) and blocks the calling thread until /// navigation completes, times out, or is cancelled. Returns true on successful navigation, false on - /// timeout or cancellation — matching NavigateAndWaitTillDone's contract for the caller. + /// timeout or cancellation — matching NavigateAndWaitTillDone's contract for the caller. The source + /// controls how BloomServer serves the page (e.g. JustCheckingPage swaps videos for placeholders, + /// Frame serves the page as-is for the editing bundle). + /// + public bool Navigate( + HtmlDom htmlDom, + int timeoutMs, + Func cancelCheck, + InMemoryHtmlFileSource source = InMemoryHtmlFileSource.JustCheckingPage + ) + { + return RunAndBlock(() => NavigateAsync(htmlDom, timeoutMs, cancelCheck, source)); + } + + /// + /// Starts navigating to the given DOM but does NOT wait for the navigation-completed ('load') event; + /// blocks only until the navigation has been dispatched. Use this for full editing pages whose 'load' + /// is unreliable off-screen (document.readyState can stick at "interactive"); the caller instead polls + /// a window flag the page's own script sets when it is actually ready (see BookProcessor). /// - public bool Navigate(HtmlDom htmlDom, int timeoutMs, Func cancelCheck) + public void NavigateWithoutWaitingForLoad(HtmlDom htmlDom, InMemoryHtmlFileSource source) { - return RunAndBlock(() => NavigateAsync(htmlDom, timeoutMs, cancelCheck)); + RunAndBlock(() => + { + _browser.Navigate(htmlDom, source: source); + return Task.FromResult(true); + }); } // Async navigation using only the public Browser API (DocumentCompleted is raised on the WebView2's @@ -149,7 +197,8 @@ public bool Navigate(HtmlDom htmlDom, int timeoutMs, Func cancelCheck) private async Task NavigateAsync( HtmlDom htmlDom, int timeoutMs, - Func cancelCheck + Func cancelCheck, + InMemoryHtmlFileSource source ) { var navigated = new TaskCompletionSource(); @@ -157,7 +206,7 @@ Func cancelCheck _browser.DocumentCompleted += Handler; try { - _browser.Navigate(htmlDom, source: InMemoryHtmlFileSource.JustCheckingPage); + _browser.Navigate(htmlDom, source: source); var timer = Stopwatch.StartNew(); while (!navigated.Task.IsCompleted) { @@ -185,6 +234,38 @@ public string RunJavascript(string script) return RunAndBlock(() => _browser.GetStringFromJavascriptAsync(script)); } + /// + /// Runs the given script without waiting for it to finish (beyond its synchronous kickoff). Use this + /// for scripts that start asynchronous work and stash their result on a window global that the caller + /// then polls for (via ). Blocks only until the script has been dispatched + /// on the browser's thread. + /// + public void RunJavascriptFireAndForget(string script) + { + RunAndBlock(() => + { + _browser.RunJavascriptFireAndForget(script); + return Task.FromResult(true); + }); + } + + /// + /// Discards the current inner browser and continues with a fresh one — a clean renderer with no + /// residual page state — reusing the same environment, then blocks until it is ready. Use this + /// whenever you need a clean browser (for example, to isolate one navigation from the previous one's + /// leftover state) without paying to recreate the environment: the browser process, user-data folder, + /// and HTTP cache stay warm across the series. + /// + public void StartFreshBrowser() + { + RunAndBlock(async () => + { + _browser?.Dispose(); + await CreateInnerBrowserAndWaitReadyAsync(); + return true; + }); + } + // Marshals an async function onto the dedicated thread and BLOCKS the calling thread for its result. // Blocking is safe here precisely because the dedicated thread — not the caller — pumps the messages // that let the awaited WebView2 operations complete. diff --git a/src/BloomExe/WebView2Browser.cs b/src/BloomExe/WebView2Browser.cs index ea37d040260b..f3bc4e320efc 100644 --- a/src/BloomExe/WebView2Browser.cs +++ b/src/BloomExe/WebView2Browser.cs @@ -49,6 +49,37 @@ public partial class WebView2Browser : Browser // to work the old way. private WebView2Browser(string dummy) { } + // When set (via CreateWithInjectedEnvironment), InitWebView uses this already-created environment + // instead of making its own. Lets OffScreenBrowser share one environment — one browser process, + // user-data folder, and HTTP cache — across the fresh browser it makes per page, thread-safely and + // without the global shared-environment batch statics. + private CoreWebView2Environment _injectedEnvironment; + + /// + /// Create a browser that reuses the given already-created CoreWebView2Environment instead of making its + /// own. As with the default constructor, initialization is kicked off unawaited; callers wait on + /// before navigating. + /// + internal static WebView2Browser CreateWithInjectedEnvironment( + CoreWebView2Environment environment + ) + { + // The string argument selects the do-nothing private constructor so we can set the injected + // environment before InitWebView runs, then initialize exactly as the normal constructor does. + var browser = new WebView2Browser("dummy"); + browser._injectedEnvironment = environment; + browser.InitializeComponent(); + browser.InitWebView(); + return browser; + } + + /// + /// The CoreWebView2Environment this browser was initialized with, or null if it is not yet ready. + /// A caller can capture this from one browser and pass it to CreateWithInjectedEnvironment to make + /// further browsers that share the same environment. + /// + internal CoreWebView2Environment CoreEnvironment => _webview?.CoreWebView2?.Environment; + /// /// This gives us a way to create a WebView2Browser with the async init properly awaited. /// It would be good to use this everywhere, but I think a lot of stuff would have to become async. @@ -432,10 +463,13 @@ private async Task InitWebView() // because EnsureCoreWebView2Async still fires this control's own InitializationCompleted, which // is what sets _readyToNavigate.) SetupEventHandling(); - // During a shared-environment batch, reuse the one environment (and its browser process + - // user-data folder + HTTP cache) so we don't pay environment creation per browser. The first - // browser of the batch falls through and creates it; subsequent ones reuse it. - var env = _useSharedEnvironment ? _sharedEnvironment : null; + // Reuse an existing environment (and its browser process + user-data folder + HTTP cache) when we + // can, so we don't pay environment creation per browser: + // - _injectedEnvironment: an environment handed to this instance (OffScreenBrowser shares one + // environment across the fresh browser it creates per page — see CreateWithInjectedEnvironment); + // - _sharedEnvironment: the legacy on-UI-thread shared-environment batch (BookProcessor's old path). + // Otherwise we fall through and create a fresh one. + var env = _injectedEnvironment ?? (_useSharedEnvironment ? _sharedEnvironment : null); if (env == null) { string dataFolder; @@ -737,30 +771,6 @@ public override async Task SaveDocumentAsync(string path) RobustFile.WriteAllText(path, html, Encoding.UTF8); } - public override string RunJavascriptWithStringResult_Sync_Dangerous(string script) - { - Task task = GetStringFromJavascriptAsync(script); - // This is dangerous. E.g. it caused this bug: https://issues.bloomlibrary.org/youtrack/issue/BL-12614 - // Came from an answer in https://stackoverflow.com/questions/65327263/how-to-get-sync-return-from-executescriptasync-in-webview2' - // It's quite possible for the user to initiate a new command while we are trying to run the - // javascript before completing the last thing the user requested. - // Note: at one point, I thought making our code async all the way would solve this, but now I'm not so sure. - // A click event handler can be async, but it is "void" async (no task to await), so I think it can still - // return control to the main message loop before all the async stuff it started has completed. - // The reentrancy possibly caused BL-13120 and friends, when we were using this method to get the page content - // to save. - // We tried using a message filter to suppress messages that might cause reentrancy, but found - // that somehow it didn't fully solve the problem, AND things could get stuck in a state where - // everything got filtered. - while (!task.IsCompleted) - { - Application.DoEvents(); - Thread.Sleep(10); - } - - return task.Result; - } - public override async Task RunJavascriptAsync(string script) { await _webview.ExecuteScriptAsync(script); diff --git a/src/BloomExe/web/controllers/ExternalApi.cs b/src/BloomExe/web/controllers/ExternalApi.cs index 473193851308..00117c0e2039 100644 --- a/src/BloomExe/web/controllers/ExternalApi.cs +++ b/src/BloomExe/web/controllers/ExternalApi.cs @@ -346,14 +346,11 @@ private void HandleProcessBook(ApiRequest request) return; } - // The heavy work needs the UI thread (it creates and pumps an off-screen WebView2). We - // are on the UI thread now, but we must NOT do that work here: holding the HTTP response - // open for the whole ~20-30s run is exactly what leaves the client hung if the connection - // is dropped (e.g. a stale keep-alive socket). Instead reply immediately with a jobId and - // BeginInvoke the work to run after this response is sent — it queues on this same UI - // thread's message loop and runs once this handler returns. The client polls - // external/process-book-status for the outcome. _processBookInProgress stays true for the - // whole async run, so the re-entrancy guard still rejects an overlapping process-book. + // We must NOT do the heavy work here: holding the HTTP response open for the whole ~20-30s run + // is exactly what leaves the client hung if the connection is dropped (e.g. a stale keep-alive + // socket). Instead reply immediately with a jobId and run the work on a background thread; the + // client polls external/process-book-status for the outcome. _processBookInProgress stays true for + // the whole async run, so the re-entrancy guard still rejects an overlapping process-book. var shell = Shell.GetShellOrNull(); if (shell == null || shell.IsDisposed) { @@ -369,17 +366,19 @@ private void HandleProcessBook(ApiRequest request) } request.ReplyWithJson(new { jobId, state = "running" }); - shell.BeginInvoke( - (Action)(() => RunProcessBookJob(jobId, folderPath, id, fitImageTextSplits)) + // Run on a background thread, NOT the UI thread: ProcessBook drives its WebView2 on the + // OffScreenBrowser's own thread and just blocks on it, so keeping this off the UI thread leaves + // the UI free to paint the "processing" overlay and stay responsive for the whole run. + _ = System.Threading.Tasks.Task.Run(() => + RunProcessBookJob(jobId, folderPath, id, fitImageTextSplits) ); } /// - /// Runs the heavy process-book work on the UI thread. Posted via BeginInvoke from - /// HandleProcessBook so it executes after that request's reply has already been sent, then - /// records the outcome on _processBookJob for the client to poll via - /// external/process-book-status. Never throws to the caller (it is a fire-and-forget UI - /// message); any failure is captured as the job's "failed" state. + /// Runs the heavy process-book work on a background thread (dispatched from HandleProcessBook after + /// that request's reply has already been sent), then records the outcome on _processBookJob for the + /// client to poll via external/process-book-status. Never throws to the caller (it is a fire-and-forget + /// background task); any failure is captured as the job's "failed" state. /// private void RunProcessBookJob( string jobId, @@ -390,17 +389,14 @@ bool fitImageTextSplits { try { - // The overlay 'show', the DoEvents/Sleep spin-up loop, and the processing all run - // inside this try so that an exception anywhere after we raise the overlay still runs - // the finally and sends 'hide'; otherwise the modal overlay would be stuck opaque - // until the user navigates away. (Sending 'hide' when 'show' never succeeded is a - // harmless no-op.) + // The overlay 'show' and the processing both run inside this try so that an exception anywhere + // after we raise the overlay still runs the finally and sends 'hide'; otherwise the modal + // overlay would be stuck opaque until the user navigates away. (Sending 'hide' when 'show' + // never succeeded is a harmless no-op.) try { - // Let the user know Bloom is busy. The work below pumps the message loop via - // Application.DoEvents(), so the main WebView2 keeps painting and this overlay - // (with its CSS spinner) stays visible/animated for the whole run. Pump a few - // events first so it actually appears before the heavy work ties up the thread. + // Let the user know Bloom is busy. We run off the UI thread, so the UI thread is free to + // paint this overlay (with its CSS spinner) and keep it animated for the whole run. dynamic overlay = new DynamicJson(); // Intentionally NOT localized, like the add-book/update-book toasts: this is an // operator-facing message shown only during a BloomBridge-driven processing run. @@ -410,11 +406,6 @@ bool fitImageTextSplits "show", overlay ); - for (var i = 0; i < 10; i++) - { - System.Windows.Forms.Application.DoEvents(); - System.Threading.Thread.Sleep(15); - } var result = !string.IsNullOrEmpty(folderPath) ? ProcessBookByPath(folderPath, fitImageTextSplits) diff --git a/src/BloomTests/Book/BookProcessorTests.cs b/src/BloomTests/Book/BookProcessorTests.cs new file mode 100644 index 000000000000..f8bbd673bba6 --- /dev/null +++ b/src/BloomTests/Book/BookProcessorTests.cs @@ -0,0 +1,173 @@ +using System; +using Bloom; +using Bloom.Api; +using Bloom.Book; +using Bloom.Collection; +using Bloom.ImageProcessing; +using Bloom.Publish; +using NUnit.Framework; +using TemporaryFolder = SIL.TestUtilities.TemporaryFolder; + +namespace BloomTests.Book +{ + /// + /// Tests for BookProcessor.WaitForJavascriptResult — the polling primitive that used to sit on top of + /// Browser.RunJavascriptWithStringResult_Sync_Dangerous and now drives an OffScreenBrowser instead. + /// + /// It exercises the two things the off-screen page processor actually relies on: (1) waiting for a value + /// that a page-load script sets on window, and (2) waiting for a value that a fire-and-forget script sets + /// asynchronously some time later — plus the timeout path when the value never appears. These are the + /// behaviors WaitForJavascriptResult provides to ProcessOnePage; the full ProcessBook path (which needs the + /// whole editing bundle to initialize off-screen) is exercised by a manual process-book run. + /// + [TestFixture] + [Category("SkipOnTeamCity")] + public class BookProcessorTests + { + private TemporaryFolder _folder; + private BloomServer _server; + private OffScreenBrowser _browser; + + [SetUp] + public void SetUp() + { + _folder = new TemporaryFolder("BookProcessorTests"); + var collectionSettings = new CollectionSettings(); + var fileLocator = new BloomFileLocator( + collectionSettings, + new XMatterPackFinder(new[] { BloomFileLocator.GetFactoryXMatterDirectory() }), + ProjectContext.GetFactoryFileLocations(), + ProjectContext.GetFoundFileLocations(), + ProjectContext.GetAfterXMatterFileLocations() + ); + // A listening server so the OffScreenBrowser can actually fetch the in-memory page over http. + _server = new BloomServer( + new RuntimeImageProcessor(new BookRenamedEvent()), + new BookSelection(), + fileLocator + ); + _server.EnsureListening(); + + // Created after the server is listening, so navigation URLs resolve to it. + _browser = new OffScreenBrowser(); + } + + [TearDown] + public void TearDown() + { + _browser?.Dispose(); + _server?.Dispose(); + _folder?.Dispose(); + } + + // Builds a minimal page (served via BloomServer) whose body is the given markup. + private HtmlDom MakePageDom(string bodyInner) + { + var dom = new HtmlDom($"{bodyInner}"); + dom.BaseForRelativePaths = _folder.Path; + return dom; + } + + private void NavigateTo(HtmlDom dom) + { + var ok = _browser.Navigate(dom, 10000, () => false, InMemoryHtmlFileSource.Frame); + Assert.That(ok, Is.True, "SANITY: navigation to the test page should succeed"); + } + + [Test] + public void WaitForJavascriptResult_ReturnsValueSetByPageLoadScript() + { + // A script that runs as the page loads and sets a window variable — the shape of the + // __bloomEditablePageReady handshake ProcessOnePage waits on. + NavigateTo( + MakePageDom("
hello
") + ); + + var result = BookProcessor.WaitForJavascriptResult( + _browser, + "window.__ready || ''", + "the page-load flag", + "testPage" + ); + + Assert.That(result, Is.EqualTo("readyValue")); + } + + [Test] + public void WaitForJavascriptResult_ReturnsValueSetByFireAndForgetAfterDelay() + { + NavigateTo(MakePageDom("
hello
")); + + // SANITY: the value we're about to wait for is not set yet, so a non-empty result later + // can only come from the fire-and-forget script. + Assert.That( + _browser.RunJavascript("window.__delayed || ''"), + Is.Empty, + "SANITY: the delayed value should not be set before we kick off the script" + ); + + // Fire-and-forget: kicks off work that finishes (and sets the window variable) only later, + // mirroring captureContentForExternalProcessing stashing onto __bloomExternalPageContent. + _browser.RunJavascriptFireAndForget( + "setTimeout(function() { window.__delayed = 'delayedValue'; }, 300);" + ); + + var result = BookProcessor.WaitForJavascriptResult( + _browser, + "window.__delayed || ''", + "the delayed value", + "testPage" + ); + + Assert.That(result, Is.EqualTo("delayedValue")); + } + + [Test] + public void StartFreshBrowser_GivesFreshRendererButKeepsNavigating() + { + // Page 1 sets a marker on window. + NavigateTo(MakePageDom("
page1
")); + Assert.That( + _browser.RunJavascript("window.__marker || ''"), + Is.EqualTo("page1"), + "SANITY: the first page should have set its marker" + ); + + // Fresh renderer for the next page (what BookProcessor does between pages). + _browser.StartFreshBrowser(); + + // The fresh browser is a clean renderer: the previous page's window state is gone. + Assert.That( + _browser.RunJavascript("window.__marker || ''"), + Is.Empty, + "a fresh browser should be a clean renderer with no residual window state" + ); + + // And it can still navigate and run script, proving the shared environment survives the switch. + NavigateTo(MakePageDom("
page2
")); + Assert.That(_browser.RunJavascript("window.__marker || ''"), Is.EqualTo("page2")); + } + + [Test] + public void WaitForJavascriptResult_ThrowsOnTimeout_WhenValueNeverSet() + { + NavigateTo(MakePageDom("
hello
")); + + // SANITY: confirm the variable really is absent, so a timeout is because it is never set, + // not because we are looking at the wrong thing. + Assert.That(_browser.RunJavascript("window.__never || ''"), Is.Empty); + + // Short timeout so the test doesn't wait the full production budget. + var ex = Assert.Throws(() => + BookProcessor.WaitForJavascriptResult( + _browser, + "window.__never || ''", + "the value that never appears", + "testPage", + timeoutMs: 1000 + ) + ); + Assert.That(ex.Message, Does.Contain("the value that never appears")); + } + } +} From c53c873d467a9097326b3702e5115ed689a4e0a4 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Tue, 7 Jul 2026 17:57:34 -0500 Subject: [PATCH 2/2] Post-review fixes (BL-16430 part C) --- src/BloomExe/WebView2Browser.cs | 3 +- src/BloomExe/web/controllers/ExternalApi.cs | 126 +++++++++++++++----- 2 files changed, 95 insertions(+), 34 deletions(-) diff --git a/src/BloomExe/WebView2Browser.cs b/src/BloomExe/WebView2Browser.cs index f3bc4e320efc..b66b1f43f53b 100644 --- a/src/BloomExe/WebView2Browser.cs +++ b/src/BloomExe/WebView2Browser.cs @@ -69,7 +69,8 @@ CoreWebView2Environment environment var browser = new WebView2Browser("dummy"); browser._injectedEnvironment = environment; browser.InitializeComponent(); - browser.InitWebView(); + // Kicked off unawaited (like the default constructor); callers wait on IsReadyToNavigate. + _ = browser.InitWebView(); return browser; } diff --git a/src/BloomExe/web/controllers/ExternalApi.cs b/src/BloomExe/web/controllers/ExternalApi.cs index 00117c0e2039..d3b3f0a890d3 100644 --- a/src/BloomExe/web/controllers/ExternalApi.cs +++ b/src/BloomExe/web/controllers/ExternalApi.cs @@ -25,16 +25,21 @@ public class ExternalApi private readonly BookServer _bookServer; private readonly CollectionSettings _collectionSettings; - // Re-entrancy guard for process-book. ProcessBook occupies the UI thread but pumps the - // Windows message loop via Application.DoEvents() (both the pre-loop below and the per-page - // waits in BookProcessor). Because external/* handlers are dispatched on the UI thread via - // message posts, a second process-book request that arrives mid-run could be delivered - // re-entrantly during one of those DoEvents pumps. The shared-environment statics in - // WebView2Browser (_useSharedEnvironment/_sharedEnvironment) are explicitly NOT designed for - // overlapping batches, so we reject the re-entrant call rather than let the two corrupt each - // other. A plain bool is sufficient: everything here is single-threaded on the UI thread, so - // there is no cross-thread race to lock against. - private bool _processBookInProgress; + // Re-entrancy guard for process-book. A process-book request is validated and dispatched on + // the UI thread (HandleProcessBook), which sets this flag true, kicks off the heavy work on a + // background thread (see the Task.Run in HandleProcessBook), and returns immediately. We + // reject a second process-book that arrives while one is still running, because the + // shared-environment statics in WebView2Browser (_useSharedEnvironment/_sharedEnvironment) are + // explicitly NOT designed for overlapping batches and would corrupt each other. + // + // The UI thread is the only writer of 'true', and it reads the flag (to reject overlaps) and + // then sets it with no message pumping in between, so that check-then-set is atomic against + // UI-thread re-entrancy. The background job clears the flag to false when it finishes. Because + // that clear happens on the background thread while the UI thread reads the flag, it is marked + // volatile so the UI thread reliably sees the cleared value rather than caching a stale 'true' + // (which would wrongly reject every later process-book). A full lock isn't needed because the + // only cross-thread write is that release to false. + private volatile bool _processBookInProgress; // The most recent (or in-progress) external/process-book job. process-book replies // immediately with a jobId and runs the heavy work asynchronously on the UI thread; the @@ -369,6 +374,8 @@ private void HandleProcessBook(ApiRequest request) // Run on a background thread, NOT the UI thread: ProcessBook drives its WebView2 on the // OffScreenBrowser's own thread and just blocks on it, so keeping this off the UI thread leaves // the UI free to paint the "processing" overlay and stay responsive for the whole run. + // The parts of the job that touch WinForms (the editor/collection refresh) are marshaled + // back to the UI thread via InvokeOnUiThread. _ = System.Threading.Tasks.Task.Run(() => RunProcessBookJob(jobId, folderPath, id, fitImageTextSplits) ); @@ -445,6 +452,28 @@ bool fitImageTextSplits } } + /// + /// Run on the UI thread and wait for it to finish. The process-book + /// job runs on a background thread (see the Task.Run in HandleProcessBook) so the heavy + /// BookProcessor.ProcessBook() work doesn't freeze the UI, but the surrounding reconciliation + /// of live editor/collection state (ReloadCurrentBookDiscardingEdits / ReloadEditableCollection + /// / UpdateThumbnailAsync) touches WinForms and so must happen on the UI thread. We use the + /// synchronous Invoke (not BeginInvoke) so the refresh has completed before we report the job + /// "done" and so its ordering relative to the rest of the job is preserved. + /// + private void InvokeOnUiThread(Action action) + { + var shell = Shell.GetShellOrNull(); + if (shell == null || shell.IsDisposed) + throw new InvalidOperationException( + "external/process-book: cannot refresh the UI because Bloom's main window is gone." + ); + if (shell.InvokeRequired) + shell.Invoke(action); + else + action(); + } + /// /// Report the state of the most recent process-book job. The client polls this (with short, /// independent requests) until State is a terminal "done"/"failed", rather than waiting on one @@ -513,10 +542,18 @@ private ProcessBookResult ProcessBookByPath(string folderPath, bool fitImageText // isInEditableCollection:true + AlwaysEditSaveContext makes Book.IsSaveable true // (Book.IsSaveable => IsInEditableCollection && BookInfo.IsSaveable), matching the semantics // of the old flow where the book was first copied into the editable collection. - var selectedBeforeProcessing = _collectionModel.GetSelectedBookOrNull(); - var processingSelectedBook = - selectedBeforeProcessing != null - && AreSameFolder(selectedBeforeProcessing.FolderPath, folderPath); + // Whether we're about to reprocess the book currently open in the editor. Read the + // _collectionModel/_bookSelection state on the UI thread that owns it; this job otherwise + // runs on a background thread (only the heavy BookProcessor.ProcessBook() call below runs + // off the UI thread). + var processingSelectedBook = false; + InvokeOnUiThread(() => + { + var selectedBeforeProcessing = _collectionModel.GetSelectedBookOrNull(); + processingSelectedBook = + selectedBeforeProcessing != null + && AreSameFolder(selectedBeforeProcessing.FolderPath, folderPath); + }); var bookInfo = new BookInfo(folderPath, true, new AlwaysEditSaveContext()); var book = _bookServer.GetBookFromBookInfo(bookInfo); @@ -532,8 +569,13 @@ private ProcessBookResult ProcessBookByPath(string folderPath, bool fitImageText { try { - _editingModel.ReloadCurrentBookDiscardingEdits(); - _collectionModel.ReloadEditableCollection(); + // These reconcile live editor/collection state and touch WinForms, so they must + // run on the UI thread even though this job runs on a background thread. + InvokeOnUiThread(() => + { + _editingModel.ReloadCurrentBookDiscardingEdits(); + _collectionModel.ReloadEditableCollection(); + }); } catch (Exception e) { @@ -588,15 +630,28 @@ private static bool AreSameFolder(string a, string b) /// private ProcessBookResult ProcessBookById(string id, bool fitImageTextSplits) { - var editableCollection = _collectionModel.TheOneEditableCollection; - var collectionPath = editableCollection.PathToDirectory; - var bookInfo = _collectionModel.BookInfoFromCollectionAndId(collectionPath, id); + // Look up the book's BookInfo in the editable collection. Read _collectionModel's collection + // state on the UI thread that owns it; this job otherwise runs on a background thread (only the + // heavy BookProcessor.ProcessBook() call below runs off the UI thread). + string collectionPath = null; + BookInfo bookInfo = null; + InvokeOnUiThread(() => + { + collectionPath = _collectionModel.TheOneEditableCollection.PathToDirectory; + bookInfo = _collectionModel.BookInfoFromCollectionAndId(collectionPath, id); + }); if (bookInfo == null) { // The book may have just been written to disk and our in-memory collection cache // doesn't know about it yet. Rescan from disk and look again before giving up. - _collectionModel.ReloadEditableCollection(); - bookInfo = _collectionModel.BookInfoFromCollectionAndId(collectionPath, id); + // ReloadEditableCollection touches WinForms state, so (along with the re-lookup that + // depends on the reloaded collection) it must run on the UI thread even though this + // job runs on a background thread. + InvokeOnUiThread(() => + { + _collectionModel.ReloadEditableCollection(); + bookInfo = _collectionModel.BookInfoFromCollectionAndId(collectionPath, id); + }); } if (bookInfo == null) { @@ -621,20 +676,25 @@ private ProcessBookResult ProcessBookById(string id, bool fitImageTextSplits) // copy and reload it from disk so a later trip through the Edit tab can't clobber what we // just wrote, then refresh the collection's view of it (list metadata + thumbnail). This // mirrors how external/update-book handles re-import of the selected book. - var selected = _collectionModel.GetSelectedBookOrNull(); - if (selected != null && selected.ID == id) + // All of this reconciles live editor/collection state and touches WinForms, so it must + // run on the UI thread even though this job runs on a background thread. + InvokeOnUiThread(() => { - _editingModel.ReloadCurrentBookDiscardingEdits(); - _collectionModel.ReloadEditableCollection(); - var refreshedInfo = _collectionModel.BookInfoFromCollectionAndId( - collectionPath, - id - ); - if (refreshedInfo != null) - _collectionModel.UpdateThumbnailAsync( - _collectionModel.GetBookFromBookInfo(refreshedInfo) + var selected = _collectionModel.GetSelectedBookOrNull(); + if (selected != null && selected.ID == id) + { + _editingModel.ReloadCurrentBookDiscardingEdits(); + _collectionModel.ReloadEditableCollection(); + var refreshedInfo = _collectionModel.BookInfoFromCollectionAndId( + collectionPath, + id ); - } + if (refreshedInfo != null) + _collectionModel.UpdateThumbnailAsync( + _collectionModel.GetBookFromBookInfo(refreshedInfo) + ); + } + }); } catch (Exception e) {