[Win32] Fix BrowserFunction missing on first page - #3479
Conversation
762e863 to
336e925
Compare
5f5f332 to
1072ca2
Compare
|
@sratz I may have finally found the last piece for reliable browser function registration with Edge/WebView2. This PR revises the according implementation to address all registration scenarios (during browser initialization, during page navigation or when page is already loaded) in a more simple way than just extending for the not-yet-addressed scenario. Could you please have a look at this fix proposal? I have iterated the fix with an AI agent for quite some time already but would be good to have someone else with experience in this piece of code have a thorough look as well. Thank you in advance! |
r-mennig
left a comment
There was a problem hiding this comment.
The changes look good to me. I've only found a minor issue regarding a missing return value check on AddScriptToExecuteOnDocumentCreated
| } | ||
| return COM.S_OK; | ||
| }); | ||
| webView.AddScriptToExecuteOnDocumentCreated(stringToWstr(functionString), completion.getAddress()); |
There was a problem hiding this comment.
Not checking the return value of AddScriptToExecuteOnDocumentCreated can lead to browser functions silently failing to register. This is made worse by the fact that those functions can be contributed from other code parts (via createFunction), so I think any registration failure should be handled here by throwing an exception.
| webView.AddScriptToExecuteOnDocumentCreated(stringToWstr(functionString), completion.getAddress()); | |
| int hr = webView.AddScriptToExecuteOnDocumentCreated(stringToWstr(functionString), completion.getAddress()); | |
| if (hr != COM.S_OK) error(SWT.ERROR_NO_HANDLES, hr); |
(Maybe something other that SWT.ERROR_NO_HANDLES would be better here)
There was a problem hiding this comment.
That's a very correct concern. Unfortunately, there are problems with either of the callers of this method when throwing an error here:
- The one caller is the immediate function registration when creating a BrowserFunction (via
new BrowserFunction()). That constructor does not have any declared errors for the case that function registration fails. Introducing a new type of exception here can lead to consumers fail unexpectedly as they do not deal with that type of exception. Thus, this would be kind of API-breaking (though not in a very strict sense). - The other caller is the browser initialization finalization that registers a function that was created before initialization was finished. This initialization process must not fail because of a failing browser function registration as well, such that the error would have to be caught there again and the further error processing is unclear, as the initialization finalization may be triggered by any kind of consumer performing operations on the browser.
While just swallowing a failure is obviously not a good idea (even though we have done that so far as well), throwing an error does not seem feasible here.
I found these types of errors to potentially occur here:
E_INVALIDARGif the script is null or the completion callback is invalid (both cannot happen)E_POINTERif a pointer argument is null (I do not see how it could happen with this call)- The HRESULT for
ERROR_INVALID_STATEif the web view was already closed (in which case failing function registration is not an issue at all).
So I am not sure if any relevant kind of error may occur here and since we have not seen reports about issues with function registration so far, it may be really seldom (if at all) that an error occurs here. For that, we may simply log the error for the sake of traceability and debuggability without throwing any error to avoid completely unexpected or unrecoverable application states. That's what I have implemented with my latest commit.
1072ca2 to
f817730
Compare
A BrowserFunction created while an Edge/WebView2 browser is still initializing (with a navigation queued via setUrl/setText) could be missing on the first loaded page: AddScriptToExecuteOnDocumentCreated was issued only after the queued navigation and raced with document creation, so the function was sometimes not yet registered when the page's document was created. With the change, we register the document-created scripts of all pending BrowserFunctions inside WebViewProvider.initializeWebView(), before completing the initialization future. Completing the future synchronously runs the queued navigation, so registering beforehand guarantees the functions are in place before the first document is created. This is the only point that is reliably "after init, before the first navigation", since a CompletableFuture navigation chain cannot be preempted retroactively. Registration is now issued asynchronously (fire-and-forget). What makes a function land on a page is issuing the registration before the navigation is issued to WebView2, not waiting for its completion. As a result the previous inCallback>0 deferral is no longer needed (an async call cannot deadlock inside a WebView2 callback) and the separate synchronous registration method is removed, leaving a single registration method and a single branch in createFunction. deregisterFunction stays correct for a registration whose script ID has not arrived yet: the async completion callback detects the removed function via the functions map and removes the just-registered script itself. Fixes eclipse-platform#3370 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
f817730 to
3e6cd35
Compare
Fixes #3370 — the sporadic failure of
test_BrowserFunction_availableOnLoad_concurrentInstances_issue20on the Edge/WebView2 backend.Builds on top of #3478 (BrowserFunction dispose/redefine regression tests) and employs the regression tests added there — #3478 should be merged first.
Problem
A
BrowserFunctioncreated while an Edge browser is still initializing (with a navigation queued viasetUrl/setText) could be missing on the first loaded page.AddScriptToExecuteOnDocumentCreatedwas issued only after the queued navigation and raced with document creation, so the function was sometimes not registered when the page's document was created — surfacing asSWTException: <function> is not defined.Fix
Register the document-created scripts of all pending
BrowserFunctions insideWebViewProvider.initializeWebView(), before completing the initialization future (which synchronously runs the queued navigation). This is the only point that is reliably "after init, before the first navigation", since theCompletableFuturenavigation chain cannot be preempted retroactively.Registration is now issued asynchronously: what makes a function land on a page is issuing the registration before the navigation is issued to WebView2, not waiting for its completion. This also removes the previous
inCallback > 0deferral (an async call cannot deadlock inside a WebView2 callback) and the separate synchronous registration method, leaving a single registration method and a single branch increateFunction.deregisterFunctionstays correct for a registration whose script ID has not arrived yet (its async completion detects the removed function and removes the script itself).Tests
Adds regression tests for function creation during concurrent initialization: creating a function from inside a callback, availability before the first page's inline scripts run, and multiple functions registered before initialization completes. These complement the dispose/redefine regression tests from #3478.
Reproducer
The underlying race is a sub-millisecond window and cannot be forced deterministically through the public API with a single browser. The following standalone reproducer makes it deterministic in practice by stressing concurrent initialization: before the fix it reports
options is not definedwithin a few iterations; after the fix it completes all iterations cleanly.Standalone reproducer (Windows/Edge)