From 52d73fe94929ac9a2041d87dc540badf154ef44e Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Tue, 16 Apr 2024 10:03:30 -0700 Subject: [PATCH 1/3] [Windows] Add/remove view failures should not hang --- .../windows/flutter_windows_engine.cc | 24 ++++++-- .../flutter_windows_engine_unittests.cc | 59 +++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/shell/platform/windows/flutter_windows_engine.cc b/shell/platform/windows/flutter_windows_engine.cc index bd1705440040d..559b81d1d42f1 100644 --- a/shell/platform/windows/flutter_windows_engine.cc +++ b/shell/platform/windows/flutter_windows_engine.cc @@ -532,7 +532,14 @@ std::unique_ptr FlutterWindowsEngine::CreateView( captures->latch.Signal(); }; - embedder_api_.AddView(engine_, &info); + FlutterEngineResult result = embedder_api_.AddView(engine_, &info); + if (result != kSuccess) { + // Starting the add view operation failed. This is unexpected and + // indicates a bug in the Windows embedder. + FML_LOG(ERROR) << "FlutterEngineAddView returned unexpected result: " + << result; + return nullptr; + } // Block the platform thread until the engine has added the view. // TODO(loicsharma): This blocks the platform thread eagerly and can @@ -573,15 +580,22 @@ void FlutterWindowsEngine::RemoveView(FlutterViewId view_id) { info.view_id = view_id; info.user_data = &captures; info.remove_view_callback = [](const FlutterRemoveViewResult* result) { - // This is invoked on the raster thread, the same thread that the present - // callback is invoked. If |FlutterRemoveViewResult.removed| is `true`, - // the engine guarantees the view won't be presented. + // This is invoked on an engine thread. If + // |FlutterRemoveViewResult.removed| is `true`, the engine guarantees the + // view won't be presented. Captures* captures = reinterpret_cast(result->user_data); captures->removed = result->removed; captures->latch.Signal(); }; - embedder_api_.RemoveView(engine_, &info); + FlutterEngineResult result = embedder_api_.RemoveView(engine_, &info); + if (result != kSuccess) { + // Starting the remove view operation failed. This is unexpected and + // indicates a bug in the Windows embedder. + FML_LOG(ERROR) << "FlutterEngineRemoveView returned unexpected result: " + << result; + return; + } // Block the platform thread until the engine has removed the view. // TODO(loicsharma): This blocks the platform thread eagerly and can diff --git a/shell/platform/windows/flutter_windows_engine_unittests.cc b/shell/platform/windows/flutter_windows_engine_unittests.cc index f9ebdbc278dfb..4d6ece38d4847 100644 --- a/shell/platform/windows/flutter_windows_engine_unittests.cc +++ b/shell/platform/windows/flutter_windows_engine_unittests.cc @@ -1265,5 +1265,64 @@ TEST_F(FlutterWindowsEngineTest, ReceivePlatformViewMessage) { } } +TEST_F(FlutterWindowsEngineTest, AddViewFailureDoesNotHang) { + fml::testing::LogCapture log_capture; + + FlutterWindowsEngineBuilder builder{GetContext()}; + auto engine = builder.Build(); + + EngineModifier modifier{engine.get()}; + + modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; }; + modifier.embedder_api().AddView = MOCK_ENGINE_PROC( + AddView, + [](FLUTTER_API_SYMBOL(FlutterEngine) engine, + const FlutterAddViewInfo* info) { return kInternalInconsistency; }); + + ASSERT_TRUE(engine->Run()); + + // Create the first view. This is the implicit view and isn't added to the + // engine. + auto implicit_window = std::make_unique>(); + + std::unique_ptr implicit_view = + engine->CreateView(std::move(implicit_window)); + + EXPECT_TRUE(implicit_view); + + // Create a second view. The embedder attempts to add it to the engine. + auto second_window = std::make_unique>(); + std::unique_ptr second_view = + engine->CreateView(std::move(second_window)); + + EXPECT_FALSE(second_view); + EXPECT_NE( + log_capture.str().find("FlutterEngineAddView returned unexpected result"), + std::string::npos); +} + +TEST_F(FlutterWindowsEngineTest, RemoveViewFailureDoesNotHang) { + fml::testing::LogCapture log_capture; + + FlutterWindowsEngineBuilder builder{GetContext()}; + builder.SetDartEntrypoint("sendCreatePlatformViewMethod"); + auto engine = builder.Build(); + + EngineModifier modifier{engine.get()}; + + modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; }; + modifier.embedder_api().RemoveView = MOCK_ENGINE_PROC( + RemoveView, + [](FLUTTER_API_SYMBOL(FlutterEngine) engine, + const FlutterRemoveViewInfo* info) { return kInternalInconsistency; }); + + ASSERT_TRUE(engine->Run()); + engine->RemoveView(123); + + EXPECT_NE(log_capture.str().find( + "FlutterEngineRemoveView returned unexpected result"), + std::string::npos); +} + } // namespace testing } // namespace flutter From c7fe24c9bead25f33fe785a382ed10ebcb12e702 Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Tue, 16 Apr 2024 12:46:42 -0700 Subject: [PATCH 2/3] Switch to assert --- .../platform/windows/flutter_windows_engine.cc | 17 +++++++++-------- .../windows/flutter_windows_engine_unittests.cc | 17 ++++------------- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/shell/platform/windows/flutter_windows_engine.cc b/shell/platform/windows/flutter_windows_engine.cc index 559b81d1d42f1..fa42e25f77c53 100644 --- a/shell/platform/windows/flutter_windows_engine.cc +++ b/shell/platform/windows/flutter_windows_engine.cc @@ -534,10 +534,10 @@ std::unique_ptr FlutterWindowsEngine::CreateView( FlutterEngineResult result = embedder_api_.AddView(engine_, &info); if (result != kSuccess) { - // Starting the add view operation failed. This is unexpected and - // indicates a bug in the Windows embedder. - FML_LOG(ERROR) << "FlutterEngineAddView returned unexpected result: " - << result; + FML_DCHECK(false) + << "Starting the add view operation failed. FlutterEngineAddView " + "returned an unexpected result: " + << result << ". This indicates a bug in the Windows embedder."; return nullptr; } @@ -590,10 +590,11 @@ void FlutterWindowsEngine::RemoveView(FlutterViewId view_id) { FlutterEngineResult result = embedder_api_.RemoveView(engine_, &info); if (result != kSuccess) { - // Starting the remove view operation failed. This is unexpected and - // indicates a bug in the Windows embedder. - FML_LOG(ERROR) << "FlutterEngineRemoveView returned unexpected result: " - << result; + FML_DCHECK(false) << "Starting the remove view operation failed. " + "FlutterEngineRemoveView " + "returned an unexpected result: " + << result + << ". This indicates a bug in the Windows embedder."; return; } diff --git a/shell/platform/windows/flutter_windows_engine_unittests.cc b/shell/platform/windows/flutter_windows_engine_unittests.cc index 4d6ece38d4847..0ae1aa7c58044 100644 --- a/shell/platform/windows/flutter_windows_engine_unittests.cc +++ b/shell/platform/windows/flutter_windows_engine_unittests.cc @@ -1292,18 +1292,12 @@ TEST_F(FlutterWindowsEngineTest, AddViewFailureDoesNotHang) { // Create a second view. The embedder attempts to add it to the engine. auto second_window = std::make_unique>(); - std::unique_ptr second_view = - engine->CreateView(std::move(second_window)); - EXPECT_FALSE(second_view); - EXPECT_NE( - log_capture.str().find("FlutterEngineAddView returned unexpected result"), - std::string::npos); + EXPECT_DEBUG_DEATH(engine->CreateView(std::move(second_window)), + "FlutterEngineAddView returned an unexpected result"); } TEST_F(FlutterWindowsEngineTest, RemoveViewFailureDoesNotHang) { - fml::testing::LogCapture log_capture; - FlutterWindowsEngineBuilder builder{GetContext()}; builder.SetDartEntrypoint("sendCreatePlatformViewMethod"); auto engine = builder.Build(); @@ -1317,11 +1311,8 @@ TEST_F(FlutterWindowsEngineTest, RemoveViewFailureDoesNotHang) { const FlutterRemoveViewInfo* info) { return kInternalInconsistency; }); ASSERT_TRUE(engine->Run()); - engine->RemoveView(123); - - EXPECT_NE(log_capture.str().find( - "FlutterEngineRemoveView returned unexpected result"), - std::string::npos); + EXPECT_DEBUG_DEATH(engine->RemoveView(123), + "FlutterEngineRemoveView returned an unexpected result"); } } // namespace testing From e115d8fcfa82504085b79acffa70e73f882f6a95 Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Tue, 16 Apr 2024 13:58:47 -0700 Subject: [PATCH 3/3] Always error, only crash in debug --- shell/platform/windows/flutter_windows_engine.cc | 14 ++++++++------ .../windows/flutter_windows_engine_unittests.cc | 2 -- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/shell/platform/windows/flutter_windows_engine.cc b/shell/platform/windows/flutter_windows_engine.cc index fa42e25f77c53..69621369096c5 100644 --- a/shell/platform/windows/flutter_windows_engine.cc +++ b/shell/platform/windows/flutter_windows_engine.cc @@ -534,10 +534,11 @@ std::unique_ptr FlutterWindowsEngine::CreateView( FlutterEngineResult result = embedder_api_.AddView(engine_, &info); if (result != kSuccess) { - FML_DCHECK(false) + FML_LOG(ERROR) << "Starting the add view operation failed. FlutterEngineAddView " "returned an unexpected result: " << result << ". This indicates a bug in the Windows embedder."; + FML_DCHECK(false); return nullptr; } @@ -590,11 +591,12 @@ void FlutterWindowsEngine::RemoveView(FlutterViewId view_id) { FlutterEngineResult result = embedder_api_.RemoveView(engine_, &info); if (result != kSuccess) { - FML_DCHECK(false) << "Starting the remove view operation failed. " - "FlutterEngineRemoveView " - "returned an unexpected result: " - << result - << ". This indicates a bug in the Windows embedder."; + FML_LOG(ERROR) << "Starting the remove view operation failed. " + "FlutterEngineRemoveView " + "returned an unexpected result: " + << result + << ". This indicates a bug in the Windows embedder."; + FML_DCHECK(false); return; } diff --git a/shell/platform/windows/flutter_windows_engine_unittests.cc b/shell/platform/windows/flutter_windows_engine_unittests.cc index 0ae1aa7c58044..a7b45f7490ef3 100644 --- a/shell/platform/windows/flutter_windows_engine_unittests.cc +++ b/shell/platform/windows/flutter_windows_engine_unittests.cc @@ -1266,8 +1266,6 @@ TEST_F(FlutterWindowsEngineTest, ReceivePlatformViewMessage) { } TEST_F(FlutterWindowsEngineTest, AddViewFailureDoesNotHang) { - fml::testing::LogCapture log_capture; - FlutterWindowsEngineBuilder builder{GetContext()}; auto engine = builder.Build();