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

Network Plugin can't parse UTF8 characters and can't decode images properly #1466

Closed
skychx opened this issue Aug 14, 2020 · 8 comments
Closed
Assignees

Comments

@skychx
Copy link

skychx commented Aug 14, 2020

🐛 Bug Report

Network Plugin can't parse UTF8 characters and can't decode images properly.

To Reproduce

1.Test Link:

https://ex.ke.com/sdk/recommend/html/100001314?hdicCityId=110000&paramMap[source]=&id=100001314&mediumId=100000037&elementId=&resblockId=1111027381003&templateConfig=%5Bobject%20Object%5D&fbExpoId=346620976471638017&fbQueryId=&required400=true&unique=1111027381003&parentSceneId=

2.Test Code(React Native):

fetchData = () => {
  fetch('https://ex.ke.com/sdk/recommend/html/100001314?hdicCityId=110000&paramMap[source]=&id=100001314&mediumId=100000037&elementId=&resblockId=1111027381003&templateConfig=%5Bobject%20Object%5D&fbExpoId=346620976471638017&fbQueryId=&required400=true&unique=1111027381003&parentSceneId=')
}

render() {
  return (
    <TouchableOpacity onPress={this.fetchData}>
      <View></View>
    </TouchableOpacity>
  )
}

3.Flipper Network Plugin parse result(Error)

not

4. Browser parse result(correctly)

yes

5.image displays

Version 0.51 decodes and displays images correctly, version 0.52 shows (empty).Sometimes images are decoded into String in the absence of content-type

0.51 ⬇️

0 51 0_Image

0.52 ⬇️

0 52 0_Image

0.52 image decode into string ⬇️

0 52 0_ImageP

Environment

iPhone11 iOS 13.3
macOS Mojave 10.14.5
Flipper 0.52.1
react-native 0.62.2
npm 6.14.1
node 13.12.0

@skychx
Copy link
Author

skychx commented Aug 14, 2020

I thought this PR was causing the problem, but after studying the Network Plagin source code, I think this.props.persistedState.responses has a parsing problem.

skychx referenced this issue Aug 14, 2020
Summary:
This was originally done in #377 to support chinese characters, but it's not clear how it works, or what problem specifically it is trying to solve.

It's causing some problems where valid responses are failing to be "decoded", so I'm removing it.

Reviewed By: mweststrate

Differential Revision: D22866879

fbshipit-source-id: a19a57b0ddba2b9f434eb3c37e6b92da1dfbef01
@mweststrate
Copy link
Contributor

mweststrate commented Aug 28, 2020

Image display will be fixed in next weeks release, that is a separate issue.

Looked a bit further in the charset issue, it seems to occur on iOS only but is correct on Android (screenshot). So I suspect it is an issue on the client, not in Flipper desktop.

Screenshot 2020-08-28 at 16 07 24

Edit: difference is that Android sends the gzipped response, but iOS unpacked. Will investigate further

@mweststrate mweststrate self-assigned this Aug 28, 2020
@yaoandy107
Copy link

yaoandy107 commented Sep 21, 2020

@mweststrate Look forward to your investigation of charset issue, but I think it is not an OS issue because it also happens on my Android devices.

Flipper version: 0.57.0

@mweststrate
Copy link
Contributor

mweststrate commented Sep 21, 2020 via email

@yaoandy107
Copy link

yaoandy107 commented Sep 22, 2020

I have written a minimal backend with Python Flask to retrieve the same issue in my Android device.

Test API code: https://gist.github.com/yaoandy107/1dc0daf2071aed0448e39100145b1ec3

Flipper ver: 0.57.0
Android ver: 11 & 9
OkHttp: 4.9.0

截圖 2020-09-22 下午12 29 23

@yaoandy107
Copy link

I have cloned the repo and give it a try to fix the issue, and it works well in my daily work, but not sure if it work on all situation. I will just PR it later for review.

@yaoandy107
Copy link

@mweststrate I found out the original decoding issue only happen in Android on response without gzip, so I add the new decode function to handle the request that does not enable gzip in the above PR, and I have tested on my Android devices, it looks fine, but not tested on iOS.

facebook-github-bot pushed a commit that referenced this issue Oct 14, 2020
Summary:
Changelog: [Network] Non-binary request are not properly utf-8 decoded on both iOS and Android, both when gzipped and when not gzipped

This diff fixes a long standing / ping-pong issue regarding network decoding differences between
* iOS vs Android
* binary vs utf-8
* gzipped vs uncompressed

The changes aren't too big, but the underlying investigating is :)

The primary contribution to this diff is:

First, adding test cases for know problematic cases. This is done by grabbing the messages that are send from the flipper client to flipper using the flipper messages plugin. This is the base64 data that is stored in the `.txt` files. Beyond that, for all tests public endpoints are used, so that we can both get a hold of the raw original files, and how we expect them to be displayed in flipper.

For testing a simple RN app was build, with a button that fires a bunch requests. The first 3 are captured in unit tests, the last one is not idempotent, but a case reported in #1466, so just left it there as manual verification.

```
const fetchData = async () => {
  await fetch(
    'https://raw.githubusercontent.com/SangKa/MobX-Docs-CN/master/docs/donating.md',
    {
      headers: {
        'Accept-Encoding': 'identity', // signals that we don't want gzip
      },
    },
  );
  await fetch('https://reactnative.dev/img/tiny_logo.png?x=' + Math.random());
  await fetch(
    'https://raw.githubusercontent.com/SangKa/MobX-Docs-CN/master/docs/donating.md',
  );
  await fetch(
    'https://ex.ke.com/sdk/recommend/html/100001314?hdicCityId=110000&paramMap[source]=&id=100001314&mediumId=100000037&elementId=&resblockId=1111027381003&templateConfig=%5Bobject%20Object%5D&fbExpoId=346620976471638017&fbQueryId=&required400=true&unique=1111027381003&parentSceneId=',
  );
};
```

The second contribution of this diff is that it doesn't use weird URLencoder hacks to convert base64 to utf8, but rather a proper library. The problem with our original solution, using `atob` is that it converts to ASCII, not to utf-8, which is the source of the original bugs. See for more background on this: https://www.npmjs.com/package/js-base64#decode-vs-atob-and-encode-vs-btoa-

Solves:
#1466
#1541
#1458

Supersedes D23837750

Future work: we don't inspect the `content-type=xxx;charset` header yet, which we should do for less common encodings, to make sure that they get displayed correctly as well

Future work: in feature like copy data and curl, we always call decode body, without check if we are actually dealing with non-binary data. Probably it is better to keep binary data in base64, rather than decoding it, as that will assume the data is an utf-8 string, which might fail.

An assumption in these changes is that binary data is never gzipped, which is generally correct; gzip is not applied by webserver to things like images, as it would increase, not decrease their size, and waste a lot of computation power.

Reviewed By: cekkaewnumchai

Differential Revision: D23403095

fbshipit-source-id: 5099cc4a7503f0f63bd10585dc6590ba893f3dde
@mweststrate
Copy link
Contributor

Should be fixed by 6b7b1fa and part of next weeks release. If it isn't solved in Flipper0.64, feel free to open a fresh issue with an example URL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants