[Mobile] Fix crash on undefined image url

[mkevins]

Description

This PR adds a check to ensure attributes.url exists before attempting to call indexOf. Currently, there are scenarios which can result in attributes.id being truthy, while attributes.url is null or undefined for an image block.

When an image is cancelled before it is finished uploading, the url is set to null, but the id of the block remains. The next time the post is edited, the local draft will contain the state which triggers the crash.

How has this been tested?

This was tested using the steps here: wordpress-mobile/WordPress-Android#9768 (comment)

Types of changes

This is a bug fix for the crash discovered here: wordpress-mobile/WordPress-Android#9768

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[hypest]

attributes.id being truthy, while attributes.url is null or undefined

👋 @mkevins , I wonder, is that combination a valid state? If not, maybe we also need to try to set the block to a valid state?

[mkevins]

 @mkevins , I wonder, is that combination a valid state? If not, maybe we also need to try to set the block to a valid state?

I don't think this state is "valid". I've added some commits to set attributes.id to null when an image upload is cancelled. Also, I've added a console.warn which may be useful in tracking, as I expect we will still encounter this state from anyone that has already experienced this crash. This is because the local draft will contain the state. This is why we must also keep the check to prevent the crash in those cases.

[daniloercoli]

I've been chatting with @mkevins and helped a bit testing and finding other flows that can result in attributes.id being set, while attributes.url is null, and didn't find another flow yet.
I think the current solution if OK and we can include this to an hotfix if we're planning to release it.

[marecar3]

Hey @mkevins, I have left some comment but besides that, it's great to work here!
So be free to merge this PR when you resolve the comment.

[mkevins]

Thank you @daniloercoli and @marecar3 for testing and reviewing!

[koke]

I don't think this state is "valid"

I was looking at the code and it seems intentional and valid to me. The mediaID is a local UUID-like identifier. If the upload fails or was canceled, the iOS app will still have a local media object representing the image, and can use that to render the image, even after a relaunch. I think the url would only be present after a successful upload?

This is just from a static analysis though, I haven't debugged this to validate, but I imagine @SergioEstevao will be able to confirm or deny this 😄

[marecar3]

Hey @koke I think that @mkevins solution is good. When a user clicks on the Cancel upload button, it will reset the image block to the default state ( media upload options are presented). But the problem is that in the code we have left id which is hanging there and which needs to be removed.

Also, a failed state is different then cancel state as it can retry uploading.

Btw, I found some bug on iOS regarding failed upload

wordpress-mobile/gutenberg-mobile#978

cc : @SergioEstevao

[koke]

When a user clicks on the Cancel upload button, it will reset the image block to the default state ( media upload options are presented)

Considering the changes in #966, it’s not clear to me whether canceling would automatically bring the state back to placeholder, or allow to retry. But a failed upload would have a failed state where having the media ID still makes sense.

We could eventually decide something different, but that is why I think this is considered a valid state on the iOS code.

[marecar3]

When a user clicks on the Cancel upload button, it will reset the image block to the default state ( media upload options are presented)

Considering the changes in #966, it’s not clear to me whether canceling would automatically bring the state back to placeholder, or allow to retry. But a failed upload would have a failed state where having the media ID still makes sense.

We could eventually decide something different, but that is why I think this is considered a valid state on the iOS code.

Not sure if the link is good (#966), but I agree with your thoughts :)

[koke]

Ah, wrong repo 😂

wordpress-mobile/gutenberg-mobile#966