Pass post id instead of post model within intents and extras #5856

oguzkocer · 2017-05-09T14:26:50Z

Fixes #5456. This is a possible fix to our TransactionTooLargeException crash. Unfortunately we do not have enough details about the crashes to make sure we have the right fix for it, so we have to keep trying until we figure it out. Having said that I think this is a step in the right direction. I think, unless it's mandatory, we should always try to pass ids instead of whole objects between activities. It makes the code more robust as well as prevent issues like this one.

Originally, I was working on the release/7.3 branch for this, so we could fix it asap. However, I believe this fix looks deceptively simple yet can cause some unseen issues, so I think it's just safer to target develop. We don't want to introduce more crashes while trying to fix one.

The changes are straightforward, however I got a little worried about the config changes (re-creating fragments and activities) since we don't pass the cached version of the post anymore and a new version from the DB is retrieved. As far as I can tell from the code, we do snyc the changes before this happens and my tests didn't result in anything lost, but it'd be good if we can test it out further.

To test:

Go into the post list, open a post, edit it, rotate the screen, assign featured images etc. Anything you can think of when a cached post might be purged and the DB version is used. For such a small PR, it actually requires some rigorous testing.

@aforcier Since this is posts related, do you mind reviewing this one, I'd feel more comfortable if I got the from you!

/cc @mzorz @maxme @theck13 (since you were all interested in this issue)

…tents

We already have a check for the existence of the post model after this one, so removing this one to avoid double check.

maxme · 2017-05-09T14:30:40Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

