Conditionally call FlutterViewDestroyOverlaySurfaces (#31464)

Author: Emmanuel Garcia

shell/common/shell.cc

@@ -869,10 +869,13 @@ void Shell::OnPlatformViewDestroyed() {
   fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(),
                                     raster_task);
   latch.Wait();
-  // On Android, the external view embedder posts a task to the platform thread,
-  // and waits until it completes.
-  // As a result, the platform thread must not be blocked prior to calling
-  // this method.
+  // On Android, the external view embedder may post a task to the platform
+  // thread, and wait until it completes if overlay surfaces must be released.
+  // However, the platform thread might be blocked when Dart is initializing.
+  // In this situation, calling TeardownExternalViewEmbedder is safe because no
+  // platform views have been created before Flutter renders the first frame.
+  // Overall, the longer term plan is to remove this implementation once
+  // https://github.com/flutter/flutter/issues/96679 is fixed.
   rasterizer_->TeardownExternalViewEmbedder();
 }
 

shell/platform/android/external_view_embedder/external_view_embedder.cc

@@ -309,6 +309,9 @@ void AndroidExternalViewEmbedder::Teardown() {
 
 // |ExternalViewEmbedder|
 void AndroidExternalViewEmbedder::DestroySurfaces() {
+  if (!surface_pool_->HasLayers()) {
+    return;
+  }
   fml::AutoResetWaitableEvent latch;
   fml::TaskRunner::RunNowOrPostTask(task_runners_.GetPlatformTaskRunner(),
                                     [&]() {

shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc

@@ -970,5 +970,16 @@ TEST(AndroidExternalViewEmbedder, Teardown) {
   embedder->Teardown();
 }
 
+TEST(AndroidExternalViewEmbedder, TeardownDoesNotCallJNIMethod) {
+  auto jni_mock = std::make_shared<JNIMock>();
+  auto android_context =
+      std::make_shared<AndroidContext>(AndroidRenderingAPI::kSoftware);
+  auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
+      *android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
+
+  EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()).Times(0);
+  embedder->Teardown();
+}
+
 }  // namespace testing
 }  // namespace flutter

shell/platform/android/external_view_embedder/surface_pool.cc

@@ -24,11 +24,11 @@ std::shared_ptr<OverlayLayer> SurfacePool::GetLayer(
     const AndroidContext& android_context,
     std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
     std::shared_ptr<AndroidSurfaceFactory> surface_factory) {
+  std::lock_guard lock(mutex_);
   // Destroy current layers in the pool if the frame size has changed.
   if (requested_frame_size_ != current_frame_size_) {
-    DestroyLayers(jni_facade);
+    DestroyLayersLocked(jni_facade);
   }
-
   intptr_t gr_context_key = reinterpret_cast<intptr_t>(gr_context);
   // Allocate a new surface if there isn't one available.
   if (available_layer_index_ >= layers_.size()) {
@@ -56,6 +56,7 @@ std::shared_ptr<OverlayLayer> SurfacePool::GetLayer(
     layer->gr_context_key = gr_context_key;
     layers_.push_back(layer);
   }
+
   std::shared_ptr<OverlayLayer> layer = layers_[available_layer_index_];
   // Since the surfaces are recycled, it's possible that the GrContext is
   // different.
@@ -73,19 +74,33 @@ std::shared_ptr<OverlayLayer> SurfacePool::GetLayer(
 }
 
 void SurfacePool::RecycleLayers() {
+  std::lock_guard lock(mutex_);
   available_layer_index_ = 0;
 }
 
+bool SurfacePool::HasLayers() {
+  std::lock_guard lock(mutex_);
+  return layers_.size() > 0;
+}
+
 void SurfacePool::DestroyLayers(
     std::shared_ptr<PlatformViewAndroidJNI> jni_facade) {
-  if (layers_.size() > 0) {
-    jni_facade->FlutterViewDestroyOverlaySurfaces();
+  std::lock_guard lock(mutex_);
+  DestroyLayersLocked(jni_facade);
+}
+
+void SurfacePool::DestroyLayersLocked(
+    std::shared_ptr<PlatformViewAndroidJNI> jni_facade) {
+  if (layers_.size() == 0) {
+    return;
   }
+  jni_facade->FlutterViewDestroyOverlaySurfaces();
   layers_.clear();
   available_layer_index_ = 0;
 }
 
 std::vector<std::shared_ptr<OverlayLayer>> SurfacePool::GetUnusedLayers() {
+  std::lock_guard lock(mutex_);
   std::vector<std::shared_ptr<OverlayLayer>> results;
   for (size_t i = available_layer_index_; i < layers_.size(); i++) {
     results.push_back(layers_[i]);
@@ -94,6 +109,7 @@ std::vector<std::shared_ptr<OverlayLayer>> SurfacePool::GetUnusedLayers() {
 }
 
 void SurfacePool::SetFrameSize(SkISize frame_size) {
+  std::lock_guard lock(mutex_);
   requested_frame_size_ = frame_size;
 }
 

shell/platform/android/external_view_embedder/surface_pool.h

@@ -5,6 +5,8 @@
 #ifndef FLUTTER_SHELL_PLATFORM_ANDROID_EXTERNAL_VIEW_EMBEDDER_SURFACE_POOL_H_
 #define FLUTTER_SHELL_PLATFORM_ANDROID_EXTERNAL_VIEW_EMBEDDER_SURFACE_POOL_H_
 
+#include <mutex>
+
 #include "flutter/flow/surface.h"
 #include "flutter/shell/platform/android/context/android_context.h"
 #include "flutter/shell/platform/android/surface/android_surface.h"
@@ -41,7 +43,6 @@ struct OverlayLayer {
   intptr_t gr_context_key;
 };
 
-// This class isn't thread safe.
 class SurfacePool {
  public:
   SurfacePool();
@@ -72,6 +73,9 @@ class SurfacePool {
   // then they are deallocated as soon as |GetLayer| is called.
   void SetFrameSize(SkISize frame_size);
 
+  // Returns true if the current pool has layers in use.
+  bool HasLayers();
+
  private:
   // The index of the entry in the layers_ vector that determines the beginning
   // of the unused layers. For example, consider the following vector:
@@ -95,6 +99,11 @@ class SurfacePool {
 
   // The frame size to be used by future layers.
   SkISize requested_frame_size_;
+
+  // Used to guard public methods.
+  std::mutex mutex_;
+
+  void DestroyLayersLocked(std::shared_ptr<PlatformViewAndroidJNI> jni_facade);
 };
 
 }  // namespace flutter

shell/platform/android/external_view_embedder/surface_pool_unittests.cc

@@ -62,6 +62,7 @@ TEST(SurfacePool, GetLayerAllocateOneLayer) {
   auto layer = pool->GetLayer(gr_context.get(), *android_context, jni_mock,
                               surface_factory);
 
+  ASSERT_TRUE(pool->HasLayers());
   ASSERT_NE(nullptr, layer);
   ASSERT_EQ(reinterpret_cast<intptr_t>(gr_context.get()),
             layer->gr_context_key);
@@ -96,6 +97,7 @@ TEST(SurfacePool, GetUnusedLayers) {
 
   pool->RecycleLayers();
 
+  ASSERT_TRUE(pool->HasLayers());
   ASSERT_EQ(1UL, pool->GetUnusedLayers().size());
   ASSERT_EQ(layer, pool->GetUnusedLayers()[0]);
 }
@@ -137,6 +139,7 @@ TEST(SurfacePool, GetLayerRecycle) {
   auto layer_2 = pool->GetLayer(gr_context_2.get(), *android_context, jni_mock,
                                 surface_factory);
 
+  ASSERT_TRUE(pool->HasLayers());
   ASSERT_NE(nullptr, layer_1);
   ASSERT_EQ(layer_1, layer_2);
   ASSERT_EQ(reinterpret_cast<intptr_t>(gr_context_2.get()),
@@ -176,6 +179,8 @@ TEST(SurfacePool, GetLayerAllocateTwoLayers) {
                                 surface_factory);
   auto layer_2 = pool->GetLayer(gr_context.get(), *android_context, jni_mock,
                                 surface_factory);
+
+  ASSERT_TRUE(pool->HasLayers());
   ASSERT_NE(nullptr, layer_1);
   ASSERT_NE(nullptr, layer_2);
   ASSERT_NE(layer_1, layer_2);
@@ -213,9 +218,11 @@ TEST(SurfacePool, DestroyLayers) {
   pool->GetLayer(gr_context.get(), *android_context, jni_mock, surface_factory);
 
   EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces());
+
+  ASSERT_TRUE(pool->HasLayers());
   pool->DestroyLayers(jni_mock);
 
-  pool->RecycleLayers();
+  ASSERT_FALSE(pool->HasLayers());
   ASSERT_TRUE(pool->GetUnusedLayers().empty());
 }
 
@@ -246,8 +253,12 @@ TEST(SurfacePool, DestroyLayersFrameSizeChanged) {
           ByMove(std::make_unique<PlatformViewAndroidJNI::OverlayMetadata>(
               0, window))));
 
+  ASSERT_FALSE(pool->HasLayers());
+
   pool->GetLayer(gr_context.get(), *android_context, jni_mock, surface_factory);
 
+  ASSERT_TRUE(pool->HasLayers());
+
   pool->SetFrameSize(SkISize::Make(20, 20));
   EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()).Times(1);
   EXPECT_CALL(*jni_mock, FlutterViewCreateOverlaySurface())
@@ -258,6 +269,7 @@ TEST(SurfacePool, DestroyLayersFrameSizeChanged) {
   pool->GetLayer(gr_context.get(), *android_context, jni_mock, surface_factory);
 
   ASSERT_TRUE(pool->GetUnusedLayers().empty());
+  ASSERT_TRUE(pool->HasLayers());
 }
 
 }  // namespace testing