Skip to content

Commit

Permalink
refactor(ivy): move bindingIndex from LView to LFrame (#33235)
Browse files Browse the repository at this point in the history 
`bindingIndex` stores the current location of the bindings in the
template function. Because it used to be stored in `LView` that `LView`
was not reentrant. This could happen if a binding was a getter and had
a side-effect of calling `detectChanges()`.

By moving the `bindingIndex` to `LFrame` where all of the global state
is kept in reentrant way we correct the issue.

PR Close #33235
  • Loading branch information
@mhevery @AndrewKushnir
mhevery authored and AndrewKushnir committed Oct 28, 2019
1 parent c61f413 commit e16f75d
Show file tree
Hide file tree
Showing 22 changed files with 204 additions and 138 deletions.
4 changes: 2 additions & 2 deletions integration/_payload-limits.json
View file
Expand Up @@ -12,7 +12,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 1485,
"main-es2015": 14678,
"main-es2015": 14861,
"polyfills-es2015": 36808
}
}
Expand Down Expand Up @@ -64,4 +64,4 @@
}
}
}
}
}
10 changes: 5 additions & 5 deletions packages/core/src/render3/i18n.ts
View file
Expand Up @@ -25,9 +25,9 @@ import {TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeType, TProjecti
import {RComment, RElement, RText} from './interfaces/renderer';
import {SanitizerFn} from './interfaces/sanitization';
import {isLContainer} from './interfaces/type_checks';
import {BINDING_INDEX, HEADER_OFFSET, LView, RENDERER, TVIEW, TView, T_HOST} from './interfaces/view';
import {HEADER_OFFSET, LView, RENDERER, TVIEW, TView, T_HOST} from './interfaces/view';
import {appendChild, applyProjection, createTextNode, nativeRemoveNode} from './node_manipulation';
import {getIsParent, getLView, getPreviousOrParentTNode, setIsNotParent, setPreviousOrParentTNode} from './state';
import {getBindingIndex, getIsParent, getLView, getPreviousOrParentTNode, nextBindingIndex, setIsNotParent, setPreviousOrParentTNode} from './state';
import {renderStringify} from './util/misc_utils';
import {getNativeByIndex, getNativeByTNode, getTNode, load} from './util/view_utils';

Expand Down Expand Up @@ -669,7 +669,7 @@ export function ɵɵi18nEnd(): void {
*/
function i18nEndFirstPass(lView: LView, tView: TView) {
ngDevMode && assertEqual(
lView[BINDING_INDEX], tView.bindingStartIndex,
getBindingIndex(), tView.bindingStartIndex,
'i18nEnd should be called before any binding');

const rootIndex = i18nIndexStack[i18nIndexStackPointer--];
Expand Down Expand Up @@ -1036,7 +1036,7 @@ let shiftsCounter = 0;
*/
export function ɵɵi18nExp<T>(value: T): TsickleIssue1009 {
const lView = getLView();
if (bindingUpdated(lView, lView[BINDING_INDEX]++, value)) {
if (bindingUpdated(lView, nextBindingIndex(), value)) {
changeMask = changeMask | (1 << shiftsCounter);
}
shiftsCounter++;
Expand Down Expand Up @@ -1065,7 +1065,7 @@ export function ɵɵi18nApply(index: number) {
updateOpCodes = (tI18n as TI18n).update;
icus = (tI18n as TI18n).icus;
}
const bindingsStartIndex = lView[BINDING_INDEX] - shiftsCounter - 1;
const bindingsStartIndex = getBindingIndex() - shiftsCounter - 1;
readUpdateOpCodes(updateOpCodes, icus, bindingsStartIndex, changeMask, lView);

// Reset changeMask & maskBit to default for the next update cycle
Expand Down
5 changes: 2 additions & 3 deletions packages/core/src/render3/instructions/attribute.ts
View file
Expand Up @@ -7,8 +7,7 @@
*/
import {bindingUpdated} from '../bindings';
import {SanitizerFn} from '../interfaces/sanitization';
import {BINDING_INDEX} from '../interfaces/view';
import {getLView, getSelectedIndex} from '../state';
import {getLView, getSelectedIndex, nextBindingIndex} from '../state';

import {TsickleIssue1009, elementAttributeInternal} from './shared';

Expand All @@ -31,7 +30,7 @@ export function ɵɵattribute(
name: string, value: any, sanitizer?: SanitizerFn | null,
namespace?: string): TsickleIssue1009 {
const lView = getLView();
if (bindingUpdated(lView, lView[BINDING_INDEX]++, value)) {
if (bindingUpdated(lView, nextBindingIndex(), value)) {
elementAttributeInternal(getSelectedIndex(), name, value, lView, sanitizer, namespace);
}
return ɵɵattribute;
Expand Down
8 changes: 4 additions & 4 deletions packages/core/src/render3/instructions/container.ts
View file
Expand Up @@ -11,12 +11,12 @@ import {attachPatchData} from '../context_discovery';
import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags, registerPostOrderHooks} from '../hooks';
import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container';
import {ComponentTemplate} from '../interfaces/definition';
import {LocalRefExtractor, TAttributes, TContainerNode, TNode, TNodeFlags, TNodeType, TViewNode} from '../interfaces/node';
import {LocalRefExtractor, TAttributes, TContainerNode, TNode, TNodeType, TViewNode} from '../interfaces/node';
import {isDirectiveHost} from '../interfaces/type_checks';
import {BINDING_INDEX, FLAGS, HEADER_OFFSET, InitPhaseState, LView, LViewFlags, RENDERER, TVIEW, T_HOST} from '../interfaces/view';
import {FLAGS, HEADER_OFFSET, InitPhaseState, LView, LViewFlags, RENDERER, TVIEW, T_HOST} from '../interfaces/view';
import {assertNodeType} from '../node_assert';
import {appendChild, removeView} from '../node_manipulation';
import {getCheckNoChangesMode, getIsParent, getLView, getPreviousOrParentTNode, setIsNotParent, setPreviousOrParentTNode} from '../state';
import {getBindingIndex, getCheckNoChangesMode, getIsParent, getLView, getPreviousOrParentTNode, setIsNotParent, setPreviousOrParentTNode} from '../state';
import {getNativeByTNode, load} from '../util/view_utils';

import {addToViewTree, createDirectivesInstances, createLContainer, createTNode, createTView, getOrCreateTNode, resolveDirectives, saveResolvedLocalsInData} from './shared';
Expand Down Expand Up @@ -172,7 +172,7 @@ function containerInternal(
lView: LView, nodeIndex: number, tagName: string | null,
attrs: TAttributes | null): TContainerNode {
ngDevMode && assertEqual(
lView[BINDING_INDEX], lView[TVIEW].bindingStartIndex,
getBindingIndex(), lView[TVIEW].bindingStartIndex,
'container nodes should be created before any bindings');

const adjustedIndex = nodeIndex + HEADER_OFFSET;
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/render3/instructions/element.ts
View file
Expand Up @@ -14,10 +14,10 @@ import {TAttributes, TNodeFlags, TNodeType} from '../interfaces/node';
import {RElement} from '../interfaces/renderer';
import {StylingMapArray, TStylingContext} from '../interfaces/styling';
import {isContentQueryHost, isDirectiveHost} from '../interfaces/type_checks';
import {BINDING_INDEX, HEADER_OFFSET, LView, RENDERER, TVIEW, T_HOST} from '../interfaces/view';
import {HEADER_OFFSET, LView, RENDERER, TVIEW, T_HOST} from '../interfaces/view';
import {assertNodeType} from '../node_assert';
import {appendChild} from '../node_manipulation';
import {decreaseElementDepthCount, getElementDepthCount, getIsParent, getLView, getNamespace, getPreviousOrParentTNode, getSelectedIndex, increaseElementDepthCount, setIsNotParent, setPreviousOrParentTNode} from '../state';
import {decreaseElementDepthCount, getBindingIndex, getElementDepthCount, getIsParent, getLView, getNamespace, getPreviousOrParentTNode, getSelectedIndex, increaseElementDepthCount, setIsNotParent, setPreviousOrParentTNode} from '../state';
import {setUpAttributes} from '../util/attrs_utils';
import {getInitialStylingValue, hasClassInput, hasStyleInput, selectClassBasedInputName} from '../util/styling_utils';
import {getNativeByTNode, getTNode} from '../util/view_utils';
Expand Down Expand Up @@ -48,7 +48,7 @@ export function ɵɵelementStart(
const tViewConsts = tView.consts;
const consts = tViewConsts === null || constsIndex == null ? null : tViewConsts[constsIndex];
ngDevMode && assertEqual(
lView[BINDING_INDEX], tView.bindingStartIndex,
getBindingIndex(), tView.bindingStartIndex,
'elements should be created before any bindings');

ngDevMode && ngDevMode.rendererCreateElement++;
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/render3/instructions/element_container.ts
View file
Expand Up @@ -11,10 +11,10 @@ import {attachPatchData} from '../context_discovery';
import {registerPostOrderHooks} from '../hooks';
import {TAttributes, TNodeType} from '../interfaces/node';
import {isContentQueryHost, isDirectiveHost} from '../interfaces/type_checks';
import {BINDING_INDEX, HEADER_OFFSET, RENDERER, TVIEW, T_HOST} from '../interfaces/view';
import {HEADER_OFFSET, RENDERER, TVIEW, T_HOST} from '../interfaces/view';
import {assertNodeType} from '../node_assert';
import {appendChild} from '../node_manipulation';
import {getIsParent, getLView, getPreviousOrParentTNode, setIsNotParent, setPreviousOrParentTNode} from '../state';
import {getBindingIndex, getIsParent, getLView, getPreviousOrParentTNode, setIsNotParent, setPreviousOrParentTNode} from '../state';

import {createDirectivesInstances, executeContentQueries, getOrCreateTNode, resolveDirectives, saveResolvedLocalsInData} from './shared';
import {registerInitialStylingOnTNode} from './styling';
Expand Down Expand Up @@ -44,7 +44,7 @@ export function ɵɵelementContainerStart(
const tViewConsts = tView.consts;
const consts = tViewConsts === null || constsIndex == null ? null : tViewConsts[constsIndex];
ngDevMode && assertEqual(
lView[BINDING_INDEX], tView.bindingStartIndex,
getBindingIndex(), tView.bindingStartIndex,
'element containers should be created before any bindings');

ngDevMode && ngDevMode.rendererCreateComment++;
Expand Down
8 changes: 4 additions & 4 deletions packages/core/src/render3/instructions/host_property.ts
View file
Expand Up @@ -7,8 +7,8 @@
*/
import {bindingUpdated} from '../bindings';
import {SanitizerFn} from '../interfaces/sanitization';
import {BINDING_INDEX, TVIEW} from '../interfaces/view';
import {getLView, getSelectedIndex} from '../state';
import {TVIEW} from '../interfaces/view';
import {getLView, getSelectedIndex, nextBindingIndex} from '../state';
import {NO_CHANGE} from '../tokens';

import {TsickleIssue1009, elementPropertyInternal, loadComponentRenderer, storePropertyBindingMetadata} from './shared';
Expand All @@ -30,7 +30,7 @@ import {TsickleIssue1009, elementPropertyInternal, loadComponentRenderer, storeP
export function ɵɵhostProperty<T>(
propName: string, value: T, sanitizer?: SanitizerFn | null): TsickleIssue1009 {
const lView = getLView();
const bindingIndex = lView[BINDING_INDEX]++;
const bindingIndex = nextBindingIndex();
if (bindingUpdated(lView, bindingIndex, value)) {
const nodeIndex = getSelectedIndex();
elementPropertyInternal(lView, nodeIndex, propName, value, sanitizer, true);
Expand Down Expand Up @@ -64,7 +64,7 @@ export function ɵɵhostProperty<T>(
export function ɵɵupdateSyntheticHostBinding<T>(
propName: string, value: T | NO_CHANGE, sanitizer?: SanitizerFn | null): TsickleIssue1009 {
const lView = getLView();
const bindingIndex = lView[BINDING_INDEX]++;
const bindingIndex = nextBindingIndex();
if (bindingUpdated(lView, bindingIndex, value)) {
const nodeIndex = getSelectedIndex();
elementPropertyInternal(
Expand Down
37 changes: 19 additions & 18 deletions packages/core/src/render3/instructions/interpolation.ts
View file
Expand Up @@ -8,7 +8,8 @@

import {assertEqual, assertLessThan} from '../../util/assert';
import {bindingUpdated, bindingUpdated2, bindingUpdated3, bindingUpdated4} from '../bindings';
import {BINDING_INDEX, LView} from '../interfaces/view';
import {LView} from '../interfaces/view';
import {getBindingIndex, incrementBindingIndex, nextBindingIndex, setBindingIndex} from '../state';
import {NO_CHANGE} from '../tokens';
import {renderStringify} from '../util/misc_utils';

Expand All @@ -30,13 +31,13 @@ export function interpolationV(lView: LView, values: any[]): string|NO_CHANGE {
ngDevMode && assertLessThan(2, values.length, 'should have at least 3 values');
ngDevMode && assertEqual(values.length % 2, 1, 'should have an odd number of values');
let isBindingUpdated = false;
let bindingIndex = lView[BINDING_INDEX];
let bindingIndex = getBindingIndex();

for (let i = 1; i < values.length; i += 2) {
// Check if bindings (odd indexes) have changed
isBindingUpdated = bindingUpdated(lView, bindingIndex++, values[i]) || isBindingUpdated;
}
lView[BINDING_INDEX] = bindingIndex;
setBindingIndex(bindingIndex);

if (!isBindingUpdated) {
return NO_CHANGE;
Expand All @@ -60,7 +61,7 @@ export function interpolationV(lView: LView, values: any[]): string|NO_CHANGE {
*/
export function interpolation1(lView: LView, prefix: string, v0: any, suffix: string): string|
NO_CHANGE {
const different = bindingUpdated(lView, lView[BINDING_INDEX]++, v0);
const different = bindingUpdated(lView, nextBindingIndex(), v0);
return different ? prefix + renderStringify(v0) + suffix : NO_CHANGE;
}

Expand All @@ -69,9 +70,9 @@ export function interpolation1(lView: LView, prefix: string, v0: any, suffix: st
*/
export function interpolation2(
lView: LView, prefix: string, v0: any, i0: string, v1: any, suffix: string): string|NO_CHANGE {
const bindingIndex = lView[BINDING_INDEX];
const bindingIndex = getBindingIndex();
const different = bindingUpdated2(lView, bindingIndex, v0, v1);
lView[BINDING_INDEX] += 2;
incrementBindingIndex(2);

return different ? prefix + renderStringify(v0) + i0 + renderStringify(v1) + suffix : NO_CHANGE;
}
Expand All @@ -82,9 +83,9 @@ export function interpolation2(
export function interpolation3(
lView: LView, prefix: string, v0: any, i0: string, v1: any, i1: string, v2: any,
suffix: string): string|NO_CHANGE {
const bindingIndex = lView[BINDING_INDEX];
const bindingIndex = getBindingIndex();
const different = bindingUpdated3(lView, bindingIndex, v0, v1, v2);
lView[BINDING_INDEX] += 3;
incrementBindingIndex(3);

return different ?
prefix + renderStringify(v0) + i0 + renderStringify(v1) + i1 + renderStringify(v2) + suffix :
Expand All @@ -97,9 +98,9 @@ export function interpolation3(
export function interpolation4(
lView: LView, prefix: string, v0: any, i0: string, v1: any, i1: string, v2: any, i2: string,
v3: any, suffix: string): string|NO_CHANGE {
const bindingIndex = lView[BINDING_INDEX];
const bindingIndex = getBindingIndex();
const different = bindingUpdated4(lView, bindingIndex, v0, v1, v2, v3);
lView[BINDING_INDEX] += 4;
incrementBindingIndex(4);

return different ?
prefix + renderStringify(v0) + i0 + renderStringify(v1) + i1 + renderStringify(v2) + i2 +
Expand All @@ -113,10 +114,10 @@ export function interpolation4(
export function interpolation5(
lView: LView, prefix: string, v0: any, i0: string, v1: any, i1: string, v2: any, i2: string,
v3: any, i3: string, v4: any, suffix: string): string|NO_CHANGE {
const bindingIndex = lView[BINDING_INDEX];
const bindingIndex = getBindingIndex();
let different = bindingUpdated4(lView, bindingIndex, v0, v1, v2, v3);
different = bindingUpdated(lView, bindingIndex + 4, v4) || different;
lView[BINDING_INDEX] += 5;
incrementBindingIndex(5);

return different ?
prefix + renderStringify(v0) + i0 + renderStringify(v1) + i1 + renderStringify(v2) + i2 +
Expand All @@ -130,10 +131,10 @@ export function interpolation5(
export function interpolation6(
lView: LView, prefix: string, v0: any, i0: string, v1: any, i1: string, v2: any, i2: string,
v3: any, i3: string, v4: any, i4: string, v5: any, suffix: string): string|NO_CHANGE {
const bindingIndex = lView[BINDING_INDEX];
const bindingIndex = getBindingIndex();
let different = bindingUpdated4(lView, bindingIndex, v0, v1, v2, v3);
different = bindingUpdated2(lView, bindingIndex + 4, v4, v5) || different;
lView[BINDING_INDEX] += 6;
incrementBindingIndex(6);

return different ?
prefix + renderStringify(v0) + i0 + renderStringify(v1) + i1 + renderStringify(v2) + i2 +
Expand All @@ -148,10 +149,10 @@ export function interpolation7(
lView: LView, prefix: string, v0: any, i0: string, v1: any, i1: string, v2: any, i2: string,
v3: any, i3: string, v4: any, i4: string, v5: any, i5: string, v6: any, suffix: string): string|
NO_CHANGE {
const bindingIndex = lView[BINDING_INDEX];
const bindingIndex = getBindingIndex();
let different = bindingUpdated4(lView, bindingIndex, v0, v1, v2, v3);
different = bindingUpdated3(lView, bindingIndex + 4, v4, v5, v6) || different;
lView[BINDING_INDEX] += 7;
incrementBindingIndex(7);

return different ?
prefix + renderStringify(v0) + i0 + renderStringify(v1) + i1 + renderStringify(v2) + i2 +
Expand All @@ -167,10 +168,10 @@ export function interpolation8(
lView: LView, prefix: string, v0: any, i0: string, v1: any, i1: string, v2: any, i2: string,
v3: any, i3: string, v4: any, i4: string, v5: any, i5: string, v6: any, i6: string, v7: any,
suffix: string): string|NO_CHANGE {
const bindingIndex = lView[BINDING_INDEX];
const bindingIndex = getBindingIndex();
let different = bindingUpdated4(lView, bindingIndex, v0, v1, v2, v3);
different = bindingUpdated4(lView, bindingIndex + 4, v4, v5, v6, v7) || different;
lView[BINDING_INDEX] += 8;
incrementBindingIndex(8);

return different ?
prefix + renderStringify(v0) + i0 + renderStringify(v1) + i1 + renderStringify(v2) + i2 +
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/render3/instructions/lview_debug.ts
View file
Expand Up @@ -19,7 +19,7 @@ import {SelectorFlags} from '../interfaces/projection';
import {TQueries} from '../interfaces/query';
import {RComment, RElement, RNode} from '../interfaces/renderer';
import {TStylingContext} from '../interfaces/styling';
import {BINDING_INDEX, CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_VIEW, ExpandoInstructions, FLAGS, HEADER_OFFSET, HOST, HookData, INJECTOR, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, RENDERER_FACTORY, SANITIZER, TData, TVIEW, TView as ITView, TView, T_HOST} from '../interfaces/view';
import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_VIEW, ExpandoInstructions, FLAGS, HEADER_OFFSET, HOST, HookData, INJECTOR, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, RENDERER_FACTORY, SANITIZER, TData, TVIEW, TView as ITView, TView, T_HOST} from '../interfaces/view';
import {DebugNodeStyling, NodeStylingDebug} from '../styling/styling_debug';
import {attachDebugObject} from '../util/debug_utils';
import {isStylingContext} from '../util/styling_utils';
Expand Down Expand Up @@ -325,7 +325,6 @@ export class LViewDebug {
get declarationView() { return toDebug(this._raw_lView[DECLARATION_VIEW]); }
get queries() { return this._raw_lView[QUERIES]; }
get tHost() { return this._raw_lView[T_HOST]; }
get bindingIndex() { return this._raw_lView[BINDING_INDEX]; }

/**
* Normalized view of child views (and containers) attached at this location.
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/render3/instructions/property.ts
View file
Expand Up @@ -7,8 +7,8 @@
*/
import {bindingUpdated} from '../bindings';
import {SanitizerFn} from '../interfaces/sanitization';
import {BINDING_INDEX, TVIEW} from '../interfaces/view';
import {getLView, getSelectedIndex} from '../state';
import {TVIEW} from '../interfaces/view';
import {getLView, getSelectedIndex, nextBindingIndex} from '../state';

import {TsickleIssue1009, elementPropertyInternal, storePropertyBindingMetadata} from './shared';

Expand All @@ -34,7 +34,7 @@ import {TsickleIssue1009, elementPropertyInternal, storePropertyBindingMetadata}
export function ɵɵproperty<T>(
propName: string, value: T, sanitizer?: SanitizerFn | null): TsickleIssue1009 {
const lView = getLView();
const bindingIndex = lView[BINDING_INDEX]++;
const bindingIndex = nextBindingIndex();
if (bindingUpdated(lView, bindingIndex, value)) {
const nodeIndex = getSelectedIndex();
elementPropertyInternal(lView, nodeIndex, propName, value, sanitizer);
Expand Down

0 comments on commit e16f75d

Please sign in to comment.