Forward exceptions while using DebugPropertyIterator

The V8 inspector is using the DebugPropertyIterator (a debug only
interface) while building RemoteObjects. The DebugPropertyIterator
uses the `KeyAccumulator::GetKeys` for this, which can potentially
throw, but the DebugPropertyIterator ignores exceptions and keeps
iterating. If multiple iteration steps throw an exception
(e.g. due to a pending stack overflow), we run into a CHECK in
Isolate::Throw, as we can't throw exceptions while another
exception is still pending.

This CL fixes the CHECK crash by properly propagating exceptions
after the iterator is created or advanced and returning early
in the inspector if an exception happens.

Please note that the regression test that showcases this behavior
is still disabled, as fixing the crash causes currently an
endless loop. While the exception in `ValueMirror::getProperties`
is handled by early returing, we still need to forward it as
the result of the `Runtime::evaluate` all the way up the stack.

R=bmeurer@chromium.org, yangguo@chromium.org

Bug: chromium:1080638
Change-Id: I1d55e0d70490a06a6bc1b0a3525236411da7f64b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2639954Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72203}

src/api/api.cc

@@ -36,6 +36,7 @@
 #include "src/date/date.h"
 #include "src/debug/debug-coverage.h"
 #include "src/debug/debug-evaluate.h"
+#include "src/debug/debug-property-iterator.h"
 #include "src/debug/debug-type-profile.h"
 #include "src/debug/debug.h"
 #include "src/debug/liveedit.h"
@@ -11333,6 +11334,24 @@ RegisterState& RegisterState::operator=(const RegisterState& other)
   return *this;
 }
 
+std::unique_ptr<debug::PropertyIterator> debug::PropertyIterator::Create(
+    Local<Context> context, Local<Object> object) {
+  internal::Isolate* isolate =
+      reinterpret_cast<internal::Isolate*>(object->GetIsolate());
+  if (IsExecutionTerminatingCheck(isolate)) {
+    return nullptr;
+  }
+  CallDepthScope<false> call_depth_scope(isolate, context);
+
+  auto result = internal::DebugPropertyIterator::Create(
+      isolate, Utils::OpenHandle(*object));
+  if (!result) {
+    DCHECK(isolate->has_pending_exception());
+    call_depth_scope.Escape();
+  }
+  return result;
+}
+
 namespace internal {
 
 const size_t HandleScopeImplementer::kEnteredContextsOffset =
@@ -11513,6 +11532,22 @@ void InvokeFinalizationRegistryCleanupFromTask(
   }
 }
 
+Maybe<bool> DebugPropertyIterator::Advance() {
+  if (IsExecutionTerminatingCheck(isolate_)) {
+    return Nothing<bool>();
+  }
+  Local<v8::Context> context =
+      Utils::ToLocal(handle(isolate_->context(), isolate_));
+  CallDepthScope<false> call_depth_scope(isolate_, context);
+
+  if (!AdvanceInternal()) {
+    DCHECK(isolate_->has_pending_exception());
+    call_depth_scope.Escape();
+    return Nothing<bool>();
+  }
+  return Just(true);
+}
+
 // Undefine macros for jumbo build.
 #undef LOG_API
 #undef ENTER_V8_DO_NOT_USE

src/debug/debug-interface.h

