[js_runtime] Don't modify inputs to Function.apply

In some cases adding default arguments modified the 'positional'
arguments input List.  Since there are 'accelerated' paths for small
numbers of arguments, this bug did not show in any other tests.

Change-Id: I5075c96ecfdc9645b85b9beeeb38305415153d20
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204747
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>

sdk/lib/_internal/js_runtime/lib/js_helper.dart

@@ -820,11 +820,10 @@ class Primitives {
   /// the value of the catch-all property.
   static applyFunction(Function function, List? positionalArguments,
       Map<String, dynamic>? namedArguments) {
-    // Fast shortcut for the common case.
+    // Fast path for common cases.
-    if (JS('bool', '# instanceof Array', positionalArguments) &&
+    if (positionalArguments is JSArray &&
         (namedArguments == null || namedArguments.isEmpty)) {
-      // Let the compiler know that we did a type-test.
+      JSArray arguments = positionalArguments;
-      List arguments = (JS('JSArray', '#', positionalArguments));
       int argumentCount = arguments.length;
       if (argumentCount == 0) {
         String selectorName = JS_GET_NAME(JsGetName.CALL_PREFIX0);
@@ -877,18 +876,17 @@ class Primitives {
       }
     }
 
-    return _genericApplyFunction2(
+    return _generalApplyFunction(function, positionalArguments, namedArguments);
-        function, positionalArguments, namedArguments);
   }
 
-  static _genericApplyFunction2(Function function, List? positionalArguments,
+  static _generalApplyFunction(Function function, List? positionalArguments,
       Map<String, dynamic>? namedArguments) {
     List arguments;
     if (positionalArguments != null) {
-      if (JS('bool', '# instanceof Array', positionalArguments)) {
+      if (positionalArguments is JSArray) {
-        arguments = JS('JSArray', '#', positionalArguments);
+        arguments = positionalArguments;
       } else {
-        arguments = new List.from(positionalArguments);
+        arguments = List.of(positionalArguments);
       }
     } else {
       arguments = [];
@@ -934,8 +932,7 @@ class Primitives {
       return functionNoSuchMethod(function, arguments, namedArguments);
     }
 
-    bool acceptsPositionalArguments =
+    bool acceptsPositionalArguments = defaultValues is JSArray;
-        JS('bool', '# instanceof Array', defaultValues);
 
     if (acceptsPositionalArguments) {
       if (namedArguments != null && namedArguments.isNotEmpty) {
@@ -950,9 +947,15 @@ class Primitives {
         // The function expects fewer arguments.
         return functionNoSuchMethod(function, arguments, null);
       }
-      List missingDefaults = JS('JSArray', '#.slice(#)', defaultValues,
+      if (argumentCount < maxArguments) {
-          argumentCount - requiredParameterCount);
+        List missingDefaults = JS('JSArray', '#.slice(#)', defaultValues,
-      arguments.addAll(missingDefaults);
+            argumentCount - requiredParameterCount);
+        if (identical(arguments, positionalArguments)) {
+          // Defensive copy to avoid modifying passed-in List.
+          arguments = List.of(arguments);
+        }
+        arguments.addAll(missingDefaults);
+      }
       return JS('var', '#.apply(#, #)', jsFunction, function, arguments);
     } else {
       // Handle named arguments.
@@ -963,6 +966,11 @@ class Primitives {
         return functionNoSuchMethod(function, arguments, namedArguments);
       }
 
+      if (identical(arguments, positionalArguments)) {
+        // Defensive copy to avoid modifying passed-in List.
+        arguments = List.of(arguments);
+      }
+
       List keys = JS('JSArray', r'Object.keys(#)', defaultValues);
       if (namedArguments == null) {
         for (String key in keys) {
@@ -987,6 +995,7 @@ class Primitives {
           }
         }
         if (used != namedArguments.length) {
+          // Named argument with name not accected by function.
           return functionNoSuchMethod(function, arguments, namedArguments);
         }
       }

tests/corelib/apply6_test.dart

@@ -0,0 +1,66 @@
+// Copyright (c) 2021, 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.
+
+// Test that the List of positional arguments to [Function.apply] is not
+// modified.
+
+import "package:expect/expect.dart";
+
+class A {
+  foo([
+    a = 10,
+    b = 20,
+    c = 30,
+    d = 40,
+    e = 50,
+    f = 60,
+    g = 70,
+    h = 80,
+    i = 90,
+  ]) =>
+      '$a $b $c $d $e $f $g $h $i';
+}
+
+String static1(a, b, {c = 30, d = 40}) => '$a $b $c $d';
+
+void test(String expected, Function function, List positional,
+    [Map<Symbol, dynamic>? named = null]) {
+  final original = List.of(positional);
+
+  Expect.equals(expected, Function.apply(function, positional, named));
+  Expect.listEquals(original, positional);
+
+  // Test again so there are multiple call sites for `Function.apply`.
+  Expect.equals(expected, Function.apply(function, positional, named));
+  Expect.listEquals(original, positional);
+}
+
+main() {
+  var a = A();
+
+  test('10 20 30 40 50 60 70 80 90', a.foo, []);
+  test('11 20 30 40 50 60 70 80 90', a.foo, [11]);
+  test('11 22 30 40 50 60 70 80 90', a.foo, [11, 22]);
+  test('11 22 33 40 50 60 70 80 90', a.foo, [11, 22, 33]);
+  test('11 22 33 44 50 60 70 80 90', a.foo, [11, 22, 33, 44]);
+  test('11 22 33 44 55 60 70 80 90', a.foo, [11, 22, 33, 44, 55]);
+  test('11 22 33 44 55 66 70 80 90', a.foo, [11, 22, 33, 44, 55, 66]);
+  test('11 22 33 44 55 66 77 80 90', a.foo, [11, 22, 33, 44, 55, 66, 77]);
+  test('11 22 33 44 55 66 77 88 90', a.foo, [11, 22, 33, 44, 55, 66, 77, 88]);
+  test('11 22 33 44 55 66 77 88 99', a.foo,
+      [11, 22, 33, 44, 55, 66, 77, 88, 99]);
+
+  // Some unmodifiable Lists. An attempt to modify the argument would fail.
+  test('11 22 33 44 55 66 77 80 90', a.foo, const [11, 22, 33, 44, 55, 66, 77]);
+  test('65 66 67 68 69 70 71 80 90', a.foo, 'ABCDEFG'.codeUnits);
+
+  test('11 22 30 40', static1, [11, 22]);
+  test('11 22 30 40', static1, [11, 22], {});
+  test('11 22 33 40', static1, [11, 22], {#c: 33});
+  test('11 22 30 44', static1, [11, 22], {#d: 44});
+  test('11 22 66 55', static1, [11, 22], {#d: 55, #c: 66});
+
+  test('11 22 88 77', static1, const [11, 22], {#d: 77, #c: 88});
+  test('65 66 11 22', static1, 'AB'.codeUnits, {#d: 22, #c: 11});
+}

tests/corelib_2/apply6_test.dart

@@ -0,0 +1,68 @@
+// Copyright (c) 2021, 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.9
+
+// Test that the List of positional arguments to [Function.apply] is not
+// modified.
+
+import "package:expect/expect.dart";
+
+class A {
+  foo([
+    a = 10,
+    b = 20,
+    c = 30,
+    d = 40,
+    e = 50,
+    f = 60,
+    g = 70,
+    h = 80,
+    i = 90,
+  ]) =>
+      '$a $b $c $d $e $f $g $h $i';
+}
+
+String static1(a, b, {c = 30, d = 40}) => '$a $b $c $d';
+
+void test(String expected, Function function, List positional,
+    [Map<Symbol, dynamic> named = null]) {
+  final original = List.of(positional);
+
+  Expect.equals(expected, Function.apply(function, positional, named));
+  Expect.listEquals(original, positional);
+
+  // Test again so there are multiple call sites for `Function.apply`.
+  Expect.equals(expected, Function.apply(function, positional, named));
+  Expect.listEquals(original, positional);
+}
+
+main() {
+  var a = A();
+
+  test('10 20 30 40 50 60 70 80 90', a.foo, []);
+  test('11 20 30 40 50 60 70 80 90', a.foo, [11]);
+  test('11 22 30 40 50 60 70 80 90', a.foo, [11, 22]);
+  test('11 22 33 40 50 60 70 80 90', a.foo, [11, 22, 33]);
+  test('11 22 33 44 50 60 70 80 90', a.foo, [11, 22, 33, 44]);
+  test('11 22 33 44 55 60 70 80 90', a.foo, [11, 22, 33, 44, 55]);
+  test('11 22 33 44 55 66 70 80 90', a.foo, [11, 22, 33, 44, 55, 66]);
+  test('11 22 33 44 55 66 77 80 90', a.foo, [11, 22, 33, 44, 55, 66, 77]);
+  test('11 22 33 44 55 66 77 88 90', a.foo, [11, 22, 33, 44, 55, 66, 77, 88]);
+  test('11 22 33 44 55 66 77 88 99', a.foo,
+      [11, 22, 33, 44, 55, 66, 77, 88, 99]);
+
+  // Some unmodifiable Lists. An attempt to modify the argument would fail.
+  test('11 22 33 44 55 66 77 80 90', a.foo, const [11, 22, 33, 44, 55, 66, 77]);
+  test('65 66 67 68 69 70 71 80 90', a.foo, 'ABCDEFG'.codeUnits);
+
+  test('11 22 30 40', static1, [11, 22]);
+  test('11 22 30 40', static1, [11, 22], {});
+  test('11 22 33 40', static1, [11, 22], {#c: 33});
+  test('11 22 30 44', static1, [11, 22], {#d: 44});
+  test('11 22 66 55', static1, [11, 22], {#d: 55, #c: 66});
+
+  test('11 22 88 77', static1, const [11, 22], {#d: 77, #c: 88});
+  test('65 66 11 22', static1, 'AB'.codeUnits, {#d: 22, #c: 11});
+}