[Gutenberg] Fix crash when showing media upload dialog

[fluiddot]

Fixes a crash in the editor related to the dialog shown as part of the upload process. Specifically, the ones displayed when trying to cancel or retry an upload (p5T066-41d-p2#comment-14730).

Error displayed in development mode:

After merging #18364, seems that some of the upgraded dependencies (probably activity), require the Dialog component to be called from the main thread when showing it. This PR updates the React Native bridge code that handles showing upload dialogs.

To test

Cancel dialog - Upload for media

1. Create/open a post.
2. Add an Image block.
3. Select an image from the device.
4. While the image is uploading, tap on the block.
5. Observe that a dialog is presented with the option to cancel the upload.

Retry dialog - Upload for media

1. Create/open a post.
2. Add an Image block.
3. Select an image from the device.
4. While the image is uploading, turn off the internet connection and wait.
5. Observe that a retry message is displayed.
6. Tap on the image.
7. Observe that a dialog is presented with the option to retry the upload.

Cancel dialog - Upload for media collection (only used in Story block)

1. Create/open a post.
2. Add a Story block.
3. Tap on "ADD MEDIA".
4. Select images from the device.
5. Apply changes to save the story.
6. While images are uploading, tap on the block.
7. Observe that a dialog is presented with the option to cancel the upload.

Retry dialog - Upload for media collection (only used in Story block)

1. Create/open a post.
2. Add a Story block.
3. Tap on "ADD MEDIA".
4. Select images from the device.
5. Apply changes to save the story.
6. While images are uploading, turn off the internet connection and wait.
7. Observe that a retry message is displayed.
8. Tap on the block.
9. Observe that a dialog is presented with the option to retry the upload.

Regression Notes

1. Potential unintended areas of impact
   N/A

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   N/A

3. What automated tests I added (or what prevented me from doing so)
   N/A

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.

UI Changes testing checklist:

• Portrait and landscape orientations.
• Light and dark modes.
• Fonts: Larger, smaller and bold text.
• High contrast.
• Talkback.
• Languages with large words or with letters/accents not frequently used in English.
• Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
• Large and small screen sizes. (Tablet and smaller phones)
• Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr18544-47c5a52
	Commit	47c5a52
	Direct Download	wordpress-prototype-build-pr18544-47c5a52.apk

Note: Google Login is not supported on these builds.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr18544-47c5a52
	Commit	47c5a52
	Direct Download	jetpack-prototype-build-pr18544-47c5a52.apk

Note: Google Login is not supported on these builds.

[peril-wordpress-mobile]

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

[ParaskP7]

👋 @fluiddot !

I have reviewed and tested this PR as per the instructions (great set of instructions btw), everything works as expected (good job on finding and fixing this btw)! 🌟 🌟 🌟

---

EXTRAS:

1. Question (❓): I have just one question, I am seeing this block of similar AlertDialog.Builder related code to have been existing since 13/6/20, which seems to be guarding against such pre-existing crashes (with a getActivity().runOnUiThread(...) block, plus this and this for toasts). I wonder how come with was done 3 years ago, but similar thread switches weren't added to the rest of such dialog functionality... 🤔
2. Question (❓): Having gotten this crash it makes me also wonder if we might have other such crashes in other places of the app, or else, whether this is specific to GB because of the way OnMediaLibraryButtonListener, OnMediaFilesCollectionBasedBlockEditorListener and other such listeners are setup, running on background and then returning to main thread. Can you help me clear any doubts about how this is set-up and working, just as an FYI and so that we can maybe figure out if other screens of the app can be impacted like that. 🙏
3. Suggestion (💡): I am also wondering whether we should consider adding the @UiThread annotation to such methods (like showRetryMediaUploadDialog(...)) just to indicate to the caller that they should be called via getActivity().runOnUiThread(...) to avoid potential crashes, that is, they the same methods gets called from elsewhere, not sure... 🤔

[geriux]

I went through the test cases using a Samsung Galaxy A23 5G (Android 13) and it works as expected 🚀 Thank you!

[fluiddot]

Thanks @ParaskP7 for reviewing and testing the PR ❤️ !

1. Question (❓): I have just one question, I am seeing this block of similar AlertDialog.Builder related code to have been existing since 13/6/20, which seems to be guarding against such pre-existing crashes (with a getActivity().runOnUiThread(...) block, plus this and this for toasts). I wonder how come with was done 3 years ago, but similar thread switches weren't added to the rest of such dialog functionality... 🤔

Good question, and to be honest I wondered the same when working on the fix 😅. I'm afraid I can't tell why it was done for some parts and not for others, maybe the ones that have the guards were introduced more recently and we identified the potential crash 🤷‍♂️ .

2. Question (❓): Having gotten this crash it makes me also wonder if we might have other such crashes in other places of the app, or else, whether this is specific to GB because of the way OnMediaLibraryButtonListener, OnMediaFilesCollectionBasedBlockEditorListener and other such listeners are setup, running on background and then returning to main thread. Can you help me clear any doubts about how this is set-up and working, just as an FYI and so that we can maybe figure out if other screens of the app can be impacted like that. 🙏

I'm not sure if this issue could be affecting other areas of the app. I mainly tested Gutenberg but also checked the classic editor (Aztec editor) as it also displays a dialog. However, it worked as it's executed in the main thread.

In the case of Gutenberg, this is produced by how the React Native bridge works. The calls from the Javascript side to execute code on the native side are done in a separate thread for the native modules. This means that all intents to show a dialog from the editor's logic need to be guarded against this issue. This gives me the impression that this crash is more likely to happen in the editor than in other areas of the app, but we can't truly tell without investigating further.

3. Suggestion (💡): I am also wondering whether we should consider adding the @UiThread annotation to such methods (like showRetryMediaUploadDialog(...)) just to indicate to the caller that they should be called via `getActivity().runOnUiThread(...) to avoid potential crashes, that is, they the same methods gets called from elsewhere, not sure... 🤔

Ah, interesting, thanks for sharing this. I've updated the PR with this suggestion (47c5a52) 👍.

[fluiddot]

3. Suggestion (💡): I am also wondering whether we should consider adding the @UiThread annotation to such methods (like showRetryMediaUploadDialog(...)) just to indicate to the caller that they should be called via `getActivity().runOnUiThread(...) to avoid potential crashes, that is, they the same methods gets called from elsewhere, not sure... 🤔

Ah, interesting, thanks for sharing this. I've updated the PR with this suggestion (47c5a52) 👍.

@ParaskP7 I tested using the annotation and calling one of the updated functions without the guard and I haven't received any warning or error. Not sure if there's anything else we should incorporate to avoid the crash in the future 🤔. I understand that adding the annotation won't have any undesired side effect to the fix, hence I'll keep it and proceed with merging the PR.

[ParaskP7]

👋 @fluiddot , thanks for the reply and adding @UiThread! 🥇

Good question, and to be honest I wondered the same when working on the fix 😅. I'm afraid I can't tell why it was done for some parts and not for others, maybe the ones that have the guards were introduced more recently and we identified the potential crash 🤷‍♂️ .

👍 🤷 😃

I'm not sure if this issue could be affecting other areas of the app. I mainly tested Gutenberg but also checked the classic editor (Aztec editor) as it also displays a dialog. However, it worked as it's executed in the main thread.

Thanks for checking! 💯

In the case of Gutenberg, this is produced by how the React Native bridge works. The calls from the Javascript side to execute code on the native side are done in a separate thread for the native modules. This means that all intents to show a dialog from the editor's logic need to be guarded against this issue.

Ah, this now makes much more sense to me, thanks for explaining! 🎯

This gives me the impression that this crash is more likely to happen in the editor than in other areas of the app, but we can't truly tell without investigating further.

I agree! 💯

Ah, interesting, thanks for sharing this. I've updated the PR with this suggestion (47c5a52) 👍.

Awesome, thanks for applying this suggestion, I have reviewed the change and everything LGTM! ✅

@ParaskP7 I tested using the annotation and calling one of the updated functions without the guard and I haven't received any warning or error. Not sure if there's anything else we should incorporate to avoid the crash in the future 🤔. I understand that adding the annotation won't have any undesired side effect to the fix, hence I'll keep it and proceed with merging the PR.

Yes, you are right, it doesn't give you a warning, but it is at least a good indication for someone that would try and use this method in the future, or a reviewer that will take a look at that code change, potentially catching a crash if that is not guarding with getActivity().runOnUiThread(...). 🤞

Though, I agree that a warning would have been much much nicer... 🤷

---

FYI: Feel free to merge this at your earliest convenience and again, many THANK for identifying and fixing this! ❤️ 🚀

[sentry]

Sentry issue: WORDPRESS-ANDROID-2N93