Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Fix null dereference error addEvent #55353

Conversation

tbondwilkinson
Copy link
Contributor

@tbondwilkinson tbondwilkinson commented Apr 15, 2024

When cleanUp is called containerManager is set to null. Currently the EventContract API assumes that users know not to call further methods on EventContract, but this lead to a bug in a downstream app. So instead, this makes a defensive check to make sure it's not null.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

addEvent after cleanUp will throw an error.

Issue Number: b/333750059

What is the new behavior?

addEvent after cleanUp will not throw an error.

Does this PR introduce a breaking change?

  • Yes
  • No

@tbondwilkinson tbondwilkinson added the area: core Issues related to the framework runtime label Apr 15, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 15, 2024
@pullapprove pullapprove bot added the requires: TGP This PR requires a passing TGP before merging is allowed label Apr 15, 2024
@tbondwilkinson tbondwilkinson added target: major This PR is targeted for the next major release and removed requires: TGP This PR requires a passing TGP before merging is allowed labels Apr 15, 2024
@pullapprove pullapprove bot added requires: TGP This PR requires a passing TGP before merging is allowed labels Apr 15, 2024
@tbondwilkinson tbondwilkinson changed the title bugfix(core): Fix null dereference error addEvent fix(core): Fix null dereference error addEvent Apr 15, 2024
Copy link
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM

When `cleanUp` is called `containerManager` is set to null. Currently
the `EventContract` API assumes that users know not to call further
methods on `EventContract`, but this lead to a bug in a downstream app,
b/333750059. So instead, this makes a defensive check to make sure it's
not null.
@tbondwilkinson tbondwilkinson added the action: merge The PR is ready for merge by the caretaker label Apr 16, 2024
@tbondwilkinson
Copy link
Contributor Author

Tested with TGP. Failures look unrelated to CL.

@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 0cec9e4.

iteriani pushed a commit to iteriani/angular that referenced this pull request Apr 18, 2024
When `cleanUp` is called `containerManager` is set to null. Currently
the `EventContract` API assumes that users know not to call further
methods on `EventContract`, but this lead to a bug in a downstream app,
b/333750059. So instead, this makes a defensive check to make sure it's
not null.

PR Close angular#55353
@tbondwilkinson tbondwilkinson deleted the jsaction-fix-null-container branch April 24, 2024 17:42
@angular-automatic-lock-bot
Copy link

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 May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime requires: TGP This PR requires a passing TGP before merging is allowed target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants