Skip to content

Conversation

@hassaanelgarem
Copy link
Contributor

@hassaanelgarem hassaanelgarem commented Mar 7, 2023

Fixes #20266

Description

The crash is caused by calling JetpackNotificationMigrationService.shared.shouldPresentNotifications() from a background thread. This gets triggered by the weekly roundup Background Processing task. This PR fixes this crash by avoiding the call to RootViewCoordinator.shared when assessing if JP features are enabled or not from a background thread.

Testing Instructions

  1. Checkout trunk
  2. Perform the reproduction steps described in NSInternalInconsistencyException: Modifications to the layout engine must not be performed from a background thread after it has be... #20266 (comment)
  3. Observe that the app crashes with the same stack trace as the original crash
  4. Checkout this branch
  5. Perform the same steps again
  6. Observe that the app doesn't crash

P.S: If the app is left running, it might crash due to the workaround done to reproduce the crash. The workaround results in us repeatedly initializing RootViewCoordinator, which can eventually lead to a different crash.

Regression Notes

  1. Potential unintended areas of impact
    Jetpack features removal

  2. 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.

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 7, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@hassaanelgarem
Copy link
Contributor Author

@mokagio FYI 🙇

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 7, 2023

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr20275-1724d71 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 7, 2023

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr20275-1724d71 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Works as described 👍

hassaanelgarem and others added 3 commits March 8, 2023 13:11
The fix in #20275 was originally made on top of the `release/21.9`
branch but we decided to ship it as a hotfix.

I could have amended 9dfe337 with this
change, but decided to keep it in one piece to preserve a record of the
original change.
@mokagio mokagio force-pushed the fix/20266-weekly-updates-background-thread-crash branch from 02ba486 to 1724d71 Compare March 8, 2023 02:15
@mokagio mokagio changed the base branch from release/21.9 to release/21.8.1 March 8, 2023 02:16
@mokagio
Copy link
Contributor

mokagio commented Mar 8, 2023

@hassaanelgarem @momo-ozawa just a note to say that I rewrote the history on this branch in order to base it on top of the new release/21.8.1 hotifx.

This is something I discussed with @hypest just a few hours ago and the intent is to preserve attribution. With this approach, the PR that will land in the hotfix branch will have been created by @hassaanelgarem as opposed to the usual ad-hoc PR I would create to cherry pick.

This is the first time I try this approach and, apart from the rewriting of history of someone else's branch which always feels odd, I think it's working alright. Keen to know what you think.

For review reference, I pushed a backup of the original history. We can use it to compare the diff of that version against release/21.9 and this one to ensure the match (aside from the RELEASE-NOTES.txt version header change).

@mokagio
Copy link
Contributor

mokagio commented Mar 8, 2023

I'll merge this as soon as CI gives the okay then move on with the hotfix deployment.

For the record, I verified the fix behavior on top of 21.8.1 via:

  1. Checked out 21.8.1 and applied the code changes to reproduce the crash, bfb0c80
  2. Reproduced the crash
  3. Checked out this branch and applied the code changes to reproduce the crash
  4. Verified the crash doesn't occur and that shouldPresentNotifications is called (via breakpoint)

@mokagio mokagio merged commit e5bec03 into release/21.8.1 Mar 8, 2023
@mokagio mokagio deleted the fix/20266-weekly-updates-background-thread-crash branch March 8, 2023 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants