[BUGFIX release] Prevent triggering V8 memory leak bug through registry / resolver access.

This fix changes the expectations of Registry to accept a `resolver`
that’s an object with a `resolve` method instead of a straight function.

This allows us to avoid closing over access to a resolver object inside
a function. It also allows us to avoid setting functions which shadow
prototype functions unnecessarily.

Setting `Registry#resolver` to a function is still allowed through a
constructor option. The `resolver` function will be converted into
an object with a `resolve` method and will result in a single deprecation
warning.

This fix also eliminates the need for application instances to override
their registry’s `normalizeFullName` and `makeToString` methods.
Instead, the fallback registry will be checked when evaluating these
methods before returning the defaults. Again, this avoids the need to
override instance functions that shadow prototype functions.

[Fixes #12618]

(cherry picked from commit 529bfc3)

Author: dgeb

packages/container/lib/registry.js

@@ -1,4 +1,4 @@
-import { assert } from 'ember-metal/debug';
+import { assert, deprecate } from 'ember-metal/debug';
 import dictionary from 'ember-metal/dictionary';
 import assign from 'ember-metal/assign';
 import Container from './container';
@@ -21,7 +21,13 @@ var VALID_FULL_NAME_REGEXP = /^[^:]+.+:[^:]+$/;
 function Registry(options) {
   this.fallback = options && options.fallback ? options.fallback : null;
 
-  this.resolver = options && options.resolver ? options.resolver : function() {};
+  if (options && options.resolver) {
+    this.resolver = options.resolver;
+
+    if (typeof this.resolver === 'function') {
+      deprecateResolverFunction(this);
+    }
+  }
 
   this.registrations  = dictionary(options && options.registrations ? options.registrations : null);
 
@@ -49,9 +55,11 @@ Registry.prototype = {
   fallback: null,
 
   /**
+   An object that has a `resolve` method that resolves a name.
+
    @private
    @property resolver
-   @type function
+   @type Resolver
    */
   resolver: null,
 
@@ -259,7 +267,13 @@ Registry.prototype = {
    @return {string} described fullName
    */
   describe(fullName) {
-    return fullName;
+    if (this.resolver && this.resolver.lookupDescription) {
+      return this.resolver.lookupDescription(fullName);
+    } else if (this.fallback) {
+      return this.fallback.describe(fullName);
+    } else {
+      return fullName;
+    }
   },
 
   /**
@@ -271,7 +285,13 @@ Registry.prototype = {
    @return {string} normalized fullName
    */
   normalizeFullName(fullName) {
-    return fullName;
+    if (this.resolver && this.resolver.normalize) {
+      return this.resolver.normalize(fullName);
+    } else if (this.fallback) {
+      return this.fallback.normalizeFullName(fullName);
+    } else {
+      return fullName;
+    }
   },
 
   /**
@@ -297,7 +317,13 @@ Registry.prototype = {
    @return {function} toString function
    */
   makeToString(factory, fullName) {
-    return factory.toString();
+    if (this.resolver && this.resolver.makeToString) {
+      return this.resolver.makeToString(factory, fullName);
+    } else if (this.fallback) {
+      return this.fallback.makeToString(factory, fullName);
+    } else {
+      return factory.toString();
+    }
   },
 
   /**
@@ -645,7 +671,7 @@ Registry.prototype = {
       fallbackKnown = this.fallback.knownForType(type);
     }
 
-    if (this.resolver.knownForType) {
+    if (this.resolver && this.resolver.knownForType) {
       resolverKnown = this.resolver.knownForType(type);
     }
 
@@ -723,12 +749,27 @@ Registry.prototype = {
   }
 };
 
+function deprecateResolverFunction(registry) {
+  deprecate('Passing a `resolver` function into a Registry is deprecated. Please pass in a Resolver object with a `resolve` method.',
+            false,
+            { id: 'ember-application.registry-resolver-as-function', until: '3.0.0', url: 'http://emberjs.com/deprecations/v2.x#toc_registry-resolver-as-function' });
+  registry.resolver = {
+    resolve: registry.resolver
+  };
+}
+
 function resolve(registry, normalizedName) {
-  var cached = registry._resolveCache[normalizedName];
+  let cached = registry._resolveCache[normalizedName];
   if (cached) { return cached; }
   if (registry._failCache[normalizedName]) { return; }
 
-  var resolved = registry.resolver(normalizedName) || registry.registrations[normalizedName];
+  let resolved;
+
+  if (registry.resolver) {
+    resolved = registry.resolver.resolve(normalizedName);
+  }
+
+  resolved = resolved || registry.registrations[normalizedName];
 
   if (resolved) {
     registry._resolveCache[normalizedName] = resolved;

packages/container/tests/container_test.js

@@ -301,9 +301,11 @@ QUnit.test('The container can use a registry hook to resolve factories lazily',
   var container = registry.container();
   var PostController = factory();
 
-  registry.resolver = function(fullName) {
-    if (fullName === 'controller:post') {
-      return PostController;
+  registry.resolver = {
+    resolve(fullName) {
+      if (fullName === 'controller:post') {
+        return PostController;
+      }
     }
   };
 
@@ -347,9 +349,11 @@ QUnit.test('Options can be registered that should be applied to a given factory'
   var container = registry.container();
   var PostView = factory();
 
-  registry.resolver = function(fullName) {
-    if (fullName === 'view:post') {
-      return PostView;
+  registry.resolver = {
+    resolve(fullName) {
+      if (fullName === 'view:post') {
+        return PostView;
+      }
     }
   };
 
@@ -369,9 +373,11 @@ QUnit.test('Options can be registered that should be applied to all factories fo
   var container = registry.container();
   var PostView = factory();
 
-  registry.resolver = function(fullName) {
-    if (fullName === 'view:post') {
-      return PostView;
+  registry.resolver = {
+    resolve(fullName) {
+      if (fullName === 'view:post') {
+        return PostView;
+      }
     }
   };
 

packages/container/tests/registry_test.js

@@ -56,27 +56,29 @@ QUnit.test('Throw exception when trying to inject `type:thing` on all type(s)',
 });
 
 QUnit.test('The registry can take a hook to resolve factories lazily', function() {
-  var registry = new Registry();
-  var PostController = factory();
-
-  registry.resolver = function(fullName) {
-    if (fullName === 'controller:post') {
-      return PostController;
+  let PostController = factory();
+  let resolver = {
+    resolve(fullName) {
+      if (fullName === 'controller:post') {
+        return PostController;
+      }
     }
   };
+  let registry = new Registry({ resolver });
 
   strictEqual(registry.resolve('controller:post'), PostController, 'The correct factory was provided');
 });
 
 QUnit.test('The registry respects the resolver hook for `has`', function() {
-  var registry = new Registry();
-  var PostController = factory();
-
-  registry.resolver = function(fullName) {
-    if (fullName === 'controller:post') {
-      return PostController;
+  let PostController = factory();
+  let resolver = {
+    resolve(fullName) {
+      if (fullName === 'controller:post') {
+        return PostController;
+      }
     }
   };
+  let registry = new Registry({ resolver });
 
   ok(registry.has('controller:post'), 'the `has` method uses the resolver hook');
 });
@@ -196,14 +198,18 @@ QUnit.test('once resolved, always return the same result', function() {
 
   var registry = new Registry();
 
-  registry.resolver = function() {
-    return 'bar';
+  registry.resolver = {
+    resolve() {
+      return 'bar';
+    }
   };
 
   var Bar = registry.resolve('models:bar');
 
-  registry.resolver = function() {
-    return 'not bar';
+  registry.resolver = {
+    resolve() {
+      return 'not bar';
+    }
   };
 
   equal(registry.resolve('models:bar'), Bar);
@@ -213,9 +219,12 @@ QUnit.test('factory resolves are cached', function() {
   var registry = new Registry();
   var PostController = factory();
   var resolveWasCalled = [];
-  registry.resolver = function(fullName) {
-    resolveWasCalled.push(fullName);
-    return PostController;
+
+  registry.resolver = {
+    resolve(fullName) {
+      resolveWasCalled.push(fullName);
+      return PostController;
+    }
   };
 
   deepEqual(resolveWasCalled, []);
@@ -230,9 +239,12 @@ QUnit.test('factory for non extendables (MODEL) resolves are cached', function()
   var registry = new Registry();
   var PostController = factory();
   var resolveWasCalled = [];
-  registry.resolver = function(fullName) {
-    resolveWasCalled.push(fullName);
-    return PostController;
+
+  registry.resolver = {
+    resolve(fullName) {
+      resolveWasCalled.push(fullName);
+      return PostController;
+    }
   };
 
   deepEqual(resolveWasCalled, []);
@@ -247,9 +259,12 @@ QUnit.test('factory for non extendables resolves are cached', function() {
   var registry = new Registry();
   var PostController = {};
   var resolveWasCalled = [];
-  registry.resolver = function(fullName) {
-    resolveWasCalled.push(fullName);
-    return PostController;
+
+  registry.resolver = {
+    resolve(fullName) {
+      resolveWasCalled.push(fullName);
+      return PostController;
+    }
   };
 
   deepEqual(resolveWasCalled, []);
@@ -271,6 +286,84 @@ QUnit.test('registry.container creates a container', function() {
   ok(postController instanceof PostController, 'The lookup is an instance of the registered factory');
 });
 
+QUnit.test('`describe` will be handled by the resolver, then by the fallback registry, if available', function() {
+  let fallback = {
+    describe(fullName) {
+      return `${fullName}-fallback`;
+    }
+  };
+
+  let resolver = {
+    lookupDescription(fullName) {
+      return `${fullName}-resolver`;
+    }
+  };
+
+  let registry = new Registry({ fallback, resolver });
+
+  equal(registry.describe('controller:post'), 'controller:post-resolver', '`describe` handled by the resolver first.');
+
+  registry.resolver = null;
+
+  equal(registry.describe('controller:post'), 'controller:post-fallback', '`describe` handled by fallback registry next.');
+
+  registry.fallback = null;
+
+  equal(registry.describe('controller:post'), 'controller:post', '`describe` by default returns argument.');
+});
+
+QUnit.test('`normalizeFullName` will be handled by the resolver, then by the fallback registry, if available', function() {
+  let fallback = {
+    normalizeFullName(fullName) {
+      return `${fullName}-fallback`;
+    }
+  };
+
+  let resolver = {
+    normalize(fullName) {
+      return `${fullName}-resolver`;
+    }
+  };
+
+  let registry = new Registry({ fallback, resolver });
+
+  equal(registry.normalizeFullName('controller:post'), 'controller:post-resolver', '`normalizeFullName` handled by the resolver first.');
+
+  registry.resolver = null;
+
+  equal(registry.normalizeFullName('controller:post'), 'controller:post-fallback', '`normalizeFullName` handled by fallback registry next.');
+
+  registry.fallback = null;
+
+  equal(registry.normalizeFullName('controller:post'), 'controller:post', '`normalizeFullName` by default returns argument.');
+});
+
+QUnit.test('`makeToString` will be handled by the resolver, then by the fallback registry, if available', function() {
+  let fallback = {
+    makeToString(fullName) {
+      return `${fullName}-fallback`;
+    }
+  };
+
+  let resolver = {
+    makeToString(fullName) {
+      return `${fullName}-resolver`;
+    }
+  };
+
+  let registry = new Registry({ fallback, resolver });
+
+  equal(registry.makeToString('controller:post'), 'controller:post-resolver', '`makeToString` handled by the resolver first.');
+
+  registry.resolver = null;
+
+  equal(registry.makeToString('controller:post'), 'controller:post-fallback', '`makeToString` handled by fallback registry next.');
+
+  registry.fallback = null;
+
+  equal(registry.makeToString('controller:post'), 'controller:post', '`makeToString` by default returns argument.');
+});
+
 QUnit.test('`resolve` can be handled by a fallback registry', function() {
   var fallback = new Registry();
 
@@ -375,12 +468,13 @@ QUnit.test('`knownForType` includes fallback registry results', function() {
 QUnit.test('`knownForType` is called on the resolver if present', function() {
   expect(3);
 
-  function resolver() { }
-  resolver.knownForType = function(type) {
-    ok(true, 'knownForType called on the resolver');
-    equal(type, 'foo', 'the type was passed through');
+  let resolver = {
+    knownForType(type) {
+      ok(true, 'knownForType called on the resolver');
+      equal(type, 'foo', 'the type was passed through');
 
-    return { 'foo:yorp': true };
+      return { 'foo:yorp': true };
+    }
   };
 
   var registry = new Registry({
@@ -395,3 +489,19 @@ QUnit.test('`knownForType` is called on the resolver if present', function() {
     'foo:bar-baz': true
   });
 });
+
+QUnit.test('A registry can be created with a deprecated `resolver` function instead of an object', function() {
+  expect(2);
+
+  let registry;
+
+  expectDeprecation(function() {
+    registry = new Registry({
+      resolver(fullName) {
+        return `${fullName}-resolved`;
+      }
+    });
+  }, 'Passing a `resolver` function into a Registry is deprecated. Please pass in a Resolver object with a `resolve` method.');
+
+  equal(registry.resolve('foo:bar'), 'foo:bar-resolved', '`resolve` still calls the deprecated function');
+});