Fix 'Take a Photo' option failing after adding an image to gallery block #14299

ceyhun · 2020-06-11T18:54:31Z

Fixes wordpress-mobile/gutenberg-mobile#1965

Could potentially fix https://sentry.io/organizations/a8c/issues/1535683817/?project=1438083

To test:

Add a gallery block
Add an image using "Choose from device"
Tap the "+" button at the bottom of the gallery block
Add an image using the "Take a Photo" option
Newly taken photo should be added to the gallery block
Now add an image block
Try adding an image to it using the "Take a Photo" option
Newly taken photo should be added to the image block

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 · 2020-06-11T18:54:36Z

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 30153. IPA is available here. If you need access to this, you can ask a maintainer to add you.

peril-wordpress-mobile · 2020-06-11T19:10:55Z

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

guarani

I tested this and ran into a crash on step 4 of the test steps. The crash is on this line:

WordPress-iOS/WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaPickerHelper.swift

Line 137 in 5721e14

guard picker == navigationPicker?.mediaPicker else {

and it reads:

Fatal error: Attempted to read an unowned reference but the object was already deallocated

it's referring to the navigationPicker.

This crash only happens when you first pick a photo from the library—which initializes navigationPicker—and then you take a photo, which causes navigationPicker to be accessed.

This wouldn't be an issue except for the fact that navigationPicker is an unowned reference, and is only retained while it is being presented on the screen. As soon as the navigationPicker is dismissed, it's released. Then when you take a photo, navigationPicker is accessed and crashes since it's now deallocated.

I think there's also a (minor) bug in WPMediaPicker because it probably shouldn't be calling its delegate method emptyViewController(forMediaPickerController:) when a photo is taken since an empty state isn't applicable when taking a photo.

guarani · 2020-06-12T04:23:56Z

I'm not sure why this crash isn't closing the app in production, it must be caught somewhere 🤔.

ceyhun · 2020-06-12T16:52:42Z

This crash only happens when you first pick a photo from the library—which initializes navigationPicker—and then you take a photo, which causes navigationPicker to be accessed.

@guarani I couldn't reproduce the crash with debugger attached. Do you mean it happens selecting first from "Choose from device" or "Free Photo Library"? Both worked fine for me.

I think there's also a (minor) bug in WPMediaPicker because it probably shouldn't be calling its delegate method emptyViewController(forMediaPickerController:) when a photo is taken since an empty state isn't applicable when taking a photo.

For me this method is not called when a photo is taken via "Take a Photo":
https://github.com/wordpress-mobile/WordPress-iOS/blob/issue/1965-take-a-photo-failure/WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaPickerHelper.swift#L137

Could it be an iOS or Xcode version difference? I'm using iOS 13.5.1 and Xcode 11.5?

Can you please try again maybe with the build in this comment? It also worked fine for me.

guarani · 2020-06-12T19:11:08Z

Do you mean it happens selecting first from "Choose from device" or "Free Photo Library"? Both worked fine for me.

Hey @ceyhun, it happened to me when selecting "Choose from device".

For me this method is not called when a photo is taken via "Take a Photo":

It's strange this method is being called on my device but not on yours.

Could it be an iOS or Xcode version difference? I'm using iOS 13.5.1 and Xcode 11.5?

I was also on Xcode 11.5, but my device was on iOS 13.4.1.

Can you please try again maybe with the build in this comment? It also worked fine for me.

I should have mentioned I tested build 29996 yesterday. Although I'll try the new 30055 now.

Just tested 30055 and it's crashing (closing) the app for me.
I can also see the crash 100% of the time on the the current App Store version 14.9, as well as the soon to be released 15.0.

It happens when using just the gallery block (first photo from device gallery, second photo captured by camera), or when using two image blocks (again, first from device gallery, second from camera).

I'm going to update my device to the latest iOS and try again.

guarani · 2020-06-12T21:13:46Z

I've updated to 13.5.1 and still see the crash, both with the current App Store version and build 30055. I tried to follow the exact sequence of steps you performed in the gif above but the second image in the gallery never gets added for me (or crashes the app if I'm on build 30055 instead of the App Store version).

If we were to try to implement a fix at the moment, I think changing unowned to weak is a safe move:

WordPress-iOS/WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaPickerHelper.swift

Line 19 in 5721e14

    
           fileprivate unowned var navigationPicker: WPNavigationMediaPickerViewController?

because this will allow navigationPicker to become nil and no longer crash the app when accessed from emptyViewController(forMediaPickerController:). We could take this one step further and leave it as a plain optional (no weak or unowned). (WPNavigationMediaPickerViewController is used in a bunch of places in the app and in none of them is it unowned or weak.)

I know this must be awkward to debug since it's not crashing on your end, but I thought I'd mention this fix since 15.1 is getting cut on Monday.

guarani · 2020-06-12T21:17:35Z

@SergioEstevao – do you know why this is a weak reference (it crashes the gallery block on some devices but not others)?

WordPress-iOS/WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaPickerHelper.swift

Line 19 in 5721e14

    
           fileprivate unowned var navigationPicker: WPNavigationMediaPickerViewController?

SergioEstevao · 2020-06-12T21:24:53Z