@@ -592,12 +592,17 @@ struct PropertyDescriptor {
 
 class PropertyIterator {
  public:
-  static std::unique_ptr<PropertyIterator> Create(v8::Local<v8::Object> object);
+  // Creating a PropertyIterator can potentially throw an exception.
+  // The returned std::unique_ptr is empty iff that happens.
+  V8_WARN_UNUSED_RESULT static std::unique_ptr<PropertyIterator> Create(
+      v8::Local<v8::Context> context, v8::Local<v8::Object> object);
 
   virtual ~PropertyIterator() = default;
 
   virtual bool Done() const = 0;
-  virtual void Advance() = 0;
+  // Returns |Nothing| should |Advance| throw an exception,
+  // |true| otherwise.
+  V8_WARN_UNUSED_RESULT virtual Maybe<bool> Advance() = 0;
 
   virtual v8::Local<v8::Name> name() const = 0;
 

src/debug/debug-property-iterator.cc

@@ -12,37 +12,39 @@
 #include "src/objects/property-details.h"
 
 namespace v8 {
+namespace internal {
 
-std::unique_ptr<debug::PropertyIterator> debug::PropertyIterator::Create(
-    v8::Local<v8::Object> v8_object) {
-  internal::Isolate* isolate =
-      reinterpret_cast<internal::Isolate*>(v8_object->GetIsolate());
-  return std::unique_ptr<debug::PropertyIterator>(
-      new internal::DebugPropertyIterator(isolate,
-                                          Utils::OpenHandle(*v8_object)));
-}
+std::unique_ptr<DebugPropertyIterator> DebugPropertyIterator::Create(
+    Isolate* isolate, Handle<JSReceiver> receiver) {
+  // Can't use std::make_unique as Ctor is private.
+  auto iterator = std::unique_ptr<DebugPropertyIterator>(
+      new DebugPropertyIterator(isolate, receiver));
 
-namespace internal {
+  if (receiver->IsJSProxy()) {
+    iterator->is_own_ = false;
+    iterator->prototype_iterator_.AdvanceIgnoringProxies();
+  }
+  if (iterator->prototype_iterator_.IsAtEnd()) return iterator;
+
+  if (!iterator->FillKeysForCurrentPrototypeAndStage()) return nullptr;
+  if (iterator->should_move_to_next_stage() && !iterator->AdvanceInternal()) {
+    return nullptr;
+  }
+
+  return iterator;
+}
 
 DebugPropertyIterator::DebugPropertyIterator(Isolate* isolate,
                                              Handle<JSReceiver> receiver)
     : isolate_(isolate),
       prototype_iterator_(isolate, receiver, kStartAtReceiver,
-                          PrototypeIterator::END_AT_NULL) {
-  if (receiver->IsJSProxy()) {
-    is_own_ = false;
-    prototype_iterator_.AdvanceIgnoringProxies();
-  }
-  if (prototype_iterator_.IsAtEnd()) return;
-  FillKeysForCurrentPrototypeAndStage();
-  if (should_move_to_next_stage()) Advance();
-}
+                          PrototypeIterator::END_AT_NULL) {}
 
 bool DebugPropertyIterator::Done() const {
   return prototype_iterator_.IsAtEnd();
 }
 
-void DebugPropertyIterator::Advance() {
+bool DebugPropertyIterator::AdvanceInternal() {
   ++current_key_index_;
   calculated_native_accessor_flags_ = false;
   while (should_move_to_next_stage()) {
@@ -59,8 +61,9 @@ void DebugPropertyIterator::Advance() {
         prototype_iterator_.AdvanceIgnoringProxies();
         break;
     }
-    FillKeysForCurrentPrototypeAndStage();
+    if (!FillKeysForCurrentPrototypeAndStage()) return false;
   }
+  return true;
 }
 
 bool DebugPropertyIterator::is_native_accessor() {
@@ -138,19 +141,19 @@ bool DebugPropertyIterator::is_array_index() {
   return raw_name()->AsArrayIndex(&index);
 }
 
-void DebugPropertyIterator::FillKeysForCurrentPrototypeAndStage() {
+bool DebugPropertyIterator::FillKeysForCurrentPrototypeAndStage() {
   current_key_index_ = 0;
   exotic_length_ = 0;
   keys_ = Handle<FixedArray>::null();
-  if (prototype_iterator_.IsAtEnd()) return;
+  if (prototype_iterator_.IsAtEnd()) return true;
   Handle<JSReceiver> receiver =
       PrototypeIterator::GetCurrent<JSReceiver>(prototype_iterator_);
   bool has_exotic_indices = receiver->IsJSTypedArray();
   if (stage_ == kExoticIndices) {
-    if (!has_exotic_indices) return;
+    if (!has_exotic_indices) return true;
     Handle<JSTypedArray> typed_array = Handle<JSTypedArray>::cast(receiver);
     exotic_length_ = typed_array->WasDetached() ? 0 : typed_array->length();
-    return;
+    return true;
   }
   bool skip_indices = has_exotic_indices;
   PropertyFilter filter =
@@ -160,7 +163,9 @@ void DebugPropertyIterator::FillKeysForCurrentPrototypeAndStage() {
                                skip_indices)
            .ToHandle(&keys_)) {
     keys_ = Handle<FixedArray>::null();
+    return false;
   }
+  return true;
 }
 
 bool DebugPropertyIterator::should_move_to_next_stage() const {

src/debug/debug-property-iterator.h

@@ -19,13 +19,14 @@ class JSReceiver;
 
 class DebugPropertyIterator final : public debug::PropertyIterator {
  public:
-  DebugPropertyIterator(Isolate* isolate, Handle<JSReceiver> receiver);
+  V8_WARN_UNUSED_RESULT static std::unique_ptr<DebugPropertyIterator> Create(
+      Isolate* isolate, Handle<JSReceiver> receiver);
   ~DebugPropertyIterator() override = default;
   DebugPropertyIterator(const DebugPropertyIterator&) = delete;
   DebugPropertyIterator& operator=(const DebugPropertyIterator&) = delete;
 
   bool Done() const override;
-  void Advance() override;
+  V8_WARN_UNUSED_RESULT Maybe<bool> Advance() override;
 
   v8::Local<v8::Name> name() const override;
   bool is_native_accessor() override;
@@ -38,10 +39,13 @@ class DebugPropertyIterator final : public debug::PropertyIterator {
   bool is_array_index() override;
 
  private:
-  void FillKeysForCurrentPrototypeAndStage();
+  DebugPropertyIterator(Isolate* isolate, Handle<JSReceiver> receiver);
+
+  V8_WARN_UNUSED_RESULT bool FillKeysForCurrentPrototypeAndStage();
   bool should_move_to_next_stage() const;
   void CalculateNativeAccessorFlags();
   Handle<Name> raw_name() const;
+  V8_WARN_UNUSED_RESULT bool AdvanceInternal();
 
   Isolate* isolate_;
   PrototypeIterator prototype_iterator_;

src/inspector/value-mirror.cc

@@ -1227,14 +1227,24 @@ bool ValueMirror::getProperties(v8::Local<v8::Context> context,
 
   bool formatAccessorsAsProperties =
       clientFor(context)->formatAccessorsAsProperties(object);
-  for (auto iterator = v8::debug::PropertyIterator::Create(object);
-       !iterator->Done(); iterator->Advance()) {
+  auto iterator = v8::debug::PropertyIterator::Create(context, object);
+  if (!iterator) {
+    CHECK(tryCatch.HasCaught());
+    return false;
+  }
+  while (!iterator->Done()) {
     bool isOwn = iterator->is_own();
     if (!isOwn && ownProperties) break;
     v8::Local<v8::Name> v8Name = iterator->name();
     v8::Maybe<bool> result = set->Has(context, v8Name);
     if (result.IsNothing()) return false;
-    if (result.FromJust()) continue;
+    if (result.FromJust()) {
+      if (!iterator->Advance().FromMaybe(false)) {
+        CHECK(tryCatch.HasCaught());
+        return false;
+      }
+      continue;
+    }
     if (!set->Add(context, v8Name).ToLocal(&set)) return false;
 
     String16 name;
@@ -1330,6 +1340,11 @@ bool ValueMirror::getProperties(v8::Local<v8::Context> context,
                                  std::move(symbolMirror),
                                  std::move(exceptionMirror)};
     if (!accumulator->Add(std::move(mirror))) return true;
+
+    if (!iterator->Advance().FromMaybe(false)) {
+      CHECK(tryCatch.HasCaught());
+      return false;
+    }
   }
   if (!shouldSkipProto && ownProperties && !object->IsProxy() &&
       !accessorPropertiesOnly) {

test/inspector/inspector.status

@@ -19,6 +19,11 @@
   # This test worked in the wasm interpreter, but fails when using Liftoff for
   # debugging.
   'debugger/wasm-externref-global': [FAIL],
+
+  # https://crbug.com/1080638
+  # The initial CL only fixed the crash. The test still causes an endless
+  # loop instead of properly reporting a RangeError for a stack overflow.
+  'regress/regress-crbug-1080638': [SKIP],
 }],  # ALWAYS
 
 ##############################################################################

test/inspector/regress/regress-crbug-1080638.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.
+
+const {Protocol} = InspectorTest.start('Recursive proxy prototype does not crash inspector crbug.com/1080638');
+
+const reproductionCode = `
+const t = { id: 1 }
+const p = new Proxy(t, {
+  get(target, prop, receiver) {
+    console.log(receiver);
+    return Reflect.get(target, prop);
+  }
+});
+
+const q = Object.create(p);
+console.log(q.id);
+`;
+
+(async function logPropertyWithProxyPrototype() {
+  await Protocol.Runtime.enable();
+  const response = await Protocol.Runtime.evaluate({
+    expression: reproductionCode,
+    replMode: true,
+  });
+  InspectorTest.logMessage(response);
+  InspectorTest.completeTest();
+})();