Embedder VsyncWaiter must schedule a frame for the right time (#29408) (#29508)

* Embedder VsyncWaiter must schedule a frame for the right time (#29408)

* Ensure vsync callback completes before resetting the engine. (#29488)

Fixes: https://github.com/flutter/flutter/issues/92934

Co-authored-by: Kaushik Iska <iska.kaushik@gmail.com>
diff --git a/shell/platform/embedder/embedder_engine.cc b/shell/platform/embedder/embedder_engine.cc
index 7d1f74b..d96fa0c 100644
--- a/shell/platform/embedder/embedder_engine.cc
+++ b/shell/platform/embedder/embedder_engine.cc
@@ -229,8 +229,8 @@
     return false;
   }
 
-  return VsyncWaiterEmbedder::OnEmbedderVsync(baton, frame_start_time,
-                                              frame_target_time);
+  return VsyncWaiterEmbedder::OnEmbedderVsync(
+      task_runners_, baton, frame_start_time, frame_target_time);
 }
 
 bool EmbedderEngine::ReloadSystemFonts() {
diff --git a/shell/platform/embedder/tests/embedder_config_builder.cc b/shell/platform/embedder/tests/embedder_config_builder.cc
index ac6c86a..d04188e 100644
--- a/shell/platform/embedder/tests/embedder_config_builder.cc
+++ b/shell/platform/embedder/tests/embedder_config_builder.cc
@@ -261,6 +261,13 @@
   project_args_.custom_task_runners = &custom_task_runners_;
 }
 
+void EmbedderConfigBuilder::SetupVsyncCallback() {
+  project_args_.vsync_callback = [](void* user_data, intptr_t baton) {
+    auto context = reinterpret_cast<EmbedderTestContext*>(user_data);
+    context->RunVsyncCallback(baton);
+  };
+}
+
 void EmbedderConfigBuilder::SetRenderTaskRunner(
     const FlutterTaskRunnerDescription* runner) {
   if (runner == nullptr) {
diff --git a/shell/platform/embedder/tests/embedder_config_builder.h b/shell/platform/embedder/tests/embedder_config_builder.h
index 40f9bfc..17a1dd4 100644
--- a/shell/platform/embedder/tests/embedder_config_builder.h
+++ b/shell/platform/embedder/tests/embedder_config_builder.h
@@ -104,6 +104,10 @@
 
   UniqueEngine InitializeEngine() const;
 
+  // Sets up the callback for vsync, the callbacks needs to be specified on the
+  // text context vis `SetVsyncCallback`.
+  void SetupVsyncCallback();
+
  private:
   EmbedderTestContext& context_;
   FlutterProjectArgs project_args_ = {};
diff --git a/shell/platform/embedder/tests/embedder_test_context.cc b/shell/platform/embedder/tests/embedder_test_context.cc
index 44d39a3..79eb749 100644
--- a/shell/platform/embedder/tests/embedder_test_context.cc
+++ b/shell/platform/embedder/tests/embedder_test_context.cc
@@ -224,5 +224,14 @@
   callback(image_callback());
 }
 
+void EmbedderTestContext::SetVsyncCallback(
+    std::function<void(intptr_t)> callback) {
+  vsync_callback_ = callback;
+}
+
+void EmbedderTestContext::RunVsyncCallback(intptr_t baton) {
+  vsync_callback_(baton);
+}
+
 }  // namespace testing
 }  // namespace flutter
diff --git a/shell/platform/embedder/tests/embedder_test_context.h b/shell/platform/embedder/tests/embedder_test_context.h
index 366b5b0..6efca54 100644
--- a/shell/platform/embedder/tests/embedder_test_context.h
+++ b/shell/platform/embedder/tests/embedder_test_context.h
@@ -89,6 +89,15 @@
 
   virtual EmbedderTestContextType GetContextType() const = 0;
 
