WP Stories: relaxing the HEADING_END definition to avoid IOB

[mzorz]

Fixes #13697

The PR is too simple, but it's all what's needed in order to circumvent this problem.

As explained in #13697 (comment) I was able to reproduce and explain this issue.

The problem is, having a Story block and trying to edit the Post in Aztec will make it invisible to the user. Then the user might try adding an image in the place where the Story was, and Aztec will then break the Story block structure, making it invalid for Gutenberg and causing trouble in our integration in WPAndroid when processing the mediaFiles attribute.

This is a sample content before editing with Aztec:

 <!-- wp:jetpack/story -->
<div class="wp-block-jetpack-story wp-story"/>
<!-- /wp:jetpack/story -->
<!-- wp:image {"id":2225,"sizeSlug":"large"} --><figure class="wp-block-image size-large"><img class="wp-image-2225" src="https://testmzorz3.files.wordpress.com/2021/04/wp_story1618432037649_0-2.jpg?w=576" alt="" /></figure> <!-- /wp:image -->

And this is the content at the time of crashing, after editing with Aztec and trying to add an image:

 <!-- wp:jetpack/story --><div class="wp-block-jetpack-story wp-story"><img data-wpid="51" src="/storage/emulated/0/WhatsApp/Media/WhatsApp Images/IMG-20210415-WA0016.jpg" class="uploading size-full" /></div><!-- /wp:jetpack/story --> <!-- wp:image {"id":2225,"sizeSlug":"large"} --><figure class="wp-block-image size-large"><img class="wp-image-2225" src="https://testmzorz3.files.wordpress.com/2021/04/wp_story1618432037649_0-2.jpg?w=576" alt="" /></figure> <!-- /wp:image -->

This situation will be avoided altogether at least for simple sites by having the switch to classic editor button removed in 17.1 (*), but nonetheless users who already have written a Post and modified it like this will keep running into it, so I also went ahead and added a check to avoid running into IOB situation.

The particular problem here is editing on Aztec will remove the Story block header's new line we expect to find in const val HEADING_END = " -->\n". If you check the reported content, it starts like this: <!-- wp:jetpack/story --><div class.... So this PR just relaxes the HEADING_END matcher a bit and only defines the closing comment tag, as in " -->". This prevents the issue from happening (although of course nothing can be done in terms of having the block already mangled). Gutenberg will detect this invalidity at least.

(*) context: see deprecation of Aztec related PRs #14381 (use Block Editor by default) and #14334 (removal of "Switch to classic editor" option in the Editor menu)
Also see: p7cLQ7-1sn-p2
And: p1618236627386400-slack-C6UJ0KRKQ

To Test:

0. checkout and install version 17.0 of WPAndroid https://github.com/wordpress-mobile/WordPress-Android/releases/download/17.0/wpandroid-17.0-signed.apk (I recommend instead checking out the commit hash and building so you can then apply this patch easily - you can find the associate commit hash here e8490c9)
   remember to git submodule update --recursive if you want to build gutenberg from source, or comment the following line in gradle.properties
   # wp.BUILD_GUTENBERG_FROM_SOURCE = true (or set it to false)

1. create a Post with Gutenberg on mobile

2. add a Story block. Please now add an image to it. If you leave the Story block empty, you'll hit the other IOB exception handled in Stories: add bounds check in saving, don't process an empty Story block #14460. (I was tempted to make the change on that PR but I wanted to keep things separate to be able to explain how we came about this).

3. now switch to classic editor (Aztec)

4. the cursor will be found at the start (sometimes it doesn't appear blinking, but internally it's pointing to the beginning of content)

5. add an image

6. observe the image is displayed and the upload progress starts, but then everything freezes.

7. the logs show an infinite loop of this:

2021-04-15 10:33:22.923 4652-4652/? E/WordPress-EDITOR: Error while parsing Story blocks: String index out of range: -23
2021-04-15 10:33:22.924 4652-4652/? E/WordPress-EDITOR: HTML content of the post before the crash: <!-- wp:jetpack/story {"mediaFiles":[{"alt":"","caption":"","id":"204","link":"https://testmzorz3.files.wordpress.com\2021/04/wp_story1618006190441_04280759730505573739.jpg","mime":"image/jpeg","type":"image","url":"https://testmzorz3.files.wordpress.com/2021/04/wp_story1618006190441_04280759730505573739.jpg"}]} --><div class="wp-block-jetpack-story wp-story"><img data-wpid="51" src="/storage/emulated/0/WhatsApp/Media/WhatsApp Images/IMG-20210415-WA0016.jpg" class="uploading size-full" /></div><!-- /wp:jetpack/story --> <!-- wp:image {"id":2225,"sizeSlug":"large"} --><figure class="wp-block-image size-large"><img class="wp-image-2225" src="https://testmzorz3.files.wordpress.com/2021/04/wp_story1618432037649_0-2.jpg?w=576" alt="" /></figure> <!-- /wp:image -->

8. update to this branch (this was based off the detached HEAD at the commit mentioned above) and install on your phone. Note on this: if you don't use the same signature the app will be uninstalled and its data removed. Alternatively, you can take the Post content as expressed in that log and write a Post in HTML mode (for example on wp-admin on the web), save it, then open it again in visual mode and exit again. It's a problem to install and run a debug version of 17.0 because mobile gutenberg changed from being a submodule to composite builds en between 17.0 and 17.1, so it's hard to test the "normal" upgrade a regular user would do.
9. when the app starts, go and open the same Post.
10. observe the crash doesn't happen anymore (see logcat).

Regression Notes

1. Potential unintended areas of impact
   UploadService, but only when switching between editors.

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   Tested adding media manually, and verified the current tests still run

3. What automated tests I added (or what prevented me from doing so)
   Tested adding media manually, and verified the current tests still run

PR submission checklist:

• I have completed the Regression Notes.
• 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.

[renanferrari]

Hey @mzorz, thanks for submitting this and for keeping me in the loop on this issue!

I tested the app both before and after applying your changes and was able to successfully reproduce the error and the fix. (By the way, thanks a lot for writing such a detailed PR and for basing this off the 17.0 commit, both of those things made it really easy to test this!)

Code-wise, I can say your changes make sense to me and they look pretty good – at least as far as my editor skills go 😄

LGTM 👍

[mzorz]

Thank you for your review @renanferrari 🙇 😄 very much appreciated; worth to mention how the effort you put in adding logging in #14185 and #14191 helped to solve the issue 🥇