commit e767dca0a69ae25121f83e21e736c66d1b4ef1f4
author Martin Kustermann <kustermann@google.com> Wed Jan 12 08:30:42 2022 +0000
committer Commit Bot <commit-bot@chromium.org> Wed Jan 12 08:30:42 2022 +0000
tree 88d599cbcc3c88144cab55cdb4c3cef5f39db28c
parent 1bf93d73a1076223dad79c9af9113de368089436

[vm] Make stack trace generation code more conservative

If the _FutureListener.handleValue function gets deoptimized at a point
when the `this` is no longer needed/live the deoptimization procedure
may override the `this` slot in the caller frame with "<optimized out>".

This has been triggered on the "iso-stress" builder due to using VM
testing flags that cause many deoptimizations in many places.

The stack walking code should take this possibility into consideration
and be therefore conservative.

Alternatives considered:

  * Use "copy parameters" into our own frame approach - like we do
    with methods that  have optional parameters
    => This will make all unoptimized code slower.

  * Add a reachability fence to the _FutureListener.handleValue function
    => The reachabilityFench is only available in dart:_internal for VM
    => If the function could be debugged after fence but before return
       one might be able to trigger the issue nonetheless.

Closes https://github.com/dart-lang/sdk/issues/46823

TEST=Should remove flaky crashes on "iso-stress" builder.

Change-Id: I9d953706b292e02d2ba75fd794c872865faf45d0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/227401
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>

runtime/vm/stack_trace.cc

diff --git a/runtime/vm/stack_trace.cc b/runtime/vm/stack_trace.cc
index 1f29e90..583e055 100644
--- a/runtime/vm/stack_trace.cc
+++ b/runtime/vm/stack_trace.cc
@@ -395,6 +395,10 @@
   if (function.IsAsyncClosure() || function.IsAsyncGenClosure()) {
     // Next, look up caller's closure on the stack and walk backwards
     // through the yields.
+    //
+    // Due the async/async* closures having optional parameters, the
+    // caller-frame's pushed arguments includes the closure and should never be
+    // modified (even in the event of deopts).
     ObjectPtr* last_caller_obj =
         reinterpret_cast<ObjectPtr*>(frame->GetCallerSp());
     closure = FindClosureInFrame(last_caller_obj, function);
@@ -432,6 +436,12 @@
     Object& receiver =
         Object::Handle(*(reinterpret_cast<ObjectPtr*>(frame->GetCallerSp()) +
                          kNumArgsFutureListenerHandleValue));
+    if (receiver.ptr() == Symbols::OptimizedOut().ptr()) {
+      // In the very rare case that _FutureListener.handleValue has deoptimized
+      // it may override the receiver slot in the caller frame with "<optimized
+      // out>" due to the `this` no longer being needed.
+      return Closure::null();
+    }
 
     return caller_closure_finder->GetCallerInFutureListener(receiver);
   }
@@ -579,6 +589,9 @@
         function.parent_function() != Function::null()) {
       if (async_function.ptr() == function.parent_function()) {
         if (function.IsAsyncClosure() || function.IsAsyncGenClosure()) {
+          // Due the async/async* closures having optional parameters, the
+          // caller-frame's pushed arguments includes the closure and should
+          // never be modified (even in the event of deopts).
           ObjectPtr* last_caller_obj =
               reinterpret_cast<ObjectPtr*>(frame->GetCallerSp());
           closure = FindClosureInFrame(last_caller_obj, function);