From cc80ccfaa0b66a89a04cd07081d9f42bcf3d2bcf Mon Sep 17 00:00:00 2001 From: Anna Gringauze Date: Fri, 4 Nov 2022 21:25:08 +0000 Subject: [PATCH] Fix stack overflow in mixin application from separate library Make sure that calls to super class methods from mixin application stubs are always generated using `super` and not `this`. Replacing `super` by `this` should not be used as non-virtual field access optimization in forwarding stubs that intend to redirect to the super class members (using `this` redirects to itself and causes infinite recursion). Closes: https://github.com/dart-lang/sdk/issues/50119 Change-Id: Ib9c9aebdc88555518998db64aefa1fd0905c5b11 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/267822 Reviewed-by: Nicholas Shahan Commit-Queue: Anna Gringauze --- pkg/dev_compiler/lib/src/kernel/compiler.dart | 25 +++++++++- .../mixin/mixin_application_test.dart | 40 ++++++++++++++++ tests/dartdevc/mixin/mixin_declaration.dart | 8 ++++ .../mixin_declaration_application_test.dart | 44 ++++++++++++++++++ .../mixin/mixin_application_test.dart | 42 +++++++++++++++++ tests/dartdevc_2/mixin/mixin_declaration.dart | 10 ++++ .../mixin_declaration_application_test.dart | 46 +++++++++++++++++++ 7 files changed, 213 insertions(+), 2 deletions(-) create mode 100644 tests/dartdevc/mixin/mixin_application_test.dart create mode 100644 tests/dartdevc/mixin/mixin_declaration.dart create mode 100644 tests/dartdevc/mixin/mixin_declaration_application_test.dart create mode 100644 tests/dartdevc_2/mixin/mixin_application_test.dart create mode 100644 tests/dartdevc_2/mixin/mixin_declaration.dart create mode 100644 tests/dartdevc_2/mixin/mixin_declaration_application_test.dart diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index 460a44f00f4a..1829b076c5bc 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -184,6 +184,7 @@ class ProgramCompiler extends ComputeOnceConstantVisitor final JSTypeRep _typeRep; bool _superAllowed = true; + bool _optimizeNonVirtualFieldAccess = true; final _superHelpers = {}; @@ -2008,7 +2009,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor } fn = _emitNativeFunctionBody(member); } else { - fn = _emitFunction(member.function, member.name.text); + fn = _withMethodDeclarationContext( + member, () => _emitFunction(member.function, member.name.text)); } var method = js_ast.Method(_declareMemberName(member), fn, @@ -3660,6 +3662,22 @@ class ProgramCompiler extends ComputeOnceConstantVisitor return result; } + /// Executes [action] in context of the current [member]. + /// + /// Saves and restores important context information about the member + /// that can be used to generate code inside the body of the member. + T _withMethodDeclarationContext(Procedure member, T Function() action) { + // Mixin applications require using 'super' in calls to members of + // the super class. Store this information to disable non-virtual + // super field access optimization when compiling the member body. + var savedOptimizeNonVirtualFieldAccess = _optimizeNonVirtualFieldAccess; + _optimizeNonVirtualFieldAccess = + member.stubKind != ProcedureStubKind.ConcreteMixinStub; + var result = action(); + _optimizeNonVirtualFieldAccess = savedOptimizeNonVirtualFieldAccess; + return result; + } + /// Returns true if the underlying type does not accept a null value. bool _mustBeNonNullable(DartType type) => type.nullability == Nullability.nonNullable; @@ -5485,7 +5503,10 @@ class ProgramCompiler extends ComputeOnceConstantVisitor /// [jsTarget].[jsName], replacing `super` if it is not allowed in scope. js_ast.PropertyAccess _emitSuperTarget(Member member, {bool setter = false}) { var jsName = _emitMemberName(member.name.text, member: member); - if (member is Field && !_virtualFields.isVirtual(member)) { + // Optimize access to non-virtual fields, if allowed in the current context. + if (_optimizeNonVirtualFieldAccess && + member is Field && + !_virtualFields.isVirtual(member)) { return js_ast.PropertyAccess(js_ast.This(), jsName); } if (_superAllowed) return js_ast.PropertyAccess(js_ast.Super(), jsName); diff --git a/tests/dartdevc/mixin/mixin_application_test.dart b/tests/dartdevc/mixin/mixin_application_test.dart new file mode 100644 index 000000000000..610459d3952a --- /dev/null +++ b/tests/dartdevc/mixin/mixin_application_test.dart @@ -0,0 +1,40 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:expect/expect.dart'; +import 'mixin_declaration.dart'; + +void main() { + // Getter `foo` returns `B._foo` that is coming from the mixin declaration. + Expect.equals('B._foo', C0().foo); + Expect.equals('B._foo', D0().foo); + + // When mixin application is in a separate library from the declaration, + // private symbol from the current library is used to access `_foo`. + Expect.equals('A._foo', C()._foo); + Expect.equals('A._foo', D()._foo); + // Getter `foo` returns `B._foo` that is coming from the mixin declaration. + Expect.equals('B._foo', C().foo); + Expect.equals('B._foo', D().foo); + + // E overrides `_foo`. + Expect.equals('E._foo', E()._foo); +} + +class C0 = A0 with B; +class C = A with B; + +class D0 extends A0 with B {} + +class D extends A with B {} + +class E extends A with B { + String? _foo = 'E._foo'; +} + +class A0 {} + +class A { + String? _foo = 'A._foo'; +} diff --git a/tests/dartdevc/mixin/mixin_declaration.dart b/tests/dartdevc/mixin/mixin_declaration.dart new file mode 100644 index 000000000000..23eb22692eba --- /dev/null +++ b/tests/dartdevc/mixin/mixin_declaration.dart @@ -0,0 +1,8 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +class B { + String? _foo = 'B._foo'; + String get foo => _foo!; +} diff --git a/tests/dartdevc/mixin/mixin_declaration_application_test.dart b/tests/dartdevc/mixin/mixin_declaration_application_test.dart new file mode 100644 index 000000000000..e61d48c44909 --- /dev/null +++ b/tests/dartdevc/mixin/mixin_declaration_application_test.dart @@ -0,0 +1,44 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:expect/expect.dart'; + +void main() { + // Getter `foo` returns `B._foo` that is coming from the mixin declaration. + Expect.equals('B._foo', C0().foo); + Expect.equals('B._foo', D0().foo); + + // When mixin application is in the same library as the declaration, + // private symbol from the mixin delaration `B._foo` overrides `A._foo`. + Expect.equals('B._foo', C()._foo); + Expect.equals('B._foo', D()._foo); + // Getter `foo` returns `B._foo` that is coming from the mixin declaration. + Expect.equals('B._foo', C().foo); + Expect.equals('B._foo', D().foo); + + // E overrides `_foo`. + Expect.equals('E._foo', E()._foo); +} + +class C0 = A0 with B; +class C = A with B; + +class D0 extends A0 with B {} + +class D extends A with B {} + +class E extends A with B { + String? _foo = 'E._foo'; +} + +class A0 {} + +class A { + String? _foo = 'A._foo'; +} + +class B { + String? _foo = 'B._foo'; + String get foo => _foo!; +} diff --git a/tests/dartdevc_2/mixin/mixin_application_test.dart b/tests/dartdevc_2/mixin/mixin_application_test.dart new file mode 100644 index 000000000000..5c8273c95721 --- /dev/null +++ b/tests/dartdevc_2/mixin/mixin_application_test.dart @@ -0,0 +1,42 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// @dart=2.6 + +import 'package:expect/expect.dart'; +import 'mixin_declaration.dart'; + +void main() { + // Getter `foo` returns `B._foo` that is coming from the mixin declaration. + Expect.equals('B._foo', C0().foo); + Expect.equals('B._foo', D0().foo); + + // When mixin application is in a separate library from the declaration, + // private symbol from the current library is used to access `_foo`. + Expect.equals('A._foo', C()._foo); + Expect.equals('A._foo', D()._foo); + // Getter `foo` returns `B._foo` that is coming from the mixin declaration. + Expect.equals('B._foo', C().foo); + Expect.equals('B._foo', D().foo); + + // E overrides `_foo`. + Expect.equals('E._foo', E()._foo); +} + +class C0 = A0 with B; +class C = A with B; + +class D0 extends A0 with B {} + +class D extends A with B {} + +class E extends A with B { + String _foo = 'E._foo'; +} + +class A0 {} + +class A { + String _foo = 'A._foo'; +} diff --git a/tests/dartdevc_2/mixin/mixin_declaration.dart b/tests/dartdevc_2/mixin/mixin_declaration.dart new file mode 100644 index 000000000000..a217632ee433 --- /dev/null +++ b/tests/dartdevc_2/mixin/mixin_declaration.dart @@ -0,0 +1,10 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// @dart = 2.6 + +class B { + String _foo = 'B._foo'; + String get foo => _foo; +} diff --git a/tests/dartdevc_2/mixin/mixin_declaration_application_test.dart b/tests/dartdevc_2/mixin/mixin_declaration_application_test.dart new file mode 100644 index 000000000000..246f8b331a8c --- /dev/null +++ b/tests/dartdevc_2/mixin/mixin_declaration_application_test.dart @@ -0,0 +1,46 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// @dart=2.6 + +import 'package:expect/expect.dart'; + +void main() { + // Getter `foo` returns `B._foo` that is coming from the mixin declaration. + Expect.equals('B._foo', C0().foo); + Expect.equals('B._foo', D0().foo); + + // When mixin application is in the same library as the declaration, + // private symbol from the mixin delaration `B._foo` overrides `A._foo`. + Expect.equals('B._foo', C()._foo); + Expect.equals('B._foo', D()._foo); + // Getter `foo` returns `B._foo` that is coming from the mixin declaration. + Expect.equals('B._foo', C().foo); + Expect.equals('B._foo', D().foo); + + // E overrides `_foo`. + Expect.equals('E._foo', E()._foo); +} + +class C0 = A0 with B; +class C = A with B; + +class D0 extends A0 with B {} + +class D extends A with B {} + +class E extends A with B { + String _foo = 'E._foo'; +} + +class A0 {} + +class A { + String _foo = 'A._foo'; +} + +class B { + String _foo = 'B._foo'; + String get foo => _foo; +}