[webview_flutter_lwe] Add integration test based on upstream v4.13.1#1056
[webview_flutter_lwe] Add integration test based on upstream v4.13.1#1056seungsoo47 wants to merge 10 commits into
Conversation
The lightweight web engine has no dedicated URL-change callback, so report a URL change from the page-started handler: the native side now also invokes onUrlChange with the navigation URL, and LweNavigationDelegate.setOnUrlChange stores the callback and forwards the onUrlChange channel event instead of throwing UnimplementedError. Bump version to 0.4.2.
Port the 'can receive url changes' NavigationDelegate test from upstream webview_flutter v4.13.1, now runnable thanks to the onUrlChange implementation. Other upstream tests not present in this suite remain omitted because the lightweight web engine does not support the required features (custom request headers, HTTP error status callback, HTTP basic auth, web storage clearing, window.open, and media playback policy).
…e replacement When the navigation delegate is replaced after the WebView is created, the new delegate's platform channel handler was never registered because it is only set up when the view is first created. As a result the new delegate's callbacks (onUrlChange, onPageFinished, onProgress, etc.) were still delivered to the previous delegate and silently dropped. Bind the new delegate's channel handler when the delegate is set after the view has been created, and guard the handler setup so a delegate is bound at most once. This makes onUrlChange work when a delegate is swapped, which the 'can receive url changes' integration test exercises.
Disposing a WebView unregistered the texture and then immediately destroyed the web engine and its TBM surfaces on the same call. The Flutter raster thread could still be compositing the last frame that referenced a TBM surface, so the surfaces were freed out from under the GPU (tbm_bo_free with a non-zero lock count), crashing the raster thread. Dispose now stops the web engine first, unregisters the texture, and blocks the raster thread from obtaining or touching buffers via a disposing flag. Because the Tizen embedder does not invoke the UnregisterTexture completion callback, the TBM surfaces are released on a later main-loop turn (ownership is moved out of the WebView) so any in-flight composite finishes first. This eliminates the raster-thread crash seen when running the integration test suite, where each test disposes a WebView.
… callbacks The navigation-delegate callbacks (onPageStarted/onUrlChange, onPageFinished, onProgress, onWebResourceError, navigationRequest) and the JavaScript-channel callback are dispatched to the main loop asynchronously, and the navigationRequest result is resolved asynchronously by the Dart side. Any of them could run after the WebView was disposed and dereference the destroyed WebView (and its now-freed method channels), causing a use-after-free. Add a shared is_alive_ flag that Dispose() clears; every deferred callback captures a copy and bails out when the WebView is gone, and NavigationRequestResult checks it before touching the WebView.
There was a problem hiding this comment.
Code Review
This pull request implements onUrlChange for the navigation delegate, fixes navigation delegate callback dropping when replaced after WebView creation, and addresses crashes and use-after-free issues during WebView disposal. Feedback was provided regarding a potential LateInitializationError in LweNavigationDelegate.onCreate if the platform view is recreated, as the _navigationDelegateChannel is declared as late final and cannot be re-assigned.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| LweNavigationDelegate(super.params) : super.implementation(); | ||
|
|
||
| late final MethodChannel _navigationDelegateChannel; | ||
| bool _isChannelCreated = false; |
There was a problem hiding this comment.
If the platform view is recreated, onCreate will be called again with a new viewId on the same LweNavigationDelegate instance. Since _navigationDelegateChannel is declared as late final (on line 396), re-assigning it will throw a LateInitializationError.
To fix this, consider:
- Removing the
finalkeyword from_navigationDelegateChannel(making itlate MethodChannel _navigationDelegateChannel;). - Tracking the
_viewIdinstead of a boolean_isChannelCreatedto check if the channel actually needs to be re-bound.
For example:
late MethodChannel _navigationDelegateChannel;
int? _viewId;
// ...
void onCreate(int viewId) {
if (_viewId == viewId) {
return;
}
_viewId = viewId;
_navigationDelegateChannel = MethodChannel(
kLweNavigationDelegateChannelName + viewId.toString(),
);
// ...
}There was a problem hiding this comment.
@seungsoo47 Could you please review this suggestion once again? It looks that there is no need to add _isChannelCreated.
There was a problem hiding this comment.
I have applied it.
The navigation-delegate and JavaScript-channel callback lambdas added in the preceding commits were not clang-format clean, failing the repository's format CI check. Reflow them to match clang-format (Google style).
f302a99 to
a6ba98e
Compare
| homepage: https://github.com/flutter-tizen/plugins | ||
| repository: https://github.com/flutter-tizen/plugins/tree/master/packages/webview_flutter_lwe | ||
| version: 0.4.1 | ||
| version: 0.4.2 |
There was a problem hiding this comment.
I recommend updating to 0.5.0 because the implementation of the onUrlChange feature has been added.
| // Flip this before anything else: any dispatcher_ callback already queued | ||
| // on the main loop (e.g. a navigation event that fired just before this | ||
| // WebView was disposed) checks it and bails out instead of touching a | ||
| // WebView that may be fully destroyed by the time it runs. | ||
| *is_alive_ = false; | ||
|
|
||
| // Stop the web engine first so its renderer thread stops writing into the | ||
| // shared TBM surfaces before anything is torn down. | ||
| if (webview_instance_) { | ||
| webview_instance_->Destroy(); | ||
| webview_instance_ = nullptr; | ||
| } | ||
|
|
||
| // Flag the view as disposing and detach the buffers under the render lock so | ||
| // the raster thread's ObtainGpuSurface() immediately stops handing out (and | ||
| // stops touching) buffers that are about to be released. | ||
| std::shared_ptr<BufferPool> pool; | ||
| { | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
| is_disposing_ = true; | ||
| working_surface_ = nullptr; | ||
| candidate_surface_ = nullptr; | ||
| rendered_surface_ = nullptr; | ||
| pool = std::move(tbm_pool_); | ||
| } | ||
|
|
||
| // Unregister the texture with a completion callback. The embedder tears | ||
| // down the external texture on the render (raster) thread — after any | ||
| // in-flight frame callback for it has finished — and only then invokes this | ||
| // callback, so by the time it runs the raster thread is guaranteed done | ||
| // with the TBM surfaces. The callback fires on the render thread, so it | ||
| // hops to the main thread to actually free the pool. dispatcher_ itself | ||
| // (not a raw pointer to it) is captured so that if this callback fires | ||
| // after the WebView has been destroyed, the (stateless) dispatcher is kept | ||
| // alive by the callback's own shared_ptr copy instead of dangling. | ||
| texture_registrar_->UnregisterTexture( | ||
| GetTextureId(), [pool, dispatcher = dispatcher_]() { | ||
| // Invoked on the render thread once the texture is fully unregistered. | ||
| // Hand the pool to a main-thread task for destruction; by this point | ||
| // the raster thread has released all GPU resources and the LWE engine | ||
| // has unmapped all TBM surfaces, so no in-flight frames remain. | ||
| dispatcher->dispatchTaskOnMainThread([pool]() { | ||
| // Pool destructor calls tbm_surface_destroy on all buffers. | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Not all code needs to be commented on. Be careful with automatically generated comments.
If this code change is unusual, please write it briefly.
And if a detailed explanation is needed, writing it in the PR body allows us to track the history.
| LweNavigationDelegate(super.params) : super.implementation(); | ||
|
|
||
| late final MethodChannel _navigationDelegateChannel; | ||
| bool _isChannelCreated = false; |
There was a problem hiding this comment.
@seungsoo47 Could you please review this suggestion once again? It looks that there is no need to add _isChannelCreated.
| // The lightweight web engine has no dedicated URL-change | ||
| // callback, so report a URL change whenever a navigation | ||
| // starts. | ||
| navigation_delegate_channel_->InvokeMethod( | ||
| "onUrlChange", std::make_unique<flutter::EncodableValue>(args)); |
There was a problem hiding this comment.
Why should we call urlChange on PageStarted instead of PageLoaded(or onPageFinished)? Is there a reason?
Per review: replace the _isChannelCreated boolean guard with view id tracking. The boolean guard kept the delegate bound to the first view's channel forever, so if the platform view were recreated with a new view id the delegate would silently stop receiving navigation callbacks. Comparing against the stored view id keeps duplicate onCreate calls for the same view a no-op while rebinding when the view actually changes, matching how LweWebView.onCreate already rebinds its channel.
Per review: onUrlChange is a new feature, so bump the minor version instead of the patch version, matching how past feature additions (scrollbar APIs in 0.4.0, getUserAgent in 0.3.0) were versioned.
Per review: keep only the constraints the code cannot show (why the alive flag is checked, why the pool is destroyed on the main thread, why dispatcher_ is captured by value) and drop the narrative detail, which belongs in the PR description.
Port the 'can receive url changes' NavigationDelegate integration test from upstream webview_flutter v4.13.1, and fix three bugs found while making it pass on Tizen:
onUrlChangefor the navigation delegate.Bumped version 0.4.1 → 0.4.2.
All tests pass on TV/RPi4 emulator or device