The Wayback Machine - https://web.archive.org/web/20210907004649/https://github.com/angular/angular/pull/42881
Skip to content

/ angular

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

Jump to bottom

fix(core): correctly handle null or undefined in ErrorHandler#handleError() #42881

Closed
wants to merge 2 commits into from
Closed

fix(core): correctly handle null or undefined in ErrorHandler#handleError() #42881

wants to merge 2 commits into from
+18 −7

Conversation

@gkalpak
Copy link
Member

@gkalpak gkalpak commented Jul 17, 2021 •
edited

Since ErrorHandler#handleError() expects an argument of type any it should be able to handle values such as null and undefined. Previously, it failed to handle these values, because it was trying to access properties on them.

This PR fixes it by ensuring no properties are accessed on null or undefined values.

NOTE: This is part of fully addressing #28106.

Fixes #21252.
@google-cla google-cla bot added the cla: yes label Jul 17, 2021
@ngbot ngbot bot modified the milestone: Backlog Jul 17, 2021
@JoostK
Copy link
Member

@JoostK JoostK commented Jul 17, 2021

Nice! This also addresses #21252.

@JoostK JoostK linked an issue that may be closed by this pull request Jul 17, 2021
APP_INITIALIZER: rejected promise throws "Cannot read property 'ngOriginalError' of undefined" #21252
Closed
gkalpak added a commit to gkalpak/angular that referenced this pull request Jul 17, 2021
@gkalpak
fix(docs-infra): correctly handle reported errors
49ac745 
Error-handling in AIO happens mainly in two places:
1. For errors happening inside the app we have a custom `ErrorHandler`
   implementation, `ReportingErrorHandler`. `ReportingErrorHandler`
   passes errors to the default `ErrorHandler` (for them to be logged to
   the console) and also forwards them to `window.onerror()`.
2. Errors happening outside the app and errors forwarded by
   `ReportingErrorHandler` are handled by `window.onerror()`, which in
   turn reports them to Google analytics.