+  // Sets up the callback for vsync. This callback will be invoked
+  // for every vsync. This should be used in conjunction with SetupVsyncCallback
+  // on the EmbedderConfigBuilder. Any callback setup here must call
+  // `FlutterEngineOnVsync` from the platform task runner.
+  void SetVsyncCallback(std::function<void(intptr_t)> callback);
+
+  // Runs the vsync callback.
+  void RunVsyncCallback(intptr_t baton);
+
   // TODO(gw280): encapsulate these properly for subclasses to use
  protected:
   // This allows the builder to access the hooks.
@@ -112,6 +121,7 @@
   std::unique_ptr<EmbedderTestCompositor> compositor_;
   NextSceneCallback next_scene_callback_;
   SkMatrix root_surface_transformation_;
+  std::function<void(intptr_t)> vsync_callback_ = nullptr;
 
   static VoidCallback GetIsolateCreateCallbackHook();
 
diff --git a/shell/platform/embedder/tests/embedder_unittests.cc b/shell/platform/embedder/tests/embedder_unittests.cc
index ea9904d..1998ebe 100644
--- a/shell/platform/embedder/tests/embedder_unittests.cc
+++ b/shell/platform/embedder/tests/embedder_unittests.cc
@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "fml/task_runner.h"
 #define FML_USED_ON_EMBEDDER
 
 #include <string>
@@ -18,7 +17,10 @@
 #include "flutter/fml/paths.h"
 #include "flutter/fml/synchronization/count_down_latch.h"
 #include "flutter/fml/synchronization/waitable_event.h"
+#include "flutter/fml/task_runner.h"
 #include "flutter/fml/thread.h"
+#include "flutter/fml/time/time_delta.h"
+#include "flutter/fml/time/time_point.h"
 #include "flutter/runtime/dart_vm.h"
 #include "flutter/shell/platform/embedder/tests/embedder_assertions.h"
 #include "flutter/shell/platform/embedder/tests/embedder_config_builder.h"
@@ -29,6 +31,16 @@
 #include "third_party/skia/include/core/SkSurface.h"
 #include "third_party/tonic/converter/dart_converter.h"
 
+namespace {
+
+static uint64_t NanosFromEpoch(int millis_from_now) {
+  const auto now = fml::TimePoint::Now();
+  const auto delta = fml::TimeDelta::FromMilliseconds(millis_from_now);
+  return (now + delta).ToEpochDelta().ToNanoseconds();
+}
+
+}  // namespace
+
 namespace flutter {
 namespace testing {
 
@@ -1575,5 +1587,63 @@
   shutdown_latch.Wait();
 }
 
+// This test schedules a frame for the future and asserts that vsync waiter
+// posts the event at the right frame start time (which is in the future).
+TEST_F(EmbedderTest, VsyncCallbackPostedIntoFuture) {
+  UniqueEngine engine;
+  fml::AutoResetWaitableEvent present_latch;
+  fml::AutoResetWaitableEvent vsync_latch;
+
+  // One of the threads that the callback (FlutterEngineOnVsync) will be posted
+  // to is the platform thread. So we cannot wait for assertions to complete on
+  // the platform thread. Create a new thread to manage the engine instance and
+  // wait for assertions on the test thread.
+  auto platform_task_runner = CreateNewThread("platform_thread");
+
+  platform_task_runner->PostTask([&]() {
+    auto& context =
+        GetEmbedderContext(EmbedderTestContextType::kSoftwareContext);
+
+    context.SetVsyncCallback([&](intptr_t baton) {
+      platform_task_runner->PostTask([baton = baton, &engine, &vsync_latch]() {
+        FlutterEngineOnVsync(engine.get(), baton, NanosFromEpoch(16),
+                             NanosFromEpoch(32));
+        vsync_latch.Signal();
+      });
+    });
+    context.AddNativeCallback(
+        "SignalNativeTest", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) {
+          present_latch.Signal();
+        }));
+
+    EmbedderConfigBuilder builder(context);
+    builder.SetSoftwareRendererConfig();
+    builder.SetupVsyncCallback();
+    builder.SetDartEntrypoint("empty_scene");
+    engine = builder.LaunchEngine();
+    ASSERT_TRUE(engine.is_valid());
+
+    // Send a window metrics events so frames may be scheduled.
+    FlutterWindowMetricsEvent event = {};
+    event.struct_size = sizeof(event);
+    event.width = 800;
+    event.height = 600;
+    event.pixel_ratio = 1.0;
+
+    ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event),
+              kSuccess);
+  });
+
+  vsync_latch.Wait();
+  present_latch.Wait();
+
+  fml::AutoResetWaitableEvent shutdown_latch;
+  platform_task_runner->PostTask([&]() {
+    engine.reset();
+    shutdown_latch.Signal();
+  });
+  shutdown_latch.Wait();
+}
+
 }  // namespace testing
 }  // namespace flutter
