fix(core): attempt to recover from user errors during creation #36381

crisbeto · 2020-04-01T19:27:25Z

If there's an error during the first creation pass of a TView, the data structure may be corrupted which will cause framework assertion failures downstream which can mask the user's error. These changes add a new flag to the TView that indicates whether the first creation pass was successful, and if it wasn't try re-create the TView.

Fixes #31221.

packages/core/src/render3/instructions/shared.ts

integration/_payload-limits.json

IgorMinar

lgtm - but only for the unrelated size change

integration/_payload-limits.json

AndrewKushnir

Thanks for fixing this problem and creating a test case for it (which was tricky)! 👍

The changes LGTM (with a couple nits), but I think it'd be great if @pkozlowski-opensource can have a look at this change as well, so I'm adding him as a reviewer to this PR.

Thank you.

packages/core/test/acceptance/view_insertion_spec.ts

packages/core/src/render3/instructions/shared.ts

crisbeto · 2020-04-02T19:04:21Z

I've addressed the feedback @AndrewKushnir. Also sorry for the larger diff, clang-format ended up reformatting the files I changed.

crisbeto · 2020-04-20T19:10:52Z

This is ready for review again. Also thanks @pkozlowski-opensource for the help with the test cases.

crisbeto · 2020-04-20T19:11:37Z

packages/core/src/render3/interfaces/view.ts

+   * Indicates that there was an error before we managed to complete the first create pass of the
+   * view. This means that the view is likely corrupted and we should try to recover it.
+   */
+  incompleteFirstPass: boolean;


I was thinking of potentially making this a counter so that we can stop trying after a certain number of attempts. I decided to keep it simple for now, but I'm open to changing it.

Yeh, I think that keeping it simple for now is the way to go 👍

pkozlowski-opensource

LGTM.

pkozlowski-opensource · 2020-04-28T12:32:49Z

packages/core/src/render3/interfaces/view.ts

+   * Indicates that there was an error before we managed to complete the first create pass of the
+   * view. This means that the view is likely corrupted and we should try to recover it.
+   */
+  incompleteFirstPass: boolean;


Yeh, I think that keeping it simple for now is the way to go 👍

pkozlowski-opensource · 2020-04-28T12:39:41Z

packages/core/test/acceptance/view_insertion_spec.ts

