Use local post ID in HistoryList to fix TransactionTooLargeException #10240

shiki · 2019-07-17T19:47:35Z

Findings

When I edit a post with 300K words and put the app to the background, the app will consistently crash.

Using toolargetool, I found that the EditPostActivity's Bundle grows as the PostModel contents grows. At 2000 words:

2019-07-16 12:52:01.792 32319-32319/org.wordpress.android D/TooLargeTool: EditPostActivity.onSaveInstanceState wrote: Bundle161877096 contains 16 keys and measures 38.3 KB when serialized as a Parcel
    * stateKeyDroppedMediaUri = 0.1 KB
    * android:viewHierarchyState = 1.0 KB
    * stateKeyRevision = 0.0 KB
    * android:support:fragments = 33.7 KB
	...

At 5000 words:

2019-07-16 12:52:54.698 32319-32319/org.wordpress.android D/TooLargeTool: EditPostActivity.onSaveInstanceState wrote: Bundle106500119 contains 16 keys and measures 58.7 KB when serialized as a Parcel
    * stateKeyDroppedMediaUri = 0.1 KB
    * android:viewHierarchyState = 1.0 KB
    * stateKeyRevision = 0.0 KB
    * android:support:fragments = 54.1 KB
	...

I honed in on the android:support:fragments which grows similarly. Digging deeper, I found that HistoryListFragment bundles a key_post object which is a PostModel. At 2000 words:

2019-07-16 12:52:02.058 32319-32319/org.wordpress.android D/TooLargeTool: HistoryListFragment.onSaveInstanceState wrote: Bundle27877272 contains 3 keys and measures 2.8 KB when serialized as a Parcel
    * SITE = 1.9 KB
    * android:user_visible_hint = 0.1 KB
    * android:view_state = 0.9 KB
    * fragment arguments = Bundle57837811 contains 2 keys and measures 16.6 KB when serialized as a Parcel
    * key_post = 14.7 KB
    * key_site = 1.9 KB

And at 5000 words:

2019-07-16 12:52:54.838 32319-32319/org.wordpress.android D/TooLargeTool: HistoryListFragment.onSaveInstanceState wrote: Bundle148956272 contains 3 keys and measures 2.8 KB when serialized as a Parcel
    * SITE = 1.9 KB
    * android:user_visible_hint = 0.1 KB
    * android:view_state = 0.9 KB
    * fragment arguments = Bundle231264648 contains 2 keys and measures 36.9 KB when serialized as a Parcel
    * key_post = 35.1 KB
    * key_site = 1.9 KB

If the post contents is even bigger, we would hit the Bundle size limit.

Solution

This PR changes the HistoryListFragment so it will only accept a local post ID and it handles the loading of the PostModel itself in the background. When the state is saved, only the local post ID will be saved. The EditPostActivity android:support:fragments object no longer grows and stays at about 19.1KB.

Editing the post with 300K words no longer crashes. It takes 5 seconds to load like always but that is currently not the focus of this fix. I tested this with a post that has a lot of images and embedded Youtube videos and I have not observed any crashes.

Other Changes

Since the HistoryListFragment now only has the local post id, I added a simple routine for the unlikely scenario where the PostModel cannot be loaded. The history page will show an empty view.

Testing

History List

Please do a regression test for the revision history like:

Loading the list while online or offline
Viewing a revision
Restoring a revision
Reloading the list when the device is back online
Swiping to refresh

All the existing features should still work without problems.

TransactionTooLargeException

Please also test if you will still receive a TransactionTooLargeException crash after this change. There are reproduction steps in the linked issues like:

I found that the most consistent way I will receive the crash is:

Create a post with a very big content in the Web.
On the device, enable Developer → Don't keep activities.
Edit the post.
Put the app in the background.

The crash happens after this.

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.

Usage of background threads will be applied later on.

To minimize regressions, use uiDispatcher as the default coroutine dispatcher. This is because HistoryViewModel has assumptions in some of its routines that it is running on the main thread.

Keeping the post in the `Bundle` adds to the Editor’s saved instance for state restoration. Storing the local post id only decreases the size of the saved instance. Therefore, decreasing the chances of TransactionTooLargeException exceptions.

An “empty” UI will be shown for now. It’s unlikely to have a `null` post so I think this is good enough.

WordPress/src/test/java/org/wordpress/android/viewmodel/history/HistoryViewModelTest.kt

WordPress/src/main/java/org/wordpress/android/viewmodel/history/HistoryViewModel.kt

malinajirka

Good job @shiki! Thanks for diving into this and for the great description ❤️.

I did some regressions test of the History fragment and everything worked as expected. However, I wasn't able to reproduce the crash on any of my devices/emulators. The EditPostActivity either never loads (black screen or frozen editor screen) or it doesn't crash. Having said that, I've encountered the TransactionTooLargeException in many other applications and I'm positive this fix will solve it (We still store SiteModel into a Bundle at a lot of places, but it shouldn't be that big hopefully).

WordPress/src/main/java/org/wordpress/android/ui/posts/HistoryListFragment.kt

shiki · 2019-07-19T13:57:58Z

@malinajirka Thank you for spending the time to test this!

However, I wasn't able to reproduce the crash on any of my devices/emulators. The EditPostActivity either never loads (black screen or frozen editor screen) or it doesn't crash.

That's weird. And no errors in the logs too?

Having said that, I've encountered the TransactionTooLargeException in many other applications and I'm positive this fix will solve it

Thanks for confirming that. This is the first time I've seen and fixed an error like this. 😄

We still store SiteModel into a Bundle at a lot of places, but it shouldn't be that big hopefully

Right. I looked at them and they weren't that big. Is it worth it to create an issue for it?

I responded to your concern. Please let me know if I merge this. 🙂

malinajirka · 2019-07-22T06:27:53Z

That's weird. And no errors in the logs too?

Yep, no errors. I have encountered some segmentation fault in C code once, but the log wasn't very helpful and I wasn't able to reproduce it.

Right. I looked at them and they weren't that big. Is it worth it to create an issue for it?

I don't think it's necessary to fix them. We just need to make sure we don't introduce the same issue in any new code. I know we have discussed this issue about a year ago, so hopefully at least most people should be aware that it's not a best practice to store potentially huge objects into Bundles. Thank you though;)!

