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

Fix 302 ImageLoader caching problem on iOS #7262

Closed
wants to merge 1 commit into from

Conversation

hayeah
Copy link
Contributor

@hayeah hayeah commented Apr 27, 2016

Test demo loads a Tumblr avatar image using a URL that 301 to a CDN. Try to edit and save the file to trigger image reloads.

Problem Description

The 302 image request succeeds the first time:

screen shot 2016-04-27 at 9 37 03 am

But it fails for subsequent loads. You should see:

screen shot 2016-04-27 at 9 37 22 am

- The first image is a 302, only succeeds to load the first time. - The second image in the column adds a nonce as request parameter to render caching ineffective (but still a 302), and the problem doesn't occur. - The last image is the canonical url location (200). # Analysis

Although NSURLSession handles redirect and caching properly, the RCTImageLoader checks the cache directly to see if an image url has cached response. Like so:

See:

NSCachedURLResponse *cachedResponse = [_URLCache cachedResponseForRequest:request];

    NSCachedURLResponse *cachedResponse = [_URLCache cachedResponseForRequest:request];
    if (cachedResponse) {
      processResponse(cachedResponse.response, cachedResponse.data, nil);
      return;
    }

This is a problem because cachedResponseForRequest does not handle redirect. The returned response is a 302, which causes ImageLoading failure later on.

This PR changes this chunk of code to follow the redirect.

@ghost
Copy link

ghost commented Apr 27, 2016

