[dart:js_util] Add checks for functions in js_util

Closes #44145

Introduces assertions in js_util for `setProperty`,
`callMethod`, and `callConstructor` for when the provided
values are Functions but not wrapped with `allowInterop`.
Adds a test to check for assertions in each of those methods.

Change-Id: I7681cf81170d7975abc7e7dd4172f2c5673f203b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/176740
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Srujan Gaddam <srujzs@google.com>

sdk/lib/_internal/js_dev_runtime/private/js_helper.dart

@@ -813,3 +813,15 @@ class PrivateSymbol implements Symbol {
   // TODO(jmesserly): is this equivalent to _nativeSymbol toString?
   toString() => 'Symbol("$_name")';
 }
+
+/// Asserts that if [value] is a function, it is a JavaScript function or has
+/// been wrapped by [allowInterop].
+///
+/// This function does not recurse if [value] is a collection.
+void assertInterop(Object? value) {
+  if (value is Function) dart.assertInterop(value);
+}
+
+/// Like [assertInterop], except iterates over a list of arguments
+/// non-recursively.
+void assertInteropArgs(List<Object?> args) => args.forEach(assertInterop);

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

@@ -3023,3 +3023,26 @@ bool isRequired(Object? value) => identical(kRequiredSentinel, value);
 
 /// Called by generated code to throw a LateInitializationError.
 void throwLateInitializationError(String name) => throw LateError(name);
+
+/// Checks that [f] is a function that supports interop.
+@pragma('dart2js:tryInline')
+bool isJSFunction(Function f) => JS('bool', 'typeof(#) == "function"', f);
+
+/// Asserts that if [value] is a function, it is a JavaScript function or has
+/// been wrapped by [allowInterop].
+///
+/// This function does not recurse if [value] is a collection.
+void assertInterop(Object? value) {
+  assert(value is! Function || isJSFunction(value),
+      'Dart function requires `allowInterop` to be passed to JavaScript.');
+}
+
+/// Like [assertInterop], except iterates over a list of arguments
+/// non-recursively.
+///
+/// This function intentionally avoids using [assertInterop] so that this
+/// function can become a no-op if assertions are disabled.
+void assertInteropArgs(List<Object?> args) {
+  assert(args.every((arg) => arg is! Function || isJSFunction(arg)),
+      'Dart function requires `allowInterop` to be passed to JavaScript.');
+}

sdk/lib/_internal/js_runtime/lib/js_patch.dart

@@ -8,7 +8,8 @@ import 'dart:typed_data' show TypedData;
 
 import 'dart:_foreign_helper' show JS, DART_CLOSURE_TO_JS;
 import 'dart:_interceptors' show DART_CLOSURE_PROPERTY_NAME;
-import 'dart:_js_helper' show patch, Primitives, getIsolateAffinityTag;
+import 'dart:_js_helper'
+    show patch, Primitives, getIsolateAffinityTag, isJSFunction;
 import 'dart:_js' show isBrowserObject, convertFromBrowserObject;
 
 @patch
