Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes an issue that causing our previews to cancel loading. #12303

Merged
merged 3 commits into from Aug 20, 2019

Conversation

@diegoreymendez
Copy link
Contributor

commented Aug 13, 2019

Fixes #10677

The issue seems to be that our UIWebView delegate methods call stopLoading. This would be fine if the delegate methods were called once for the web view, but they can be called multiple times for any resources (like embeds) that the page loads.

Testing requirements / suggestions

@shiki hasn't been able to reproduce this reliably, while I've managed to reproduce it every single time. When testing this I suggest to use the network link conditioner and try to make sure it's set to have a very slow connection.

The reason for this is that the bug relies on the second request being started BEFORE the first one completes. On a very fast connection, the first request could complete really fast, and make it look like everything's fine.

To test:

@rachelmcr - Can I ask you to test this fix on several different site types with and without custom domains?

It should be enough to:

  1. Have a slow connection.
  2. Preview any post with an image, to make the post loading interruption more apparent (as crazy as it sounds, this bug isn't really specific to images though - but they do make it easy to test the bug).

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@diegoreymendez diegoreymendez added this to the 13.1 milestone Aug 13, 2019

@diegoreymendez diegoreymendez requested review from shiki and rachelmcr Aug 13, 2019

@diegoreymendez diegoreymendez self-assigned this Aug 13, 2019

@diegoreymendez

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Fyi, @elibud

@loremattei loremattei modified the milestones: 13.1 ❄️, 13.2 Aug 14, 2019

@loremattei

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

I'm moving this to 13.2 since 13.1 has been cut. If you want this to make it to 13.1, please move it back, target the release/13.1 branch and ping me to build a new beta.

@rachelmcr

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

While logged in to an account with 2FA, whenever I try to view/preview a post I end up on a 2FA screen like this (and I can't get past the 2FA to view the post):

This also happens on develop in a simulator, so it isn't specific to the changes in this PR, but I wanted to highlight it in case it could cause issues later on. (I don't see it in the beta release on my device so maybe it's just a simulator-specific issue? I'm not sure and I'm not set up to test dev branches on device right now — let me know if you'd like me to follow up by testing this on device.)

I'll test various scenarios on an account without 2FA to confirm the fix that way.

@rachelmcr
Copy link
Member

left a comment

Using an account without 2FA, I confirmed that I can reproduce the issue in develop consistently and then confirmed that on this branch I can view/preview posts from the following site types:

  • WordPress.com simple with *.wordpress.com address
  • WordPress.com simple with custom domain
  • WordPress.com atomic
  • Self-hosted with Jetpack
  • Self-hosted without Jetpack

Looks good! My only question/concern is the one noted above about accounts with 2FA.

@diegoreymendez

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I'm not sure and I'm not set up to test dev branches on device right now — let me know if you'd like me to follow up by testing this on device

That's a really good / interesting find!

Unfortunately I can't see this issue on my end, even thought I'm testing on an account with 2FA. It sounds to me like the site is challenging you to re-enter the 2FA code, to validate your identity. My initial hunch is that this is a security challenge that responds to some criteria that isn't great in mobile - but would need to digg deeper to know.

In my modest opinion it's in our best interest that we record this issue separately unless we feel it's better to couple it in this PR for a reason. If this is already in develop it could help us assess its impact as well.

It would make sense to test this in a device, if you can reproduce it easily. I'd believe this is more of a state condition related to the API wanting to re-validate who you are, but again, that's just an initial guess.

@rachelmcr

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

It would make sense to test this in a device, if you can reproduce it easily.

I tested this branch on a device and couldn't reproduce it there, so there must be something odd with the state on my simulator. (I'll keep an eye out for this in the wild and open a separate issue if I can reproduce it on a device.)

I confirmed that with a 2FA account on my device the previews are all working on the same site types I tested with a non-2FA account. From a testing perspective this looks good! 🎉

@shiki
Copy link
Member

left a comment

@diegoreymendez Requesting changes here to emphasize my concern. Please see my inline comment.

@diegoreymendez

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Thanks to @shiki 's observation I managed to spot that, while my assessment was right that loading other resources was causing the web view to cancel loading, my proposed fix wasn't!

Thanks @shiki!

I'm now proposing what I feel is the correct fix for this issue (actually @shiki had proposed this yesterday but I didn't quite understand it at first). This is now ready for review again.

That said, @koke do you happen to remember if there was a reason why we were calling stopLoading in the delegates? (I couldn't spot anything too obvious)

@diegoreymendez diegoreymendez requested review from shiki and rachelmcr Aug 16, 2019

@diegoreymendez

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

I'm also adding you as a reviewer again @rachelmcr to try and validate that the new fix works fine. Apologies for the back and forth!

@shiki
Copy link
Member

left a comment

@diegoreymendez This is looking great. Just one last thing for me. 🙂

@koke

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

That said, @koke do you happen to remember if there was a reason why we were calling stopLoading in the delegates? (I couldn't spot anything too obvious)

I think stopLoading originally was only stopping the loading indicator (5fe6b44), but later on it also started calling stopLoading on the web view (39b3f04)

@diegoreymendez diegoreymendez requested a review from shiki Aug 20, 2019

@diegoreymendez

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

This is ready for review, @shiki.

@shiki
shiki approved these changes Aug 20, 2019
Copy link
Member

left a comment

It works really well for me. Thank you and well done, @diegoreymendez! :shipit:

@diegoreymendez diegoreymendez merged commit dbb7a2b into develop Aug 20, 2019

6 checks passed

Hound No violations found. Woof!
Peril All green. Congrats.
Details
ci/circleci: Build UI Tests Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPad 6th generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone Xs) Your tests passed on CircleCI!
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

@diegoreymendez diegoreymendez deleted the issue/10677-fix-preview-not-loading-images branch Aug 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.