fix(core): memory leak in event listeners inside embedded views (#43075)

When we have an event listener inside an embedded view, we generate a `restoreView` call which saves the view inside of the LFrame. The problem is that we don't clear it until it gets overwritten which can lead to memory leaks. These changes rework the generated code in order to generate a `resetView` call which will clear the view from the LFrame. Fixes #42848. PR Close #43075
angular · Mar 25, 2022 · d817d37 · d817d37
1 parent 6d4a3f7
commit d817d37
Show file tree

Hide file tree

Showing 17 changed files with 68 additions and 27 deletions.
diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json
@@ -15,7 +15,7 @@
     "master": {
       "uncompressed": {
         "runtime": 4343,
-        "main": 450342,
+        "main": 450913,
         "polyfills": 33869,
         "styles": 70416,
         "light-theme": 77582,

diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json
@@ -24,7 +24,7 @@
     "master": {
       "uncompressed": {
         "runtime": 1105,
-        "main": 131882,
+        "main": 132392,
         "polyfills": 33846
       }
     }

diff --git a/...ompliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context_template.js b/...ompliance/test_cases/r3_view_compiler_listener/embedded_view_listener_context_template.js
@@ -5,7 +5,7 @@ function MyComponent_ng_template_0_Template(rf, $ctx$) {
     $i0$.ɵɵlistener("click", function MyComponent_ng_template_0_Template_button_click_0_listener() {
       const restoredCtx = $i0$.ɵɵrestoreView(_r3);
       const $obj_r1$ = restoredCtx.$implicit;
-      return $obj_r1$.value = 1;
+      return $i0$.ɵɵresetView($obj_r1$.value = 1);
     });
     $i0$.ɵɵtext(1, "Change");
     $i0$.ɵɵelementEnd();

diff --git a/...cases/r3_view_compiler_listener/implicit_receiver_keyed_write_inside_template_template.js b/...cases/r3_view_compiler_listener/implicit_receiver_keyed_write_inside_template_template.js
@@ -5,7 +5,7 @@ function MyComponent_ng_template_0_Template(rf, $ctx$) {
     $i0$.ɵɵlistener("click", function MyComponent_ng_template_0_Template_button_click_0_listener() {
       $i0$.ɵɵrestoreView(_r3);
       const $ctx_2$ = $i0$.ɵɵnextContext();
-      return ($ctx_2$["mes" + "sage"] = "hello");
+      return $i0$.ɵɵresetView(($ctx_2$["mes" + "sage"] = "hello"));
     });
     $i0$.ɵɵtext(1, "Click me");
     $i0$.ɵɵelementEnd();

diff --git a/...est/compliance/test_cases/r3_view_compiler_listener/local_ref_before_listener_template.js b/...est/compliance/test_cases/r3_view_compiler_listener/local_ref_before_listener_template.js
@@ -12,7 +12,7 @@ MyComponent.ɵcmp = /*@__PURE__*/ $r3$.ɵɵdefineComponent({
         $r3$.ɵɵlistener("click", function MyComponent_Template_button_click_0_listener() {
            $r3$.ɵɵrestoreView($s$);
            const $user$ = $r3$.ɵɵreference(3);
-           return ctx.onClick($user$.value);
+           return $r3$.ɵɵresetView(ctx.onClick($user$.value));
         });
         $r3$.ɵɵtext(1, "Save");
       $r3$.ɵɵelementEnd();

diff --git a/...est/compliance/test_cases/r3_view_compiler_listener/shared_snapshot_listeners_template.js b/...est/compliance/test_cases/r3_view_compiler_listener/shared_snapshot_listeners_template.js
@@ -5,14 +5,14 @@ function MyComponent_div_0_Template(rf, ctx) {
     $r3$.ɵɵlistener("click", function MyComponent_div_0_Template_div_click_1_listener() {
       $r3$.ɵɵrestoreView($s$);
       const $comp$ = $r3$.ɵɵnextContext();
-      return $comp$.onClick($comp$.foo);
+      return $i0$.ɵɵresetView($comp$.onClick($comp$.foo));
     });
     $r3$.ɵɵelementEnd();
     $r3$.ɵɵelementStart(2, "button", 1);
     $r3$.ɵɵlistener("click", function MyComponent_div_0_Template_button_click_2_listener() {
       $r3$.ɵɵrestoreView($s$);
       const $comp2$ = $r3$.ɵɵnextContext();
-      return $comp2$.onClick2($comp2$.bar);
+      return $i0$.ɵɵresetView($comp2$.onClick2($comp2$.bar));
     });
     $r3$.ɵɵelementEnd()();
   }

diff --git a/...er-cli/test/compliance/test_cases/r3_view_compiler_template/implicit_receiver_template.js b/...er-cli/test/compliance/test_cases/r3_view_compiler_template/implicit_receiver_template.js
@@ -5,7 +5,7 @@ function MyComponent_div_0_Template(rf, ctx) {
     $r3$.ɵɵlistener("click", function MyComponent_div_0_Template_div_click_0_listener() {
       i0.ɵɵrestoreView($_r2$);
       const $ctx_r1$ = i0.ɵɵnextContext();
-      return $ctx_r1$.greet($ctx_r1$);
+      return $i0$.ɵɵresetView($ctx_r1$.greet($ctx_r1$));
     });
     $r3$.ɵɵelementEnd();
   }

diff --git a/...ce/test_cases/r3_view_compiler_template/nested_template_context_many_bindings_template.js b/...ce/test_cases/r3_view_compiler_template/nested_template_context_many_bindings_template.js
@@ -7,7 +7,7 @@ function MyComponent_div_0_Template(rf, ctx) {
       const $d$ = $sr$.$implicit;
       const $i$ = $sr$.index;
       const $comp$ = $r3$.ɵɵnextContext();
-      return $comp$._handleClick($d$, $i$);
+      return $i0$.ɵɵresetView($comp$._handleClick($d$, $i$));
     });
     $r3$.ɵɵelementEnd();
   }

diff --git a/.../test/compliance/test_cases/r3_view_compiler_template/nested_template_context_template.js b/.../test/compliance/test_cases/r3_view_compiler_template/nested_template_context_template.js
@@ -8,7 +8,7 @@ function MyComponent_ul_0_li_1_div_1_Template(rf, ctx) {
       const $middle$ = $i0$.ɵɵnextContext().$implicit;
       const $outer$ = $i0$.ɵɵnextContext().$implicit;
       const $myComp$ = $i0$.ɵɵnextContext();
-      return $myComp$.onClick($outer$, $middle$, $inner$);
+      return $i0$.ɵɵresetView($myComp$.onClick($outer$, $middle$, $inner$));
     });
     $i0$.ɵɵtext(1);
     $i0$.ɵɵelementEnd();

diff --git a/packages/compiler/src/render3/r3_identifiers.ts b/packages/compiler/src/render3/r3_identifiers.ts
@@ -135,6 +135,8 @@ export class Identifiers {
 
   static nextContext: o.ExternalReference = {name: 'ɵɵnextContext', moduleName: CORE};
 
+  static resetView: o.ExternalReference = {name: 'ɵɵresetView', moduleName: CORE};
+
   static templateCreate: o.ExternalReference = {name: 'ɵɵtemplate', moduleName: CORE};
 
   static text: o.ExternalReference = {name: 'ɵɵtext', moduleName: CORE};

diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts
@@ -81,14 +81,32 @@ export function prepareEventListenerParameters(
       scope, implicitReceiverExpr, handler, 'b', eventAst.handlerSpan, implicitReceiverAccesses,
       EVENT_BINDING_SCOPE_GLOBALS);
   const statements = [];
-  if (scope) {
+  const variableDeclarations = scope?.variableDeclarations();
+  const restoreViewStatement = scope?.restoreViewStatement();
+
+  if (variableDeclarations) {
     // `variableDeclarations` needs to run first, because
     // `restoreViewStatement` depends on the result.
-    statements.push(...scope.variableDeclarations());
-    statements.unshift(...scope.restoreViewStatement());
+    statements.push(...variableDeclarations);
   }
+
   statements.push(...bindingStatements);
 
+  if (restoreViewStatement) {
+    statements.unshift(restoreViewStatement);
+
+    // If there's a `restoreView` call, we need to reset the view at the end of the listener
+    // in order to avoid a leak. If there's a `return` statement already, we wrap it in the
+    // call, e.g. `return resetView(ctx.foo())`. Otherwise we add the call as the last statement.
+    const lastStatement = statements[statements.length - 1];
+    if (lastStatement instanceof o.ReturnStatement) {
+      statements[statements.length - 1] = new o.ReturnStatement(
+          invokeInstruction(lastStatement.value.sourceSpan, R3.resetView, [lastStatement.value]));
+    } else {
+      statements.push(new o.ExpressionStatement(invokeInstruction(null, R3.resetView, [])));
+    }
+  }
+
   const eventName: string =
       type === ParsedEventType.Animation ? prepareSyntheticListenerName(name, phase!) : name;
   const fnName = handlerName && sanitizeIdentifier(handlerName);
@@ -1760,18 +1778,16 @@ export class BindingScope implements LocalResolver {
     }
   }
 
-  restoreViewStatement(): o.Statement[] {
-    const statements: o.Statement[] = [];
+  restoreViewStatement(): o.Statement|null {
     if (this.restoreViewVariable) {
       const restoreCall = invokeInstruction(null, R3.restoreView, [this.restoreViewVariable]);
       // Either `const restoredCtx = restoreView($state$);` or `restoreView($state$);`
       // depending on whether it is being used.
-      statements.push(
-          this.usesRestoredViewContext ?
-              o.variable(RESTORED_VIEW_CONTEXT_NAME).set(restoreCall).toConstDecl() :
-              restoreCall.toStmt());
+      return this.usesRestoredViewContext ?
+          o.variable(RESTORED_VIEW_CONTEXT_NAME).set(restoreCall).toConstDecl() :
+          restoreCall.toStmt();
     }
-    return statements;
+    return null;
   }
 
   viewSnapshotStatements(): o.Statement[] {

diff --git a/packages/core/src/core_render3_private_export.ts b/packages/core/src/core_render3_private_export.ts
@@ -164,6 +164,7 @@ export {
   ɵɵpureFunctionV,
   ɵɵqueryRefresh,
   ɵɵreference,
+  ɵɵresetView,
   ɵɵresolveBody,
   ɵɵresolveDocument,
   ɵɵresolveWindow,

diff --git a/packages/core/src/render3/index.ts b/packages/core/src/render3/index.ts
@@ -168,6 +168,7 @@ export {
   ɵɵdisableBindings,
 
   ɵɵenableBindings,
+  ɵɵresetView,
   ɵɵrestoreView,
 } from './state';
 export {NO_CHANGE} from './tokens';

diff --git a/packages/core/src/render3/jit/environment.ts b/packages/core/src/render3/jit/environment.ts
@@ -44,6 +44,7 @@ export const angularCoreEnv: {[name: string]: Function} =
        'ɵɵinvalidFactory': r3.ɵɵinvalidFactory,
        'ɵɵinvalidFactoryDep': ɵɵinvalidFactoryDep,
        'ɵɵtemplateRefExtractor': r3.ɵɵtemplateRefExtractor,
+       'ɵɵresetView': r3.ɵɵresetView,
        'ɵɵNgOnChangesFeature': r3.ɵɵNgOnChangesFeature,
        'ɵɵProvidersFeature': r3.ɵɵProvidersFeature,
        'ɵɵCopyDefinitionFeature': r3.ɵɵCopyDefinitionFeature,

diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts
@@ -8,6 +8,7 @@
 
 import {InjectFlags} from '../di/interface/injector';
 import {assertDefined, assertEqual, assertGreaterThanOrEqual, assertLessThan, assertNotEqual} from '../util/assert';
+
 import {assertLViewOrUndefined, assertTNodeForLView, assertTNodeForTView} from './assert';
 import {DirectiveDef} from './interfaces/definition';
 import {TNode, TNodeType} from './interfaces/node';
@@ -82,7 +83,7 @@ interface LFrame {
    *
    * e.g. const inner = x().$implicit; const outer = x().$implicit;
    */
-  contextLView: LView;
+  contextLView: LView|null;
 
   /**
    * Store the element depth count. This is used to identify the root elements of the template
@@ -294,6 +295,18 @@ export function ɵɵrestoreView<T = any>(viewToRestore: OpaqueViewState): T {
 }
 
 
+/**
+ * Clears the view set in `ɵɵrestoreView` from memory. Returns the passed in
+ * value so that it can be used as a return value of an instruction.
+ *
+ * @codeGenApi
+ */
+export function ɵɵresetView<T>(value?: T): T|undefined {
+  instructionState.lFrame.contextLView = null;
+  return value;
+}
+
+
 export function getCurrentTNode(): TNode|null {
   let currentTNode = getCurrentTNodePlaceholderOk();
   while (currentTNode !== null && currentTNode.type === TNodeType.Placeholder) {
@@ -331,7 +344,9 @@ export function setCurrentTNodeAsParent(): void {
 }
 
 export function getContextLView(): LView {
-  return instructionState.lFrame.contextLView;
+  const contextLView = instructionState.lFrame.contextLView;
+  ngDevMode && assertDefined(contextLView, 'contextLView must be defined.');
+  return contextLView!;
 }
 
 export function isInCheckNoChangesMode(): boolean {
@@ -553,7 +568,7 @@ export function enterView(newView: LView): void {
   newLFrame.currentTNode = tView.firstChild!;
   newLFrame.lView = newView;
   newLFrame.tView = tView;
-  newLFrame.contextLView = newView!;
+  newLFrame.contextLView = newView;
   newLFrame.bindingIndex = tView.bindingStartIndex;
   newLFrame.inI18n = false;
 }
@@ -575,7 +590,7 @@ function createLFrame(parent: LFrame|null): LFrame {
     lView: null!,
     tView: null!,
     selectedIndex: -1,
-    contextLView: null!,
+    contextLView: null,
     elementDepthCount: 0,
     currentNamespace: null,
     currentDirectiveIndex: -1,
@@ -628,7 +643,7 @@ export function leaveView() {
   oldLFrame.isParent = true;
   oldLFrame.tView = null!;
   oldLFrame.selectedIndex = -1;
-  oldLFrame.contextLView = null!;
+  oldLFrame.contextLView = null;
   oldLFrame.elementDepthCount = 0;
   oldLFrame.currentDirectiveIndex = -1;
   oldLFrame.currentNamespace = null;

diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -872,6 +872,9 @@
   {
     "name": "ɵɵreference"
   },
+  {
+    "name": "ɵɵresetView"
+  },
   {
     "name": "ɵɵrestoreView"
   },

diff --git a/packages/core/test/render3/listeners_spec.ts b/packages/core/test/render3/listeners_spec.ts
@@ -8,11 +8,13 @@
 
 import {HEADER_OFFSET} from '@angular/core/src/render3/interfaces/view';
 import {dispatchEvent} from '@angular/platform-browser/testing/src/browser_util';
+
 import {ɵɵdefineComponent, ɵɵdefineDirective, ɵɵreference, ɵɵresolveBody, ɵɵresolveDocument} from '../../src/render3/index';
 import {ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵgetCurrentView, ɵɵlistener, ɵɵtext} from '../../src/render3/instructions/all';
 import {RenderFlags} from '../../src/render3/interfaces/definition';
 import {GlobalTargetResolver} from '../../src/render3/interfaces/renderer';
-import {ɵɵrestoreView} from '../../src/render3/state';
+import {ɵɵresetView, ɵɵrestoreView} from '../../src/render3/state';
+
 import {getRendererFactory2} from './imported_renderer2';
 import {ComponentFixture, containerEl, createComponent, getDirectiveOnNode, renderToHtml, TemplateFixture} from './render_util';
 
@@ -489,7 +491,7 @@ describe('event listeners', () => {
                   ɵɵlistener('click', function() {
                     ɵɵrestoreView(state);
                     const comp = ɵɵreference(1);
-                    return ctx.onClick(comp);
+                    return ɵɵresetView(ctx.onClick(comp));
                   });
                 }
                 ɵɵelementEnd();