[vm] Improve detection of handled async errors

mraleph · Commit Queue · commit 2280b06202b5 · 2024-02-16T12:55:22.000Z
Improve the handling of code where the async flow forks: e.g. for `fut.whenComplete(c.complete)` we might proceed unwinding through awaiters of `c.future` and forget about the future returned from `whenComplete`. This happens because we choose to present result of the unwinding as a single stack and not a tree. In this situation the error will only propagate into that future and whether or not the error will be handled depends on that future alone. As part of this change also start respecting ignored bit on futures without listeners. TEST=pkg/vm_service/test/pause_on_unhandled_async_exceptions7_test.dart CoreLibraryReviewExempt: Comment only changes to core library Change-Id: I27b689ab07a725e8faa8d91cf40e88ebc8c441a0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352904 Reviewed-by: Martin Kustermann <kustermann@google.com> Reviewed-by: Lasse Nielsen <lrn@google.com> Commit-Queue: Slava Egorov <vegorov@google.com>
diff --git a/pkg/vm_service/test/pause_on_unhandled_async_exceptions7_test.dart b/pkg/vm_service/test/pause_on_unhandled_async_exceptions7_test.dart
@@ -20,7 +20,7 @@ import 'pause_on_unhandled_async_exceptions6_test.dart' as test6;
 
 Future<int> alwaysThrow() async {
   // Ensure that we suspend at least once and throw an error asynchronously.
-  await Future.delayed(const Duration(milliseconds: 10));
+  await Future.delayed(Duration.zero);
   print(StackTrace.current);
   throw 'Error';
 }
@@ -70,6 +70,24 @@ Future<void> throwSomeCaughtAsyncErrors() async {
   } finally {
     client.close();
   }
