Skip to content

Remove HistoryListFragment.onSaveInstanceState#10405

Merged
shiki merged 1 commit intorelease/13.0from
issue/10401-fix-state-save-crash
Aug 19, 2019
Merged

Remove HistoryListFragment.onSaveInstanceState#10405
shiki merged 1 commit intorelease/13.0from
issue/10401-fix-state-save-crash

Conversation

@shiki
Copy link
Contributor

@shiki shiki commented Aug 16, 2019

Fixes #10401.

Findings

This was caused by the “fix” in #10240. The only way I could think of why this happens is this scenario:

  1. The user opens the editor and opens the History page.
  2. There's a delay in loading the Post data in HistoryViewModel.
  3. The user immediately puts the app in the background.
  4. State saving kicks in and crashes because the HistoryViewModel wasn't fully initialized yet.

I can consistently reproduce this scenario by adding a delay(). To do that, change the val post: PostModel? line in HistoryViewModel.create() to this:

val post: PostModel? = withContext(bgDispatcher) {
    kotlinx.coroutines.delay(5_000)
    postStore.getPostByLocalPostId(localPostId)
}

This adds a 5 second delay of the loading. Turn on Don't keep activities in your device's Developer options. And then perform the steps above. The app should crash.

Solution

I removed the onSaveInstanceState of HistoryListFragment. It looks like we do not use that key anyway. We rely on the Fragment arguments to load the SiteModel. Using the arguments for state restoration should be enough since they should be retained automatically.

Testing

Please repeat the steps described in the Findings section and validate that the app does not crash anymore.

Reviewing

Only 1 reviewer is needed but anyone can review.

Release Notes

  • If there are user-facing changes, I have added an item to RELEASE-NOTES.txt.

The value we save in WordPress.SITE is not used at all. Also, this causes a crash if the bundle saving kicks in before the ViewModel is fully initialized.
@shiki shiki added this to the 13.0 milestone Aug 16, 2019
@shiki shiki marked this pull request as ready for review August 16, 2019 15:41
Copy link
Contributor

@maxme maxme left a comment

Choose a reason for hiding this comment

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

It looks like we do not use that key anyway.

Yep, I had a look at the history and from the first version it seems unused. I also checked that the viewModel couldn't be used beforeviewModel.create in this file.

Using the arguments for state restoration should be enough since they should be retained automatically.

Looks good, the post id and the SiteModel will be restored.

@shiki shiki merged commit 26d61b4 into release/13.0 Aug 19, 2019
@shiki shiki deleted the issue/10401-fix-state-save-crash branch August 19, 2019 14:26
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.

2 participants