[compiler] Fix invalid MakeRef use in JSArrayRef::length_unsafe()

ripsawridge · V8 LUCI CQ · commit cc6d54c3ddfb · 2021-10-04T09:03:21.000Z
Since we are reading an Object field, it could be that the gc predicate fails. Therefore, this CL changes to TryMakeRef, and makes the return value of length_unsafe() optional. Bug: v8:7790, v8:12282 Change-Id: I86a8bcc6649d5e8121e52f8947b8331fcf242887 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3200078 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Commit-Queue: Michael Stanton <mvstanton@chromium.org> Cr-Commit-Position: refs/heads/main@{#77209}
diff --git a/src/compiler/heap-refs.cc b/src/compiler/heap-refs.cc
@@ -2456,13 +2456,14 @@ ObjectRef JSArrayRef::GetBoilerplateLength() const {
   // Safe to read concurrently because:
   // - boilerplates are immutable after initialization.
   // - boilerplates are published into the feedback vector.
-  return length_unsafe();
+  // These facts also mean we can expect a valid value.
+  return length_unsafe().value();
 }
 
-ObjectRef JSArrayRef::length_unsafe() const {
+base::Optional<ObjectRef> JSArrayRef::length_unsafe() const {
   if (data_->should_access_heap() || broker()->is_concurrent_inlining()) {
-    return MakeRef(broker(),
-                   object()->length(broker()->isolate(), kRelaxedLoad));
+    return TryMakeRef(broker(),
+                      object()->length(broker()->isolate(), kRelaxedLoad));
   } else {
     return ObjectRef{broker(), data()->AsJSArray()->length()};
   }
@@ -2491,14 +2492,16 @@ base::Optional<ObjectRef> JSArrayRef::GetOwnCowElement(
   // `elements`. We rely on the invariant that any `length` change will
   // also result in an `elements` change to make this safe. The `elements`
   // consistency check in the caller thus also guards the value of `length`.
-  ObjectRef length_ref = length_unsafe();
+  base::Optional<ObjectRef> length_ref = length_unsafe();
+
+  if (!length_ref.has_value()) return {};
 
   // Likewise we only deal with smi lengths.
-  if (!length_ref.IsSmi()) return {};
+  if (!length_ref->IsSmi()) return {};
 
   base::Optional<Object> result = ConcurrentLookupIterator::TryGetOwnCowElement(
       broker()->isolate(), *elements_ref.AsFixedArray().object(), elements_kind,
-      length_ref.AsSmi(), index);
+      length_ref->AsSmi(), index);
   if (!result.has_value()) return {};
 
   return TryMakeRef(broker(), result.value());
diff --git a/src/compiler/heap-refs.h b/src/compiler/heap-refs.h
@@ -866,8 +866,9 @@ class JSArrayRef : public JSObjectRef {
 
   // The `JSArray::length` property; not safe to use in general, but can be
   // used in some special cases that guarantee a valid `length` value despite
-  // concurrent reads.
-  ObjectRef length_unsafe() const;
+  // concurrent reads. The result needs to be optional in case the
+  // return value was created too recently to pass the gc predicate.
+  base::Optional<ObjectRef> length_unsafe() const;
 };
 
 class ScopeInfoRef : public HeapObjectRef {
