[protectors] Move regexp species protector back to the isolate

This reverts the changes made in

https://chromium-review.googlesource.com/c/v8/v8/+/1695465
https://chromium-review.googlesource.com/c/v8/v8/+/1776078

We originally moved this protector to the native context to avoid
cross-native-context pollution of protector state. Ideally,
invalidating a protector in one NC should not affect any other NC.

But as it turns out, having the protector on the NC causes more
problems than it solves since all affected callers now need to find
the correct native context to check. Sometimes (e.g. in CSA regexp
builtins) it is possible to blindly check the current NC, but the
reasoning behind this optimization is tricky to understand.
Sometimes, fetching the correct NC is not possible due to access
restrictions. These implementation complexities outweigh the (unknown)
potential performance benefits.

In the future we should attempt to move away from the protector
concept for these kinds of checks.

Bug: chromium:1069964,v8:9463
Change-Id: I2cbb2ec7266282165dae5e4a6c8bdbda520c50a9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2157382Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67415}

src/builtins/builtins-regexp-gen.cc

@@ -729,13 +729,9 @@ void RegExpBuiltinsAssembler::BranchIfFastRegExp(
 
   // This should only be needed for String.p.(split||matchAll), but we are
   // conservative here.
-  // Note: we are using the current native context here, which may or may not
-  // match the object's native context. That's fine: in case of a mismatch, we
-  // will bail in the next step when comparing the object's map against the
-  // current native context's initial regexp map.
-  TNode<NativeContext> native_context = LoadNativeContext(context);
-  GotoIf(IsRegExpSpeciesProtectorCellInvalid(native_context), if_ismodified);
+  GotoIf(IsRegExpSpeciesProtectorCellInvalid(), if_ismodified);
 
+  TNode<NativeContext> native_context = LoadNativeContext(context);
   TNode<JSFunction> regexp_fun =
       CAST(LoadContextElement(native_context, Context::REGEXP_FUNCTION_INDEX));
   TNode<Map> initial_map = CAST(

src/codegen/code-stub-assembler.cc

@@ -5879,12 +5879,10 @@ TNode<BoolT> CodeStubAssembler::IsTypedArraySpeciesProtectorCellInvalid() {
   return TaggedEqual(cell_value, invalid);
 }
 
-TNode<BoolT> CodeStubAssembler::IsRegExpSpeciesProtectorCellInvalid(
-    TNode<NativeContext> native_context) {
-  TNode<PropertyCell> cell = CAST(LoadContextElement(
-      native_context, Context::REGEXP_SPECIES_PROTECTOR_INDEX));
-  TNode<Object> cell_value = LoadObjectField(cell, PropertyCell::kValueOffset);
+TNode<BoolT> CodeStubAssembler::IsRegExpSpeciesProtectorCellInvalid() {
   TNode<Smi> invalid = SmiConstant(Protectors::kProtectorInvalid);
+  TNode<PropertyCell> cell = RegExpSpeciesProtectorConstant();
+  TNode<Object> cell_value = LoadObjectField(cell, PropertyCell::kValueOffset);
   return TaggedEqual(cell_value, invalid);
 }
 

src/codegen/code-stub-assembler.h

@@ -44,6 +44,7 @@ enum class PrimitiveType { kBoolean, kNumber, kString, kSymbol };
   V(PromiseSpeciesProtector, promise_species_protector,                       \
     PromiseSpeciesProtector)                                                  \
   V(PromiseThenProtector, promise_then_protector, PromiseThenProtector)       \
+  V(RegExpSpeciesProtector, regexp_species_protector, RegExpSpeciesProtector) \
   V(SetIteratorProtector, set_iterator_protector, SetIteratorProtector)       \
   V(SingleCharacterStringCache, single_character_string_cache,                \
     SingleCharacterStringCache)                                               \
@@ -2657,8 +2658,7 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   TNode<BoolT> IsPromiseThenProtectorCellInvalid();
   TNode<BoolT> IsArraySpeciesProtectorCellInvalid();
   TNode<BoolT> IsTypedArraySpeciesProtectorCellInvalid();
-  TNode<BoolT> IsRegExpSpeciesProtectorCellInvalid(
-      TNode<NativeContext> native_context);
+  TNode<BoolT> IsRegExpSpeciesProtectorCellInvalid();
   TNode<BoolT> IsPromiseSpeciesProtectorCellInvalid();
 
   TNode<BoolT> IsMockArrayBufferAllocatorFlag() {

src/execution/protectors-inl.h

@@ -13,14 +13,6 @@
 namespace v8 {
 namespace internal {
 
-#define DEFINE_PROTECTOR_ON_NATIVE_CONTEXT_CHECK(name, cell)                \
-  bool Protectors::Is##name##Intact(Handle<NativeContext> native_context) { \
-    PropertyCell species_cell = native_context->cell();                     \
-    return species_cell.value().IsSmi() &&                                  \
-           Smi::ToInt(species_cell.value()) == kProtectorValid;             \
-  }
-DECLARED_PROTECTORS_ON_NATIVE_CONTEXT(DEFINE_PROTECTOR_ON_NATIVE_CONTEXT_CHECK)
-
 #define DEFINE_PROTECTOR_ON_ISOLATE_CHECK(name, root_index, unused_cell) \
   bool Protectors::Is##name##Intact(Isolate* isolate) {                  \
     PropertyCell cell =                                                  \
@@ -29,6 +21,7 @@ DECLARED_PROTECTORS_ON_NATIVE_CONTEXT(DEFINE_PROTECTOR_ON_NATIVE_CONTEXT_CHECK)
            Smi::ToInt(cell.value()) == kProtectorValid;                  \
   }
 DECLARED_PROTECTORS_ON_ISOLATE(DEFINE_PROTECTOR_ON_ISOLATE_CHECK)
+#undef DEFINE_PROTECTORS_ON_ISOLATE_CHECK
 
 }  // namespace internal
 }  // namespace v8

src/execution/protectors.cc

@@ -41,29 +41,10 @@ constexpr bool IsDefined(v8::Isolate::UseCounterFeature) { return true; }
   STATIC_ASSERT(IsDefined(v8::Isolate::kInvalidated##Name##Protector));
 
 DECLARED_PROTECTORS_ON_ISOLATE(V)
-DECLARED_PROTECTORS_ON_NATIVE_CONTEXT(V)
 #undef V
 
 }  // namespace
 
-#define INVALIDATE_PROTECTOR_ON_NATIVE_CONTEXT_DEFINITION(name, cell)       \
-  void Protectors::Invalidate##name(Isolate* isolate,                       \
-                                    Handle<NativeContext> native_context) { \
-    DCHECK(native_context->cell().value().IsSmi());                         \
-    DCHECK(Is##name##Intact(native_context));                               \
-    if (FLAG_trace_protector_invalidation) {                                \
-      TraceProtectorInvalidation(#name);                                    \
-    }                                                                       \
-    Handle<PropertyCell> species_cell(native_context->cell(), isolate);     \
-    PropertyCell::SetValueWithInvalidation(                                 \
-        isolate, #cell, species_cell,                                       \
-        handle(Smi::FromInt(kProtectorInvalid), isolate));                  \
-    DCHECK(!Is##name##Intact(native_context));                              \
-  }
-DECLARED_PROTECTORS_ON_NATIVE_CONTEXT(
-    INVALIDATE_PROTECTOR_ON_NATIVE_CONTEXT_DEFINITION)
-#undef INVALIDATE_PROTECTOR_ON_NATIVE_CONTEXT_DEFINITION
-
 #define INVALIDATE_PROTECTOR_ON_ISOLATE_DEFINITION(name, unused_index, cell) \
   void Protectors::Invalidate##name(Isolate* isolate) {                      \
     DCHECK(isolate->factory()->cell()->value().IsSmi());                     \

src/execution/protectors.h

@@ -15,9 +15,6 @@ class Protectors : public AllStatic {
   static const int kProtectorValid = 1;
   static const int kProtectorInvalid = 0;
 
-#define DECLARED_PROTECTORS_ON_NATIVE_CONTEXT(V) \
-  V(RegExpSpeciesLookupChain, regexp_species_protector)
-
 #define DECLARED_PROTECTORS_ON_ISOLATE(V)                                     \
   V(ArrayBufferDetaching, ArrayBufferDetachingProtector,                      \
     array_buffer_detaching_protector)                                         \
@@ -41,6 +38,8 @@ class Protectors : public AllStatic {
   /*   property holder is the %IteratorPrototype%. Note that this also     */ \
   /*   invalidates the SetIterator protector (see below).                  */ \
   V(MapIteratorLookupChain, MapIteratorProtector, map_iterator_protector)     \
+  V(RegExpSpeciesLookupChain, RegExpSpeciesProtector,                         \
+    regexp_species_protector)                                                 \
   V(PromiseHook, PromiseHookProtector, promise_hook_protector)                \
   V(PromiseThenLookupChain, PromiseThenProtector, promise_then_protector)     \
   V(PromiseResolveLookupChain, PromiseResolveProtector,                       \
@@ -82,19 +81,9 @@ class Protectors : public AllStatic {
   V(TypedArraySpeciesLookupChain, TypedArraySpeciesProtector,                 \
     typed_array_species_protector)
 
-#define DECLARE_PROTECTOR_ON_NATIVE_CONTEXT(name, unused_cell) \
-  V8_EXPORT_PRIVATE static inline bool Is##name##Intact(       \
-      Handle<NativeContext> native_context);                   \
-  V8_EXPORT_PRIVATE static void Invalidate##name(              \
-      Isolate* isolate, Handle<NativeContext> native_context);
-
-  DECLARED_PROTECTORS_ON_NATIVE_CONTEXT(DECLARE_PROTECTOR_ON_NATIVE_CONTEXT)
-#undef DECLARE_PROTECTOR_ON_NATIVE_CONTEXT
-
 #define DECLARE_PROTECTOR_ON_ISOLATE(name, unused_root_index, unused_cell) \
   V8_EXPORT_PRIVATE static inline bool Is##name##Intact(Isolate* isolate); \
   V8_EXPORT_PRIVATE static void Invalidate##name(Isolate* isolate);
-
   DECLARED_PROTECTORS_ON_ISOLATE(DECLARE_PROTECTOR_ON_ISOLATE)
 #undef DECLARE_PROTECTOR_ON_ISOLATE
 };

src/heap/setup-heap-internal.cc

@@ -911,6 +911,13 @@ void Heap::CreateInitialObjects() {
     set_promise_species_protector(*cell);
   }
 
+  {
+    Handle<PropertyCell> cell =
+        factory->NewPropertyCell(factory->empty_string());
+    cell->set_value(Smi::FromInt(Protectors::kProtectorValid));
+    set_regexp_species_protector(*cell);
+  }
+
   {
     Handle<PropertyCell> cell =
         factory->NewPropertyCell(factory->empty_string());

src/objects/lookup.cc

@@ -192,20 +192,11 @@ void LookupIterator::InternalUpdateProtector(Isolate* isolate,
   if (!receiver_generic->IsHeapObject()) return;
   Handle<HeapObject> receiver = Handle<HeapObject>::cast(receiver_generic);
 
-  // Getting the native_context from the isolate as a fallback. If possible, we
-  // use the receiver's creation context instead.
-  Handle<NativeContext> native_context = isolate->native_context();
-
   ReadOnlyRoots roots(isolate);
   if (*name == roots.constructor_string()) {
-    // Fetching the context in here since the operation is rather expensive.
-    if (receiver->IsJSReceiver()) {
-      native_context = Handle<JSReceiver>::cast(receiver)->GetCreationContext();
-    }
-
     if (!Protectors::IsArraySpeciesLookupChainIntact(isolate) &&
         !Protectors::IsPromiseSpeciesLookupChainIntact(isolate) &&
-        !Protectors::IsRegExpSpeciesLookupChainIntact(native_context) &&
+        !Protectors::IsRegExpSpeciesLookupChainIntact(isolate) &&
         !Protectors::IsTypedArraySpeciesLookupChainIntact(isolate)) {
       return;
     }
@@ -221,10 +212,8 @@ void LookupIterator::InternalUpdateProtector(Isolate* isolate,
       Protectors::InvalidatePromiseSpeciesLookupChain(isolate);
       return;
     } else if (receiver->IsJSRegExp(isolate)) {
-      if (!Protectors::IsRegExpSpeciesLookupChainIntact(native_context)) {
-        return;
-      }
-      Protectors::InvalidateRegExpSpeciesLookupChain(isolate, native_context);
+      if (!Protectors::IsRegExpSpeciesLookupChainIntact(isolate)) return;
+      Protectors::InvalidateRegExpSpeciesLookupChain(isolate);
       return;
     } else if (receiver->IsJSTypedArray(isolate)) {
       if (!Protectors::IsTypedArraySpeciesLookupChainIntact(isolate)) return;
@@ -250,10 +239,8 @@ void LookupIterator::InternalUpdateProtector(Isolate* isolate,
         Protectors::InvalidatePromiseSpeciesLookupChain(isolate);
       } else if (isolate->IsInAnyContext(*receiver,
                                          Context::REGEXP_PROTOTYPE_INDEX)) {
-        if (!Protectors::IsRegExpSpeciesLookupChainIntact(native_context)) {
-          return;
-        }
-        Protectors::InvalidateRegExpSpeciesLookupChain(isolate, native_context);
+        if (!Protectors::IsRegExpSpeciesLookupChainIntact(isolate)) return;
+        Protectors::InvalidateRegExpSpeciesLookupChain(isolate);
       } else if (isolate->IsInAnyContext(
                      receiver->map(isolate).prototype(isolate),
                      Context::TYPED_ARRAY_PROTOTYPE_INDEX)) {
@@ -289,14 +276,9 @@ void LookupIterator::InternalUpdateProtector(Isolate* isolate,
       Protectors::InvalidateStringIteratorLookupChain(isolate);
     }
   } else if (*name == roots.species_symbol()) {
-    // Fetching the context in here since the operation is rather expensive.
-    if (receiver->IsJSReceiver()) {
-      native_context = Handle<JSReceiver>::cast(receiver)->GetCreationContext();
-    }
-
     if (!Protectors::IsArraySpeciesLookupChainIntact(isolate) &&
         !Protectors::IsPromiseSpeciesLookupChainIntact(isolate) &&
-        !Protectors::IsRegExpSpeciesLookupChainIntact(native_context) &&
+        !Protectors::IsRegExpSpeciesLookupChainIntact(isolate) &&
         !Protectors::IsTypedArraySpeciesLookupChainIntact(isolate)) {
       return;
     }
@@ -313,10 +295,8 @@ void LookupIterator::InternalUpdateProtector(Isolate* isolate,
       Protectors::InvalidatePromiseSpeciesLookupChain(isolate);
     } else if (isolate->IsInAnyContext(*receiver,
                                        Context::REGEXP_FUNCTION_INDEX)) {
-      if (!Protectors::IsRegExpSpeciesLookupChainIntact(native_context)) {
-        return;
-      }
-      Protectors::InvalidateRegExpSpeciesLookupChain(isolate, native_context);
+      if (!Protectors::IsRegExpSpeciesLookupChainIntact(isolate)) return;
+      Protectors::InvalidateRegExpSpeciesLookupChain(isolate);
     } else if (IsTypedArrayFunctionInAnyContext(isolate, *receiver)) {
       if (!Protectors::IsTypedArraySpeciesLookupChainIntact(isolate)) return;
       Protectors::InvalidateTypedArraySpeciesLookupChain(isolate);

src/regexp/regexp-utils.cc

@@ -185,10 +185,7 @@ bool RegExpUtils::IsUnmodifiedRegExp(Isolate* isolate, Handle<Object> obj) {
   // property. Similar spots in CSA would use BranchIfFastRegExp_Strict in this
   // case.
 
-  if (!Protectors::IsRegExpSpeciesLookupChainIntact(
-          recv.GetCreationContext())) {
-    return false;
-  }
+  if (!Protectors::IsRegExpSpeciesLookupChainIntact(isolate)) return false;
 
   // The smi check is required to omit ToLength(lastIndex) calls with possible
   // user-code execution on the fast path.

src/roots/roots.h

@@ -214,6 +214,7 @@ class Symbol;
   V(PropertyCell, array_species_protector, ArraySpeciesProtector)              \
   V(PropertyCell, typed_array_species_protector, TypedArraySpeciesProtector)   \
   V(PropertyCell, promise_species_protector, PromiseSpeciesProtector)          \
+  V(PropertyCell, regexp_species_protector, RegExpSpeciesProtector)            \
   V(PropertyCell, string_length_protector, StringLengthProtector)              \
   V(PropertyCell, array_iterator_protector, ArrayIteratorProtector)            \
   V(PropertyCell, array_buffer_detaching_protector,                            \

test/mjsunit/regress/regress-1069964.js

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+Realm.createAllowCrossRealmAccess();
+const c = Realm.global(1);
+Realm.detachGlobal(1);
+try { c.constructor = () => {}; } catch {}

tools/v8heapconst.py

@@ -417,19 +417,20 @@ KNOWN_OBJECTS = {
   ("old_space", 0x004e1): "ArraySpeciesProtector",
   ("old_space", 0x004f5): "TypedArraySpeciesProtector",
   ("old_space", 0x00509): "PromiseSpeciesProtector",
-  ("old_space", 0x0051d): "StringLengthProtector",
-  ("old_space", 0x00531): "ArrayIteratorProtector",
-  ("old_space", 0x00545): "ArrayBufferDetachingProtector",
-  ("old_space", 0x00559): "PromiseHookProtector",
-  ("old_space", 0x0056d): "PromiseResolveProtector",
-  ("old_space", 0x00581): "MapIteratorProtector",
-  ("old_space", 0x00595): "PromiseThenProtector",
-  ("old_space", 0x005a9): "SetIteratorProtector",
-  ("old_space", 0x005bd): "StringIteratorProtector",
-  ("old_space", 0x005d1): "SingleCharacterStringCache",
-  ("old_space", 0x009d9): "StringSplitCache",
-  ("old_space", 0x00de1): "RegExpMultipleCache",
-  ("old_space", 0x011e9): "BuiltinsConstantsTable",
+  ("old_space", 0x0051d): "RegExpSpeciesProtector",
+  ("old_space", 0x00531): "StringLengthProtector",
+  ("old_space", 0x00545): "ArrayIteratorProtector",
+  ("old_space", 0x00559): "ArrayBufferDetachingProtector",
+  ("old_space", 0x0056d): "PromiseHookProtector",
+  ("old_space", 0x00581): "PromiseResolveProtector",
+  ("old_space", 0x00595): "MapIteratorProtector",
+  ("old_space", 0x005a9): "PromiseThenProtector",
+  ("old_space", 0x005bd): "SetIteratorProtector",
+  ("old_space", 0x005d1): "StringIteratorProtector",
+  ("old_space", 0x005e5): "SingleCharacterStringCache",
+  ("old_space", 0x009ed): "StringSplitCache",
+  ("old_space", 0x00df5): "RegExpMultipleCache",
+  ("old_space", 0x011fd): "BuiltinsConstantsTable",
 }
 
 # Lower 32 bits of first page addresses for various heap spaces.