Skip to content

fix(zone.js): do not run stack formatting when the lazy stack frames policy is used - #45447

Closed
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/43240
Closed

fix(zone.js): do not run stack formatting when the lazy stack frames policy is used#45447
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/43240

Conversation

@arturovt

@arturovt arturovt commented Mar 25, 2022

Copy link
Copy Markdown
Contributor

We don't have to read the stack property when the lazy stack frames policy is used.
The V8 triggers the stack trace serialization only when the stack property is read for
the first time within the Accessors::ErrorStackGetter. We want to avoid formatting the stack
trace when the lazy policy is used, and we don't need the stack until the zoneAwareStack
property is read.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

  • Bugfix

What is the current behavior?

Issue Number: #43240

Does this PR introduce a breaking change?

  • Yes
  • No

@pullapprove
pullapprove Bot requested a review from JiaLiPassion March 25, 2022 23:36
@dylhunn dylhunn added the area: zones Issues related to zone.js label Mar 26, 2022
@ngbot ngbot Bot added this to the Backlog milestone Mar 26, 2022
@arturovt
arturovt force-pushed the fix/43240 branch 5 times, most recently from 442558b to db9abde Compare March 26, 2022 04:22
JiaLiPassion
JiaLiPassion approved these changes Jun 22, 2022
View reviewed changes

@JiaLiPassion JiaLiPassion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fix.

@JiaLiPassion JiaLiPassion added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Jun 22, 2022
@ngbot

ngbot Bot commented Jun 22, 2022

Copy link
Copy Markdown

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@JiaLiPassion JiaLiPassion removed the action: merge The PR is ready for merge by the caretaker label Jun 22, 2022
@AndrewKushnir

AndrewKushnir commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

@arturovt I've started a full set of relevant tests in Google's codebase (internal-only link) and let you know if there are any failures.

@AndrewKushnir AndrewKushnir added the action: global presubmit The PR is in need of a google3 global presubmit label Jun 23, 2022
@AndrewKushnir

AndrewKushnir commented Jun 25, 2022

Copy link
Copy Markdown
Contributor

@arturovt quick update: there is a number of failing targets in Google's codebase, which would take sizable time to investigate. Given that the original issue has no duplicates or upvotes, we may not be able to prioritize this investigation in the near future. We'll discuss this with the team early next week and I'll let you know.

@AndrewKushnir AndrewKushnir mentioned this pull request Jun 25, 2022
Closed
3 tasks done
@arturovt
fix(zone.js): do not run stack formatting when the lazy stack frames …
b580f7b 
…policy is used

We don't have to read the `stack` property when the lazy stack frames policy is used.
The V8 triggers the stack trace serialization only when the `stack` property is read for
the first time within the `Accessors::ErrorStackGetter`. We want to avoid formatting the stack
trace when the lazy policy is used, and we don't need the stack until the `zoneAwareStack`
property is read.

PR Close angular#43240
@arturovt

arturovt commented May 20, 2024

Copy link
Copy Markdown
Contributor Author

Closing since this breaks some tests internally, which would require some effort to investigate and land in g3.

@arturovt arturovt closed this May 20, 2024
@arturovt
arturovt deleted the fix/43240 branch May 20, 2024 09:45
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Jun 24, 2024

Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@JiaLiPassion JiaLiPassion JiaLiPassion approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: global presubmit The PR is in need of a google3 global presubmit area: zones Issues related to zone.js state: blocked target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@arturovt @AndrewKushnir @JiaLiPassion @dylhunn