From 506449813835e871cfc00046111a5ae523b1ed37 Mon Sep 17 00:00:00 2001 From: John McDole Date: Thu, 19 Sep 2024 14:28:09 -0700 Subject: [PATCH 1/5] Disallow time traveling frame times Address bad developer experience in #106277 Leave as an error log and hope for more repro reports --- lib/ui/window/platform_configuration.cc | 13 ++++++++++++- shell/platform/android/vsync_waiter_android.cc | 4 +++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc index e6a934a63c69b..a031461285912 100644 --- a/lib/ui/window/platform_configuration.cc +++ b/lib/ui/window/platform_configuration.cc @@ -377,7 +377,18 @@ void PlatformConfiguration::BeginFrame(fml::TimePoint frameTime, } tonic::DartState::Scope scope(dart_state); - int64_t microseconds = (frameTime - fml::TimePoint()).ToMicroseconds(); + int64_t microseconds = frameTime.ToEpochDelta().ToMicroseconds(); + + static int64_t last_microseconds = 0; + if (last_microseconds > microseconds) { + // Do not allow time traveling frametimes + // github.com/flutter/flutter/issues/106277 + FML_LOG(ERROR) + << "Reported frame time is older than the last one; clamping. " + << microseconds << " < " << last_microseconds + << " ~= " << last_microseconds - microseconds; + microseconds = last_microseconds; + } tonic::CheckAndHandleError( tonic::DartInvoke(begin_frame_.Get(), { diff --git a/shell/platform/android/vsync_waiter_android.cc b/shell/platform/android/vsync_waiter_android.cc index b69e3389d89ec..2dde2a37ce666 100644 --- a/shell/platform/android/vsync_waiter_android.cc +++ b/shell/platform/android/vsync_waiter_android.cc @@ -27,7 +27,9 @@ VsyncWaiterAndroid::~VsyncWaiterAndroid() = default; // |VsyncWaiter| void VsyncWaiterAndroid::AwaitVSync() { - if (impeller::android::Choreographer::IsAvailableOnPlatform()) { + static bool use_choreographer = + impeller::android::Choreographer::IsAvailableOnPlatform(); + if (use_choreographer) { auto* weak_this = new std::weak_ptr(shared_from_this()); fml::TaskRunner::RunNowOrPostTask( task_runners_.GetUITaskRunner(), [weak_this]() { From 09eb440e5af0be197d02db569485084779e81008 Mon Sep 17 00:00:00 2001 From: John McDole Date: Fri, 20 Sep 2024 09:12:00 -0700 Subject: [PATCH 2/5] consty McConst const --- lib/ui/window/platform_configuration.cc | 2 +- shell/platform/android/vsync_waiter_android.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc index a031461285912..863776973e15b 100644 --- a/lib/ui/window/platform_configuration.cc +++ b/lib/ui/window/platform_configuration.cc @@ -377,7 +377,7 @@ void PlatformConfiguration::BeginFrame(fml::TimePoint frameTime, } tonic::DartState::Scope scope(dart_state); - int64_t microseconds = frameTime.ToEpochDelta().ToMicroseconds(); + const int64_t microseconds = frameTime.ToEpochDelta().ToMicroseconds(); static int64_t last_microseconds = 0; if (last_microseconds > microseconds) { diff --git a/shell/platform/android/vsync_waiter_android.cc b/shell/platform/android/vsync_waiter_android.cc index 2dde2a37ce666..1c46b7dbf3175 100644 --- a/shell/platform/android/vsync_waiter_android.cc +++ b/shell/platform/android/vsync_waiter_android.cc @@ -27,7 +27,7 @@ VsyncWaiterAndroid::~VsyncWaiterAndroid() = default; // |VsyncWaiter| void VsyncWaiterAndroid::AwaitVSync() { - static bool use_choreographer = + const static bool use_choreographer = impeller::android::Choreographer::IsAvailableOnPlatform(); if (use_choreographer) { auto* weak_this = new std::weak_ptr(shared_from_this()); From 7840de786cec04112c5b821b10eb2e890cd275d3 Mon Sep 17 00:00:00 2001 From: John McDole Date: Fri, 20 Sep 2024 09:14:22 -0700 Subject: [PATCH 3/5] non-consts mcconst --- lib/ui/window/platform_configuration.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc index 863776973e15b..a031461285912 100644 --- a/lib/ui/window/platform_configuration.cc +++ b/lib/ui/window/platform_configuration.cc @@ -377,7 +377,7 @@ void PlatformConfiguration::BeginFrame(fml::TimePoint frameTime, } tonic::DartState::Scope scope(dart_state); - const int64_t microseconds = frameTime.ToEpochDelta().ToMicroseconds(); + int64_t microseconds = frameTime.ToEpochDelta().ToMicroseconds(); static int64_t last_microseconds = 0; if (last_microseconds > microseconds) { From 193b7b4b0cdda6589585ee82592ca48aad98cba5 Mon Sep 17 00:00:00 2001 From: John McDole Date: Tue, 24 Sep 2024 10:50:20 -0700 Subject: [PATCH 4/5] Add tests and move to class variable --- fml/macros.h | 8 +- lib/ui/fixtures/ui_test.dart | 10 ++ lib/ui/window/platform_configuration.cc | 19 ++-- lib/ui/window/platform_configuration.h | 11 +++ .../platform_configuration_unittests.cc | 92 ++++++++++++++++++- 5 files changed, 131 insertions(+), 9 deletions(-) diff --git a/fml/macros.h b/fml/macros.h index 7de4ddf1f1f42..291e193ee0969 100644 --- a/fml/macros.h +++ b/fml/macros.h @@ -38,7 +38,13 @@ TypeName() = delete; \ FML_DISALLOW_COPY_ASSIGN_AND_MOVE(TypeName) +#define FML_TEST_NAME(test_case_name, test_name) \ + test_case_name##_##test_name##_Test + +#define FML_TEST_CLASS(test_case_name, test_name) \ + class FML_TEST_NAME(test_case_name, test_name) + #define FML_FRIEND_TEST(test_case_name, test_name) \ - friend class test_case_name##_##test_name##_Test + friend FML_TEST_CLASS(test_case_name, test_name) #endif // FLUTTER_FML_MACROS_H_ diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index a02e0ce29499b..19688d108a647 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -492,6 +492,16 @@ void convertPaintToDlPaint() { @pragma('vm:external-name', 'ConvertPaintToDlPaint') external void _convertPaintToDlPaint(Paint paint); +/// Hooks for platform_configuration_unittests.cc +@pragma('vm:entry-point') +void _beginFrameHijack(int microseconds, int frameNumber) { + nativeBeginFrame(microseconds, frameNumber); +} + +@pragma('vm:entry-point') +@pragma('vm:external-name', 'BeginFrame') +external nativeBeginFrame(int microseconds, int frameNumber); + @pragma('vm:entry-point') void hooksTests() async { Future test(String name, FutureOr Function() testFunction) async { diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc index a031461285912..a63d098726e5b 100644 --- a/lib/ui/window/platform_configuration.cc +++ b/lib/ui/window/platform_configuration.cc @@ -377,18 +377,25 @@ void PlatformConfiguration::BeginFrame(fml::TimePoint frameTime, } tonic::DartState::Scope scope(dart_state); - int64_t microseconds = frameTime.ToEpochDelta().ToMicroseconds(); + if (last_frame_number_ > frame_number) { + FML_LOG(ERROR) << "Frame number is out of order: " << frame_number << " < " + << last_frame_number_; + } + last_frame_number_ = frame_number; - static int64_t last_microseconds = 0; - if (last_microseconds > microseconds) { + // frameTime is not a delta; its the timestamp of the presentation. + // This is just a type conversion. + int64_t microseconds = frameTime.ToEpochDelta().ToMicroseconds(); + if (last_microseconds_ > microseconds) { // Do not allow time traveling frametimes // github.com/flutter/flutter/issues/106277 FML_LOG(ERROR) << "Reported frame time is older than the last one; clamping. " - << microseconds << " < " << last_microseconds - << " ~= " << last_microseconds - microseconds; - microseconds = last_microseconds; + << microseconds << " < " << last_microseconds_ + << " ~= " << last_microseconds_ - microseconds; + microseconds = last_microseconds_; } + last_microseconds_ = microseconds; tonic::CheckAndHandleError( tonic::DartInvoke(begin_frame_.Get(), { diff --git a/lib/ui/window/platform_configuration.h b/lib/ui/window/platform_configuration.h index b668b85cfd603..ddf3e3d394645 100644 --- a/lib/ui/window/platform_configuration.h +++ b/lib/ui/window/platform_configuration.h @@ -18,6 +18,7 @@ #include "flutter/lib/ui/window/pointer_data_packet.h" #include "flutter/lib/ui/window/viewport_metrics.h" #include "flutter/shell/common/display.h" +#include "fml/macros.h" #include "third_party/tonic/dart_persistent_value.h" #include "third_party/tonic/typed_data/dart_byte_data.h" @@ -28,6 +29,11 @@ class PlatformMessageHandler; class PlatformIsolateManager; class Scene; +// Forward declaration of friendly tests. +namespace testing { +FML_TEST_CLASS(PlatformConfigurationTest, BeginFrameMonotonic); +} + //-------------------------------------------------------------------------- /// @brief An enum for defining the different kinds of accessibility features /// that can be enabled by the platform. @@ -517,6 +523,8 @@ class PlatformConfiguration final { Dart_Handle on_error() { return on_error_.Get(); } private: + FML_FRIEND_TEST(testing::PlatformConfigurationTest, BeginFrameMonotonic); + PlatformConfigurationClient* client_; tonic::DartPersistentValue on_error_; tonic::DartPersistentValue add_view_; @@ -535,6 +543,9 @@ class PlatformConfiguration final { tonic::DartPersistentValue draw_frame_; tonic::DartPersistentValue report_timings_; + uint64_t last_frame_number_; + int64_t last_microseconds_; + // All current views' view metrics mapped from view IDs. std::unordered_map metrics_; diff --git a/lib/ui/window/platform_configuration_unittests.cc b/lib/ui/window/platform_configuration_unittests.cc index 7410caeb66d6c..e181a27963dc9 100644 --- a/lib/ui/window/platform_configuration_unittests.cc +++ b/lib/ui/window/platform_configuration_unittests.cc @@ -4,17 +4,21 @@ #define FML_USED_ON_EMBEDDER -#include "flutter/lib/ui/window/platform_configuration.h" - +#include #include #include "flutter/common/task_runners.h" #include "flutter/fml/synchronization/waitable_event.h" #include "flutter/lib/ui/painting/vertices.h" +#include "flutter/lib/ui/window/platform_configuration.h" #include "flutter/runtime/dart_vm.h" #include "flutter/shell/common/shell_test.h" #include "flutter/shell/common/thread_host.h" #include "flutter/testing/testing.h" +#include "gmock/gmock.h" +#include "googletest/googletest/include/gtest/gtest.h" +#include "third_party/dart/runtime/include/dart_api.h" +#include "tonic/converter/dart_converter.h" namespace flutter { namespace testing { @@ -332,5 +336,89 @@ TEST_F(PlatformConfigurationTest, SetDartPerformanceMode) { DestroyShell(std::move(shell), task_runners); } +TEST_F(PlatformConfigurationTest, BeginFrameMonotonic) { + auto message_latch = std::make_shared(); + + PlatformConfiguration* platform; + + // Called at the load time and will be in an Dart isolate. + auto nativeValidateConfiguration = [message_latch, + &platform](Dart_NativeArguments args) { + platform = UIDartState::Current()->platform_configuration(); + + // Hijacks the `_begin_frame` in hooks.dart so we can get a callback for + // validation. + auto field = + Dart_GetField(Dart_RootLibrary(), tonic::ToDart("_beginFrameHijack")); + platform->begin_frame_.Clear(); + platform->begin_frame_.Set(UIDartState::Current(), field); + + message_latch->Signal(); + }; + AddNativeCallback("ValidateConfiguration", + CREATE_NATIVE_ENTRY(nativeValidateConfiguration)); + + std::vector frame_times; + std::vector frame_numbers; + + auto frame_latch = std::make_shared(); + + // Called for each `_begin_frame` that is hijacked. + auto nativeBeginFrame = [frame_latch, &frame_times, + &frame_numbers](Dart_NativeArguments args) { + int64_t microseconds; + uint64_t frame_number; + Dart_IntegerToInt64(Dart_GetNativeArgument(args, 0), µseconds); + Dart_IntegerToUint64(Dart_GetNativeArgument(args, 1), &frame_number); + + frame_times.push_back(microseconds); + frame_numbers.push_back(frame_number); + + if (frame_times.size() == 3) { + frame_latch->Signal(); + } + }; + AddNativeCallback("BeginFrame", CREATE_NATIVE_ENTRY(nativeBeginFrame)); + + Settings settings = CreateSettingsForFixture(); + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + std::unique_ptr shell = CreateShell(settings, task_runners); + ASSERT_TRUE(shell->IsSetup()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("validateConfiguration"); + + shell->RunEngine(std::move(configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + // Wait for `nativeValidateConfiguration` to get called. + message_latch->Wait(); + + fml::TaskRunner::RunNowOrPostTask( + shell->GetTaskRunners().GetUITaskRunner(), [platform]() { + auto offset = fml::TimeDelta::FromMilliseconds(10); + auto zero = fml::TimePoint(); + auto one = zero + offset; + auto two = one + offset; + + platform->BeginFrame(zero, 1); + platform->BeginFrame(two, 2); + platform->BeginFrame(one, 3); + }); + + frame_latch->Wait(); + + ASSERT_THAT(frame_times, ::testing::ElementsAre(0, 20000, 20000)); + ASSERT_THAT(frame_numbers, ::testing::ElementsAre(1, 2, 3)); + DestroyShell(std::move(shell), task_runners); +} + } // namespace testing } // namespace flutter From bcddf9d8cb47de47c0ecef6eada14a8de7e019a1 Mon Sep 17 00:00:00 2001 From: John McDole Date: Tue, 24 Sep 2024 12:36:06 -0700 Subject: [PATCH 5/5] Init scalars --- lib/ui/window/platform_configuration.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ui/window/platform_configuration.h b/lib/ui/window/platform_configuration.h index ddf3e3d394645..6d227ce347ed8 100644 --- a/lib/ui/window/platform_configuration.h +++ b/lib/ui/window/platform_configuration.h @@ -543,8 +543,8 @@ class PlatformConfiguration final { tonic::DartPersistentValue draw_frame_; tonic::DartPersistentValue report_timings_; - uint64_t last_frame_number_; - int64_t last_microseconds_; + uint64_t last_frame_number_ = 0; + int64_t last_microseconds_ = 0; // All current views' view metrics mapped from view IDs. std::unordered_map metrics_;