+
+  {
+    print('ignoring an error (1)');
+    final c = Completer<void>();
+    // Here async unwinder might follow awaiter chain to `await c.future`
+    // because it detects forwarding automatically.
+    alwaysThrow().whenComplete(c.complete).ignore();
+    await c.future;
+  }
+
+  {
+    print('ignoring an error (2)');
+    final c = Completer<void>();
+    alwaysThrow().whenComplete(() {
+      c.complete();
+    }).ignore();
+    await c.future;
+  }
 }
 
 Future<void> testeeMain() async {
diff --git a/runtime/vm/stack_trace.cc b/runtime/vm/stack_trace.cc
@@ -22,6 +22,8 @@ const intptr_t k_StreamController__STATE_SUBSCRIPTION_MASK = 3;
 const intptr_t k_StreamController__STATE_ADDSTREAM = 8;
 // - _BufferingStreamSubscription._STATE_HAS_ERROR_HANDLER.
 const intptr_t k_BufferingStreamSubscription__STATE_HAS_ERROR_HANDLER = 1 << 5;
+// - _Future._stateIgnoreError
+const intptr_t k_Future__stateIgnoreError = 1;
 // - _FutureListener.stateThen.
 const intptr_t k_FutureListener_stateThen = 1;
 // - _FutureListener.stateCatchError.
@@ -79,31 +81,34 @@ bool TryGetAwaiterLink(const Closure& closure, Object* link) {
 // of awaiters.
 class AsyncAwareStackUnwinder : public ValueObject {
  public:
-  explicit AsyncAwareStackUnwinder(Thread* thread)
+  explicit AsyncAwareStackUnwinder(Thread* thread,
+                                   bool* encountered_async_catch_error)
       : zone_(thread->zone()),
         sync_frames_(thread, StackFrameIterator::kNoCrossThreadIteration),
         sync_frame_(nullptr),
         awaiter_frame_{Closure::Handle(zone_), Object::Handle(zone_)},
+        encountered_async_catch_error_(encountered_async_catch_error),
         closure_(Closure::Handle(zone_)),
         code_(Code::Handle(zone_)),
         context_(Context::Handle(zone_)),
         function_(Function::Handle(zone_)),
         parent_function_(Function::Handle(zone_)),
         object_(Object::Handle(zone_)),
+        result_future_(Object::Handle(zone_)),
         suspend_state_(SuspendState::Handle(zone_)),
         controller_(Object::Handle(zone_)),
         subscription_(Object::Handle(zone_)),
         stream_iterator_(Object::Handle(zone_)),
         async_lib_(Library::Handle(zone_, Library::AsyncLibrary())),
-        null_closure_(Closure::Handle(zone_)) {}
+        null_closure_(Closure::Handle(zone_)) {
+    if (encountered_async_catch_error_ != nullptr) {
+      *encountered_async_catch_error_ = false;
+    }
+  }
 
   void Unwind(intptr_t skip_frames,
               std::function<void(const StackTraceUtils::Frame&)> handle_frame);
 
-  bool encountered_async_catch_error() const {
-    return encountered_async_catch_error_;
-  }
-
  private:
   bool HandleSynchronousFrame();
 
@@ -137,6 +142,28 @@ class AsyncAwareStackUnwinder : public ValueObject {
 
   ObjectPtr GetReceiver() const;
 
+  // Returns |true| if propagating an error to the listeners of this `_Future`
+  // will always encounter an error handler. Future handles error iff:
+  //
+  // * It has no listeners and is marked as ignoring errors
+  // * All of its listeners either have an error handler or corresponding
+  //   result future handles error.
+  //
+  // Note: this ignores simple error forwarding/rethrow which occurs in `await`
+  // or patterns like `fut.then(onError: c.completeError)`.
+  bool WillFutureHandleError(const Object& future, intptr_t depth = 0);
+
+  void MarkAsHandlingAsyncError() const {
+    if (ShouldComputeIfAsyncErrorIsHandled()) {
+      *encountered_async_catch_error_ = true;
+    }
+  }
+
+  bool ShouldComputeIfAsyncErrorIsHandled() const {
+    return encountered_async_catch_error_ != nullptr &&
+           !*encountered_async_catch_error_;
+  }
+
 #define USED_CLASS_LIST(V)                                                     \
   V(_AsyncStarStreamController)                                                \
   V(_BufferingStreamSubscription)                                              \
@@ -167,9 +194,11 @@ class AsyncAwareStackUnwinder : public ValueObject {
   V(_BufferingStreamSubscription, _state)                                      \
   V(_Completer, future)                                                        \
   V(_Future, _resultOrListeners)                                               \
+  V(_Future, _state)                                                           \
   V(_FutureListener, callback)                                                 \
   V(_FutureListener, result)                                                   \
   V(_FutureListener, state)                                                    \
+  V(_FutureListener, _nextListener)                                            \
   V(_StreamController, _state)                                                 \
   V(_StreamController, _varData)                                               \
   V(_StreamControllerAddStreamState, _varData)                                 \
@@ -226,14 +255,15 @@ class AsyncAwareStackUnwinder : public ValueObject {
   StackFrame* sync_frame_;
   AwaiterFrame awaiter_frame_;
 
-  bool encountered_async_catch_error_ = false;
+  bool* encountered_async_catch_error_;
 
   Closure& closure_;
   Code& code_;
   Context& context_;
   Function& function_;
   Function& parent_function_;
   Object& object_;
+  Object& result_future_;
   SuspendState& suspend_state_;
 
   Object& controller_;
@@ -423,11 +453,47 @@ void AsyncAwareStackUnwinder::UnwindFrameToFutureListener() {
   if (object_.GetClassId() == _FutureListener().id()) {
     InitializeAwaiterFrameFromFutureListener(object_);
   } else {
+    if (ShouldComputeIfAsyncErrorIsHandled()) {
+      // Check if error on this future was ignored through |Future.ignore|.
+      const auto state =
+          Smi::Value(Smi::RawCast(Get_Future__state(awaiter_frame_.next)));
+      if ((state & k_Future__stateIgnoreError) != 0) {
+        MarkAsHandlingAsyncError();
+      }
+    }
+
     awaiter_frame_.closure = Closure::null();
     awaiter_frame_.next = Object::null();
   }
 }
 
+bool AsyncAwareStackUnwinder::WillFutureHandleError(const Object& future,
+                                                    intptr_t depth /* = 0 */) {
+  if (depth > 100 || future.GetClassId() != _Future().id()) {
+    return true;  // Conservative.
+  }
+
+  if (Get_Future__resultOrListeners(future) == Object::null()) {
+    // No listeners: check if future is simply ignoring errors.
+    const auto state = Smi::Value(Smi::RawCast(Get_Future__state(future)));
+    return (state & k_Future__stateIgnoreError) != 0;
+  }
+
+  for (auto& listener = Object::Handle(Get_Future__resultOrListeners(future));
+       listener.GetClassId() == _FutureListener().id();
+       listener = Get_FutureListener__nextListener(listener)) {
+    const auto state =
+        Smi::Value(Smi::RawCast(Get_FutureListener_state(listener)));
+    if ((state & k_FutureListener_stateCatchError) == 0 &&
+        !WillFutureHandleError(
+            Object::Handle(Get_FutureListener_result(listener)), depth + 1)) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
 void AsyncAwareStackUnwinder::InitializeAwaiterFrameFromFutureListener(
     const Object& listener) {
   if (listener.GetClassId() != _FutureListener().id()) {
@@ -445,15 +511,30 @@ void AsyncAwareStackUnwinder::InitializeAwaiterFrameFromFutureListener(
           (k_FutureListener_stateThen | k_FutureListener_stateCatchError) ||
       state == (k_FutureListener_stateThen | k_FutureListener_stateCatchError |
                 k_FutureListener_maskAwait)) {
-    awaiter_frame_.next = Get_FutureListener_result(listener);
+    result_future_ = Get_FutureListener_result(listener);
   } else {
-    awaiter_frame_.next = Object::null();
+    result_future_ = Object::null();
   }
   awaiter_frame_.closure =
       Closure::RawCast(Get_FutureListener_callback(listener));
+  awaiter_frame_.next = result_future_.ptr();
 
   ComputeNextFrameFromAwaiterLink();
 
+  if (ShouldComputeIfAsyncErrorIsHandled() && !result_future_.IsNull() &&
+      result_future_.ptr() != awaiter_frame_.next.ptr()) {
+    // We have unwound through closure rather than followed result future, this
+    // can be caused by unwinding through `await` or code like
+    // `fut.whenComplete(c.complete)`. If the current future does not
+    // catch the error then the error will be forwarded into result_future_.
+    // Check if result_future_ handles it and set
+    // encountered_async_catch_error_ respectively.
+    if ((state & k_FutureListener_stateCatchError) == 0 &&
+        WillFutureHandleError(result_future_)) {
+      MarkAsHandlingAsyncError();
+    }
+  }
+
   // If the Future has catchError callback attached through either
   // `Future.catchError` or `Future.then(..., onError: ...)` then we should
   // treat this listener as a catch all exception handler. However we should
@@ -468,7 +549,7 @@ void AsyncAwareStackUnwinder::InitializeAwaiterFrameFromFutureListener(
   if ((state & k_FutureListener_stateCatchError) != 0 &&
       ((state & k_FutureListener_maskAwait) == 0 ||
        !awaiter_frame_.next.IsSuspendState())) {
-    encountered_async_catch_error_ = true;
+    MarkAsHandlingAsyncError();
   }
 }
 
@@ -531,7 +612,7 @@ void AsyncAwareStackUnwinder::UnwindFrameToStreamController() {
       if (Get_StreamIterator__hasValue(stream_iterator_) ==
           Object::bool_true().ptr()) {
         if (has_error_handler) {
-          encountered_async_catch_error_ = true;
+          MarkAsHandlingAsyncError();
         }
         return;
       }
@@ -546,7 +627,7 @@ void AsyncAwareStackUnwinder::UnwindFrameToStreamController() {
         awaiter_frame_.next = object_.ptr();
       } else {
         if (has_error_handler) {
-          encountered_async_catch_error_ = true;
+          MarkAsHandlingAsyncError();
         }
       }
       return;
@@ -611,7 +692,7 @@ void AsyncAwareStackUnwinder::UnwindFrameToStreamController() {
 
   if (has_error_handler && object_.GetClassId() != _Future().id() &&
       object_.GetClassId() != _SyncStreamController().id()) {
-    encountered_async_catch_error_ = true;
+    MarkAsHandlingAsyncError();
   }
 
   if (found_awaiter_link_in_sibling_handler) {
@@ -666,11 +747,8 @@ void StackTraceUtils::CollectFrames(
     int skip_frames,
     const std::function<void(const StackTraceUtils::Frame&)>& handle_frame,
     bool* has_async_catch_error /* = null */) {
-  AsyncAwareStackUnwinder it(thread);
+  AsyncAwareStackUnwinder it(thread, has_async_catch_error);
   it.Unwind(skip_frames, handle_frame);
-  if (has_async_catch_error != nullptr) {
-    *has_async_catch_error = it.encountered_async_catch_error();
-  }
 }
 
 bool StackTraceUtils::GetSuspendState(const Closure& closure,
diff --git a/runtime/vm/symbols.h b/runtime/vm/symbols.h
@@ -454,6 +454,7 @@ class ObjectPointerVisitor;
   V(_mapGet, "_mapGet")                                                        \
   V(_mapKeys, "_mapKeys")                                                      \
   V(_name, "_name")                                                            \
+  V(_nextListener, "_nextListener")                                            \
   V(_nativeGetFloat32, "_nativeGetFloat32")                                    \
   V(_nativeSetFloat32, "_nativeSetFloat32")                                    \
   V(_nativeGetFloat64, "_nativeGetFloat64")                                    \
diff --git a/sdk/lib/async/future_impl.dart b/sdk/lib/async/future_impl.dart
@@ -88,6 +88,7 @@ class _FutureListener<S, T> {
       maskValue | maskError | maskTestError | maskWhenComplete;
 
   // Listeners on the same future are linked through this link.
+  @pragma("vm:entry-point")
   _FutureListener? _nextListener;
 
   // The future to complete when this listener is activated.
@@ -223,6 +224,8 @@ class _Future<T> implements Future<T> {
   /// Set by the [FutureExtensions.ignore] method to avoid
   /// having to introduce an unnecessary listener.
   /// Only relevant until the future is completed.
+  ///
+  /// When changing update runtime/vm/stack_trace.cc
   static const int _stateIgnoreError = 1;
 
   /// Pending completion. Set when completed using [_asyncComplete] or
@@ -254,6 +257,7 @@ class _Future<T> implements Future<T> {
   static const int _completionStateMask = 30;
 
   /// Whether the future is complete, and as what.
+  @pragma('vm:entry-point')
   int _state = _stateIncomplete;
 
   /// Zone that the future was completed from.
