Wrong context for closure in finally with await in try.

[zanderso]

The program below should print 5.0 forever. However, after doSync is optimized, the closure passed to doPrint apparently has the wrong context, and extracts a bogus value for next.

Invoke with, e.g.:

./out/DebugX64/dart --optimization-counter-threshold=10 --no-background-compilation ../duration_test.dart

The bug can be worked around by pulling out the finally block of doSync into its own function, wrapped in a try so that we don't try to inline it.

import 'dart:async';
import 'dart:io';

void doPrint(Function f) {
  print(f());
}

foo() {
  try {
    var next = 5.0;
    doPrint(() {
      if (next != 5.0) {
        print("next was crazy!");
        exit(-1);
      }
      return next;
    });
  } finally {}
}

Future doSync() async {
   try {
     await 123;
  } finally {
    var next = 5.0;
    doPrint(() {
      if (next != 5.0) {
        print("next was crazy!");
        exit(-1);
      }
      return next;
    });
    //foo();
  }
}

main() async {
  int seconds = 0;
  while (true) {
    await doSync();
  }
}

[johnmccutchan]

Might be related:

#26941

[fsc8000]

It seems to happen after doSync gets optimized. I used the following command using Zach's repro and optimization-filter:

out/ReleaseX64/dart --print-flow-graph-optimized --no-background-compilation --optimization-filter=doSync --print-scopes --print-ast fail.dart

[mhausner]

Two observations:

1. The CL that changed how and when nested functions are parsed (and variables are captured) is not the culprit. The bug exists before that change (https://codereview.chromium.org/2010283004/).

2. If variable next is declared outside the finally block, the crash does not happen.

[mit-mit]

The most recent comment by hixie in the source bug says this is likely not related.

[crelier]

I think that the culprit is this cl: https://codereview.chromium.org/1569523002

It introduced a reparsing of finally clauses to fix a bug with try-catch inside of try-finally.
That is a pretty big hammer to sort out try indices. Reparsing has all kind of non-desirable side effects, e.g. allocating local variables a second time in different local scopes. In this case, we have a closure in the finally clause. It gets parsed twice. The captured variable 'next' ends up allocated twice. Depending on the existence of a context in a enclosing scope or not, it is allocated twice in the same context, or in two different locally allocated contexts. Parsing the closure twice will result in two preserved outer scopes. However, the closure itself is not duplicated, because it has the same anonymous name at the same token index. So the same outer context will be referred to from both finally clauses, which is not correct, unless they happen to share the same outer context containing 'next'.

I think that the proper fix here is to revisit the cl above and avoid reparsing finally clauses.

What do you think?

[mhausner]

Reparsing finally clauses is definitely the issue here. The CL you reference did not introduce the reparsing. That was introcuded even before the begin of history in this repo.

Your observation about creating multiple instances of the same variable is true. In non-async code it's harmless, because each copy of the finally block code accesses only its own copy of the variable. In async code, each instance of the variable gets saved to a different context slot. (That by itself should not be a problem either.)

However, we also get multiple instances of the anonymous closure. Why? Because the compiler only registers the closure function once the backend compiles the closure node. So, each time the finally block gets reparsed, a new anonymous closure function is created because no function for that token position has been registered yet. I'm not sure yet whether that is a correctness problem.

I have observed though that in the sample code above, the VM compiles two different anonymous functions for the literal in the finally clause, and they do not agree in which context slot the 'next' variable is stored.

I agree that the right fix is to avoid reparsing the code in the finally block. Maybe the AST for the finally code can be shared, or it has to be cloned in a different way.

[whesse]

Because this issue is already in earlier stable releases, and does not have an immediate fix, I am not considering it a blocker for 1.18, and removing the milestone. Let me know if you disagree, or have a fix that should be put in a patch release.

[mit-mit]

I agree with this approach. Once we have a fix, we can consider if that meets the bar for a merge to stable patch release.