By analyzing the blame information on this pull request, we identified @nicklockwood and @ide to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Apr 27, 2016
if (cachedResponse) {
processResponse(cachedResponse.response, cachedResponse.data, nil);
return;
while(true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while (true) is a big code smell. I'd prefer it if we repeated the NSCachedURLResponse *cachedResponse = [_URLCache cachedResponseForRequest:request]; line instead and use that as the loop condition.

@javache
Copy link
Member

javache commented Apr 27, 2016

Thanks for fixing this! The solution looks good but I'd restructure the loop a bit so it's easier to maintain.

@ghost
Copy link

ghost commented Apr 27, 2016

@hayeah updated the pull request.

@hayeah
Copy link
Contributor Author

hayeah commented Apr 27, 2016

@javache I rewrote the loop per your advice.

In Golang while(true) loop is much more common in the form of:

for {
  cond := ...
  if cond {
    return err;
  }

  if cond2 {
    break
  }
}

In this case there is just one loop condition, so probably better to be explicit in the loop structure. Thanks for the tip!

@@ -371,11 +371,23 @@ - (RCTImageLoaderCancellationBlock)loadImageOrDataWithTag:(NSString *)imageTag
// Check for cached response before reloading
// TODO: move URL cache out of RCTImageLoader into its own module
NSCachedURLResponse *cachedResponse = [_URLCache cachedResponseForRequest:request];
if (cachedResponse) {

while(cachedResponse) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: while (cachedResponse) {

@ghost
Copy link

ghost commented Apr 27, 2016

@hayeah updated the pull request.

if (cachedResponse) {

while (cachedResponse) {
if ([cachedResponse.response isKindOfClass: [NSHTTPURLResponse class]]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last style nits, I promise :)

There shouldn't be a space between the : and the argument, so isKindOfClass:[NSHTTPURLResponse class]

@ghost
Copy link

ghost commented Apr 27, 2016

@hayeah updated the pull request.

@ghost
Copy link

ghost commented Apr 27, 2016

@hayeah updated the pull request.

@hayeah
Copy link
Contributor Author

hayeah commented Apr 27, 2016

@javache If Location is nil, would now call the completion handler with error.

@javache
Copy link
Member

javache commented Apr 27, 2016

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Apr 27, 2016
@ghost
Copy link

ghost commented Apr 27, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 192ab66 Apr 27, 2016
ptmt pushed a commit to ptmt/react-native that referenced this pull request May 9, 2016
Summary:
+ Fixes facebook#5616
+ Bug RNPlay Demo: https://rnplay.org/apps/Eg2goQ

Test demo loads a Tumblr avatar image using a URL that 301 to a CDN. Try to edit and save the file to trigger image reloads.

The 302 image request succeeds the first time:

<img width="318" alt="screen shot 2016-04-27 at 9 37 03 am" src="https://cloud.githubusercontent.com/assets/50120/14860038/b2c04e8a-0c5b-11e6-9edf-78309048368b.png">

But it fails for subsequent loads. You should see:

<img width="307" alt="screen shot 2016-04-27 at 9 37 22 am" src="https://cloud.githubusercontent.com/assets/50120/14860048/b756e170-0c5b-11e6-9031-8f3cca8f2994.png">

+ The first image is a 302, only succeeds to load the first time.
+ The second image in the column adds a nonce as request parameter to render caching ineffective (but still a 302), and the problem doesn't occur.
+ The last image is the canonical url location (200).

Although NSURLSession hand
Closes facebook#7262

Differential Revision: D3231702

Pulled By: javache

fb-gh-sync-id: 364fcf9819188c63310768411d49e6431b2a01d3
fbshipit-source-id: 364fcf9819188c63310768411d49e6431b2a01d3
zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
Summary:
+ Fixes facebook#5616
+ Bug RNPlay Demo: https://rnplay.org/apps/Eg2goQ

Test demo loads a Tumblr avatar image using a URL that 301 to a CDN. Try to edit and save the file to trigger image reloads.

The 302 image request succeeds the first time:

<img width="318" alt="screen shot 2016-04-27 at 9 37 03 am" src="https://cloud.githubusercontent.com/assets/50120/14860038/b2c04e8a-0c5b-11e6-9edf-78309048368b.png">

But it fails for subsequent loads. You should see:

<img width="307" alt="screen shot 2016-04-27 at 9 37 22 am" src="https://cloud.githubusercontent.com/assets/50120/14860048/b756e170-0c5b-11e6-9031-8f3cca8f2994.png">

+ The first image is a 302, only succeeds to load the first time.
+ The second image in the column adds a nonce as request parameter to render caching ineffective (but still a 302), and the problem doesn't occur.
+ The last image is the canonical url location (200).

Although NSURLSession hand
Closes facebook#7262

Differential Revision: D3231702

Pulled By: javache

fb-gh-sync-id: 364fcf9819188c63310768411d49e6431b2a01d3
fbshipit-source-id: 364fcf9819188c63310768411d49e6431b2a01d3
samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
+ Fixes facebook#5616
+ Bug RNPlay Demo: https://rnplay.org/apps/Eg2goQ

Test demo loads a Tumblr avatar image using a URL that 301 to a CDN. Try to edit and save the file to trigger image reloads.

The 302 image request succeeds the first time:

<img width="318" alt="screen shot 2016-04-27 at 9 37 03 am" src="https://cloud.githubusercontent.com/assets/50120/14860038/b2c04e8a-0c5b-11e6-9edf-78309048368b.png">

But it fails for subsequent loads. You should see:

<img width="307" alt="screen shot 2016-04-27 at 9 37 22 am" src="https://cloud.githubusercontent.com/assets/50120/14860048/b756e170-0c5b-11e6-9031-8f3cca8f2994.png">

+ The first image is a 302, only succeeds to load the first time.
+ The second image in the column adds a nonce as request parameter to render caching ineffective (but still a 302), and the problem doesn't occur.
+ The last image is the canonical url location (200).

Although NSURLSession hand
Closes facebook#7262

Differential Revision: D3231702

Pulled By: javache

fb-gh-sync-id: 364fcf9819188c63310768411d49e6431b2a01d3
fbshipit-source-id: 364fcf9819188c63310768411d49e6431b2a01d3
@diogoca
Copy link

diogoca commented Jul 25, 2018

Hi @hayeah, do you know if this PR was merged? Or why was closed?
I'm still getting same error on 55.2v when trying to load external FB profile picture (http return 302).
Thanks.

@hayeah
Copy link
Contributor Author

hayeah commented Jul 28, 2018

@diogoca I believe that this was merged a long time ago.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants