Skip to content

Fix infinite recursion in FirebaseStorageError message getter #4760

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

Closed
wants to merge 1 commit into from

Conversation

mitchellwills
Copy link

This was originally broken in the refactor in #3945

@changeset-bot
Copy link

changeset-bot bot commented Apr 8, 2021

🦋 Changeset detected

Latest commit: 253dc67

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
firebase Patch
@firebase/storage Patch
@firebase/rules-unit-testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -27,16 +27,15 @@ export class FirebaseStorageError extends FirebaseError {
*/
customData: { serverResponse: string | null } = { serverResponse: null };

private readonly baseMessage: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

_baseMessage

Copy link
Author

Choose a reason for hiding this comment

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

Done

'@firebase/storage': patch
---

Fix infinite recursion in FirebaseStorageError message getter
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an associated Github issue? Or an example of when the problem comes up in usage? So I can better explain to users in the release notes.

Copy link
Author

Choose a reason for hiding this comment

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

No there isn't an existing issue (beyond this PR). Basically any time the message getter is accessed you would get a Maximum call stack size exceeded Error raised.

hsubox76
hsubox76 previously approved these changes Apr 20, 2021
@hsubox76 hsubox76 dismissed their stale review April 20, 2021 19:24

Missed test failures.

Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

Sorry, I thought the test failures were flaky and missed the actual problem. super() needs to be called with the message or else when the error is thrown it doesn't actually throw with the message. I don't think the property can be overridden with a getter. I think we should keep _baseMessage, get rid of get message(), and add some code to set serverResponse to either set this.message to _baseMessage + serverResponse or _baseMessage depending on the arg.

@mitchellwills
Copy link
Author

Superseded by #4807

@firebase firebase locked and limited conversation to collaborators May 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants