[vm/concurrency] Make main Kernel loading independent of current isolate

Since loading kernel will affect all isolates within a group, there
should be nothing isolate-specific inside the kernel loader. To ensure
we don't have (or introduce) any accidental uses, we add a
NoActiveIsolateScope to the main parts of kernel loading.

Only loading of native extensions - done by the kernel loader -
currently requires an active isolate. The reason for this is that
loading native extensions happen by calling out to embedder, which calls
back into the VM using the our embedding API (which currently requires
an active isolate)

Issue dart-lang/sdk#36097

TEST=Refactoring of existing implementation.

Change-Id: I96e64dbfe7148b76b8fa006fe5dbde8c1f904504
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184269
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Alexander Aprelev <[email protected]>

Author: mkustermann

runtime/observatory/tests/service/get_vm_timeline_rpc_test.dart

@@ -111,7 +111,9 @@ void allEventsHaveIsolateNumber(List events) {
     Map arguments = event['args'];
     expect(arguments, isA<Map>());
     expect(arguments['isolateGroupId'], isA<String>());
-    if (!const ['GC', 'Compiler', 'CompilerVerbose'].contains(event['cat'])) {
+    if (!const ['GC', 'Compiler', 'CompilerVerbose'].contains(event['cat']) &&
+        !const ['FinishTopLevelClassLoading', 'FinishClassLoading']
+            .contains(event['name'])) {
       expect(arguments['isolateId'], isA<String>());
     }
   }

runtime/observatory_2/tests/service_2/get_vm_timeline_rpc_test.dart

@@ -111,7 +111,9 @@ void allEventsHaveIsolateNumber(List events) {
     Map arguments = event['args'];
     expect(arguments, isA<Map>());
     expect(arguments['isolateGroupId'], isA<String>());
-    if (!const ['GC', 'Compiler', 'CompilerVerbose'].contains(event['cat'])) {
+    if (!const ['GC', 'Compiler', 'CompilerVerbose'].contains(event['cat']) &&
+        !const ['FinishTopLevelClassLoading', 'FinishClassLoading']
+            .contains(event['name'])) {
       expect(arguments['isolateId'], isA<String>());
     }
   }

runtime/vm/isolate.h

@@ -187,11 +187,11 @@ typedef FixedCache<intptr_t, CatchEntryMovesRefPtr, 16> CatchEntryMovesCache;
   V(NONPRODUCT, asserts, EnableAsserts, enable_asserts, FLAG_enable_asserts)   \
   V(NONPRODUCT, use_field_guards, UseFieldGuards, use_field_guards,            \
     FLAG_use_field_guards)                                                     \
+  V(PRODUCT, should_load_vmservice_library, ShouldLoadVmService,               \
+    load_vmservice_library, false)                                             \
   V(NONPRODUCT, use_osr, UseOsr, use_osr, FLAG_use_osr)
 
 #define BOOL_ISOLATE_FLAG_LIST_DEFAULT_GETTER(V)                               \
-  V(PRODUCT, should_load_vmservice_library, ShouldLoadVmService,               \
-    load_vmservice_library, false)                                             \
   V(PRODUCT, copy_parent_code, CopyParentCode, copy_parent_code, false)        \
   V(PRODUCT, is_system_isolate, IsSystemIsolate, is_system_isolate, false)
 
@@ -512,6 +512,14 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
     return null_safety() || FLAG_strict_null_safety_checks;
   }
 
+  bool should_load_vmservice() const {
+    return ShouldLoadVmServiceBit::decode(isolate_group_flags_);
+  }
+  void set_should_load_vmservice(bool value) {
+    isolate_group_flags_ =
+        ShouldLoadVmServiceBit::update(value, isolate_group_flags_);
+  }
+
 #if !defined(PRODUCT)
 #if !defined(DART_PRECOMPILED_RUNTIME)
   bool HasAttemptedReload() const {
@@ -820,6 +828,7 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
   V(HasAttemptedReload)                                                        \
   V(NullSafety)                                                                \
   V(RemappingCids)                                                             \
+  V(ShouldLoadVmService)                                                       \
   V(NullSafetySet)                                                             \
   V(Obfuscate)                                                                 \
   V(UseFieldGuards)                                                            \
@@ -1360,13 +1369,6 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
            FLAG_randomize_optimization_counter;
   }
 
-  bool should_load_vmservice() const {
-    return ShouldLoadVmServiceBit::decode(isolate_flags_);
-  }
-  void set_should_load_vmservice(bool value) {
-    isolate_flags_ = ShouldLoadVmServiceBit::update(value, isolate_flags_);
-  }
-
   const DispatchTable* dispatch_table() const {
     return group()->dispatch_table();
   }
@@ -1556,7 +1558,6 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
   V(HasAttemptedStepping)                                                      \
   V(ShouldPausePostServiceRequest)                                             \
   V(CopyParentCode)                                                            \
-  V(ShouldLoadVmService)                                                       \
   V(IsSystemIsolate)
 
   // Isolate specific flags.
@@ -1764,9 +1765,11 @@ class EnterIsolateGroupScope {
 //
 // This can be used in code (e.g. GC, Kernel Loader) that should not operate on
 // an individual isolate.
-class NoActiveIsolateScope {
+class NoActiveIsolateScope : public StackResource {
  public:
-  NoActiveIsolateScope() : thread_(Thread::Current()) {
+  NoActiveIsolateScope()
+      : StackResource(Thread::Current()),
+        thread_(static_cast<Thread*>(thread())) {
     saved_isolate_ = thread_->isolate_;
     thread_->isolate_ = nullptr;
   }

runtime/vm/kernel_loader.cc

@@ -238,6 +238,7 @@ void KernelLoader::ReadLoadingUnits() {
 Object& KernelLoader::LoadEntireProgram(Program* program,
                                         bool process_pending_classes) {
   Thread* thread = Thread::Current();
+
   TIMELINE_DURATION(thread, Isolate, "LoadKernel");
 
   if (program->is_single_program()) {
@@ -772,6 +773,8 @@ ObjectPtr KernelLoader::LoadProgram(bool process_pending_classes) {
 }
 
 void KernelLoader::LoadLibrary(const Library& library) {
+  NoActiveIsolateScope no_active_isolate_scope;
+
   // This will be invoked by VM bootstrapping code.
   SafepointWriteRwLocker ml(thread_, thread_->isolate_group()->program_lock());
 
@@ -986,6 +989,8 @@ LibraryPtr KernelLoader::LoadLibrary(intptr_t index) {
         "not allowed");
   }
 
+  NoActiveIsolateScope no_active_isolate_scope;
+
   // Read library index.
   library_kernel_offset_ = library_offset(index);
   correction_offset_ = library_kernel_offset_;
@@ -1001,7 +1006,7 @@ LibraryPtr KernelLoader::LoadLibrary(intptr_t index) {
 
   LibraryHelper library_helper(&helper_, kernel_binary_version_);
   library_helper.ReadUntilIncluding(LibraryHelper::kCanonicalName);
-  if (!FLAG_precompiled_mode && !I->should_load_vmservice()) {
+  if (!FLAG_precompiled_mode && !IG->should_load_vmservice()) {
     StringIndex lib_name_index =
         H.CanonicalNameString(library_helper.canonical_name_);
     if (H.StringEquals(lib_name_index, kVMServiceIOLibraryUri)) {
@@ -1791,6 +1796,8 @@ void KernelLoader::FinishClassLoading(const Class& klass,
 }
 
 void KernelLoader::FinishLoading(const Class& klass) {
+  NoActiveIsolateScope no_active_isolate_scope;
+
   ASSERT(klass.IsTopLevel() || (klass.kernel_offset() > 0));
 
   Zone* zone = Thread::Current()->zone();

runtime/vm/kernel_loader.h

@@ -214,16 +214,16 @@ class KernelLoader : public ValueObject {
                                        intptr_t kernel_buffer_length,
                                        const String& url);
 
-  void FinishTopLevelClassLoading(const Class& toplevel_class,
-                                  const Library& library,
-                                  const LibraryIndex& library_index);
-
   static void FinishLoading(const Class& klass);
 
   void ReadObfuscationProhibitions();
   void ReadLoadingUnits();
 
  private:
+  void FinishTopLevelClassLoading(const Class& toplevel_class,
+                                  const Library& library,
+                                  const LibraryIndex& library_index);
+
   // Check for the presence of a (possibly const) constructor for the
   // 'ExternalName' class. If found, returns the name parameter to the
   // constructor.