fix($compile): do not bind twice transclude functions

[lgalfaso]

Do not double bind transclude functions that are passed to the compiler that were already bound

Closes #9413

[dlongley]

Note that the containingScope still needs to get passed through to the already-bound transclude function somehow so that it can be used when creating the new transcluded scope. Either that, or the bound scope could be made available via the $$bound property (instead of using a boolean) and accessed that way when re-wrapping the transclude function. Then transcludeFn.$$bound.$new(false, containingScope); could be called to prevent memory leaks.

The problem here is that both the bound scope and the containing scope both need to be known when creating the new transcluded scope. This can be addressed by allowing the containing scope to be passed to the (bound) transclude function as a fourth argument or by making the bound scope accessible via a property like $$bound. The second approach would probably need to do this on both the "bound transclude function" and the "controllers bound transclude function".

See: 9300ed2#commitcomment-8130430

[dlongley]

I believe this diff is one way to address the containingScope memory leak issue.

index 8be86d7..d52da46 100644
--- a/src/ng/compile.js
+++ b/src/ng/compile.js
@@ -1153,7 +1153,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
                            maxPriority, ignoreDirective, previousCompileContext);
       compile.$$addScopeClass($compileNodes);
       var namespace = null;
-      return function publicLinkFn(scope, cloneConnectFn, transcludeControllers, parentBoundTranscludeFn, futureParentElement){
+      return function publicLinkFn(scope, cloneConnectFn, futureParentElement, transcludeControllers, parentBoundTranscludeFn){
         assertArg(scope, 'scope');
         if (!namespace) {
           namespace = detectNamespaceForChildElements(futureParentElement);
@@ -1317,16 +1317,23 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

     function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn, elementTransclusion) {

-      var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement, containingScope) {
+      // preserve previously bound scope
+      if (transcludeFn.$$bound) {
+        scope = transcludeFn.$$bound;
+      }
+
+      var boundTranscludeFn = function(transcludedScope, cloneFn, futureParentElement, controllers, containingScope) {

         if (!transcludedScope) {
           transcludedScope = scope.$new(false, containingScope);
           transcludedScope.$$transcluded = true;
         }

-        return transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement);
+        return transcludeFn(transcludedScope, cloneFn, futureParentElement, controllers, previousBoundTranscludeFn);
       };

+      boundTranscludeFn.$$bound = scope;
+
       return boundTranscludeFn;
     }

@@ -1793,7 +1800,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
           isolateScope = scope.$new(true);
         }

-        transcludeFn = boundTranscludeFn && controllersBoundTransclude;
+        if (boundTranscludeFn) {
+          transcludeFn = controllersBoundTransclude;
+          if (boundTranscludeFn.$$bound) {
+            transcludeFn.$$bound = boundTranscludeFn.$$bound;
+          }
+        }
         if (controllerDirectives) {
           // TODO: merge `controllers` and `elementControllers` into single object.
           controllers = {};
@@ -1953,7 +1965,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
           var transcludeControllers;

           // No scope passed in:
-          if (!isScope(scope)) {
+          if (!isScope(scope) && !isUndefined(scope)) {
             futureParentElement = cloneAttachFn;
             cloneAttachFn = scope;
             scope = undefined;
@@ -1965,7 +1977,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
           if (!futureParentElement) {
             futureParentElement = hasElementTranscludeDirective ? $element.parent() : $element;
           }
-          return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers, futureParentElement, scopeToChild);
+          return boundTranscludeFn(scope, cloneAttachFn, futureParentElement, transcludeControllers, scopeToChild);
         }
       }
     }

[lgalfaso]

@dlongley can you please create a test that there is a leak?

[dlongley]

@lgalfaso, yeah, currently working on it.

[lgalfaso]

@dlongley ok, I am able to create a leak, but the patch that you posted does not fix this

[dlongley]

@lgalfaso, does the patch improve upon the leak you found? The leak I was trying to get into a test would occur when the isolate scope would be destroyed (or its element removed) -- this would not destroy the transcluded scope. You may have found yet another leak.

[lgalfaso]

no, it is the same leak, but the patch you posted makes no difference. I need some extra time trying to think of the right way to fix it

[dlongley]

@lgalfaso, I was able to use that patch to fix it when checking via chrome dev tools ... hmm.

[dlongley]

@lgalfaso, I'm available on IRC #angularjs for a bit to chat about this if it would help. I'm trying to run the angular.js test suite to try out a test I'm working on, but my machine can barely even run the tests (too slow, not enough RAM, etc). I was just copying some other tests that count scopes (found just below the test you had in this PR) and was looking to see if the scopes get properly destroyed and removed when you remove the uses-transclude element, etc.

[petebacondarwin]

We could discuss this here if you like:
https://gitter.im/angular/PR-9579

[petebacondarwin]

Why change the order of these parameters?

[petebacondarwin]

Oh I see it makes it simpler to call later on but I worry that this is not really necessary and is obscuring the PR

[lgalfaso]

At boundTranscludeFn we do not know if we have controllersBoundTransclude or a publicLinkFn. Reordering the parameters so they match was the simplest alternative

[petebacondarwin]

I think that if we are going to refactor the params then we should go with an option object style approach:

transcludeFn(scope, cloneConnectFn, {
  transcludeControllers: ...,
  parentBoundTranscludeFn: ...,
  futureParentElement: ...
});

[tbosch]

+1!

[lgalfaso]

Agree

[dlongley]

+1

[dlongley]

LGTM

[tbosch]

Hi,
this might actually be a simpler solution than the one I proposed...
Let's look into this right after we release 1.3...

[tbosch]

Ok, thought over it. I don't like passing this additional scope around... Also the rewrapping could cause problems in the future because the arguments of the bound and unbound transclude functions do not match (starting with the 2nd argument). Closing this in favor of a solution that passes in the transcludeFn into the publicLinkFn as explained in the issue...

[lgalfaso]

If transcludeFn is a private API, then it should be removed from https://docs.angularjs.org/api/ng/service/$compile#usage and not cause any future confusion

[tbosch]

Actually, it should be a deprecated API, similar to the transcludeFn that is passed to the compile function of directives.