[flutter_inappwebview] Introduce flutter_inappwebview_tizen#1015
[flutter_inappwebview] Introduce flutter_inappwebview_tizen#1015JSUYA wants to merge 3 commits intoflutter-tizen:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the flutter_inappwebview_tizen package, providing a Tizen implementation for the flutter_inappwebview plugin using the EWK API. The implementation supports core features such as URL loading, JavaScript evaluation, and offscreen rendering via a TBM surface buffer pool. Feedback on the native C++ implementation identifies a potential memory leak regarding the management of string allocations for HTTP headers.
9214876 to
29d9cee
Compare
29d9cee to
79c335c
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces the flutter_inappwebview_tizen plugin, providing a Tizen implementation for the flutter_inappwebview package. The changes include the necessary plugin infrastructure, Tizen-specific platform implementation using the EWK API, and an example application. Review feedback identified a memory leak in the touch event handling logic, a potential issue with automatic JSON decoding of JavaScript evaluation results, and a suggestion to use specific error codes in error reporting.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the Tizen implementation for the flutter_inappwebview plugin, featuring offscreen rendering using the EWK API. The native C++ implementation manages WebView instances and buffer pools for texture integration. Review feedback highlights stability concerns, including race conditions in the resizing and buffer management methods, potential crashes, and resource leaks of Ecore_Evas objects. Suggestions also include ensuring one-time initialization of EWK arguments and transitioning to a multi-buffer pool to improve rendering performance.
| void WebView::Resize(double width, double height) { | ||
| width_ = width; | ||
| height_ = height; | ||
|
|
||
| if (candidate_surface_) { | ||
| candidate_surface_ = nullptr; | ||
| } | ||
|
|
||
| tbm_pool_->Prepare(width_, height_); | ||
| evas_object_resize(webview_instance_, width_, height_); | ||
| } |
There was a problem hiding this comment.
The Resize method has several issues:
- It accesses and modifies
candidate_surface_without holdingmutex_, which is a race condition withObtainGpuSurfaceandOnFrameRendered. - If
candidate_surface_is not null, it should be released back to the pool before being cleared, otherwise that buffer unit will be leaked (marked as in-use indefinitely). tbm_pool_->Preparedestroys existing surfaces. Ifrendered_surface_is currently being accessed by the Flutter GPU thread, this will result in a Use-After-Free (UAF) crash.
void WebView::Resize(double width, double height) {
std::lock_guard<std::mutex> lock(mutex_);
width_ = width;
height_ = height;
if (candidate_surface_) {
tbm_pool_->Release(candidate_surface_);
candidate_surface_ = nullptr;
}
tbm_pool_->Prepare(width_, height_);
evas_object_resize(webview_instance_, width_, height_);
}| // temporarily comment out ewk_init() and ewk_shutdown(). It can be reverted | ||
| // depending on updates to chromium-efl. | ||
| // ewk_init(); | ||
| Ecore_Evas* evas = ecore_evas_new("wayland_egl", 0, 0, 1, 1, 0); |
| if (rendered_surface_ && rendered_surface_->IsUsed()) { | ||
| tbm_pool_->Release(rendered_surface_); | ||
| } |
There was a problem hiding this comment.
This logic is incorrect. rendered_surface_ is the buffer currently held by Flutter. If IsUsed() is true, it means Flutter is still reading from it. Calling Release here marks the buffer as available in the pool, allowing OnFrameRendered to acquire and overwrite it while Flutter is still using it. These lines should be removed; the buffer is automatically returned to the pool when Flutter is done via the release_callback.
| webview->working_surface_ = webview->tbm_pool_->GetAvailableBuffer(); | ||
| webview->working_surface_->UseExternalBuffer(); |
There was a problem hiding this comment.
GetAvailableBuffer() can return nullptr if all buffers in the pool are currently in use. Dereferencing it without a null check will cause a crash.
| webview->working_surface_ = webview->tbm_pool_->GetAvailableBuffer(); | |
| webview->working_surface_->UseExternalBuffer(); | |
| if (!webview->working_surface_) { | |
| webview->working_surface_ = webview->tbm_pool_->GetAvailableBuffer(); | |
| if (!webview->working_surface_) { | |
| return; | |
| } | |
| webview->working_surface_->UseExternalBuffer(); | |
| } |
| BufferUnit* SingleBufferPool::GetAvailableBuffer() { | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
| BufferUnit* buffer = pool_[0].get(); | ||
| buffer->MarkInUse(); | ||
| return buffer; | ||
| } |
There was a problem hiding this comment.
SingleBufferPool::GetAvailableBuffer ignores the return value of MarkInUse(). If the buffer is already in use (e.g., held by Flutter), MarkInUse() returns false, but the function still returns the buffer. This allows the same buffer to be acquired multiple times simultaneously, leading to race conditions. It should return nullptr if the buffer is in use.
| BufferUnit* SingleBufferPool::GetAvailableBuffer() { | |
| std::lock_guard<std::mutex> lock(mutex_); | |
| BufferUnit* buffer = pool_[0].get(); | |
| buffer->MarkInUse(); | |
| return buffer; | |
| } | |
| BufferUnit* SingleBufferPool::GetAvailableBuffer() { | |
| std::lock_guard<std::mutex> lock(mutex_); | |
| BufferUnit* buffer = pool_[0].get(); | |
| if (buffer->MarkInUse()) { | |
| return buffer; | |
| } | |
| return nullptr; | |
| } |
Introduce
flutter_inappwebview_tizen, the Tizen implementation offlutter_inappwebview.Like
webview_flutter_tizen, it is built on top of EWK (chromium-efl) andrenders offscreen via a TBM surface buffer pool. Only the surface that maps
cleanly onto the Tizen WebView is implemented; APIs that have no EWK
counterpart raise
UnsupportedError/UnimplementedError.+#1008