@@ -398,11 +398,11 @@ describe('view insertion', () => {
        const fixture = TestBed.createComponent(AppComponent);
        fixture.detectChanges();

-        expect(fixture.debugElement.nativeElement.textContent).toBe('start|test|end');
+        expect(fixture.nativeElement.textContent).toBe('start|test|end');


pkozlowski-opensource · 2020-04-28T12:39:47Z

packages/core/test/acceptance/view_insertion_spec.ts


        fixture.componentInstance.insertTpl = true;
        fixture.detectChanges();
-        expect(fixture.debugElement.nativeElement.textContent).toBe('start|testtest|end');
+        expect(fixture.nativeElement.textContent).toBe('start|testtest|end');


AndrewKushnir · 2020-04-28T16:18:10Z

Presubmit

AndrewKushnir · 2020-04-28T18:42:37Z

@crisbeto could you please rebase this PR? There is no conflict, but the merge script failed since this PR was rebased/created prior to the most recent update of the base SHA in the merge script. Feel free to add "merge" label back once it's done.

FYI, g3 presubmit is successful.

If there's an error during the first creation pass of a `TView`, the data structure may be corrupted which will cause framework assertion failures downstream which can mask the user's error. These changes add a new flag to the `TView` that indicates whether the first creation pass was successful, and if it wasn't we try re-create the `TView`. Fixes angular#31221.

crisbeto · 2020-04-28T19:08:05Z

Rebased.

AndrewKushnir · 2020-04-28T19:34:29Z

@crisbeto thanks for rebasing! It looks like payload size limit checks failed on CI, could you please have a look when you get a chance? Thank you.

crisbeto · 2020-04-28T20:29:32Z

Fixed the payload size failures.

AndrewKushnir · 2020-04-28T20:32:06Z

Thanks @crisbeto.

I've re-requested review from @IgorMinar to review/approve payload size increase.

kara

LGTM for size changes (look like accumulation)

kara · 2020-04-28T21:35:16Z

goldens/size-tracking/integration-payloads.json

@@ -30,7 +30,7 @@
    "master": {
      "uncompressed": {
        "runtime-es2015": 1485,
-        "main-es2015": 137320,
+        "main-es2015": 137897,


It looks like this is mostly accumulation from other PRs? The change itself doesn't seem to merit this jump in size.

IgorMinar

Lgtm! Thanks!

If there's an error during the first creation pass of a `TView`, the data structure may be corrupted which will cause framework assertion failures downstream which can mask the user's error. These changes add a new flag to the `TView` that indicates whether the first creation pass was successful, and if it wasn't we try re-create the `TView`. Fixes #31221. PR Close #36381

angular-automatic-lock-bot · 2020-05-30T16:09:50Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Apr 1, 2020

crisbeto commented Apr 1, 2020

View reviewed changes

packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the FW-1974/creation-error-mask branch from e10a3b5 to 526ff89 Compare April 1, 2020 19:46

crisbeto commented Apr 1, 2020

View reviewed changes

integration/_payload-limits.json Outdated Show resolved Hide resolved

crisbeto marked this pull request as ready for review April 1, 2020 20:26

pullapprove bot requested review from AndrewKushnir and IgorMinar April 1, 2020 20:26

crisbeto added area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix labels Apr 1, 2020

ngbot bot modified the milestone: needsTriage Apr 1, 2020

IgorMinar approved these changes Apr 1, 2020

View reviewed changes

integration/_payload-limits.json Outdated Show resolved Hide resolved

AndrewKushnir approved these changes Apr 2, 2020

View reviewed changes

packages/core/test/acceptance/view_insertion_spec.ts Outdated Show resolved Hide resolved

packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved

AndrewKushnir requested a review from pkozlowski-opensource April 2, 2020 00:13

crisbeto force-pushed the FW-1974/creation-error-mask branch from 526ff89 to 113648d Compare April 2, 2020 19:03

crisbeto force-pushed the FW-1974/creation-error-mask branch from 113648d to 3113e17 Compare April 4, 2020 08:44

AndrewKushnir added the state: blocked label Apr 9, 2020

crisbeto marked this pull request as draft April 15, 2020 15:32

crisbeto force-pushed the FW-1974/creation-error-mask branch from 3113e17 to cedc991 Compare April 20, 2020 18:49

crisbeto changed the title ~~fix(core): user errors during creation masked by framework errors~~ fix(core): attempt to recover from user errors during creation Apr 20, 2020

crisbeto marked this pull request as ready for review April 20, 2020 19:10

crisbeto removed the state: blocked label Apr 20, 2020

crisbeto commented Apr 20, 2020

View reviewed changes

pkozlowski-opensource approved these changes Apr 28, 2020

View reviewed changes

pkozlowski-opensource added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 28, 2020

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Apr 28, 2020

AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker labels Apr 28, 2020

crisbeto force-pushed the FW-1974/creation-error-mask branch from cedc991 to c497fe1 Compare April 28, 2020 19:07

crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 28, 2020

AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Apr 28, 2020

fixup! fix(core): attempt to recover from user errors during creation

77c64ee

crisbeto force-pushed the FW-1974/creation-error-mask branch from 180df60 to 77c64ee Compare April 28, 2020 20:02

crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 28, 2020

AndrewKushnir requested a review from IgorMinar April 28, 2020 20:31

kara approved these changes Apr 28, 2020

View reviewed changes

IgorMinar approved these changes Apr 29, 2020

View reviewed changes

AndrewKushnir closed this in 3d82aa7 Apr 29, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): attempt to recover from user errors during creation #36381

fix(core): attempt to recover from user errors during creation #36381

crisbeto commented Apr 1, 2020 •

edited

Loading

IgorMinar left a comment

AndrewKushnir left a comment

crisbeto commented Apr 2, 2020

crisbeto commented Apr 20, 2020 •

edited

Loading

crisbeto Apr 20, 2020

pkozlowski-opensource Apr 28, 2020

pkozlowski-opensource left a comment

pkozlowski-opensource Apr 28, 2020

pkozlowski-opensource Apr 28, 2020

pkozlowski-opensource Apr 28, 2020

AndrewKushnir commented Apr 28, 2020

AndrewKushnir commented Apr 28, 2020

crisbeto commented Apr 28, 2020

AndrewKushnir commented Apr 28, 2020

crisbeto commented Apr 28, 2020

AndrewKushnir commented Apr 28, 2020

kara left a comment

kara Apr 28, 2020

IgorMinar left a comment

angular-automatic-lock-bot bot commented May 30, 2020

fix(core): attempt to recover from user errors during creation #36381

fix(core): attempt to recover from user errors during creation #36381

Conversation

crisbeto commented Apr 1, 2020 • edited Loading

IgorMinar left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

crisbeto commented Apr 2, 2020

crisbeto commented Apr 20, 2020 • edited Loading

crisbeto Apr 20, 2020

Choose a reason for hiding this comment

pkozlowski-opensource Apr 28, 2020

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

pkozlowski-opensource Apr 28, 2020

Choose a reason for hiding this comment

pkozlowski-opensource Apr 28, 2020

Choose a reason for hiding this comment

pkozlowski-opensource Apr 28, 2020

Choose a reason for hiding this comment

AndrewKushnir commented Apr 28, 2020

AndrewKushnir commented Apr 28, 2020

crisbeto commented Apr 28, 2020

AndrewKushnir commented Apr 28, 2020

crisbeto commented Apr 28, 2020

AndrewKushnir commented Apr 28, 2020

kara left a comment

Choose a reason for hiding this comment

kara Apr 28, 2020

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented May 30, 2020

crisbeto commented Apr 1, 2020 •

edited

Loading

crisbeto commented Apr 20, 2020 •

edited

Loading