diff --git a/shell/platform/embedder/vsync_waiter_embedder.cc b/shell/platform/embedder/vsync_waiter_embedder.cc
index a3d483f..bf56692 100644
--- a/shell/platform/embedder/vsync_waiter_embedder.cc
+++ b/shell/platform/embedder/vsync_waiter_embedder.cc
@@ -17,26 +17,35 @@
 // |VsyncWaiter|
 void VsyncWaiterEmbedder::AwaitVSync() {
   auto* weak_waiter = new std::weak_ptr<VsyncWaiter>(shared_from_this());
-  vsync_callback_(reinterpret_cast<intptr_t>(weak_waiter));
+  intptr_t baton = reinterpret_cast<intptr_t>(weak_waiter);
+  vsync_callback_(baton);
 }
 
 // static
-bool VsyncWaiterEmbedder::OnEmbedderVsync(intptr_t baton,
-                                          fml::TimePoint frame_start_time,
-                                          fml::TimePoint frame_target_time) {
+bool VsyncWaiterEmbedder::OnEmbedderVsync(
+    const flutter::TaskRunners& task_runners,
+    intptr_t baton,
+    fml::TimePoint frame_start_time,
+    fml::TimePoint frame_target_time) {
   if (baton == 0) {
     return false;
   }
 
-  auto* weak_waiter = reinterpret_cast<std::weak_ptr<VsyncWaiter>*>(baton);
-  auto strong_waiter = weak_waiter->lock();
-  delete weak_waiter;
+  // If the time here is in the future, the contract for `FlutterEngineOnVsync`
+  // says that the engine will only process the frame when the time becomes
+  // current.
+  task_runners.GetUITaskRunner()->PostTaskForTime(
+      [frame_start_time, frame_target_time, baton]() {
+        std::weak_ptr<VsyncWaiter>* weak_waiter =
+            reinterpret_cast<std::weak_ptr<VsyncWaiter>*>(baton);
+        auto vsync_waiter = weak_waiter->lock();
+        delete weak_waiter;
+        if (vsync_waiter) {
+          vsync_waiter->FireCallback(frame_start_time, frame_target_time);
+        }
+      },
+      frame_start_time);
 
-  if (!strong_waiter) {
-    return false;
-  }
-
-  strong_waiter->FireCallback(frame_start_time, frame_target_time);
   return true;
 }
 
diff --git a/shell/platform/embedder/vsync_waiter_embedder.h b/shell/platform/embedder/vsync_waiter_embedder.h
index c1b0632..24e98e4 100644
--- a/shell/platform/embedder/vsync_waiter_embedder.h
+++ b/shell/platform/embedder/vsync_waiter_embedder.h
@@ -19,7 +19,8 @@
 
   ~VsyncWaiterEmbedder() override;
 
-  static bool OnEmbedderVsync(intptr_t baton,
+  static bool OnEmbedderVsync(const flutter::TaskRunners& task_runners,
+                              intptr_t baton,
                               fml::TimePoint frame_start_time,
                               fml::TimePoint frame_target_time);