diff --git a/engine/src/flutter/shell/common/fixtures/shell_test.dart b/engine/src/flutter/shell/common/fixtures/shell_test.dart index f1610b5debf..981e184c65f 100644 --- a/engine/src/flutter/shell/common/fixtures/shell_test.dart +++ b/engine/src/flutter/shell/common/fixtures/shell_test.dart @@ -13,6 +13,29 @@ void nativeReportTimingsCallback(List timings) native 'NativeReportTimingsC void nativeOnBeginFrame(int microseconds) native 'NativeOnBeginFrame'; void nativeOnPointerDataPacket(List sequences) native 'NativeOnPointerDataPacket'; + +@pragma('vm:entry-point') +void drawFrames() { + // Wait for native to tell us to start going. + notifyNative(); + + PlatformDispatcher.instance.onBeginFrame = (Duration beginTime) { + final SceneBuilder builder = SceneBuilder(); + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + canvas.drawPaint(Paint()..color = const Color(0xFFABCDEF)); + final Picture picture = recorder.endRecording(); + builder.addPicture(Offset.zero, picture); + + final Scene scene = builder.build(); + window.render(scene); + + scene.dispose(); + picture.dispose(); + }; + PlatformDispatcher.instance.scheduleFrame(); +} + @pragma('vm:entry-point') void reportTimingsMain() { PlatformDispatcher.instance.onReportTimings = (List timings) { diff --git a/engine/src/flutter/shell/common/shell.cc b/engine/src/flutter/shell/common/shell.cc index 9143525be78..24061c24210 100644 --- a/engine/src/flutter/shell/common/shell.cc +++ b/engine/src/flutter/shell/common/shell.cc @@ -733,16 +733,11 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { const bool should_post_raster_task = !task_runners_.GetRasterTaskRunner()->RunsTasksOnCurrentThread(); - // Note: - // This is a synchronous operation because certain platforms depend on - // setup/suspension of all activities that may be interacting with the GPU in - // a synchronous fashion. fml::AutoResetWaitableEvent latch; auto raster_task = fml::MakeCopyable([&waiting_for_first_frame = waiting_for_first_frame_, rasterizer = rasterizer_->GetWeakPtr(), // - surface = std::move(surface), // - &latch]() mutable { + surface = std::move(surface)]() mutable { if (rasterizer) { // Enables the thread merger which may be used by the external view // embedder. @@ -751,29 +746,15 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { } waiting_for_first_frame.store(true); - - // Step 3: All done. Signal the latch that the platform thread is - // waiting on. - latch.Signal(); }); - auto ui_task = [engine = engine_->GetWeakPtr(), // - raster_task_runner = task_runners_.GetRasterTaskRunner(), // - raster_task, should_post_raster_task, - &latch // - ] { + // TODO(91717): This probably isn't necessary. The engine should be able to + // handle things here via normal lifecycle messages. + // https://github.com/flutter/flutter/issues/91717 + auto ui_task = [engine = engine_->GetWeakPtr()] { if (engine) { engine->OnOutputSurfaceCreated(); } - // Step 2: Next, tell the raster thread that it should create a surface for - // its rasterizer. - if (should_post_raster_task) { - fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task); - } else { - // See comment on should_post_raster_task, in this case we just unblock - // the platform thread. - latch.Signal(); - } }; // Threading: Capture platform view by raw pointer and not the weak pointer. @@ -786,7 +767,9 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { auto io_task = [io_manager = io_manager_->GetWeakPtr(), platform_view, ui_task_runner = task_runners_.GetUITaskRunner(), ui_task, - shared_resource_context = shared_resource_context_] { + shared_resource_context = shared_resource_context_, + raster_task_runner = task_runners_.GetRasterTaskRunner(), + raster_task, should_post_raster_task, &latch] { if (io_manager && !io_manager->GetResourceContext()) { sk_sp resource_context; if (shared_resource_context) { @@ -796,9 +779,16 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { } io_manager->NotifyResourceContextAvailable(resource_context); } - // Step 1: Next, post a task on the UI thread to tell the engine that it has + // Step 1: Post a task on the UI thread to tell the engine that it has // an output surface. fml::TaskRunner::RunNowOrPostTask(ui_task_runner, ui_task); + + // Step 2: Tell the raster thread that it should create a surface for + // its rasterizer. + if (should_post_raster_task) { + fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task); + } + latch.Signal(); }; fml::TaskRunner::RunNowOrPostTask(task_runners_.GetIOTaskRunner(), io_task); @@ -826,26 +816,15 @@ void Shell::OnPlatformViewDestroyed() { // configuration is changed by a task, and the assumption is no longer true. // // This incorrect assumption can lead to deadlock. - // See `should_post_raster_task` for more. rasterizer_->DisableThreadMergerIfNeeded(); - // The normal flow executed by this method is that the platform thread is - // starting the sequence and waiting on the latch. Later the UI thread posts - // raster_task to the raster thread triggers signaling the latch(on the IO - // thread). If the raster and the platform threads are the same this results - // in a deadlock as the raster_task will never be posted to platform/raster - // thread that is blocked on a latch. To avoid the described deadlock, if the - // raster and the platform threads are the same, should_post_raster_task will - // be false, and then instead of posting a task to the raster thread, the ui - // thread just signals the latch and the platform/raster thread follows with - // executing raster_task. - const bool should_post_raster_task = - !task_runners_.GetRasterTaskRunner()->RunsTasksOnCurrentThread(); - // Note: // This is a synchronous operation because certain platforms depend on // setup/suspension of all activities that may be interacting with the GPU in // a synchronous fashion. + // The UI thread does not need to be serialized here - there is sufficient + // guardrailing in the rasterizer to allow the UI thread to post work to it + // even after the surface has been torn down. fml::AutoResetWaitableEvent latch; @@ -855,7 +834,7 @@ void Shell::OnPlatformViewDestroyed() { io_manager->GetIsGpuDisabledSyncSwitch()->Execute( fml::SyncSwitch::Handlers().SetIfFalse( [&] { io_manager->GetSkiaUnrefQueue()->Drain(); })); - // Step 3: All done. Signal the latch that the platform thread is waiting + // Step 4: All done. Signal the latch that the platform thread is waiting // on. latch.Signal(); }; @@ -870,37 +849,28 @@ void Shell::OnPlatformViewDestroyed() { rasterizer->EnableThreadMergerIfNeeded(); rasterizer->Teardown(); } - // Step 2: Next, tell the IO thread to complete its remaining work. + // Step 3: Tell the IO thread to complete its remaining work. fml::TaskRunner::RunNowOrPostTask(io_task_runner, io_task); }; - auto ui_task = [engine = engine_->GetWeakPtr(), - raster_task_runner = task_runners_.GetRasterTaskRunner(), - raster_task, should_post_raster_task, &latch]() { + // TODO(91717): This probably isn't necessary. The engine should be able to + // handle things here via normal lifecycle messages. + // https://github.com/flutter/flutter/issues/91717 + auto ui_task = [engine = engine_->GetWeakPtr()]() { if (engine) { engine->OnOutputSurfaceDestroyed(); } - if (should_post_raster_task) { - fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task); - } else { - // See comment on should_post_raster_task, in this case we just unblock - // the platform thread. - latch.Signal(); - } }; - // Step 0: Post a task onto the UI thread to tell the engine that its output + // Step 1: Post a task onto the UI thread to tell the engine that its output // surface is about to go away. fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(), ui_task); + // Step 2: Post a task to the Raster thread (possibly this thread) to tell the + // rasterizer the output surface is going away. + fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), + raster_task); latch.Wait(); - if (!should_post_raster_task) { - // See comment on should_post_raster_task, in this case the raster_task - // wasn't executed, and we just run it here as the platform thread - // is the raster thread. - raster_task(); - latch.Wait(); - } } // |PlatformView::Delegate| diff --git a/engine/src/flutter/shell/common/shell_unittests.cc b/engine/src/flutter/shell/common/shell_unittests.cc index c8922a172df..0e38e3f6329 100644 --- a/engine/src/flutter/shell/common/shell_unittests.cc +++ b/engine/src/flutter/shell/common/shell_unittests.cc @@ -3098,5 +3098,67 @@ TEST_F(ShellTest, PrefetchDefaultFontManager) { DestroyShell(std::move(shell)); } +TEST_F(ShellTest, OnPlatformViewCreatedWhenUIThreadIsBusy) { + // This test will deadlock if the threading logic in + // Shell::OnCreatePlatformView is wrong. + auto settings = CreateSettingsForFixture(); + auto shell = CreateShell(std::move(settings)); + + fml::AutoResetWaitableEvent latch; + fml::TaskRunner::RunNowOrPostTask(shell->GetTaskRunners().GetUITaskRunner(), + [&latch]() { latch.Wait(); }); + + ShellTest::PlatformViewNotifyCreated(shell.get()); + latch.Signal(); + + DestroyShell(std::move(shell)); +} + +TEST_F(ShellTest, UIWorkAfterOnPlatformViewDestroyed) { + auto settings = CreateSettingsForFixture(); + auto shell = CreateShell(std::move(settings)); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("drawFrames"); + + fml::AutoResetWaitableEvent latch; + fml::AutoResetWaitableEvent notify_native_latch; + AddNativeCallback("NotifyNative", CREATE_NATIVE_ENTRY([&](auto args) { + notify_native_latch.Signal(); + latch.Wait(); + })); + + RunEngine(shell.get(), std::move(configuration)); + // Wait to make sure we get called back from Dart and thus have latched + // the UI thread before we create/destroy the platform view. + notify_native_latch.Wait(); + + ShellTest::PlatformViewNotifyCreated(shell.get()); + + fml::AutoResetWaitableEvent destroy_latch; + fml::TaskRunner::RunNowOrPostTask( + shell->GetTaskRunners().GetPlatformTaskRunner(), + [&shell, &destroy_latch]() { + shell->GetPlatformView()->NotifyDestroyed(); + destroy_latch.Signal(); + }); + + destroy_latch.Wait(); + + // Unlatch the UI thread and let it send us a scene to render. + latch.Signal(); + + // Flush the UI task runner to make sure we process the render/scheduleFrame + // request. + fml::AutoResetWaitableEvent ui_flush_latch; + fml::TaskRunner::RunNowOrPostTask(shell->GetTaskRunners().GetUITaskRunner(), + [&ui_flush_latch]() { + FML_LOG(ERROR) << "123"; + ui_flush_latch.Signal(); + }); + ui_flush_latch.Wait(); + + DestroyShell(std::move(shell)); +} + } // namespace testing } // namespace flutter