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

XHR fails with "Invalid response for blob" on Android #18440

Closed
obsidianart opened this issue Mar 19, 2018 · 79 comments
Closed

XHR fails with "Invalid response for blob" on Android #18440

obsidianart opened this issue Mar 19, 2018 · 79 comments
Assignees
Labels
🌐Networking Related to a networking API. Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@obsidianart
Copy link

React crash on fetch, Android only
Similar to #18223 (but this is only ios)
Similar to #18190 (but my response is not empty and the response code is 200)

Environment

Environment:
OS: macOS High Sierra 10.13.3
Node: 9.6.1
Yarn: 1.5.0
npm: 5.6.0
Watchman: 4.9.0
Xcode: Xcode 9.2 Build version 9C40b
Android Studio: 3.0 AI-171.4443003

Packages: (wanted => installed)
react: 16.2.0 => 16.2.0
react-native: 0.54.0 => 0.54.0

Steps to Reproduce

that's enough to crash on Android (it works all right on ios)
Emulated on google Pixel android 27

fetch(https://crypto-viewer.rebeleo.com/updates/en-US/android)
.then(res => res.json())
.then(data => ({ data }))
.catch(err => {
alert('Something went wrong with the Updatesfeed :( => ' + err)
return { err }
})

Expected Behavior

Expect the type of the response to be seen as json

Actual Behavior

This is a regression issue.
The response is seen as empty, and as blob

screenshot_1521418646

// Is going in this branch
case 'blob':
if (typeof this._response === 'object' && this._response) {
this._cachedResponse = BlobManager.createFromOptions(this._response);
} else {
throw new Error(Invalid response for blob: ${this._response});
}
break;

@react-native-bot react-native-bot added Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. 🌐Networking Related to a networking API. Platform: macOS Building on macOS. labels Mar 19, 2018
@satya164 satya164 self-assigned this Mar 26, 2018
@satya164
Copy link
Contributor

satya164 commented Mar 26, 2018

I ran the following code and cannot repro the issue:

const res = await fetch('https://crypto-viewer.rebeleo.com/updates/en-US/android');
const data = await res.json();

console.log(data);

I get the following from console.log

[{"lang":"en","date":1516298651109,"IOs":true,"android":true,"title":"Release 66","messages":["Finally I have a way to let you know what I'm working on!","Added Kukoin, Cryptopia and Bitso. If you want a specific exchange write to me.","Added gain for exchanges which support history.","Thinking about dropping \"Market\" entirely, if you use it please let me know.","Donations address can be copied on click."]},{"lang":"en","date":1516469344176,"IOs":true,"android":true,"title":"Release 68","messages":["Update available on the store, go grab it!","Since you are there maybe you can help and leave a 5 star review, it would be very kind","-> Added Kraken, many of you have asked for it","-> Added an option to use exchanges prices instead of coinmarketcap prices"]},{"lang":"en","date":1516748562864,"IOs":true,"android":true,"title":"Release 73","messages":["Update available on the store (now or soon), go grab it!","I will not bore you with small bugfix, but there's a lot every time","-> Instant price update (optional)","-> Split options and API","-> More decimals on small numbers"]},{"lang":"en","date":1517243564828,"IOs":true,"android":true,"title":"Release 78","messages":["Update available on the store (now or soon), go grab it!","-> Added Coinbase (Please check what the difference between coinbase and Gdax is, maybe Gdax is better for you)","-> Changed Binance sync to also show amounts used for orders not filled","-> Improved gain calculation for multiple pairs on the same coin holding","-> Evaluate history completion for transactions","-> Added AUD, CHF, ETH and other currencies","In progress:","-> Poloniex history is way harder to get than it seems (you can only ask for a pair per time). I talked with a few other developers and they all have a way to \"guess\" what to ask but no tecnique would work 100%. I need a few changes in the app structure to run so many requests."]},{"lang":"en","date":1518279586343,"IOs":true,"android":true,"title":"Release 91","messages":["Update available on the store (now or soon), go grab it!","-> Created a Telegram channel called \"Crypto Viewer App\", I can't add links in the updates text but I'll add a link later on in the app :)","-> Added DSX and QuadrigaCX","-> Added Ghanaian cedi","-> More coin images. If you miss one, just let me know","-> Big changes on the backend to make it more reliable (multiple servers etc).","-> Binance History. Please let me know if it looks right to you, Binance is quite different from the other exchanges.","-> Way more news sources."]},{"lang":"en","date":1519633296957,"IOs":true,"android":true,"title":"Release 120","messages":["I write here once in a while, for a detailed daily update join on Telegram","-> We are over 20 thousands downloads!","-> Moved all the data to secure storage (it was safe before too but just in case...)","-> Added Yobit and Bleutrade to Wallet","-> Added Gdax History","-> Added few more currencies (ZAR, CAD, etc)","-> Calculate gain on multiple pairs (USDT, BTC, ETH) at the same time","-> Reduce install size from 10MB to 6MB","-> Bugfixes"]}]

I'll be happy to look if you can provide a repo where I can repro the bug.

@obsidianart
Copy link
Author

Sure thing, I will create it soon

@hramos hramos removed the Platform: macOS Building on macOS. label Mar 29, 2018
@elicwhite
Copy link
Member

I ran into this as well. I'm controlling a roku where it appears to respond with a blob url with an empty string response:

screen shot 2018-04-01 at 1 31 43 am

The request went through successfully (and sent the right command to my roku) but is failing on the response (which I don't care about). Unfortunately, since this exception is thrown in a callback there is no way for me to catch it / ignore it.

@satya164
Copy link
Contributor

satya164 commented Apr 1, 2018

Can you please post a sample which reproduces the issue? It's impossible for me to debug and fix it without a repro.

@petarjs
Copy link

petarjs commented Apr 1, 2018

Having the same issue - when the response from the server is an empty string it throws. But I don't think this should happen, as maybe you care just about the headers in that response.

@satya164
Copy link
Contributor

satya164 commented Apr 1, 2018

Posting that "it happens for me" doesn't help. Please post a repro.

@petarjs
Copy link

petarjs commented Apr 1, 2018

I agree 100%, but not sure how to do that, as my use case is posting to a private IPFS node which returns an empty string and hash info in the header. Couldn't find any public writeable IPFS nodes that work, sorry.

@elicwhite
Copy link
Member

@satya164

Here is a repro:

https://github.com/TheSavior/react-native-blob-repro

The relevant lines are the server:

app.post('/', (req, res) => res.end('', 'binary'));

and the react-native app:

fetch('http://localhost:8798', {method: 'POST'});

@elicwhite
Copy link
Member

Any more thoughts on this @satya164? I’m thinking that since the error is due to an invalid response from the server and not due to user error this should just return null instead of throw.

Do you agree?

@obsidianart
Copy link
Author

wait, my issue was with a request with actual content not an empty one, I need to make this repo (I was away with almost no internet)

@elicwhite
Copy link
Member

Your best bet is probably to fork my repro and change the server response.

It is likely the same problem though, some content that the server says is a binary response but the content isn't parseable as a Blob. There is nothing special about the empty string. The problem is that the client throws on invalid server responses but there is no way to handle it.

@obsidianart
Copy link
Author

@TheSavior if you still have it there ready would you mind trying with the url I posted in the bug?

@elicwhite
Copy link
Member

I can't spend the time on that right now, sorry. I also don't have that repo on this computer so it is no better than you forking it and doing it yourself. 👍

@satya164
Copy link
Contributor

satya164 commented Apr 4, 2018

@TheSavior the project you provided uses CRNA which uses an older version of React Native. are you sure you have the same issue with the latest version of React Native?

@elicwhite
Copy link
Member

Crna was just released with the latest release with blob support. Even if it isn’t the actual latest, nothing has changed here with in the blob code and the throws still exists, right?

@peacechen
Copy link

Same error in Android app using RN 0.55.1

@satya164
Copy link
Contributor

Crna was just released with the latest release with blob support.

CRNA is always behind one release. Unless I repro with latest React Native, I won't be able to debug it.

Anyway, I created a new project with react-native init (RN 0.55.2), copied your code and I don't get an error.

Same error in Android app using RN 0.55.1

Can you post your code please?

@peacechen
Copy link

This error occurs in the production build of our app (reported by our crash analysis tool), but NOT the debug build. This seems very odd. I tested with the XMLHttpRequest fix in our production build and it no longer crashes. In this case I can't provide a demo. Experimentally the fix does work as intended.

@hmenzagh
Copy link

hmenzagh commented Apr 23, 2018

@satya164 I'm still getting that error on RN 0.55.3, it is triggered by BugSnag I think .

@satya164
Copy link
Contributor

@hmenzagh I still don't have a repro. I cannot help without a repro.

@peacechen
Copy link

@satya164 I understand that you need a repro, but not everything is easily reproducible. In this case one of the triggers is BugSnag (which we're using), so the repro would be to sign up for BugSnag, integrate their library and build a production release.

I've validated that the XMLHttpRequest fix works. It sounds like you don't trust us, which quite frankly is insulting. We're professionals who have been developing and architecting for decades.

@elicwhite
Copy link
Member

Regardless of a repro, we shouldn’t be throwing there because it isn’t an error caused by the developer and there is no way for it to be handled by the developer. A bad server response will crash the app.

Returning null seems to make the most sense as that is what is done for other issues that are from the server response.

Do you agree or disagree with this approach?

@satya164
Copy link
Contributor

peacechen: It sounds like you don't trust us, which quite frankly is insulting

Take it personally if you will. But as a professional, you should know better than treating causes than fixing the symptoms.

we shouldn’t be throwing there because it isn’t an error caused by the developer

It's likely that the error happens because of some incorrect code on the native side. In iOS, there was some incorrect handling of response which was found because of this error. Suppressing the error doesn't fix the issue and might also hide other bugs in the native code. Also, this error doesn't happen on iOS with the same exact JS code, which means that the bug is most likely in the native code, not in JS.

Anyway, this code path shouldn't be called unless there's a bug in native side, which I want to fix.

Returning null seems to make the most sense as that is what is done for other issues that are from the server response

Don't know what issues you're referring to, but it should behave the same as browser. If server gives a bad response, browser will trigger the error handler and we should do the same instead of ignoring any errors. It's probably a bug in the code than bad server response.

@psegalen
Copy link

@obsidianart sorry to hear that, maybe SSL issues are not the only thing Android doesn't handle well from a server? If you try to use a different server within your app, does it change anything or do you stil have empty blob responses?

@obsidianart
Copy link
Author

@psegalen the response is not empty. can you try with my url (first message of this thread) if you have the same problem?

@psegalen
Copy link

psegalen commented May 15, 2018

@obsidianart I got an answer from your server, here is my attempt to repro I spoked about earlier, I've modified it to fetch data from your server and it seems to be ok on my Android phone: https://github.com/psegalen/rn-android-blob-failure

@tmaly1980
Copy link

tmaly1980 commented May 15, 2018

@psegalen No, the issue with CodePush is that I have my wifi/data turned off (as a part of testing my app's offline mode), so it's not able to connect to the server and gets an empty response. I believe it's asking for a binary file so that might explain the 'blob' part. Has nothing to do with SSL certificates in my case.

Regardless, the code should be able to handle an empty/invalid response without crashing the app. At least with the case of CodePush, the request lifecycle is not something I have control over (ie the fetch code is so deeply buried in 3rd party code), so I can't put everything into a try/catch to ignore the error.

@obsidianart
Copy link
Author

I don't have the issue on 0.55 anymore. Not sure if I should close this (my problem is solved) or keep it open (other people have a related issue)

@peacechen
Copy link

peacechen commented May 16, 2018

We're using CodePush as well and this is a critical bug preventing us from deploying with 0.55. I've lobbied for the JS fix which improves the robustness of blob handling. Until that's merged, use a postinstall script to patch it as so in the package.json:

  "scripts": {
    "postinstall": "rn-patches/apply-patches.sh"
  },

And the apply-patches.sh script:

#!/bin/bash
# Critical patches for RN 0.55

# Fix "Invalid response for blob" with fetch https://github.com/facebook/react-native/issues/18223
curl -O https://github.com/xiamx/react-native/commit/b35071b5daabf370e7789e00e1593788fcf5aecf.patch
patch $PWD/node_modules/react-native/Libraries/Network/XMLHttpRequest.js b35071b5daabf370e7789e00e1593788fcf5aecf.patch
rm -f b35071b5daabf370e7789e00e1593788fcf5aecf.patch

PS: some folks asked about how to resolve this so posted here to help others.

@CapitanRedBeard
Copy link

@obsidianart The code that causes this error to be thrown still exists in the latest here. It's caused by an empty string response being returned which fails that blob check because it is neither an object, nor truthy.
It's possible it's working for you because with 0.55 something changed where you are no longer getting that "" response, but this issue should remain open as it hasn't gone anywhere.

@psegalen I just pulled down your repro and besides a yellow warning box it seems to be fine (no red screen). I assume because we aren't properly receiving the "" blob.

I for sure get this bug with my flavor and configuration of CodePush, however it's difficult to set up an isolated project that causes the bug.

@obsidianart
Copy link
Author

@CapitanRedBeard ahem, no, the empty blob response is #18190 , this ticket was about something else and the response has NEVER been empty for me. can you check the other 2 tickets linked in my original post and see if you think we need this one?
Please notice on line 4 "Similar to #18190 (but my response is not empty and the response code is 200)"

@CapitanRedBeard
Copy link

@obsidianart Yeah my bad I missed that, got derailed with the CodePush empty string conversation.

#18190 is closer to my ( and I think a few others in this thread) issue but it was closed out by @satya164 with the reason being that #18223 should land a fix soon, although it doesn't actually fix the issue.

So this seems like the best place to talk about this issue. The status of this is we need an isolated repro and no one (including me) can seem to figure out how to put one together reliably.

I'm going to keep trying to come up with one as this is pretty critical for my project, but I would love to keep this thread open until we have something concrete to go off of. That or we can reopen #18190 since #18223 doesn't actually solve it. Which would you prefer @satya164.

@NobodyButMe-Haiya
Copy link

NobodyButMe-Haiya commented May 17, 2018

The bug on latest android 0.55.4 .. @CapitanRedBeard . For me , non server connection is a test also. Empty respond also need to check but since the red page appear first.. It is bug.Long time ago i complain, can we get real status XHR like 400,404,200.

@hramos
Copy link
Contributor

hramos commented May 24, 2018

The fix for #18223 (iOS) landed in f5207ba and as far as I can tell, it is not in 0.55.4. You can tell because the commit is not tagged as such. If this were in 0.55.4, you would see a v0.55.4 tag next to master in the lower left corner:

screen shot 2018-05-24 at 10 29 50 am

For anyone who claims this is still an issue on Android, can you make sure you are building React Native from source? That's the best way to ensure you are actually using a release with Janic's fix.

@NobodyButMe-Haiya
Copy link

@hramos build from source ? i only knew to use normal yarn/npm only. I see this patch 4edc83e ..And it work as intended. Any hope anybody merge as updated ?

@hramos
Copy link
Contributor

hramos commented Jun 12, 2018

@idcmsbeta the fix for #18223 is in the 0.56.0-rc release.

@wadim
Copy link

wadim commented Jun 21, 2018

@hramos It seems the fix has actually been included in 0.55.4 via this commit 093a78d99d0

@zibs
Copy link

zibs commented Jun 21, 2018

@wadim that fixes it on iOS - but the issue remains on 0.55.4 on Android, which I'm currently running.

@stueynet
Copy link

This appears to be the last remaining open issue on this bug as it relates to Android. Can we ressurect it?

@obsidianart
Copy link
Author

I'm currently getting this issue when the connection drops but, again, it's hard to create an example :(

@stueynet
Copy link

stueynet commented Aug 3, 2018

Guess I’ll bump this again. This issue is still on all versions for Android. iOS fixed but Android not so much.

@shashank19909
Copy link

@stueynet did you try 0.56.0 on android? Is it still happening in 0.56.0 version?

@anaerobic
Copy link

anaerobic commented Aug 9, 2018

@shashank19909 with 0.56.0 on android I am no longer getting "Invalid response on blob", however I am getting a "Network request failed" red screen of death when hitting an intentionally offline API endpoint. I would expect to be able to catch that error in my code.

screenshot-2018-08-08_22 04 33 732

@hramos
Copy link
Contributor

hramos commented Sep 11, 2018

Is anybody interested in supplying a fix? While it's useful to add any information here that can help people troubleshoot the source of the problem, let's keep in mind that this is a issue tracker and not a support forum. The best way to resolve this is to send a pull request with a fix, and work with other collaborators to get it merged.

@stale
Copy link

stale bot commented Dec 10, 2018

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "For Discussion" or "Good first issue" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 10, 2018
@stale
Copy link

stale bot commented Dec 17, 2018

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.

@stale stale bot closed this as completed Dec 17, 2018
@facebook facebook locked as resolved and limited conversation to collaborators Dec 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🌐Networking Related to a networking API. Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

No branches or pull requests