From 8ee467361bb7d3f74d74f88961075ee5ffb7d84d Mon Sep 17 00:00:00 2001 From: Stephen Adams Date: Thu, 10 Jun 2021 03:30:09 +0000 Subject: [PATCH] [dart2js] Fix for #46175 Pass compile-time shadowing information through tearoff code, rather than trying to feature-test for potential overrides of the method accessed via a super-getter. Bug: 46175 Change-Id: Idca8e440cf12bb6cbae020f305763575e6a37b08 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202803 Commit-Queue: Stephen Adams Reviewed-by: Sigmund Cherem --- .../src/js_emitter/class_stub_generator.dart | 56 +++++------ pkg/compiler/lib/src/js_emitter/model.dart | 39 +++++--- .../program_builder/program_builder.dart | 8 +- .../startup_emitter/fragment_emitter.dart | 59 ++++++------ .../_internal/js_runtime/lib/js_helper.dart | 94 ++++++++++--------- 5 files changed, 136 insertions(+), 120 deletions(-) diff --git a/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart b/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart index b016b864c29..f14d9c09dbf 100644 --- a/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart +++ b/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart @@ -228,21 +228,21 @@ List buildTearOffCode( js(r'''function() { throw "Helper 'closureFromTearOff' missing." }'''); } - jsAst.Statement tearOffGetter; + jsAst.Statement instanceTearOffGetter; if (options.useContentSecurityPolicy) { - tearOffGetter = js.statement( + instanceTearOffGetter = js.statement( ''' - function tearOffGetter(funcs, applyTrampolineIndex, reflectionInfo, name, isIntercepted) { + function instanceTearOffGetter(funcs, applyTrampolineIndex, reflectionInfo, name, isIntercepted, needsDirectAccess) { var cache = null; return isIntercepted ? function(receiver) { if (cache === null) cache = #createTearOffClass( - this, funcs, applyTrampolineIndex, reflectionInfo, false, true, name); + funcs, applyTrampolineIndex, reflectionInfo, false, true, name, needsDirectAccess); return new cache(this, funcs[0], receiver, name); } : function() { if (cache === null) cache = #createTearOffClass( - this, funcs, applyTrampolineIndex, reflectionInfo, false, false, name); + funcs, applyTrampolineIndex, reflectionInfo, false, false, name, needsDirectAccess); return new cache(this, funcs[0], null, name); }; }''', @@ -270,46 +270,40 @@ List buildTearOffCode( // create a context with the closed-over values. The closed-over values // include parameters, (Dart) top-level definitions, and the local `cache` // variable all in one context (passing `null` to initialize `cache`). - tearOffGetter = js.statement( + instanceTearOffGetter = js.statement( ''' -function tearOffGetter(funcs, applyTrampolineIndex, reflectionInfo, name, isIntercepted) { - return isIntercepted - - ? new Function("funcs, applyTrampolineIndex, reflectionInfo, name, createTearOffClass, cache", +function instanceTearOffGetter(funcs, applyTrampolineIndex, reflectionInfo, name, isIntercepted, needsDirectAccess) { + if (isIntercepted) + return new Function("funcs, applyTrampolineIndex, reflectionInfo, name, needsDirectAccess, createTearOffClass, cache", "return function tearOff_" + name + (functionCounter++) + "(receiver) {" + "if (cache === null) cache = createTearOffClass(" + - "this, funcs, applyTrampolineIndex, reflectionInfo, false, true, name);" + + "funcs, applyTrampolineIndex, reflectionInfo, false, true, name, needsDirectAccess);" + "return new cache(this, funcs[0], receiver, name);" + - "}")(funcs, applyTrampolineIndex, reflectionInfo, name, #createTearOffClass, null) - - : new Function("funcs, applyTrampolineIndex, reflectionInfo, name, createTearOffClass, cache", + "}")(funcs, applyTrampolineIndex, reflectionInfo, name, needsDirectAccess, #createTearOffClass, null); + else + return new Function("funcs, applyTrampolineIndex, reflectionInfo, name, needsDirectAccess, createTearOffClass, cache", "return function tearOff_" + name + (functionCounter++)+ "() {" + "if (cache === null) cache = createTearOffClass(" + - "this, funcs, applyTrampolineIndex, reflectionInfo, false, false, name);" + + "funcs, applyTrampolineIndex, reflectionInfo, false, false, name, needsDirectAccess);" + "return new cache(this, funcs[0], null, name);" + - "}")(funcs, applyTrampolineIndex, reflectionInfo, name, #createTearOffClass, null); + "}")(funcs, applyTrampolineIndex, reflectionInfo, name, needsDirectAccess, #createTearOffClass, null); }''', {'createTearOffClass': closureFromTearOffAccessExpression}, ); } - jsAst.Statement tearOff = js.statement( + jsAst.Statement staticTearOffGetter = js.statement( ''' - function tearOff(funcs, applyTrampolineIndex, - reflectionInfo, isStatic, name, isIntercepted) { - var cache = null; - return isStatic - ? function() { - if (cache === null) cache = #createTearOffClass( - this, funcs, applyTrampolineIndex, - reflectionInfo, true, false, name).prototype; - return cache; - } - : tearOffGetter(funcs, applyTrampolineIndex, - reflectionInfo, name, isIntercepted); - }''', +function staticTearOffGetter(funcs, applyTrampolineIndex, reflectionInfo, name) { + var cache = null; + return function() { + if (cache === null) cache = #createTearOffClass( + funcs, applyTrampolineIndex, reflectionInfo, true, false, name).prototype; + return cache; + } +}''', {'createTearOffClass': closureFromTearOffAccessExpression}, ); - return [tearOffGetter, tearOff]; + return [instanceTearOffGetter, staticTearOffGetter]; } diff --git a/pkg/compiler/lib/src/js_emitter/model.dart b/pkg/compiler/lib/src/js_emitter/model.dart index 8903eecbb95..d5579b257b0 100644 --- a/pkg/compiler/lib/src/js_emitter/model.dart +++ b/pkg/compiler/lib/src/js_emitter/model.dart @@ -475,6 +475,14 @@ class InstanceMethod extends DartMethod { /// and [aliasName]. final js.Name aliasName; + /// `true` if the tear-off needs to access methods directly rather than rely + /// on JavaScript prototype lookup. This happens when a tear-off getter is + /// called via `super.method` and there is a shadowing definition of `method` + /// in some sublcass. + // TODO(sra): Consider instead having an alias per stub, creating tear-off + // trampolines that target the stubs. + final bool tearOffNeedsDirectAccess; + /// True if this is the implicit `call` instance method of an anonymous /// closure. This predicate is false for explicit `call` methods and for /// functions that can be torn off. @@ -486,19 +494,24 @@ class InstanceMethod extends DartMethod { /// Name called via the general 'catch all' path of Function.apply. ///final js.Name applyName; - InstanceMethod(FunctionEntity element, js.Name name, js.Expression code, - List parameterStubs, js.Name callName, - {bool needsTearOff, - js.Name tearOffName, - this.aliasName, - bool canBeApplied, - int requiredParameterCount, - /* List | Map */ optionalParameterDefaultValues, - this.isClosureCallMethod, - this.isIntercepted, - js.Expression functionType, - int applyIndex}) - : super(element, name, code, parameterStubs, callName, + InstanceMethod( + FunctionEntity element, + js.Name name, + js.Expression code, + List parameterStubs, + js.Name callName, { + bool needsTearOff, + js.Name tearOffName, + this.aliasName, + this.tearOffNeedsDirectAccess, + bool canBeApplied, + int requiredParameterCount, + /* List | Map */ optionalParameterDefaultValues, + this.isClosureCallMethod, + this.isIntercepted, + js.Expression functionType, + int applyIndex, + }) : super(element, name, code, parameterStubs, callName, needsTearOff: needsTearOff, tearOffName: tearOffName, canBeApplied: canBeApplied, diff --git a/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart b/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart index d210c3c81da..2c8489fa171 100644 --- a/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart +++ b/pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart @@ -800,6 +800,7 @@ class ProgramBuilder { if (code == null) return null; bool canTearOff = false; + bool tearOffNeedsDirectAccess = false; js.Name tearOffName; bool isClosureCallMethod = false; bool isNotApplyTarget = @@ -819,9 +820,11 @@ class ProgramBuilder { isClosureCallMethod = true; } else { // Careful with operators. - canTearOff = _codegenWorld.hasInvokedGetter(element) || - _codegenWorld.methodsNeedsSuperGetter(element); + bool needsSuperGetter = _codegenWorld.methodsNeedsSuperGetter(element); + canTearOff = + _codegenWorld.hasInvokedGetter(element) || needsSuperGetter; tearOffName = _namer.getterForElement(element); + tearOffNeedsDirectAccess = needsSuperGetter; } } @@ -865,6 +868,7 @@ class ProgramBuilder { _generateParameterStubs(element, canTearOff, canBeApplied), callName, needsTearOff: canTearOff, tearOffName: tearOffName, + tearOffNeedsDirectAccess: tearOffNeedsDirectAccess, isClosureCallMethod: isClosureCallMethod, isIntercepted: isIntercepted, aliasName: aliasName, diff --git a/pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart b/pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart index 6590471bb6f..a32d4a178d4 100644 --- a/pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart +++ b/pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart @@ -4,17 +4,6 @@ part of dart2js.js_emitter.startup_emitter.model_emitter; -/// The name of the property that stores the tear-off getter on a static -/// function. -/// -/// This property is only used when isolates are used. -/// -/// When serializing static functions we transmit the -/// name of the static function, but not the name of the function's getter. We -/// store the getter-function on the static function itself, which allows us to -/// find it easily. -const String _tearOffPropertyName = r'$tearOff'; - /// The fast startup emitter's goal is to minimize the amount of work that the /// JavaScript engine has to do before it can start running user code. /// @@ -232,7 +221,7 @@ var functionCounter = 0; // Each deferred hunk comes with its own types which are added to the end // of the types-array. -// The `funTypes` passed to the `installTearOff` function below is relative to +// The `funType` passed to the `installTearOff` function below is relative to // the hunk the function comes from. The `typesOffset` variable encodes the // offset at which the new types will be added. var typesOffset = 0; @@ -253,13 +242,18 @@ var typesOffset = 0; // different tearOffCode? function installTearOff( container, getterName, isStatic, isIntercepted, requiredParameterCount, - optionalParameterDefaultValues, callNames, funsOrNames, funType, applyIndex) { + optionalParameterDefaultValues, callNames, funsOrNames, funType, applyIndex, + needsDirectAccess) { // A function can have several stubs (for example to fill in optional // arguments). We collect these functions in the `funs` array. var funs = []; for (var i = 0; i < funsOrNames.length; i++) { var fun = funsOrNames[i]; - if ((typeof fun) == "string") fun = container[fun]; + if ((typeof fun) == "string") { + var stubName = fun; + fun = container[fun]; + fun.#stubName = stubName; + } fun.#callName = callNames[i]; funs.push(fun); } @@ -271,19 +265,18 @@ function installTearOff( fun[#defaultArgumentValues] = optionalParameterDefaultValues; var reflectionInfo = funType; if (typeof reflectionInfo == "number") { - // The reflectionInfo can either be a function, or a pointer into the types - // table. If it points into the types-table we need to update the index, - // in case the tear-off is part of a deferred hunk. + // The reflectionInfo can be a string type recipe or an index into the types + // table. If it points into the types-table we need to update the index, in + // case the tear-off is part of a deferred hunk. reflectionInfo = reflectionInfo + typesOffset; } var name = funsOrNames[0]; - fun.#stubName = name; - var getterFunction = - tearOff(funs, applyIndex || 0, reflectionInfo, isStatic, name, isIntercepted); + applyIndex = applyIndex || 0; + var getterFunction = isStatic + ? staticTearOffGetter(funs, applyIndex, reflectionInfo, name) + : instanceTearOffGetter(funs, applyIndex, reflectionInfo, name, + isIntercepted, needsDirectAccess); container[getterName] = getterFunction; - if (isStatic) { - fun.$_tearOffPropertyName = getterFunction; - } } function installStaticTearOff( @@ -295,18 +288,18 @@ function installStaticTearOff( return installTearOff( container, getterName, true, false, requiredParameterCount, optionalParameterDefaultValues, - callNames, funsOrNames, funType, applyIndex); + callNames, funsOrNames, funType, applyIndex, false); } function installInstanceTearOff( container, getterName, isIntercepted, requiredParameterCount, optionalParameterDefaultValues, - callNames, funsOrNames, funType, applyIndex) { + callNames, funsOrNames, funType, applyIndex, needsDirectAccess) { // TODO(sra): Specialize installTearOff for instance methods. return installTearOff( container, getterName, false, isIntercepted, requiredParameterCount, optionalParameterDefaultValues, - callNames, funsOrNames, funType, applyIndex); + callNames, funsOrNames, funType, applyIndex, !!needsDirectAccess); } // Instead of setting the interceptor tags directly we use this update @@ -359,7 +352,8 @@ var #hunkHelpers = (function(){ return installInstanceTearOff( container, getterName, isIntercepted, requiredParameterCount, optionalParameterDefaultValues, - callNames, [name], funType, applyIndex); + callNames, [name], funType, applyIndex, + /*needsDirectAccess:*/ false); } }, @@ -1471,10 +1465,13 @@ class FragmentEmitter { "applyIndex": applyIndex, }); } else { + bool tearOffNeedsDirectAccess = + (method as InstanceMethod).tearOffNeedsDirectAccess; if (requiredParameterCount <= 2 && callNames.length == 1 && optionalParameterDefaultValues is js.LiteralNull && - method.applyIndex == 0) { + method.applyIndex == 0 && + !tearOffNeedsDirectAccess) { js.Statement finish(int arity) { // Short form for exactly 0/1/2 arguments. String isInterceptedTag = isIntercepted ? 'i' : 'u'; @@ -1501,7 +1498,8 @@ class FragmentEmitter { return js.js.statement(''' #install(#container, #getterName, #isIntercepted, #requiredParameterCount, #optionalParameterDefaultValues, - #callNames, #funsOrNames, #funType, #applyIndex)''', { + #callNames, #funsOrNames, #funType, #applyIndex, + #tearOffNeedsDirectAccess)''', { "install": install, "container": container, "getterName": js.quoteName(method.tearOffName), @@ -1513,6 +1511,9 @@ class FragmentEmitter { "funsOrNames": funsOrNamesArray, "funType": method.functionType, "applyIndex": applyIndex, + // 'Truthy' values are ok for `tearOffNeedsDirectAccess`. + "tearOffNeedsDirectAccess": + js.number(tearOffNeedsDirectAccess ? 1 : 0), }); } } diff --git a/sdk/lib/_internal/js_runtime/lib/js_helper.dart b/sdk/lib/_internal/js_runtime/lib/js_helper.dart index 7fe98029040..0706331ad0b 100644 --- a/sdk/lib/_internal/js_runtime/lib/js_helper.dart +++ b/sdk/lib/_internal/js_runtime/lib/js_helper.dart @@ -1831,8 +1831,7 @@ convertDartClosureToJS(closure, int arity) { /// Superclass for Dart closures. /// /// All static, tear-off, function declaration and function expression closures -/// extend this class, but classes that implement Function via a `call` method -/// do not. +/// extend this class. abstract class Closure implements Function { // TODO(ahe): These constants must be in sync with // reflection_data_parser.dart. @@ -1874,13 +1873,13 @@ abstract class Closure implements Function { /// Caution: this function may be called when building constants. /// TODO(ahe): Don't call this function when building constants. static fromTearOff( - receiver, List functions, - int? applyTrampolineIndex, + int applyTrampolineIndex, var reflectionInfo, bool isStatic, bool isIntercepted, String propertyName, + bool needsDirectAccess, ) { JS_EFFECT(() { // The functions are called here to model the calls from JS forms below. @@ -1931,7 +1930,7 @@ abstract class Closure implements Function { // we only use the new instance to access the constructor property and use // Object.create to create the desired prototype. // - // TODO(sra): Perhaps cache the prototype to avoid the allocation. + // TODO(sra): Cache the prototype to avoid the allocation. var prototype = isStatic ? JS('StaticClosure', 'Object.create(#.constructor.prototype)', new StaticClosure()) @@ -1963,7 +1962,7 @@ abstract class Closure implements Function { // Create a closure and "monkey" patch it with call stubs. var trampoline = function; if (!isStatic) { - trampoline = forwardCallTo(receiver, function, isIntercepted); + trampoline = forwardCallTo(function, isIntercepted, needsDirectAccess); JS('', '#.\$reflectionInfo = #', trampoline, reflectionInfo); } else { JS('', '#[#] = #', prototype, STATIC_FUNCTION_NAME_PROPERTY_NAME, @@ -1982,7 +1981,9 @@ abstract class Closure implements Function { var stubCallName = JS('String|Null', '#[#]', stub, JS_GET_NAME(JsGetName.CALL_NAME_PROPERTY)); if (stubCallName != null) { - stub = isStatic ? stub : forwardCallTo(receiver, stub, isIntercepted); + stub = isStatic + ? stub + : forwardCallTo(stub, isIntercepted, needsDirectAccess); JS('', '#[#] = #', prototype, stubCallName, stub); } if (i == applyTrampolineIndex) { @@ -2041,10 +2042,22 @@ abstract class Closure implements Function { } static cspForwardCall( - int arity, bool isSuperCall, String? stubName, function) { + int arity, bool needsDirectAccess, String? stubName, function) { var getSelf = RAW_DART_FUNCTION_REF(BoundClosure.selfOf); - // Handle intercepted stub-names with the default slow case. - if (isSuperCall) arity = -1; + + // We have the target method (or an arity stub for the method) in + // [function]. These fixed-arity forwarding stubs could use + // `Function.prototype.call` on the target directly, but on some browsers it + // is quite a bit faster to do a property access again to get the + // function. Accessing the property again will fail (retrieve the wrong + // function) if the desired property is shadowed. This can happen, e.g., + // when the tear-off was created by a super-getter call `super.method` and + // `method` has an override on some subclass. + // + // To handle the shadowing-of-a-method-that-has-a-super-tearoff case, we use + // the default slow case that uses `Function.prototype.apply`. + if (needsDirectAccess) arity = -1; + switch (arity) { case 0: return JS( @@ -2121,19 +2134,14 @@ abstract class Closure implements Function { static bool get isCsp => JS_GET_FLAG('USE_CONTENT_SECURITY_POLICY'); - static forwardCallTo(receiver, function, bool isIntercepted) { - if (isIntercepted) return forwardInterceptedCallTo(receiver, function); + static forwardCallTo(function, bool isIntercepted, bool needsDirectAccess) { + if (isIntercepted) + return forwardInterceptedCallTo(function, needsDirectAccess); String? stubName = JS('String|Null', '#.\$stubName', function); int arity = JS('int', '#.length', function); - var lookedUpFunction = JS('', '#[#]', receiver, stubName); - // The receiver[stubName] may not be equal to the function if we try to - // forward to a super-method. Especially when we create a bound closure - // of a super-call we need to make sure that we don't forward back to the - // dynamically looked up function. - bool isSuperCall = !identical(function, lookedUpFunction); - if (isCsp || isSuperCall || arity >= 27) { - return cspForwardCall(arity, isSuperCall, stubName, function); + if (isCsp || needsDirectAccess || arity >= 27) { + return cspForwardCall(arity, needsDirectAccess, stubName, function); } if (arity == 0) { @@ -2160,11 +2168,11 @@ abstract class Closure implements Function { } static cspForwardInterceptedCall( - int arity, bool isSuperCall, String? name, function) { + int arity, bool needsDirectAccess, String? name, function) { var getSelf = RAW_DART_FUNCTION_REF(BoundClosure.selfOf); var getReceiver = RAW_DART_FUNCTION_REF(BoundClosure.receiverOf); // Handle intercepted stub-names with the default slow case. - if (isSuperCall) arity = -1; + if (needsDirectAccess) arity = -1; switch (arity) { case 0: // Intercepted functions always takes at least one argument (the @@ -2252,21 +2260,16 @@ abstract class Closure implements Function { } } - static forwardInterceptedCallTo(receiver, function) { + static forwardInterceptedCallTo(function, bool needsDirectAccess) { String selfField = BoundClosure.selfFieldName(); String receiverField = BoundClosure.receiverFieldName(); String? stubName = JS('String|Null', '#.\$stubName', function); int arity = JS('int', '#.length', function); bool isCsp = JS_GET_FLAG('USE_CONTENT_SECURITY_POLICY'); - var lookedUpFunction = JS('', '#[#]', receiver, stubName); - // The receiver[stubName] may not be equal to the function if we try to - // forward to a super-method. Especially when we create a bound closure - // of a super-call we need to make sure that we don't forward back to the - // dynamically looked up function. - bool isSuperCall = !identical(function, lookedUpFunction); - if (isCsp || isSuperCall || arity >= 28) { - return cspForwardInterceptedCall(arity, isSuperCall, stubName, function); + if (isCsp || needsDirectAccess || arity >= 28) { + return cspForwardInterceptedCall( + arity, needsDirectAccess, stubName, function); } if (arity == 1) { return JS( @@ -2310,16 +2313,17 @@ abstract class Closure implements Function { } /// Called from implicit method getter (aka tear-off). -closureFromTearOff(receiver, functions, applyTrampolineIndex, reflectionInfo, - isStatic, isIntercepted, name) { +closureFromTearOff(functions, applyTrampolineIndex, reflectionInfo, isStatic, + isIntercepted, name, needsDirectAccess) { return Closure.fromTearOff( - receiver, - JS('JSArray', '#', functions), - JS('int|Null', '#', applyTrampolineIndex), - reflectionInfo, - JS('bool', '!!#', isStatic), - JS('bool', '!!#', isIntercepted), - JS('String', '#', name)); + JS('JSArray', '#', functions), + JS('int', '#||0', applyTrampolineIndex), + reflectionInfo, + JS('bool', '!!#', isStatic), + JS('bool', '!!#', isIntercepted), + JS('String', '#', name), + JS('bool', '!!#', needsDirectAccess), + ); } /// Represents an implicit closure of a function. @@ -2337,18 +2341,18 @@ class StaticClosure extends TearOffClosure { /// Represents a 'tear-off' or property extraction closure of an instance /// method, that is an instance method bound to a specific receiver (instance). class BoundClosure extends TearOffClosure { - /// The receiver or interceptor. - // TODO(ahe): This could just be the interceptor, we always know if - // we need the interceptor when generating the call method. + /// The JavaScript receiver, which is the Dart receiver or the interceptor. final _self; /// The method. final _target; - /// The receiver. Null if [_self] is not an interceptor. + /// The Dart receiver if [_target] is an intercepted method (in which case + /// [_self] is the interceptor), otherwise `null`. final _receiver; - /// The name of the function. Only used by the mirror system. + /// The name of the function. Only used by `toString()`. + // TODO(sra): This should be part of the generated tear-off class. final String _name; BoundClosure(this._self, this._target, this._receiver, this._name);