Repository navigation
Conversation
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves resource lifecycle management and adds null safety checks in the Flutter Linux platform shell. It introduces several null checks for the engine, window, and cancellable objects in fl_view.cc and fl_view_renderer_opengl.cc. To prevent race conditions and crashes during teardown, resource cleanup for the renderer, task runner, and frame mutex has been deferred from the dispose stage to the finalize stage. Additionally, the mock GTK testing framework has been updated to track the current GL context. There are no review comments, and I have no feedback to provide.
6040e2b to
8c7f34b
Compare
…ter threads When destroying an active secondary window on Linux: 1. FlView did not retain a strong reference to self->renderer, allowing gtk_container_dispose() on self->event_box to unreference and free self->renderer during dispose() while the Flutter raster thread may still call fl_view_present_layers(self->renderer). Retain a strong reference to self->renderer until fl_view_finalize(). 2. FlViewRendererOpenGL and FlViewRendererSubsurface destroyed frame_mutex and task_runner in dispose() while leaving compositor and frame for finalize(). Move task_runner and frame_mutex cleanup to finalize() so present_layers() can safely access them after dispose(). 3. Synchronize compositor, frame, and render_context teardown under frame_mutex in cleanup_opengl_resources() during unrealize() and dispose() on the GTK main thread. Only clear the GTK thread's current GdkGLContext if this window's render_context is the one currently bound.
8c7f34b to
abacb74
Compare
|
|
||
| // Called when the mouse cursor changes. | ||
| static void cursor_changed_cb(FlView* self) { | ||
| if (self->engine == nullptr) { |
There was a problem hiding this comment.
This check is unnecessary, because the callback should have been cancelled before it was set to NULL. It looks like this is the model not understanding the GObject callback system.
| static void handle_geometry_changed(FlView* self) { | ||
| // No updates required when size controlled by Flutter. | ||
| if (self->sized_to_content) { | ||
| if (self->sized_to_content || self->engine == nullptr) { |
There was a problem hiding this comment.
Same as above, engine should always be non-null
| // Retain a strong reference owned by FlView (released in fl_view_finalize). | ||
| // Without this, gtk_container_dispose() on self->event_box unrefs and frees | ||
| // self->renderer during dispose() while the raster thread may still call | ||
| // fl_view_present_layers(self->renderer). |
There was a problem hiding this comment.
This comments seems overly wordy - very much a model leaving its logic trail behind.
| FlView* self = FL_VIEW(object); | ||
|
|
||
| g_cancellable_cancel(self->cancellable); | ||
| if (self->cancellable != nullptr) { |
There was a problem hiding this comment.
Not needed, because g_cancellable_cancel handles nullptr being passed to it (for this exact sort of reason).
robert-ancell
left a comment
robert-ancell
left a comment
There was a problem hiding this comment.
I've run out of time reviewing this, I will have another look next week. The changes are OK, but the actual fix is lost in the noise. There are a number of unnecessary changes and some heavy commenting that either suggests the code is not clearly written or the AI model is leaving a breadcrumb trail for the next time it looks. I'd like this to slim down to just the change that is required (probably could be done with a few more challenges to the AI).
|
I reproduced the The test below deterministically observes renderer finalization while the caller still owns the view. It fails on master and passes when renderer release is moved from TEST_F(FlViewTest, RendererSurvivesWhileViewReferenced) {
FlView* view = fl_view_new(project);
g_object_ref_sink(view);
g_autoptr(FlEngine) view_engine =
FL_ENGINE(g_object_ref(fl_view_get_engine(view)));
StartEngine(view_engine);
g_autoptr(GList) children = gtk_container_get_children(GTK_CONTAINER(view));
ASSERT_NE(children, nullptr);
ASSERT_TRUE(GTK_IS_BIN(children->data));
GtkWidget* renderer = gtk_bin_get_child(GTK_BIN(children->data));
ASSERT_NE(renderer, nullptr);
bool renderer_finalized = false;
g_object_set_data_full(G_OBJECT(renderer), "test-renderer-finalized",
&renderer_finalized, [](gpointer data) {
*static_cast<bool*>(data) = true;
});
fl_gtk_widget_destroy(GTK_WIDGET(view));
EXPECT_FALSE(renderer_finalized);
g_object_unref(view);
EXPECT_TRUE(renderer_finalized);
if (!renderer_finalized) {
g_object_steal_data(G_OBJECT(renderer), "test-renderer-finalized");
}
}The qdata destroy notifier observes finalization; a weak-pointer notification would be insufficient because forced disposal can clear weak references before finalization. Separately, realizing and forcibly disposing a software renderer while retaining it exposed a null task-runner crash. That backend is not changed by this PR; I reported it in #196545 and proposed the complementary two-file fix in #196546. AI-assisted investigation; the failing/passing controls were built and executed locally. |
Synchronizes renderer lifetime and resource teardown across the GTK main thread and the Flutter raster thread during secondary window destruction.
FlViewdid not retain a strong reference toself->renderer, allowinggtk_container_dispose()onself->event_boxto unreference and freeself->rendererduringdispose()while the Flutter raster thread may still callfl_view_present_layers(self->renderer). Retain a strong reference toself->rendereruntilfl_view_finalize().FlViewRendererOpenGLandFlViewRendererSubsurfacedestroyedframe_mutexandtask_runnerindispose()while leavingcompositorandframeforfinalize(). Movetask_runnerandframe_mutexcleanup tofinalize()so in-flightpresent_layers()calls on the raster thread can safely lockframe_mutexafterdispose().frame_mutexincleanup_opengl_resources()duringunrealize()anddispose()on the GTK main thread. Only clear the GTK thread activeGdkGLContextif this windowrender_contextis the one currently bound.Part of stack #193534.
Pre-launch Checklist
///).