Previously, we were making some assumptions (which turned out to be
incorrect based on the info captured in Google analytics - see angular#28106):
- `ReportingErrorHandler` assumed that the errors passed to its
  `handleError()` method would be either strings or `Error` instances.
  _Apparently, other values (such as `null` or `undefined`) may also be
  passed._
- `window.onerror()` assumed that if an `Error` instance was passed in,
  it would always have a stacktrace (i.e. its `stack` property would be
  defined).
  _This is not necessarily true, although it is not clear (based on the
  logs) whether reported errors of this type are caused by `Error`
  instance with no stacktrace or by non-string error objects which are
  incorrectly treated as `Error` instances.

This commit ensures that all types of error arguments can be handled
correctly, including `Error` instances with no stacktrace and other
types of objects or primitives.

NOTE:
PR angular#42881 is related as it fixes handling `null` and `undefined`
arguments in the default `ErrorHandler`.

Fixes angular#28106
gkalpak added a commit to gkalpak/angular that referenced this pull request Jul 17, 2021
@gkalpak
fix(docs-infra): correctly handle reported errors
3048697 
Error-handling in AIO happens mainly in two places:
1. For errors happening inside the app we have a custom `ErrorHandler`
   implementation, `ReportingErrorHandler`. `ReportingErrorHandler`
   passes errors to the default `ErrorHandler` (for them to be logged to
   the console) and also forwards them to `window.onerror()`.
2. Errors happening outside the app and errors forwarded by
   `ReportingErrorHandler` are handled by `window.onerror()`, which in
   turn reports them to Google analytics.

Previously, we were making some assumptions (which turned out to be
incorrect based on the info captured in Google analytics - see angular#28106):
- `ReportingErrorHandler` assumed that the errors passed to its
  `handleError()` method would be either strings or `Error` instances.
  _Apparently, other values (such as `null` or `undefined`) may also be
  passed._
- `window.onerror()` assumed that if an `Error` instance was passed in,
  it would always have a stacktrace (i.e. its `stack` property would be
  defined).
  _This is not necessarily true, although it is not clear (based on the
  logs) whether reported errors of this type are caused by `Error`
  instance with no stacktrace or by non-string error objects which are
  incorrectly treated as `Error` instances.

This commit ensures that all types of error arguments can be handled
correctly, including `Error` instances with no stacktrace and other
types of objects or primitives.

NOTE:
PR angular#42881 is related as it fixes handling `null` and `undefined`
arguments in the default `ErrorHandler`.

Fixes angular#28106
@gkalpak gkalpak force-pushed the gkalpak:fix-core-handle-null-error branch 2 times, most recently from 88ca121 to bc425ef Jul 17, 2021
@gkalpak
Copy link
Member Author

@gkalpak gkalpak commented Jul 17, 2021

Cool, I didn't know about #21252 😅

@gkalpak gkalpak mentioned this pull request Jul 17, 2021
Closed
gkalpak added a commit to gkalpak/angular that referenced this pull request Jul 17, 2021
@gkalpak
fix(docs-infra): correctly handle reported errors
496be9c 
Error-handling in AIO happens mainly in two places:
1. For errors happening inside the app we have a custom `ErrorHandler`
   implementation, `ReportingErrorHandler`. `ReportingErrorHandler`
   passes errors to the default `ErrorHandler` (for them to be logged to
   the console) and also forwards them to `window.onerror()`.
2. Errors happening outside the app and errors forwarded by
   `ReportingErrorHandler` are handled by `window.onerror()`, which in
   turn reports them to Google analytics.

Previously, we were making some assumptions (which turned out to be
incorrect based on the info captured in Google analytics - see angular#28106):
- `ReportingErrorHandler` assumed that the errors passed to its
  `handleError()` method would be either strings or `Error` instances.
  _Apparently, other values (such as `null` or `undefined`) may also be
  passed._
- `window.onerror()` assumed that if an `Error` instance was passed in,
  it would always have a stacktrace (i.e. its `stack` property would be
  defined).
  _This is not necessarily true, although it is not clear (based on the
  logs) whether reported errors of this type are caused by `Error`
  instance with no stacktrace or by non-string error objects which are
  incorrectly treated as `Error` instances.

This commit ensures that all types of error arguments can be handled
correctly, including `Error` instances with no stacktrace and other
types of objects or primitives.

NOTE:
PR angular#42881 is related as it fixes handling `null` and `undefined`
arguments in the default `ErrorHandler`.

Fixes angular#28106
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Jul 17, 2021

You can preview bc425ef at https://pr42881-bc425ef.ngbuilds.io/.

@gkalpak gkalpak marked this pull request as ready for review Jul 17, 2021
@pullapprove pullapprove bot requested a review from AndrewKushnir Jul 17, 2021
@AndrewKushnir
AndrewKushnir reviewed Jul 19, 2021
View changes
packages/core/src/error_handler.ts Show resolved Hide resolved
packages/core/src/errors.ts Show resolved Hide resolved
alxhub added a commit that referenced this pull request Jul 20, 2021
@gkalpak @alxhub
fix(docs-infra): correctly handle reported errors (#42883)
e7c7707 
Error-handling in AIO happens mainly in two places:
1. For errors happening inside the app we have a custom `ErrorHandler`
   implementation, `ReportingErrorHandler`. `ReportingErrorHandler`
   passes errors to the default `ErrorHandler` (for them to be logged to
   the console) and also forwards them to `window.onerror()`.
2. Errors happening outside the app and errors forwarded by
   `ReportingErrorHandler` are handled by `window.onerror()`, which in
   turn reports them to Google analytics.

Previously, we were making some assumptions (which turned out to be
incorrect based on the info captured in Google analytics - see #28106):
- `ReportingErrorHandler` assumed that the errors passed to its
  `handleError()` method would be either strings or `Error` instances.
  _Apparently, other values (such as `null` or `undefined`) may also be
  passed._
- `window.onerror()` assumed that if an `Error` instance was passed in,
  it would always have a stacktrace (i.e. its `stack` property would be
  defined).
  _This is not necessarily true, although it is not clear (based on the
  logs) whether reported errors of this type are caused by `Error`
  instance with no stacktrace or by non-string error objects which are
  incorrectly treated as `Error` instances.

This commit ensures that all types of error arguments can be handled
correctly, including `Error` instances with no stacktrace and other
types of objects or primitives.

NOTE:
PR #42881 is related as it fixes handling `null` and `undefined`
arguments in the default `ErrorHandler`.

Fixes #28106

PR Close #42883
alxhub added a commit that referenced this pull request Jul 20, 2021
@gkalpak @alxhub
fix(docs-infra): correctly handle reported errors (#42883)
8989397 
Error-handling in AIO happens mainly in two places:
1. For errors happening inside the app we have a custom `ErrorHandler`
   implementation, `ReportingErrorHandler`. `ReportingErrorHandler`
   passes errors to the default `ErrorHandler` (for them to be logged to
   the console) and also forwards them to `window.onerror()`.
2. Errors happening outside the app and errors forwarded by
   `ReportingErrorHandler` are handled by `window.onerror()`, which in
   turn reports them to Google analytics.

Previously, we were making some assumptions (which turned out to be
incorrect based on the info captured in Google analytics - see #28106):
- `ReportingErrorHandler` assumed that the errors passed to its
  `handleError()` method would be either strings or `Error` instances.
  _Apparently, other values (such as `null` or `undefined`) may also be
  passed._
- `window.onerror()` assumed that if an `Error` instance was passed in,
  it would always have a stacktrace (i.e. its `stack` property would be
  defined).
  _This is not necessarily true, although it is not clear (based on the
  logs) whether reported errors of this type are caused by `Error`
  instance with no stacktrace or by non-string error objects which are
  incorrectly treated as `Error` instances.

This commit ensures that all types of error arguments can be handled
correctly, including `Error` instances with no stacktrace and other
types of objects or primitives.

NOTE:
PR #42881 is related as it fixes handling `null` and `undefined`
arguments in the default `ErrorHandler`.

Fixes #28106

PR Close #42883
gkalpak added 2 commits Jul 20, 2021
@gkalpak
fix(core): correctly handle null or undefined in `ErrorHandler#ha…
8a62df1 
…ndleError()`

Since `ErrorHandler#handleError()` expects an argument of type `any` it
should be able to handle values such as `null` and `undefined`.
Previously, it failed to handle these values, because it was trying to
access properties on them.

This commit fixes it by ensuring no properties are accessed on `null` or
`undefined` values.

NOTE: This is part of fully addressing #28106.

Fixes #21252
@gkalpak
fixup! fix(core): correctly handle null or undefined in `ErrorHan…
0d96e56 
…dler#handleError()`
@gkalpak gkalpak force-pushed the gkalpak:fix-core-handle-null-error branch from bc425ef to 0d96e56 Jul 20, 2021
@gkalpak gkalpak requested a review from AndrewKushnir Jul 20, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Jul 20, 2021

You can preview 0d96e56 at https://pr42881-0d96e56.ngbuilds.io/.

@AndrewKushnir
AndrewKushnir approved these changes Jul 20, 2021
View changes
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 20, 2021

Presubmit.

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 21, 2021

@gkalpak FYI, presubmits went well, adding this PR to the merge queue.

@dylhunn dylhunn closed this in eefe168 Jul 21, 2021
dylhunn added a commit that referenced this pull request Jul 21, 2021
@gkalpak @dylhunn
fix(core): correctly handle null or undefined in `ErrorHandler#ha…
a779a10 
…ndleError()` (#42881)

Since `ErrorHandler#handleError()` expects an argument of type `any` it
should be able to handle values such as `null` and `undefined`.
Previously, it failed to handle these values, because it was trying to
access properties on them.

This commit fixes it by ensuring no properties are accessed on `null` or
`undefined` values.

NOTE: This is part of fully addressing #28106.

Fixes #21252

PR Close #42881
@gkalpak gkalpak deleted the gkalpak:fix-core-handle-null-error branch Jul 22, 2021
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Aug 22, 2021

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 Aug 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir

Assignees
No one assigned
Labels
action: merge cla: yes comp: core target: patch type: bug/fix
Projects
None yet
Milestone
Backlog
Linked issues

Successfully merging this pull request may close these issues.

APP_INITIALIZER: rejected promise throws "Cannot read property 'ngOriginalError' of undefined"
4 participants
@gkalpak @JoostK @mary-poppins @AndrewKushnir