Create separate objects for isolate state and isolate group state (#14268)

Isolate data may need to be deleted on the same thread where it was allocated.
In particular, the task observer set up in the UIDartState ctor must be removed
from the same message loop where it was added.

The engine had been using the same DartIsolate object as the root isolate data
and as the isolate group data.  This object would be deleted when the isolate
group was shut down.  However, group shutdown may occur on a thread associated
with a secondary isolate.  When this happens, cleanup of any state tied to the
root isolate's thread will fail.

This change adds a DartIsolateGroupData object holding state that is common
among all isolates in a group.  DartIsolateGroupData can be deleted on any
thread.

See https://github.com/flutter/flutter/issues/45578

ci/licenses_golden/licenses_flutter

@@ -491,6 +491,8 @@ FILE: ../../../flutter/lib/web_ui/lib/ui.dart
 FILE: ../../../flutter/lib/web_ui/tool/unicode_sync_script.dart
 FILE: ../../../flutter/runtime/dart_isolate.cc
 FILE: ../../../flutter/runtime/dart_isolate.h
+FILE: ../../../flutter/runtime/dart_isolate_group_data.cc
+FILE: ../../../flutter/runtime/dart_isolate_group_data.h
 FILE: ../../../flutter/runtime/dart_isolate_unittests.cc
 FILE: ../../../flutter/runtime/dart_lifecycle_unittests.cc
 FILE: ../../../flutter/runtime/dart_service_isolate.cc

runtime/BUILD.gn

@@ -46,6 +46,8 @@ source_set("runtime") {
   sources = [
     "dart_isolate.cc",
     "dart_isolate.h",
+    "dart_isolate_group_data.cc",
+    "dart_isolate_group_data.h",
     "dart_service_isolate.cc",
     "dart_service_isolate.h",
     "dart_snapshot.cc",

runtime/dart_isolate.h

@@ -24,6 +24,7 @@
 namespace flutter {
 
 class DartVM;
+class DartIsolateGroupData;
 
 //------------------------------------------------------------------------------
 /// @brief      Represents an instance of a live isolate. An isolate is a
@@ -205,13 +206,6 @@ class DartIsolate : public UIDartState {
   // |UIDartState|
   ~DartIsolate() override;
 
-  //----------------------------------------------------------------------------
-  /// @brief      Get the settings used to create this isolate instance.
-  ///
-  /// @return     The settings used in the `CreateRootIsolate` call.
-  ///
-  const Settings& GetSettings() const;
-
   //----------------------------------------------------------------------------
   /// @brief      The current phase of the isolate. The engine represents all
   ///             dart isolates as being in one of the known phases. By invoking
@@ -376,14 +370,6 @@ class DartIsolate : public UIDartState {
   ///
   void AddIsolateShutdownCallback(const fml::closure& closure);
 
-  //----------------------------------------------------------------------------
-  /// @brief      The snapshot used to launch this isolate. This is referenced
-  ///             by any child isolates launched by the root isolate.
-  ///
-  /// @return     The isolate snapshot.
-  ///
-  fml::RefPtr<const DartSnapshot> GetIsolateSnapshot() const;
-
   //----------------------------------------------------------------------------
   /// @brief      A weak pointer to the Dart isolate instance. This instance may
   ///             only be used on the task runner that created the root isolate.
@@ -403,14 +389,8 @@ class DartIsolate : public UIDartState {
 
   // Root isolate of the VM application
   bool IsRootIsolate() const { return is_root_isolate_; }
-  // Isolate that owns IsolateGroup it lives in.
-  // When --no-enable-isolate-groups dart vm flag is set,
-  // all child isolates will have their own IsolateGroups.
-  bool IsGroupRootIsolate() const { return is_group_root_isolate_; }
 
  private:
-  using ChildIsolatePreparer = std::function<bool(DartIsolate*)>;
-
   class AutoFireClosure {
    public:
     AutoFireClosure(const fml::closure& closure);
@@ -424,19 +404,12 @@ class DartIsolate : public UIDartState {
   friend class DartVM;
 
   Phase phase_ = Phase::Unknown;
-  const Settings settings_;
-  const fml::RefPtr<const DartSnapshot> isolate_snapshot_;
   std::vector<std::shared_ptr<const fml::Mapping>> kernel_buffers_;
   std::vector<std::unique_ptr<AutoFireClosure>> shutdown_callbacks_;
-  ChildIsolatePreparer child_isolate_preparer_ = nullptr;
   fml::RefPtr<fml::TaskRunner> message_handling_task_runner_;
-  const fml::closure isolate_create_callback_;
-  const fml::closure isolate_shutdown_callback_;
   const bool is_root_isolate_;
-  const bool is_group_root_isolate_;
 
   DartIsolate(const Settings& settings,
-              fml::RefPtr<const DartSnapshot> isolate_snapshot,
               TaskRunners task_runners,
               fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
               fml::WeakPtr<IOManager> io_manager,
@@ -444,11 +417,7 @@ class DartIsolate : public UIDartState {
               fml::WeakPtr<ImageDecoder> image_decoder,
               std::string advisory_script_uri,
               std::string advisory_script_entrypoint,
-              const ChildIsolatePreparer& child_isolate_preparer,
-              const fml::closure& isolate_create_callback,
-              const fml::closure& isolate_shutdown_callback,
-              bool is_root_isolate,
-              bool is_group_root_isolate);
+              bool is_root_isolate);
   FML_WARN_UNUSED_RESULT bool Initialize(Dart_Isolate isolate);
 
   void SetMessageHandlingTaskRunner(fml::RefPtr<fml::TaskRunner> runner);
@@ -465,6 +434,8 @@ class DartIsolate : public UIDartState {
 
   void OnShutdownCallback();
 
+  DartIsolateGroupData& GetIsolateGroupData();
+
   // |Dart_IsolateGroupCreateCallback|
   static Dart_Isolate DartIsolateGroupCreateCallback(
       const char* advisory_script_uri,
@@ -472,7 +443,7 @@ class DartIsolate : public UIDartState {
       const char* package_root,
       const char* package_config,
       Dart_IsolateFlags* flags,
-      std::shared_ptr<DartIsolate>* embedder_isolate,
+      std::shared_ptr<DartIsolate>* parent_isolate_group,
       char** error);
 
   // |Dart_IsolateInitializeCallback|
@@ -485,16 +456,10 @@ class DartIsolate : public UIDartState {
       Dart_IsolateFlags* flags,
       char** error);
 
-  static std::pair<Dart_Isolate /* vm */,
-                   std::weak_ptr<DartIsolate> /* embedder */>
-  CreateDartVMAndEmbedderObjectPair(
-      const char* advisory_script_uri,
-      const char* advisory_script_entrypoint,
-      const char* package_root,
-      const char* package_config,
+  static Dart_Isolate CreateDartIsolateGroup(
+      std::unique_ptr<std::shared_ptr<DartIsolateGroupData>> isolate_group_data,
+      std::unique_ptr<std::shared_ptr<DartIsolate>> isolate_data,
       Dart_IsolateFlags* flags,
-      std::shared_ptr<DartIsolate>* parent_embedder_isolate,
-      bool is_root_isolate,
       char** error);
 
   static bool InitializeIsolate(std::shared_ptr<DartIsolate> embedder_isolate,
@@ -503,17 +468,17 @@ class DartIsolate : public UIDartState {
 
   // |Dart_IsolateShutdownCallback|
   static void DartIsolateShutdownCallback(
-      std::shared_ptr<DartIsolate>* isolate_group_data,
+      std::shared_ptr<DartIsolateGroupData>* isolate_group_data,
       std::shared_ptr<DartIsolate>* isolate_data);
 
   // |Dart_IsolateCleanupCallback|
   static void DartIsolateCleanupCallback(
-      std::shared_ptr<DartIsolate>* isolate_group_data,
+      std::shared_ptr<DartIsolateGroupData>* isolate_group_data,
       std::shared_ptr<DartIsolate>* isolate_data);
 
   // |Dart_IsolateGroupCleanupCallback|
   static void DartIsolateGroupCleanupCallback(
-      std::shared_ptr<DartIsolate>* isolate_group_data);
+      std::shared_ptr<DartIsolateGroupData>* isolate_group_data);
 
   FML_DISALLOW_COPY_AND_ASSIGN(DartIsolate);
 };

runtime/dart_isolate_group_data.cc

+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "flutter/runtime/dart_isolate_group_data.h"
+#include "flutter/runtime/dart_snapshot.h"
+
+namespace flutter {
+
+DartIsolateGroupData::DartIsolateGroupData(
+    const Settings& settings,
+    fml::RefPtr<const DartSnapshot> isolate_snapshot,
+    std::string advisory_script_uri,
+    std::string advisory_script_entrypoint,
+    const ChildIsolatePreparer& child_isolate_preparer,
+    const fml::closure& isolate_create_callback,
+    const fml::closure& isolate_shutdown_callback)
+    : settings_(settings),
+      isolate_snapshot_(isolate_snapshot),
+      advisory_script_uri_(advisory_script_uri),
+      advisory_script_entrypoint_(advisory_script_entrypoint),
+      child_isolate_preparer_(child_isolate_preparer),
+      isolate_create_callback_(isolate_create_callback),
+      isolate_shutdown_callback_(isolate_shutdown_callback) {
+  FML_DCHECK(isolate_snapshot_) << "Must contain a valid isolate snapshot.";
+}
+
+DartIsolateGroupData::~DartIsolateGroupData() = default;
+
+const Settings& DartIsolateGroupData::GetSettings() const {
+  return settings_;
+}
+
+fml::RefPtr<const DartSnapshot> DartIsolateGroupData::GetIsolateSnapshot()
+    const {
+  return isolate_snapshot_;
+}
+
+const std::string& DartIsolateGroupData::GetAdvisoryScriptURI() const {
+  return advisory_script_uri_;
+}
+
+const std::string& DartIsolateGroupData::GetAdvisoryScriptEntrypoint() const {
+  return advisory_script_entrypoint_;
+}
+
+const ChildIsolatePreparer& DartIsolateGroupData::GetChildIsolatePreparer()
+    const {
+  return child_isolate_preparer_;
+}
+
+const fml::closure& DartIsolateGroupData::GetIsolateCreateCallback() const {
+  return isolate_create_callback_;
+}
+
+const fml::closure& DartIsolateGroupData::GetIsolateShutdownCallback() const {
+  return isolate_shutdown_callback_;
+}
+
+void DartIsolateGroupData::SetChildIsolatePreparer(
+    const ChildIsolatePreparer& value) {
+  child_isolate_preparer_ = value;
+}
+
+}  // namespace flutter

runtime/dart_isolate_group_data.h

+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef FLUTTER_RUNTIME_DART_ISOLATE_GROUP_DATA_H_
+#define FLUTTER_RUNTIME_DART_ISOLATE_GROUP_DATA_H_
+
+#include <string>
+
+#include "flutter/common/settings.h"
+#include "flutter/fml/closure.h"
+#include "flutter/fml/memory/ref_ptr.h"
+
+namespace flutter {
+
+class DartIsolate;
+class DartSnapshot;
+
+using ChildIsolatePreparer = std::function<bool(DartIsolate*)>;
+
+// Object holding state associated with a Dart isolate group.  An instance of
+// this class will be provided to Dart_CreateIsolateGroup as the
+// isolate_group_data.
+//
+// This object must be thread safe because the Dart VM can invoke the isolate
+// group cleanup callback on any thread.
+class DartIsolateGroupData {
+ public:
+  DartIsolateGroupData(const Settings& settings,
+                       fml::RefPtr<const DartSnapshot> isolate_snapshot,
+                       std::string advisory_script_uri,
+                       std::string advisory_script_entrypoint,
+                       const ChildIsolatePreparer& child_isolate_preparer,
+                       const fml::closure& isolate_create_callback,
+                       const fml::closure& isolate_shutdown_callback);
+
+  ~DartIsolateGroupData();
+
+  const Settings& GetSettings() const;
+  fml::RefPtr<const DartSnapshot> GetIsolateSnapshot() const;
+  const std::string& GetAdvisoryScriptURI() const;
+  const std::string& GetAdvisoryScriptEntrypoint() const;
+  const ChildIsolatePreparer& GetChildIsolatePreparer() const;
+  const fml::closure& GetIsolateCreateCallback() const;
+  const fml::closure& GetIsolateShutdownCallback() const;
+
+  void SetChildIsolatePreparer(const ChildIsolatePreparer& value);
+
+ private:
+  FML_DISALLOW_COPY_AND_ASSIGN(DartIsolateGroupData);
+
+  const Settings settings_;
+  const fml::RefPtr<const DartSnapshot> isolate_snapshot_;
+  const std::string advisory_script_uri_;
+  const std::string advisory_script_entrypoint_;
+  ChildIsolatePreparer child_isolate_preparer_;
+  const fml::closure isolate_create_callback_;
+  const fml::closure isolate_shutdown_callback_;
+};
+
+}  // namespace flutter
+
+#endif  // FLUTTER_RUNTIME_DART_ISOLATE_GROUP_DATA_H_

runtime/dart_isolate_unittests.cc

@@ -109,14 +109,15 @@ class AutoIsolateShutdown {
       return;
     }
     fml::AutoResetWaitableEvent latch;
-    fml::TaskRunner::RunNowOrPostTask(runner_, [isolate = isolate_, &latch]() {
-      FML_LOG(INFO) << "Shutting down isolate.";
-      if (!isolate->Shutdown()) {
-        FML_LOG(ERROR) << "Could not shutdown isolate.";
-        FML_CHECK(false);
-      }
-      latch.Signal();
-    });
+    fml::TaskRunner::RunNowOrPostTask(
+        runner_, [isolate = std::move(isolate_), &latch]() {
+          FML_LOG(INFO) << "Shutting down isolate.";
+          if (!isolate->Shutdown()) {
+            FML_LOG(ERROR) << "Could not shutdown isolate.";
+            FML_CHECK(false);
+          }
+          latch.Signal();
+        });
     latch.Wait();
   }