Looks good to me. Thank you, great job!

shiki added 3 commits July 17, 2019 12:47

Convert HistoryViewModel into a ScopedViewModel

31d1b39

Usage of background threads will be applied later on.

HistoryViewModel: Use uiDispatcher as default

5d95500

To minimize regressions, use uiDispatcher as the default coroutine dispatcher. This is because HistoryViewModel has assumptions in some of its routines that it is running on the main thread.

shiki added the [Type] Enhancement label Jul 17, 2019

shiki added this to the 13.0 milestone Jul 17, 2019

shiki mentioned this pull request Jul 17, 2019

Groundskeeping (Android) Master Issue: Jul 15 to Jul 19, 2019 #10217

Closed

HistoryViewModel: Add handling for null posts

9e39f6d

An “empty” UI will be shown for now. It’s unlikely to have a `null` post so I think this is good enough.

wpmobilebot reviewed Jul 17, 2019

View reviewed changes

WordPress/src/test/java/org/wordpress/android/viewmodel/history/HistoryViewModelTest.kt Outdated Show resolved Hide resolved

wpmobilebot reviewed Jul 17, 2019

View reviewed changes

WordPress/src/test/java/org/wordpress/android/viewmodel/history/HistoryViewModelTest.kt Outdated Show resolved Hide resolved

wpmobilebot reviewed Jul 17, 2019

View reviewed changes

WordPress/src/test/java/org/wordpress/android/viewmodel/history/HistoryViewModelTest.kt Outdated Show resolved Hide resolved

wpmobilebot reviewed Jul 17, 2019

View reviewed changes

WordPress/src/test/java/org/wordpress/android/viewmodel/history/HistoryViewModelTest.kt Outdated Show resolved Hide resolved

Add tests for HistoryViewModel

3ebf364

shiki force-pushed the issue/9685-history-bundle-usage branch from 2fc9967 to 3ebf364 Compare July 17, 2019 22:02

shiki commented Jul 17, 2019

View reviewed changes

WordPress/src/main/java/org/wordpress/android/viewmodel/history/HistoryViewModel.kt Show resolved Hide resolved

shiki changed the title ~~Issue/9685 history bundle usage~~ Use local post ID in HistoryList Jul 17, 2019

shiki requested review from khaykov, malinajirka and maxme July 17, 2019 23:19

shiki marked this pull request as ready for review July 17, 2019 23:19

shiki changed the title ~~Use local post ID in HistoryList~~ Use local post ID in HistoryList to fix TransactionTooLargeException Jul 19, 2019

malinajirka self-assigned this Jul 19, 2019

malinajirka approved these changes Jul 19, 2019

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/posts/HistoryListFragment.kt Show resolved Hide resolved

malinajirka merged commit 7b32a43 into develop Jul 22, 2019

malinajirka deleted the issue/9685-history-bundle-usage branch July 22, 2019 06:28

shiki mentioned this pull request Jul 22, 2019

Crash: TransactionTooLargeException #5456

Closed

phillbaker mentioned this pull request Aug 5, 2019

Android TransactionTooLargeException facebook/react-native#19458

Closed

3 tasks

loremattei mentioned this pull request Aug 16, 2019

UninitializedPropertyAccessException: lateinit property site has not been initialized #10401

Closed

shiki mentioned this pull request Aug 16, 2019

Remove HistoryListFragment.onSaveInstanceState #10405

Merged

1 task

shiki mentioned this pull request Nov 15, 2019

Add @username suggestions to notification and reader full comment view. #10781

Merged

2 tasks

dcalhoun mentioned this pull request Oct 12, 2021

RuntimeException: Failure from system #15447

Closed

antonis mentioned this pull request Mar 11, 2022

RuntimeException: Failure from system #16012

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use local post ID in HistoryList to fix TransactionTooLargeException #10240

Use local post ID in HistoryList to fix TransactionTooLargeException #10240

Uh oh!

shiki commented Jul 17, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

malinajirka left a comment

Uh oh!

Uh oh!

shiki commented Jul 19, 2019

Uh oh!

malinajirka commented Jul 22, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Use local post ID in HistoryList to fix TransactionTooLargeException #10240

Use local post ID in HistoryList to fix TransactionTooLargeException #10240

Uh oh!

Conversation

shiki commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Findings

Solution

Other Changes

Testing

History List

TransactionTooLargeException

Reviewing

Release Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shiki commented Jul 19, 2019

Uh oh!

malinajirka commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shiki commented Jul 17, 2019 •

edited

Loading

malinajirka commented Jul 22, 2019 •

edited

Loading