@@ -153,7 +153,7 @@
 public class EditPostActivity extends AppCompatActivity implements EditorFragmentListener, EditorDragAndDropListener,
        ActivityCompat.OnRequestPermissionsResultCallback, EditorWebViewCompatibility.ReflectionFailureListener,
        PhotoPickerFragment.PhotoPickerListener {
-    public static final String EXTRA_POST = "postModel";
+    public static final String EXTRA_POST_LOCAL_ID = "postModelLocalId";


+1 for the explicit naming EXTRA_POST_LOCAL_ID

maxme · 2017-05-09T14:31:16Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPreviewFragment.java

@@ -32,7 +37,7 @@ public static EditPostPreviewFragment newInstance(SiteModel site, PostModel post
        EditPostPreviewFragment fragment = new EditPostPreviewFragment();
        Bundle bundle = new Bundle();
        bundle.putSerializable(WordPress.SITE, site);
-        bundle.putSerializable(EditPostActivity.EXTRA_POST, post);
+        bundle.putInt(EditPostActivity.EXTRA_POST_LOCAL_ID, post.getId());


I would add a comment to explain why we switched from putSerializable to putInt (and why we're still doing it for the site).

Actually, after this PR what I was going to suggest is we remove the putSerializables as much as possible.

I agree and I disagree. We use putSerializable because we want to avoid passing ids, especially we want to avoid confusion with remote and local ids.

because we want to avoid passing ids

Can you elaborate on this? I am just curious about it. Because, we can avoid the confusion about the remote & local ids issue by just explicitly naming them like EXTRA_POST_LOCAL_ID or EXTRA_POST_REMOTE_ID. It's no more confusing than having the remote and local id versions of getters in our stores.

In general I thought it was better to pass the ids, especially to avoid crashes like this. I can't see what we get out of passing the serializable except for maybe convenience.

Please note that I am not against it, I just want to better understand it.

It's no more confusing than having the remote and local id versions of getters in our stores.

In the stores, we don't really have the choice because remote ids can't be unique. So we make that distinction (remoteId vs localId) to avoid any ambiguity.

But here in the app, we want to hide this and make it easier for developers. If we can avoid any id, that's even better. But in some cases (like here) we don't really have the choice.

Also note: when FluxC is complete, we will never see any remote id in the app - we currently have some because we still have REST calls not going through FluxC.

So, once that's done we can actually start using the ids instead? That's good news!

Do you mind either committing that comment you mentioned or writing it here? I am still unsure what to write there. Thanks!

Since PostModel objects can be quite large, in order to avoid issues like TransactionTooLargeException it's better to pass the id of the post. However, we still want to keep passing the SiteModel to avoid confusion around local & remote ids.

@maxme Would a comment like the above one work for what you had in mind?

aforcier · 2017-05-09T14:36:56Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPreviewFragment.java

 public class EditPostPreviewFragment extends Fragment {
+    @Inject PostStore mPostStore;


Going to need to inject this fragment into the AppComponent, otherwise mPostStore will always be null.

I was pretty sure I was missing something in that Fragment. Fixed in cd12939 thanks @aforcier!

oguzkocer · 2017-05-09T15:04:54Z

I forgot the add a note about this in the original description, but the reason I am not reverting the changes in #5698 is because this crash might be occurring in multiple places in our app and until we know all crashes are gone, I'd like to keep the changes for it. Once it's resolved, I think we should attempt to revert that change and maybe others in the future to keep the code clean.

maxme · 2017-05-11T10:22:11Z

Note: if we want this in 7.4, we have to change the targeted branch.

oguzkocer · 2017-05-11T10:27:25Z

Note: if we want this in 7.4, we have to change the targeted branch.

@maxme I'd feel better if we still merged it into develop and target 7.5 instead as explained below. I hate to do this for a crash but I just don't want to introduce more crashes. I leave the decision to you though.

Originally, I was working on the release/7.3 branch for this, so we could fix it asap. However, I believe this fix looks deceptively simple yet can cause some unseen issues, so I think it's just safer to target develop. We don't want to introduce more crashes while trying to fix one.

maxme · 2017-05-12T05:19:38Z

@maxme I'd feel better if we still merged it into develop and target 7.5 instead as explained below.

Ok, I removed the milestone.

…too-large-exception

oguzkocer · 2017-05-23T10:31:06Z

Just a reminder that this PR is still ready to be reviewed/merged! I just merged develop into the PR since there was a conflict. This comment is waiting feedback from @maxme and the last commit cd12939 is waiting to be reviewed by @aforcier.

I'd suggest us to merge this in as soon as possible, since it's hopefully fixing a crash.

aforcier · 2017-05-23T19:32:19Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPreviewFragment.java

        updateSiteOrFinishActivity(savedInstanceState);
    }

    private void updateSiteOrFinishActivity(Bundle savedInstanceState) {
        if (savedInstanceState == null) {
            if (getArguments() != null) {
                mSite = (SiteModel) getArguments().getSerializable(WordPress.SITE);
-                mPost = (PostModel) getArguments().getSerializable(EditPostActivity.EXTRA_POST);
+                mPost = mPostStore.getPostByLocalPostId(getArguments().getInt(EditPostActivity.EXTRA_POST_LOCAL_ID));


Serializing and de-serializing the post as this used to do keeps the same reference to the post. Doing it this way creates a new object, which is not updated as mPost is updated in the EditPostActivity. As a result the preview will always show the post as it was when opened (for new posts, the preview is always blank). The loadPost() method will have to account for this.

aforcier · 2017-05-23T19:35:41Z

@oguzkocer we should probably do the same thing for EditPostSettingsFragment - and I suspect this issue will have to be addressed there too.

oguzkocer · 2017-05-24T11:29:01Z

@aforcier The reason I didn't do the same thing in EditPostSettingsFragment was the issue you raised in EditPostPreviewFragment (I missed that one). Especially on the settings page, things could get really complicated if we don't keep a reference to it.

So, what I propose to do is, when post is passed to these fragments, we should use the id and get the post from the PostStore. For the config changes, we should just keep a reference to the object as I think this crash only occurs for when it's passed with intents, so we should be safe. What do you think?

aforcier · 2017-05-24T13:26:14Z

For the config changes, we should just keep a reference to the object as I think this crash only occurs for when it's passed with intents, so we should be safe. What do you think?

I'm not sure I fully grasp the underlying issue, but it seems like the crash can happen whenever 'too much' data is passed in a Binder transaction, and my understanding is that that would also apply to the bundle usage in onSaveInstanceState(). So I'm not sure we can get away with fixing the crash without having to manage a separate PostModel reference in the fragments.

aforcier · 2017-05-30T15:19:15Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPreviewFragment.java

@@ -60,15 +58,11 @@ private void updateSiteOrFinishActivity(Bundle savedInstanceState) {
        if (savedInstanceState == null) {
            if (getArguments() != null) {
                mSite = (SiteModel) getArguments().getSerializable(WordPress.SITE);
-                mPost = mPostStore.getPostByLocalPostId(getArguments().getInt(EditPostActivity.EXTRA_POST_LOCAL_ID));


With this change this fragment no longer needs PostStore - we can drop that injection and remove EditPostPreviewFragment from the AppComponent.

aforcier · 2017-05-31T20:36:32Z

Looks good to me - but perhaps we should still add a comment like this one to explain why we're passing by ID in this case? Maybe in ActivityLauncher? What do you think @maxme?

oguzkocer · 2017-06-01T12:43:51Z

I have added that comment with a slight change (confirmed by @maxme) to ActivityLauncher as you suggested @aforcier since the original change it was suggested for is removed. Should be ready to go now!

Thank you both!

aforcier · 2017-06-01T12:58:43Z

oguzkocer added 4 commits May 9, 2017 17:12

EXTRA_POST_LOCAL_ID introduced

90df353

Change rest of the app to pass postId instead of the post model in in…

52a8eed

…tents

Remove unnecessary post id check

683859d

We already have a check for the existence of the post model after this one, so removing this one to avoid double check.

EXTRA_POST_LOCAL_ID should be retrieved by getInt not getLong

008f33c

oguzkocer added Post List Posting/Editing [Type] Crash labels May 9, 2017

oguzkocer added this to the 7.4 milestone May 9, 2017

maxme reviewed May 9, 2017

View reviewed changes

aforcier reviewed May 9, 2017

View reviewed changes

Inject EditPostPreviewFragment into the component

cd12939

maxme removed this from the 7.4 milestone May 12, 2017

maxme self-assigned this May 12, 2017

Merge remote-tracking branch 'origin/develop' into crash/transaction-…

07870c2

…too-large-exception

aforcier reviewed May 23, 2017

View reviewed changes

aforcier self-assigned this May 23, 2017

Pass the post object in loadPost for EditPostPreviewFragment

df6078b

oguzkocer mentioned this pull request May 25, 2017

Feature/sharing update to fluxc #5933

Merged

aforcier reviewed May 30, 2017

View reviewed changes

maxme mentioned this pull request May 31, 2017

Crash report 7.4-7.2.1: RuntimeException from TransactionTooLargeException in ActivityLauncher.editPostOrPageForResult #5974

Closed

oguzkocer force-pushed the crash/transaction-too-large-exception branch from 14ea3c8 to b963513 Compare May 31, 2017 20:01

Remove unused PostStore injection from EditPostPreviewFragment

5adf5dc

oguzkocer force-pushed the crash/transaction-too-large-exception branch from b963513 to 5adf5dc Compare May 31, 2017 20:07

Add a comment explaining why we pass post id instead of post object

c023b13

aforcier merged commit 9fa1680 into develop Jun 1, 2017

aforcier deleted the crash/transaction-too-large-exception branch June 1, 2017 12:58

mzorz mentioned this pull request Jul 31, 2017

Async media: Scroll to failed post from error notifications #6424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass post id instead of post model within intents and extras #5856

Pass post id instead of post model within intents and extras #5856

oguzkocer commented May 9, 2017

maxme May 9, 2017

maxme May 9, 2017

oguzkocer May 9, 2017

maxme May 9, 2017

oguzkocer May 9, 2017 •

edited

Loading

maxme May 9, 2017

oguzkocer May 9, 2017

oguzkocer May 11, 2017

aforcier May 9, 2017 •

edited

Loading

oguzkocer May 9, 2017

oguzkocer commented May 9, 2017

maxme commented May 11, 2017

oguzkocer commented May 11, 2017

maxme commented May 12, 2017

oguzkocer commented May 23, 2017

aforcier May 23, 2017

aforcier commented May 23, 2017

oguzkocer commented May 24, 2017

aforcier commented May 24, 2017

aforcier May 30, 2017

aforcier commented May 31, 2017

oguzkocer commented Jun 1, 2017

aforcier commented Jun 1, 2017

		public class EditPostPreviewFragment extends Fragment {
		@Inject PostStore mPostStore;

Pass post id instead of post model within intents and extras #5856

Pass post id instead of post model within intents and extras #5856

Conversation

oguzkocer commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aforcier May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer commented May 9, 2017

maxme commented May 11, 2017

oguzkocer commented May 11, 2017

maxme commented May 12, 2017

oguzkocer commented May 23, 2017

Choose a reason for hiding this comment

aforcier commented May 23, 2017

oguzkocer commented May 24, 2017

aforcier commented May 24, 2017

Choose a reason for hiding this comment

aforcier commented May 31, 2017

oguzkocer commented Jun 1, 2017

aforcier commented Jun 1, 2017

oguzkocer May 9, 2017 •

edited

Loading

aforcier May 9, 2017 •

edited

Loading