fix(upgrade): avoid memory leak when removing downgraded components

packages/upgrade/src/common/src/angular1.ts

@@ -126,6 +126,7 @@ export type IAugmentedJQuery = Node[]&{
   data?: (name: string, value?: any) => any;
   text?: () => string;
   inheritedData?: (name: string, value?: any) => any;
+  children?: () => IAugmentedJQuery;
   contents?: () => IAugmentedJQuery;
   parent?: () => IAugmentedJQuery;
   empty?: () => void;

packages/upgrade/src/common/src/downgrade_component.ts

@@ -174,8 +174,8 @@ export function downgradeComponent(info: {
 
           const injectorPromise = new ParentInjectorPromise(element);
           const facade = new DowngradeComponentAdapter(
-              element, attrs, scope, ngModel, injector, $injector, $compile, $parse,
-              componentFactory, wrapCallback);
+              element, attrs, scope, ngModel, injector, $compile, $parse, componentFactory,
+              wrapCallback);
 
           const projectableNodes = facade.compileContents();
           facade.createComponent(projectableNodes);

packages/upgrade/src/common/src/downgrade_component_adapter.ts

@@ -8,7 +8,7 @@
 
 import {ApplicationRef, ChangeDetectorRef, ComponentFactory, ComponentRef, EventEmitter, Injector, OnChanges, SimpleChange, SimpleChanges, StaticProvider, Testability, TestabilityRegistry, Type} from '@angular/core';
 
-import {IAttributes, IAugmentedJQuery, ICompileService, IInjectorService, INgModelController, IParseService, IScope} from './angular1';
+import {element as angularElement, IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1';
 import {PropertyBinding} from './component_info';
 import {$SCOPE} from './constants';
 import {getTypeName, hookupNgModel, strictEquals} from './util';
@@ -33,8 +33,8 @@ export class DowngradeComponentAdapter {
   constructor(
       private element: IAugmentedJQuery, private attrs: IAttributes, private scope: IScope,
       private ngModel: INgModelController, private parentInjector: Injector,
-      private $injector: IInjectorService, private $compile: ICompileService,
-      private $parse: IParseService, private componentFactory: ComponentFactory<any>,
+      private $compile: ICompileService, private $parse: IParseService,
+      private componentFactory: ComponentFactory<any>,
       private wrapCallback: <T>(cb: () => T) => () => T) {
     this.componentScope = scope.$new();
   }
@@ -216,11 +216,38 @@ export class DowngradeComponentAdapter {
     const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy());
     let destroyed = false;
 
-    this.element.on!('$destroy', () => this.componentScope.$destroy());
+    this.element.on!('$destroy', () => {
+      // The `$destroy` event may have been triggered by the `cleanData()` call in the
+      // `componentScope` `$destroy` handler below. In that case, we don't want to call
+      // `componentScope.$destroy()` again.
+      if (!destroyed) this.componentScope.$destroy();
+    });
     this.componentScope.$on('$destroy', () => {
       if (!destroyed) {
         destroyed = true;
         testabilityRegistry.unregisterApplication(this.componentRef.location.nativeElement);
+
+        // The `componentScope` might be getting destroyed, because an ancestor element is being
+        // removed/destroyed. If that is the case, jqLite/jQuery would normally invoke `cleanData()`
+        // on the removed element and all descendants.
+        //   https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/jqLite.js#L349-L355
+        //   https://github.com/jquery/jquery/blob/6984d1747623dbc5e87fd6c261a5b6b1628c107c/src/manipulation.js#L182
+        //
+        // Here, however, `destroyComponentRef()` may under some circumstances remove the element
+        // from the DOM and therefore it will no longer be a descendant of the removed element when
+        // `cleanData()` is called. This would result in a memory leak, because the element's data
+        // and event handlers (and all objects directly or indirectly referenced by them) would be
+        // retained.
+        //
+        // To ensure the element is always properly cleaned up, we manually call `cleanData()` on
+        // this element and its descendants before destroying the `ComponentRef`.
+        //
+        // NOTE:
+        // `cleanData()` also will invoke the AngularJS `$destroy` event on the element:
+        //   https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945
+        angularElement.cleanData(this.element);
+        angularElement.cleanData((this.element[0] as Element).querySelectorAll('*'));
+
         destroyComponentRef();
       }
     });
@@ -250,7 +277,6 @@ export class DowngradeComponentAdapter {
  */
 export function groupNodesBySelector(ngContentSelectors: string[], nodes: Node[]): Node[][] {
   const projectableNodes: Node[][] = [];
-  let wildcardNgContentIndex: number;
 
   for (let i = 0, ii = ngContentSelectors.length; i < ii; ++i) {
     projectableNodes[i] = [];

packages/upgrade/src/common/src/upgrade_helper.ts

@@ -41,8 +41,7 @@ export class UpgradeHelper {
   private readonly $controller: IControllerService;
 
   constructor(
-      private injector: Injector, private name: string, elementRef: ElementRef,
-      directive?: IDirective) {
+      injector: Injector, private name: string, elementRef: ElementRef, directive?: IDirective) {
     this.$injector = injector.get($INJECTOR);
     this.$compile = this.$injector.get($COMPILE);
     this.$controller = this.$injector.get($CONTROLLER);

packages/upgrade/src/common/test/downgrade_component_adapter_spec.ts

@@ -131,7 +131,6 @@ withEachNg1Version(() => {
         let scope: angular.IScope;  // mock
         let ngModel = undefined as any;
         let parentInjector: Injector;  // testbed
-        let $injector = undefined as any;
         let $compile = undefined as any;
         let $parse = undefined as any;
         let componentFactory: ComponentFactory<any>;  // testbed
@@ -166,8 +165,8 @@ withEachNg1Version(() => {
         parentInjector = TestBed;
 
         return new DowngradeComponentAdapter(
-            element, attrs, scope, ngModel, parentInjector, $injector, $compile, $parse,
-            componentFactory, wrapCallback);
+            element, attrs, scope, ngModel, parentInjector, $compile, $parse, componentFactory,
+            wrapCallback);
       }
 
       beforeEach(() => {

packages/upgrade/src/dynamic/test/upgrade_spec.ts

@@ -665,23 +665,16 @@ withEachNg1Version(() => {
       it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => {
            const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
            const ng1Module = angular.module_('ng1', []);
-           const onDestroyed: EventEmitter<string> = new EventEmitter<string>();
+           let ng2ComponentDestroyed = false;
 
-           ng1Module.directive('ng1', () => {
-             return {
-               template: '<div ng-if="!destroyIt"><ng2></ng2></div>',
-               controller: function($rootScope: any, $timeout: Function) {
-                 $timeout(() => {
-                   $rootScope.destroyIt = true;
-                 });
-               }
-             };
-           });
+           ng1Module.directive('ng1', () => ({
+                                        template: '<div ng-if="!destroyIt"><ng2></ng2></div>',
+                                      }));
 
-           @Component({selector: 'ng2', template: 'test'})
+           @Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
            class Ng2 {
              ngOnDestroy() {
-               onDestroyed.emit('destroyed');
+               ng2ComponentDestroyed = true;
              }
            }
 
@@ -695,9 +688,35 @@ withEachNg1Version(() => {
            ng1Module.directive('ng2', adapter.downgradeNg2Component(Ng2));
            const element = html('<ng1></ng1>');
            adapter.bootstrap(element, ['ng1']).ready((ref) => {
-             onDestroyed.subscribe(() => {
-               ref.dispose();
-             });
+             const ng2Element = angular.element(element.querySelector('ng2') as Element);
+             const ng2Descendants =
+                 Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
+             let ng2ElementDestroyed = false;
+             let ng2DescendantsDestroyed = ng2Descendants.map(() => false);
+
+             ng2Element.data!('test', 42);
+             ng2Descendants.forEach((elem, i) => elem.data!('test', i));
+             ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
+             ng2Descendants.forEach(
+                 (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
+
+             expect(element.textContent).toBe('test1test2');
+             expect(ng2Element.data!('test')).toBe(42);
+             ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
+             expect(ng2ElementDestroyed).toBe(false);
+             expect(ng2DescendantsDestroyed).toEqual([false, false]);
+             expect(ng2ComponentDestroyed).toBe(false);
+
+             ref.ng1RootScope.$apply('destroyIt = true');
+
+             expect(element.textContent).toBe('');
+             expect(ng2Element.data!('test')).toBeUndefined();
+             ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
+             expect(ng2ElementDestroyed).toBe(true);
+             expect(ng2DescendantsDestroyed).toEqual([true, true]);
+             expect(ng2ComponentDestroyed).toBe(true);
+
+             ref.dispose();
            });
          }));
 

packages/upgrade/static/test/integration/downgrade_component_spec.ts

@@ -536,7 +536,7 @@ withEachNg1Version(() => {
 
     it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => {
          let destroyed = false;
-         @Component({selector: 'ng2', template: 'test'})
+         @Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
          class Ng2Component implements OnDestroy {
            ngOnDestroy() {
              destroyed = true;
@@ -563,14 +563,35 @@ withEachNg1Version(() => {
          platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => {
            const adapter = ref.injector.get(UpgradeModule) as UpgradeModule;
            adapter.bootstrap(element, [ng1Module.name]);
-           expect(element.textContent).toContain('test');
+
+           const ng2Element = angular.element(element.querySelector('ng2') as Element);
+           const ng2Descendants =
+               Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
+           let ng2ElementDestroyed = false;
+           let ng2DescendantsDestroyed = [false, false];
+
+           ng2Element.data!('test', 42);
+           ng2Descendants.forEach((elem, i) => elem.data!('test', i));
+           ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
+           ng2Descendants.forEach(
+               (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
+
+           expect(element.textContent).toBe('test1test2');
            expect(destroyed).toBe(false);
+           expect(ng2Element.data!('test')).toBe(42);
+           ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
+           expect(ng2ElementDestroyed).toBe(false);
+           expect(ng2DescendantsDestroyed).toEqual([false, false]);
 
            const $rootScope = adapter.$injector.get('$rootScope');
            $rootScope.$apply('destroyIt = true');
 
-           expect(element.textContent).not.toContain('test');
+           expect(element.textContent).toBe('');
            expect(destroyed).toBe(true);
+           expect(ng2Element.data!('test')).toBeUndefined();
+           ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
+           expect(ng2ElementDestroyed).toBe(true);
+           expect(ng2DescendantsDestroyed).toEqual([true, true]);
          });
        }));
 

packages/upgrade/static/test/integration/downgrade_module_spec.ts

@@ -1187,6 +1187,69 @@ withEachNg1Version(() => {
            });
          }));
 
+      it('should properly run cleanup when a downgraded component is destroyed',
+         waitForAsync(() => {
+           let destroyed = false;
+
+           @Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
+           class Ng2Component implements OnDestroy {
+             ngOnDestroy() {
+               destroyed = true;
+             }
+           }
+
+           @NgModule({
+             declarations: [Ng2Component],
+             entryComponents: [Ng2Component],
+             imports: [BrowserModule],
+           })
+           class Ng2Module {
+             ngDoBootstrap() {}
+           }
+
+           const bootstrapFn = (extraProviders: StaticProvider[]) =>
+               platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module);
+           const lazyModuleName = downgradeModule<Ng2Module>(bootstrapFn);
+           const ng1Module =
+               angular.module_('ng1', [lazyModuleName])
+                   .directive(
+                       'ng2', downgradeComponent({component: Ng2Component, propagateDigest}));
+
+           const element = html('<div><div ng-if="!hideNg2"><ng2></ng2></div></div>');
+           const $injector = angular.bootstrap(element, [ng1Module.name]);
+           const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService;
+
+           setTimeout(() => {  // Wait for the module to be bootstrapped.
+             const ng2Element = angular.element(element.querySelector('ng2') as Element);
+             const ng2Descendants =
+                 Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
+             let ng2ElementDestroyed = false;
+             let ng2DescendantsDestroyed = [false, false];
+
+             ng2Element.data!('test', 42);
+             ng2Descendants.forEach((elem, i) => elem.data!('test', i));
+             ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
+             ng2Descendants.forEach(
+                 (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
+
+             expect(element.textContent).toBe('test1test2');
+             expect(destroyed).toBe(false);
+             expect(ng2Element.data!('test')).toBe(42);
+             ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
+             expect(ng2ElementDestroyed).toBe(false);
+             expect(ng2DescendantsDestroyed).toEqual([false, false]);
+
+             $rootScope.$apply('hideNg2 = true');
+
+             expect(element.textContent).toBe('');
+             expect(destroyed).toBe(true);
+             expect(ng2Element.data!('test')).toBeUndefined();
+             ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
+             expect(ng2ElementDestroyed).toBe(true);
+             expect(ng2DescendantsDestroyed).toEqual([true, true]);
+           });
+         }));
+
       it('should only retrieve the Angular zone once (and cache it for later use)',
          fakeAsync(() => {
            let count = 0;