@SergioEstevao – do you know why this is a weak reference (it crashes the gallery block on some devices but not others)?

I'm assuming it was to avoid a retain cycle, but is this reference being used after the picker is dismissed? I normally prefer to use a weak references to avoid unexpected crashes when accessing it references after releases.

guarani · 2020-06-12T21:36:57Z

I'm assuming it was to avoid a retain cycle, but is this reference being used after the picker is dismissed?

@SergioEstevao yes, this crashes on some devices because it gets accessed after being deallocated.
The scenario is: when choosing a photo from the gallery, navigationPicker is only retained by the window it's presented on. Then when taking a photo, it's accessed again but is now deallocated.

ceyhun · 2020-06-15T16:50:28Z

If we were to try to implement a fix at the moment, I think changing unowned to weak is a safe move:

@guarani I agree with that. Just made this change. But this still makes me think what could be the reason that make this crash for you and not me 🤔

jkmassel · 2020-06-15T18:21:09Z

Howdy friends – we're freezing 15.1 today, so I'm bumping this PR forward to 15.2. If it needs to land in the 15.1 series, please merge it to release/15.1 and DM me – I'll be happy to create a new beta release!

guarani

@guarani I agree with that. Just made this change. But this still makes me think what could be the reason that make this crash for you and not me 🤔

I'm not sure either to be honest. It's weird that emptyViewController is not called for you AFAIK, but it's called for me. Consider this scenario: when I go to take a photo, a WPMediaPickerViewController instance is instanciated and assigned to cameraPicker (which makes sense), then I take a photo, tap Use Photo on the camera screen, then as the camera modal screen is dismissed I see that viewDidLoad is called on the same cameraPicker object (same memory address). Not sure why that's happening.

Anyhow, I tested with the latest build and it works as expected 👍 .

ceyhun · 2020-06-16T10:26:07Z

@guarani Thanks for testing again 🙇 Good to hear that it's fixed now! The crash you had could be this one: https://sentry.io/organizations/a8c/issues/1535683817/?project=1438083
Since it's not affecting many users (including me :)), I'm going to target the next release for this and merge to develop.

ceyhun added 2 commits June 11, 2020 20:40

Fix 'Take a Photo' option failing after adding image to gallery block

4030c40

Update release notes

ed55db7

ceyhun added [Type] Bug Gutenberg Editing and display of Gutenberg blocks. labels Jun 11, 2020

ceyhun added this to the 15.1 milestone Jun 11, 2020

ceyhun requested a review from guarani June 11, 2020 18:55

guarani requested changes Jun 12, 2020

View reviewed changes

Merge branch 'develop' into issue/1965-take-a-photo-failure

14892a3

ceyhun added 2 commits June 15, 2020 18:47

Make WPNavigationMediaPickerViewController a weak reference

d664c98

Merge branch 'develop' into issue/1965-take-a-photo-failure

f8e596f

jkmassel modified the milestones: 15.1, 15.2 Jun 15, 2020

guarani self-requested a review June 16, 2020 02:50

guarani approved these changes Jun 16, 2020

View reviewed changes

ceyhun merged commit 10cfc87 into develop Jun 16, 2020

ceyhun deleted the issue/1965-take-a-photo-failure branch June 16, 2020 10:28

guarani mentioned this pull request Jun 16, 2020

1.24.0 Release -> Master wordpress-mobile/gutenberg-mobile#2025

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 'Take a Photo' option failing after adding an image to gallery block #14299

Fix 'Take a Photo' option failing after adding an image to gallery block #14299

ceyhun commented Jun 11, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 11, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 11, 2020 •

edited

Loading

guarani left a comment

guarani commented Jun 12, 2020

ceyhun commented Jun 12, 2020

guarani commented Jun 12, 2020 •

edited

Loading

guarani commented Jun 12, 2020

guarani commented Jun 12, 2020

SergioEstevao commented Jun 12, 2020

guarani commented Jun 12, 2020

ceyhun commented Jun 15, 2020

jkmassel commented Jun 15, 2020

guarani left a comment •

edited

Loading

ceyhun commented Jun 16, 2020

Fix 'Take a Photo' option failing after adding an image to gallery block #14299

Fix 'Take a Photo' option failing after adding an image to gallery block #14299

Conversation

ceyhun commented Jun 11, 2020 • edited Loading

peril-wordpress-mobile bot commented Jun 11, 2020 • edited Loading

peril-wordpress-mobile bot commented Jun 11, 2020 • edited Loading

guarani left a comment

Choose a reason for hiding this comment

guarani commented Jun 12, 2020

ceyhun commented Jun 12, 2020

guarani commented Jun 12, 2020 • edited Loading

guarani commented Jun 12, 2020

guarani commented Jun 12, 2020

SergioEstevao commented Jun 12, 2020

guarani commented Jun 12, 2020

ceyhun commented Jun 15, 2020

jkmassel commented Jun 15, 2020

guarani left a comment • edited Loading

Choose a reason for hiding this comment

ceyhun commented Jun 16, 2020

ceyhun commented Jun 11, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 11, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 11, 2020 •

edited

Loading

guarani commented Jun 12, 2020 •

edited

Loading

guarani left a comment •

edited

Loading