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

Attempt to fix RMessageView crash #4403

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Attempt to fix RMessageView crash #4403

merged 2 commits into from
Nov 17, 2022

Conversation

tonisevener
Copy link
Collaborator

Phabricator:
https://phabricator.wikimedia.org/T323163

Notes

This is an attempt to fix our RMessageView crashes. I cannot reproduce it, but I suspect the superview reference when setting up the constraints is occasionally nil, because when I replace that parameter with nil I see the same crash and call stack.

I think the better fix would be to have the superview set up all of these constraints, but that's too big of a change at this point in the release. So for now I'm just adding some defensive work to stop the display entirely if the superview is nil to avoid the crash.

Test Steps

  1. Regression test these toasts - subscribe/unsubscribe to a talk page topic, go to airplane mode, and confirm messages display fine as before.
  2. Force a situation where the new code is run with comments:
      //if (self.superview) {
      //    [self setupLayout];
      //} else {
          [self removeFromSuperview];
          return nil;
      //}

and repeat Step 1. Confirm toasts no longer display, and there are no crashes.

if (self.messagePosition != RMessagePositionBottom) {
[self layoutIfNeeded];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is an unrelated cleanup thing - I think it was only needed with the way the old constraints were set up, which were modified in #4384. I don't see a need for this call at this point, plus we're calling layoutIfNeeded it again further down in the method after the constraints are set up. Removing it seems to cause no regressions in my testing.

@tonisevener tonisevener requested review from a team and staykids and removed request for a team November 17, 2022 17:20
@staykids staykids self-assigned this Nov 17, 2022
Copy link
Contributor

@staykids staykids left a comment

Choose a reason for hiding this comment

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

Nice – this bit of defensive programming doesn't seem unreasonable in an attempt to resolve the crash. Haven't yet noticed regressions on iPhone or iPad. Looking at this RMessage library again makes me want to pick up my toast library work again the next time it makes sense to...

@staykids staykids removed their assignment Nov 17, 2022
@staykids staykids merged commit 6d1f45c into main Nov 17, 2022
@staykids staykids deleted the T323163 branch November 17, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants