-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 deadlock that can occur when initializing RootViewCoordinator
#20300
Fix deadlock that can occur when initializing RootViewCoordinator
#20300
Conversation
You can test the changes in WordPress from this Pull Request by:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, in some cases, the code path of initializing RootViewCoordinator ends up in NotificationsViewController.configureJetpackBanner, which calls RootViewCoordinator.shared to decide if JP features are enabled or not.
Nice work investigating! I looked at the code and everything checks out. I also tested manually according to instructions on #20275 - works as described.
FYI @mokagio 🙇♀️ |
You can test the changes in Jetpack from this Pull Request by:
|
Generated by 🚫 dangerJS |
Fixes #20297
Description
I was unable to reproduce the crash, but from the stack trace, it's obvious that it's a deadlock. The deadlock is triggered by initializing the shared instance of
RootViewCoordinator
. Apparently, in some cases, the code path of initializingRootViewCoordinator
ends up inNotificationsViewController.configureJetpackBanner
, which callsRootViewCoordinator.shared
to decide if JP features are enabled or not. To fix this deadlock, I've removed the dependency ofJetpackFeaturesRemovalCoordinator.jetpackFeaturesEnabled
onRootViewCoordinator
.Testing Instructions
I wasn't able to reproduce the crash, so there are no specific testing instructions. Please review the code carefully. And retest #20275's testing instructions because the fix in this PR overrides the changes made in #20275.
Regression Notes
Potential unintended areas of impact
Jetpack features removal
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested going back and forth from the normal phase, phase 4, and new users phase.
I also retested Jetpack Focus: Avoid premature removal of features #19954's testing instructions.
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txt
if necessary.