Catch and log exception when the end index of the Story block is out of bounds

[renanferrari]

Fixes #13697

This PR wraps part of the findAllStoryBlocksInPostAndPerformOnEachMediaFilesJson method in a try/catch block and logs any IndexOutOfBoundsException silently to Sentry instead of crashing the app.

I wasn't able to determine the root cause of this crash, but I did share some thoughts about it in the linked issue.

To test:

Not much to test here since we can't reproduce the issue, but you can smoke test the Story feature and make sure nothing looks off.

PR submission checklist:

• I have considered adding unit tests where possible.
• 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]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[planarvoid]

Thanks for the changes, it looks good 👍

[mzorz]

Thank you for this work @renanferrari !

While this will prevent the app from crashing for users ❤️, we will still probably not have enough information about what's going on as the culprit (as you said, it's hard to reproduce 👍 ). I think preventing crashes is good, but the content of the Post itself could potentially be left in a weird / broken state (malformed gb blocks perhaps), and these problems would be even harder to track down.

If we had more information to reproduce the better; what do you think about adding content logging, like it was done in this PR? Perhaps we can perform the same checks (only log post contents to the crash report if all these conditions are met, to protect privacy).

Let me know your thoughts 🙇

[renanferrari]

@mzorz Thanks a lot for taking a look at this!

I share your concerns that this might make things a bit harder to track down, but since we're still sending the crash to Sentry I believe we wouldn't lose any information compared to what we already have. As for potentially leaving the post in a weird/broken state I'd argue that it's very likely that the reason we're seeing this crash is because the post was already in such a state in the first place.

That said, I totally agree with you that the more information we have the better and I'm all for your idea of adding content logging! I wasn't sure how to deal with the inherent privacy issues of doing something like this, but the example you shared above seems pretty reasonable to me (thanks for sharing it!).

I'll make sure to submit a follow up PR and request your review there.

[mzorz]

Thanks a lot for taking my comment and suggestions gracefully @renanferrari :D and thanks a lot for taking the time to walk the extra mile and add more info with the hope it'll make the underlying issue become clearer to us 🙏 🙇

As for potentially leaving the post in a weird/broken state I'd argue that it's very likely that the reason we're seeing this crash is because the post was already in such a state in the first place.

Yes this could probably be the case, it would be great to validate how come things end up in a broken state, and having the info to reproduce could potentially open some other doors for us. As you said, if the case is it was broken before, then we may be able to produce some new hypothesis with this.

That said, I totally agree with you that the more information we have the better and I'm all for your idea of adding content logging!

Also I realize I didn't explain myself quite well on the last comment, so let me dig a bit more 🙏 What I'm thinking is this change now essentially changes the flow after it runs into the problem. Up until this PR the app crashes there, but after this PR is introduced it may crash later on with some other reason that may seem unrelated when we look into it in the reports, potentially making the problem even harder to spot. But right exactly as you said, connecting the dots between one thing and the other might be easier with more information and hopefully one or two cases we can reproduce, so that's why I thought it'd be great to have all the info we could at the time of analyzing it :)

I wasn't sure how to deal with the inherent privacy issues of doing something like this, but the example you shared above seems pretty reasonable to me (thanks for sharing it!).

Right, I thought you'd find it useful as I recall having gone through this analysis (to understand which kind of content could we use for logging, etc) in the past. BTW things have shifted slightly but the validation checks remain the same.

I'll make sure to submit a follow up PR and request your review there.

Awesome! Thanks once more 🙇

[renanferrari]

Up until this PR the app crashes there, but after this PR is introduced it may crash later on with some other reason that may seem unrelated when we look into it in the reports, potentially making the problem even harder to spot.

@mzorz Thanks for the clarifying that, it makes total sense! 🙂

BTW things have shifted slightly but the validation checks remain the same.

Thanks for letting me know! Btw, just to make sure we're on the same page: we don't want to log the content using Sentry, right? A call to AppLog::e should be enough?

[mzorz]

Btw, just to make sure we're on the same page: we don't want to log the content using Sentry, right? A call to AppLog::e should be enough?

That's a good question - yes IIUC we can just call AppLog.e as shown in the linked code above (both the old PR and the current code in develop) - this way the log will be sent to Sentry as well if they have opted in to logging / crash reports on their app.

[renanferrari]

Sorry, I just realized my question was quite confusing. What I meant to say is that we don't want to send the content as a new Sentry event by using the call to CrashLogging::reportException but instead just want to log it as a Sentry breadcrumb by using AppLog::e.

Either way, if I understand your answer correctly, it seems that we were indeed on the same page. I've submitted a PR for this here: #14191

[mzorz]

... but instead just want to log it as a Sentry breadcrumb by using AppLog::e.

Excellent, I understood the same thing 😄 👍

Either way, if I understand your answer correctly, it seems that we were indeed on the same page. I've submitted a PR for this here: #14191

🙌 thank you!