From 314112de99bb97475a0d8bdbddf84a3b3ce4a8fb Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 15 Mar 2024 13:49:59 -0700 Subject: [PATCH] fix(core): Prevent `markForCheck` during change detection from causing infinite loops (#54900) This change updates the approach to the loop in `ApplicationRef.tick` for allowing state updates in `afterRender` hooks. It is valid to update state in render hooks and we need to ensure we refresh views that may be marked for check in these hooks (this can happen simply as a result of focusing an element). This change ensures that the behavior of `markForCheck` with respect to this loop does not change while we are actively running change detection on a view tree. This approach also has the benefit of preventing a regression for #18917, where updating state in animation listeners can cause `ExpressionChanged...Error` This should be allowed - there is nothing wrong with respect to unidirectional data flow in this case. There may be other cases in the future where it is valid to update state. Rather than wrapping the render hooks and the animation flushing in something which flips a global state flag, the idea here is that `markForCheck` is safe and valid in all cases whenever change detection is not actively running. PR Close #54900 --- .../core/src/application/application_ref.ts | 2 +- .../render3/instructions/change_detection.ts | 45 +++++++++++-------- .../render3/instructions/mark_view_dirty.ts | 16 ++++++- packages/core/src/render3/state.ts | 15 +++++++ .../bundle.golden_symbols.json | 9 ++++ .../animations/bundle.golden_symbols.json | 9 ++++ .../cyclic_import/bundle.golden_symbols.json | 9 ++++ .../bundling/defer/bundle.golden_symbols.json | 9 ++++ .../forms_reactive/bundle.golden_symbols.json | 9 ++++ .../bundle.golden_symbols.json | 9 ++++ .../hello_world/bundle.golden_symbols.json | 9 ++++ .../hydration/bundle.golden_symbols.json | 9 ++++ .../router/bundle.golden_symbols.json | 9 ++++ .../bundle.golden_symbols.json | 9 ++++ .../bundling/todo/bundle.golden_symbols.json | 9 ++++ 15 files changed, 156 insertions(+), 21 deletions(-) diff --git a/packages/core/src/application/application_ref.ts b/packages/core/src/application/application_ref.ts index 6fcb859c2a1..bcbcbbdb34e 100644 --- a/packages/core/src/application/application_ref.ts +++ b/packages/core/src/application/application_ref.ts @@ -722,7 +722,7 @@ export function detectChangesInViewIfRequired( } function shouldRecheckView(view: LView): boolean { - return requiresRefreshOrTraversal(view) || !!(view[FLAGS] & LViewFlags.Dirty); + return requiresRefreshOrTraversal(view); } function detectChangesInView(lView: LView, notifyErrorHandler: boolean, isFirstPass: boolean) { diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 78cf39938f5..b0597ae853e 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -16,7 +16,7 @@ import {CONTAINER_HEADER_OFFSET, LContainer, LContainerFlags, MOVED_VIEWS} from import {ComponentTemplate, RenderFlags} from '../interfaces/definition'; import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view'; import {getOrBorrowReactiveLViewConsumer, maybeReturnReactiveLViewConsumer, ReactiveLViewConsumer} from '../reactive_lview_consumer'; -import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state'; +import {enterView, isInCheckNoChangesMode, isRefreshingViews, leaveView, setBindingIndex, setIsInCheckNoChangesMode, setIsRefreshingViews} from '../state'; import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils'; import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, requiresRefreshOrTraversal, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils'; @@ -60,26 +60,33 @@ export function detectChangesInternal( } function detectChangesInViewWhileDirty(lView: LView, mode: ChangeDetectionMode) { - detectChangesInView(lView, mode); + const lastIsRefreshingViewsValue = isRefreshingViews(); + try { + setIsRefreshingViews(true); + detectChangesInView(lView, mode); - let retries = 0; - // If after running change detection, this view still needs to be refreshed or there are - // descendants views that need to be refreshed due to re-dirtying during the change detection - // run, detect changes on the view again. We run change detection in `Targeted` mode to only - // refresh views with the `RefreshView` flag. - while (requiresRefreshOrTraversal(lView)) { - if (retries === MAXIMUM_REFRESH_RERUNS) { - throw new RuntimeError( - RuntimeErrorCode.INFINITE_CHANGE_DETECTION, - ngDevMode && - 'Infinite change detection while trying to refresh views. ' + - 'There may be components which each cause the other to require a refresh, ' + - 'causing an infinite loop.'); + let retries = 0; + // If after running change detection, this view still needs to be refreshed or there are + // descendants views that need to be refreshed due to re-dirtying during the change detection + // run, detect changes on the view again. We run change detection in `Targeted` mode to only + // refresh views with the `RefreshView` flag. + while (requiresRefreshOrTraversal(lView)) { + if (retries === MAXIMUM_REFRESH_RERUNS) { + throw new RuntimeError( + RuntimeErrorCode.INFINITE_CHANGE_DETECTION, + ngDevMode && + 'Infinite change detection while trying to refresh views. ' + + 'There may be components which each cause the other to require a refresh, ' + + 'causing an infinite loop.'); + } + retries++; + // Even if this view is detached, we still detect changes in targeted mode because this was + // the root of the change detection run. + detectChangesInView(lView, ChangeDetectionMode.Targeted); } - retries++; - // Even if this view is detached, we still detect changes in targeted mode because this was - // the root of the change detection run. - detectChangesInView(lView, ChangeDetectionMode.Targeted); + } finally { + // restore state to what it was before entering this change detection loop + setIsRefreshingViews(lastIsRefreshingViewsValue); } } diff --git a/packages/core/src/render3/instructions/mark_view_dirty.ts b/packages/core/src/render3/instructions/mark_view_dirty.ts index 6f5355cacdd..0a097d6ddd8 100644 --- a/packages/core/src/render3/instructions/mark_view_dirty.ts +++ b/packages/core/src/render3/instructions/mark_view_dirty.ts @@ -8,6 +8,7 @@ import {isRootView} from '../interfaces/type_checks'; import {ENVIRONMENT, FLAGS, LView, LViewFlags} from '../interfaces/view'; +import {isRefreshingViews} from '../state'; import {getLViewParent} from '../util/view_utils'; /** @@ -22,9 +23,22 @@ import {getLViewParent} from '../util/view_utils'; * @returns the root LView */ export function markViewDirty(lView: LView): LView|null { + const dirtyBitsToUse = isRefreshingViews() ? + // When we are actively refreshing views, we only use the `Dirty` bit to mark a view + // for check. This bit is ignored in ChangeDetectionMode.Targeted, which is used to + // synchronously rerun change detection on a specific set of views (those which have + // the `RefreshView` flag and those with dirty signal consumers). `LViewFlags.Dirty` + // does not support re-entrant change detection on its own. + LViewFlags.Dirty : + // When we are not actively refreshing a view tree, it is absolutely + // valid to update state and mark views dirty. We use the `RefreshView` flag in this + // case to allow synchronously rerunning change detection. This applies today to + // afterRender hooks as well as animation listeners which execute after detecting + // changes in a view when the render factory flushes. + LViewFlags.RefreshView | LViewFlags.Dirty; lView[ENVIRONMENT].changeDetectionScheduler?.notify(); while (lView) { - lView[FLAGS] |= LViewFlags.Dirty; + lView[FLAGS] |= dirtyBitsToUse; const parent = getLViewParent(lView); // Stop traversing up as soon as you find a root view that wasn't attached to any container if (isRootView(lView) && !parent) { diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 8edad61cd37..37c5b15d8ae 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -202,6 +202,13 @@ const instructionState: InstructionState = { */ let _isInCheckNoChangesMode = false; +/** + * Flag used to indicate that we are in the middle running change detection on a view + * + * @see detectChangesInViewWhileDirty + */ +let _isRefreshingViews = false; + /** * Returns true if the instruction state stack is empty. * @@ -399,6 +406,14 @@ export function setIsInCheckNoChangesMode(mode: boolean): void { _isInCheckNoChangesMode = mode; } +export function isRefreshingViews(): boolean { + return _isRefreshingViews; +} + +export function setIsRefreshingViews(mode: boolean): void { + _isRefreshingViews = mode; +} + // top level variables should not be exported for performance reasons (PERF_NOTES.md) export function getBindingRoot() { const lFrame = instructionState.lFrame; diff --git a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json index 937f39ccdaf..36dfb0a4f5d 100644 --- a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json @@ -584,6 +584,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -1121,6 +1124,9 @@ { "name": "isPromise" }, + { + "name": "isRefreshingViews" + }, { "name": "isSubscription" }, @@ -1358,6 +1364,9 @@ { "name": "setInputsFromAttrs" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" }, diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json index c2b76610811..bda37e4668a 100644 --- a/packages/core/test/bundling/animations/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations/bundle.golden_symbols.json @@ -641,6 +641,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -1190,6 +1193,9 @@ { "name": "isPromise" }, + { + "name": "isRefreshingViews" + }, { "name": "isSubscription" }, @@ -1430,6 +1436,9 @@ { "name": "setInputsFromAttrs" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" }, diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index ae7f517db1a..8cdbb16c799 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -470,6 +470,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -941,6 +944,9 @@ { "name": "isPromise" }, + { + "name": "isRefreshingViews" + }, { "name": "isSubscription" }, @@ -1139,6 +1145,9 @@ { "name": "setInputsFromAttrs" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" }, diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json index 4ec07a3c82e..2642ca563a0 100644 --- a/packages/core/test/bundling/defer/bundle.golden_symbols.json +++ b/packages/core/test/bundling/defer/bundle.golden_symbols.json @@ -533,6 +533,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -2081,6 +2084,9 @@ { "name": "isPromise" }, + { + "name": "isRefreshingViews" + }, { "name": "isSubscription" }, @@ -2303,6 +2309,9 @@ { "name": "setInputsFromAttrs" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" }, diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json index 18cfbfb37f6..16d1fd6b35a 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -641,6 +641,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -1349,6 +1352,9 @@ { "name": "isReadableStreamLike" }, + { + "name": "isRefreshingViews" + }, { "name": "isStylingMatch" }, @@ -1652,6 +1658,9 @@ { "name": "setInputsFromAttrs" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" }, diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json index 92b6d668a6f..42be4e8cf45 100644 --- a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json @@ -629,6 +629,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -1310,6 +1313,9 @@ { "name": "isReadableStreamLike" }, + { + "name": "isRefreshingViews" + }, { "name": "isStylingMatch" }, @@ -1637,6 +1643,9 @@ { "name": "setInputsFromAttrs" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" }, diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index eaac26c0848..40627912285 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -359,6 +359,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_platformInjector" }, @@ -737,6 +740,9 @@ { "name": "isPromise" }, + { + "name": "isRefreshingViews" + }, { "name": "isSubscription" }, @@ -896,6 +902,9 @@ { "name": "setInjectImplementation" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" }, diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json index dfcdb0e5dfc..29517ddbe87 100644 --- a/packages/core/test/bundling/hydration/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hydration/bundle.golden_symbols.json @@ -524,6 +524,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -1037,6 +1040,9 @@ { "name": "isReadableStreamLike" }, + { + "name": "isRefreshingViews" + }, { "name": "isRootView" }, @@ -1265,6 +1271,9 @@ { "name": "setInjectImplementation" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSegmentHead" }, diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 46254d5c86f..7e6d73f2103 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -788,6 +788,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -1595,6 +1598,9 @@ { "name": "isReadableStreamLike" }, + { + "name": "isRefreshingViews" + }, { "name": "isSubscription" }, @@ -1937,6 +1943,9 @@ { "name": "setInputsFromAttrs" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setRouterState" }, diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json index 91c30042a9c..73abee035b3 100644 --- a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json +++ b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json @@ -419,6 +419,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -821,6 +824,9 @@ { "name": "isPromise" }, + { + "name": "isRefreshingViews" + }, { "name": "isSubscription" }, @@ -995,6 +1001,9 @@ { "name": "setInjectImplementation" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 5cb611aa450..7078ff7b719 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -551,6 +551,9 @@ { "name": "_injectImplementation" }, + { + "name": "_isRefreshingViews" + }, { "name": "_keyMap" }, @@ -1136,6 +1139,9 @@ { "name": "isPromise" }, + { + "name": "isRefreshingViews" + }, { "name": "isStylingMatch" }, @@ -1373,6 +1379,9 @@ { "name": "setInputsFromAttrs" }, + { + "name": "setIsRefreshingViews" + }, { "name": "setSelectedIndex" },