[Win32][Edge] Fix BrowserFunction race condition using AddScriptToExecuteOnDocumentCreated

[HeikoKlare]

Summary

Fixes #20 (see also #2449 for a reproduction snippet).

In Edge/WebView2, execute() is asynchronous. When new BrowserFunction() is called, the injection script is queued via ExecuteScript(), but if a page navigation completes before WebView2 processes that queued script, the function is unavailable in the new document. This race is most easily triggered when two Browser instances are created in quick succession, as the second browser's initialization forces event-loop processing that can advance the first browser's navigation past the injection window.

Fix: Override createFunction() in Edge to register every function script via AddScriptToExecuteOnDocumentCreated. This WebView2 API guarantees the script runs on every future document creation, before any page scripts — eliminating the race condition. The script ID returned by the async registration is stored so it can be cleaned up via RemoveScriptToExecuteOnDocumentCreated when the BrowserFunction is disposed.

When createFunction() is called from within a WebView2 callback (inCallback > 0), blocking on the registration would deadlock, so the persistent registration is deferred via Display.asyncExec() to complete once the callback returns. This ensures every function is always persistently registered, removing the need for any NavigationStarting fallback re-injection.

Changes

• ICoreWebView2.java: Add missing RemoveScriptToExecuteOnDocumentCreated vtbl binding (index 28, between AddScriptToExecuteOnDocumentCreated at 27 and ExecuteScript at 29)
• Edge.java: Override createFunction() to persistently register every function via AddScriptToExecuteOnDocumentCreated (deferred via asyncExec when inside a callback); override deregisterFunction() to call RemoveScriptToExecuteOnDocumentCreated on disposal; remove the NavigationStarting fallback re-injection since all functions are now always persistently registered
• Test_org_eclipse_swt_browser_Browser.java: Two regression tests (isEdge-only):
  • test_BrowserFunction_availableBeforePageScripts_issue20: registers a function, navigates to a page whose inline <script> immediately calls it, asserts the function was invoked (tests the AddScriptToExecuteOnDocumentCreated guarantee directly)
  • test_BrowserFunction_availableOnLoad_concurrentInstances_issue20: creates two browsers concurrently without waiting for initialization, verifies both functions are callable when their pages complete loading (replicates the original bug report timing)

Test plan

• Run test_BrowserFunction_availableBeforePageScripts_issue20 on Windows with Edge
• Run test_BrowserFunction_availableOnLoad_concurrentInstances_issue20 on Windows with Edge
• Run existing test_BrowserFunction_* tests to verify no regressions
• Manually verify with the reproduction snippet from #2449 comment

[github-actions]

Test Results

  182 files  ± 0    182 suites  ±0   27m 55s ⏱️ -9s
4 705 tests + 4  4 682 ✅ +2   23 💤 + 2  0 ❌ ±0 
6 716 runs  +12  6 548 ✅ +2  168 💤 +10  0 ❌ ±0 

Results for commit fd098e6. ± Comparison against base commit 855fff4.

♻️ This comment has been updated with latest results.

[HeikoKlare]

Test plan is done. The automated tests are run via CI anyway. I also successfully executed the reproduction snippet: #2449 (comment)

@sratz can you please review this Edge enhancement?

[HeikoKlare]

@sratz It would be great if this could make it into the upcoming release. Next week is M2 promotion and we should probably not merge something like this after M2. Do you see the chance that you can have a look until then?

[sratz]

Aren't the functions now executed twice, as the handling in org.eclipse.swt.browser.Edge.handleNavigationStarting(long, long, boolean) still executes them all? Shouldn't that part be removed now?

Also,

createFunction internally calls registerFunction.

I think it would be nice if we did not need to override createFunction here, but only registerFunction for symmetry reasons with deregisterFunction below. But we require the function.functionString to be already set.

[sratz]

Aren't the functions now executed twice, as the handling in org.eclipse.swt.browser.Edge.handleNavigationStarting(long, long, boolean) still executes them all? Shouldn't that part be removed now?

Ah, it's not the function itself being executed twice, but only the registration of it is done twice, so handleNavigationStarting just replaces/re-sets redundantly what AddScriptToExecuteOnDocumentCreated already have added.

[HeikoKlare]

Ah, it's not the function itself being executed twice, but only the registration of it is done twice, so handleNavigationStarting just replaces/re-sets redundantly what AddScriptToExecuteOnDocumentCreated already have added.

Good catch. I have adapted the change so that functions are only registered ones by using the stored functionScriptIds to identify if a function was already registered.

Also,

createFunction internally calls registerFunction.

I think it would be nice if we did not need to override createFunction here, but only registerFunction for symmetry reasons with deregisterFunction below. But we require the function.functionString to be already set.

I agree that for the sake of symmetry, overwriting registerFunction would be better, but for the reason you have given that would require a larger refactoring (that would affect all platforms and not only Edge). We do not want to do that, do we?

[sratz]

should we attempt to register here again via AddScriptToExecuteOnDocumentCreated? So that for the next navigation iteration we go the new route?

[HeikoKlare]

It sounds conceptually clean to try to eventually register every function via AddScriptToExecuteOnDocumentCreated. Actually, the current solution is quite unclean, as the way how a function is registered now depends on whether the function has initially been reigstered from a callback or not.

I wondered if we could just do everything via AddScriptToExecuteOnDocumentCreated without this "fallback" here by just calling that registration asynchronously if we are in a callback. I've adapted the PR accordingly. The only potential issue I see with that is an execution order in which the function is registered in a callback, and then the next browser navigation is started is before the asyncExec is processed. I am not sure if that's a relevant scenario we need to consider.

An alternative would be to just temporarily use AddScriptToExecuteOnDocumentCreated for the current navigation and to deregister the function right after the next navigation again, so that the default way of registering functions remains the embedding in the document done in handleNavigationStarting.

One issue with the solution of using AddScriptToExecuteOnDocumentCreated is that it seems to be quite slow. The execution time of test_TimerRegression_Issue2806 (which creates 10000 browser functions) increases from less than 1 second to more than 2 minutes on my machine.