@@ -547,7 +548,7 @@ _callDartFunctionFastCaptureThis(callback, self, List arguments) {
 
 @patch
 F allowInterop<F extends Function>(F f) {
-  if (JS('bool', 'typeof(#) == "function"', f)) {
+  if (isJSFunction(f)) {
     // Already supports interop, just use the existing function.
     return f;
   } else {
@@ -557,7 +558,7 @@ F allowInterop<F extends Function>(F f) {
 
 @patch
 Function allowInteropCaptureThis(Function f) {
-  if (JS('bool', 'typeof(#) == "function"', f)) {
+  if (isJSFunction(f)) {
     // Behavior when the function is already a JS function is unspecified.
     throw ArgumentError(
         "Function is already a JS function so cannot capture this.");

sdk/lib/js_util/js_util.dart

@@ -14,7 +14,8 @@ library dart.js_util;
 import 'dart:_foreign_helper' show JS;
 import 'dart:collection' show HashMap;
 import 'dart:async' show Completer;
-import 'dart:_js_helper' show convertDartClosureToJS;
+import 'dart:_js_helper'
+    show convertDartClosureToJS, assertInterop, assertInteropArgs;
 
 /// Recursively converts a JSON-like collection to JavaScript compatible
 /// representation.
@@ -70,11 +71,15 @@ bool hasProperty(Object o, Object name) => JS('bool', '# in #', name, o);
 dynamic getProperty(Object o, Object name) =>
     JS('Object|Null', '#[#]', o, name);
 
-dynamic setProperty(Object o, Object name, Object? value) =>
-    JS('', '#[#]=#', o, name, value);
+dynamic setProperty(Object o, Object name, Object? value) {
+  assertInterop(value);
+  return JS('', '#[#]=#', o, name, value);
+}
 
-dynamic callMethod(Object o, String method, List<Object?> args) =>
-    JS('Object|Null', '#[#].apply(#, #)', o, method, o, args);
+dynamic callMethod(Object o, String method, List<Object?> args) {
+  assertInteropArgs(args);
+  return JS('Object|Null', '#[#].apply(#, #)', o, method, o, args);
+}
 
 /// Check whether [o] is an instance of [type].
 ///
@@ -83,9 +88,11 @@ dynamic callMethod(Object o, String method, List<Object?> args) =>
 bool instanceof(Object? o, Object type) =>
     JS('bool', '# instanceof #', o, type);
 
-dynamic callConstructor(Object constr, List<Object?> arguments) {
+dynamic callConstructor(Object constr, List<Object?>? arguments) {
   if (arguments == null) {
     return JS('Object', 'new #()', constr);
+  } else {
+    assertInteropArgs(arguments);
   }
 
   if (JS('bool', '# instanceof Array', arguments)) {

tests/lib/js/js_util/assert_function_interop_test.dart

@@ -0,0 +1,93 @@
+// Copyright (c) 2020, 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.
+
+// dart2jsOptions=--enable-asserts
+
+// Tests that `js_util` methods correctly check that function arguments allow
+// interop.
+
+@JS()
+library assert_function_interop_test;
+
+import 'package:js/js.dart';
+import 'package:js/js_util.dart' as js_util;
+import 'package:expect/expect.dart';
+
+@JS()
+external void eval(String code);
+
+@JS()
+class JSClass {
+  external JSClass();
+}
+
+@JS()
+external get JSClassWithFuncArgs;
+
+@JS()
+external void Function() get jsFunction;
+
+void dartFunction() {}
+
+@pragma('dart2js:noInline')
+@pragma('dart2js:assumeDynamic')
+confuse(x) => x;
+
+void main() {
+  eval(r"""
+    function JSClass() {
+      this.functionProperty = function() {};
+      this.methodWithFuncArgs = function(f1, f2) {
+        f1();
+        if (arguments.length == 2) f2();
+      };
+    }
+    function JSClassWithFuncArgs(f1, f2) {
+      f1();
+      if (arguments.length == 2) f2();
+    }
+    function jsFunction() {}
+  """);
+
+  var jsClass = JSClass();
+  dynamic d = confuse(dartFunction);
+  var interopFunction = allowInterop(dartFunction);
+  var interopDynamic = confuse(allowInterop(dartFunction));
+
+  // Functions that aren't wrapped with `allowInterop` should throw.
+  Expect.throws(
+      () => js_util.setProperty(jsClass, 'functionProperty', dartFunction));
+  Expect.throws(() => js_util.setProperty(jsClass, 'functionProperty', d));
+  // Correctly wrapped functions should not throw.
+  js_util.setProperty(jsClass, 'functionProperty', interopFunction);
+  js_util.setProperty(jsClass, 'functionProperty', interopDynamic);
+
+  // Using a JS function should not throw.
+  js_util.setProperty(jsClass, 'functionProperty', jsFunction);
+
+  Expect.throws(
+      () => js_util.callMethod(jsClass, 'methodWithFuncArgs', [dartFunction]));
+  Expect.throws(() => js_util.callMethod(jsClass, 'methodWithFuncArgs', [d]));
+  js_util.callMethod(jsClass, 'methodWithFuncArgs', [interopFunction]);
+  js_util.callMethod(jsClass, 'methodWithFuncArgs', [interopDynamic]);
+  // Check to see that all arguments are checked.
+  Expect.throws(() => js_util.callMethod(
+      jsClass, 'methodWithFuncArgs', [interopFunction, dartFunction]));
+  js_util.callMethod(
+      jsClass, 'methodWithFuncArgs', [interopFunction, interopFunction]);
+
+  js_util.callMethod(jsClass, 'methodWithFuncArgs', [jsFunction]);
+
+  Expect.throws(
+      () => js_util.callConstructor(JSClassWithFuncArgs, [dartFunction]));
+  Expect.throws(() => js_util.callConstructor(JSClassWithFuncArgs, [d]));
+  js_util.callConstructor(JSClassWithFuncArgs, [interopFunction]);
+  js_util.callConstructor(JSClassWithFuncArgs, [interopDynamic]);
+  Expect.throws(() => js_util
+      .callConstructor(JSClassWithFuncArgs, [interopFunction, dartFunction]));
+  js_util
+      .callConstructor(JSClassWithFuncArgs, [interopFunction, interopFunction]);
+
+  js_util.callConstructor(JSClassWithFuncArgs, [jsFunction]);
+}