Fix crash due to mishandling of UTF-8 in progressive download.

[dryganets]

Fixes:

Fatal Exception: java.lang.RuntimeException: Failed to create String from JSON
       at com.facebook.react.bridge.queue.NativeRunnable.run(NativeRunnable.java)
       at android.os.Handler.handleCallback(Handler.java:739)
       at android.os.Handler.dispatchMessage(Handler.java:95)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:31)
       at android.os.Looper.loop(Looper.java:234)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$3.run(MessageQueueThreadImpl.java:193)
       at java.lang.Thread.run(Thread.java:818)

JavaScriptCore is very strict about invalid UTF symbols.
So if you pass an invalid UTF-8 string to it the string will be decoded as an empty string.

The current implementation of progressive downloading for Android blindly cuts the response in 8KB chunks.
That could cause a problem in case the last symbol in the chunk is multi-byte.

To prevent it I added a class which determines if this is the case and cut the string in the appropriate place.
A remainder is prepended to the next chunk of data.

This should fix the root cause of this issue:
#10756

Test plan:
fetch for this http://www.00ksw.org/html/1/1391/614905.html
shouldn't crash android app anymore.

Release Notes:

[dryganets]

continuous-integration/travis-ci/pr seems to be false-negative.
At least it looks like a bug in the CI from the logs.
I didn't change any c++/ObjC or javascript code to fail xcode tests.

[javache]

I believe we've had a similar issue elsewhere in the Android codebase. It should be possible to merge/reuse your solution with that.

[dryganets]

@javache, could you point me to another place?

I grep the code locally and it looks like NetworkModule is the only place where we split user data to chunks.

I was looking for the String creation with constructors:
https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#String(byte[])
https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#String(byte[],%20java.nio.charset.Charset)

etc.

[javache]

Hmm, you're right. I think we just avoided crashing in the C++ layer when receiving invalid input.

An equivalent fix to this was made on iOS a while ago: 3ac3749

[dryganets]

Tell me if I could do something to push it forward.
It seems to be a pretty frequent crash.
At least at Skype, it was one of the top crashes.

To make this fool proof I changed fromJson method to use serialize dynamic with skipInvalidUnicode + have a folly slightly patched.
But I think that should be part of the separate commit.

[javache]

I'll try to have a look at the code, but in general I'm not comfortable with using our own utf8/utf16 decoding solution. I much prefer the iOS solution where we use the system decoding and then try to fall back / trim the end of the string in case it ends in an invalid character.

[dryganets]

@javache I don't have a custom decoding, just a validation, and alignment to the last symbol

But I got what you mean. The validation could be done in O(1) by analyzing the last 4 symbols.
Didn't notice that initially.

But anyway there is a https://docs.oracle.com/javase/7/docs/api/java/nio/charset/CharsetDecoder.html
API which allows doing what is needed with a support of multiple encodings.

I'll update the algorithm to support other encodings as well.

Thank you for feedback.

[dryganets]

@javache.
I updated PR.
Added tests for UTF-16 and UTF-32.

It works as all these encodings have up to 4 bytes per symbol.
This code is not going to work for UTF8 modified for example, but I don't think it's used somewhere in wild except JNI and Java serialization.

[dryganets]

Is there anything I could do to help to move it to master?
The patch fixes the root cause of one of the most frequent react-native crashes.

The code I wrote is covered by UnitTests and supports multiple encodings.
It works the same way as iOS implementation does.

[facebook-github-bot]

@dryganets I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[dryganets]

Rebased.

[dryganets]

@hramos, did you find someone to take a look at this PR?

It's quite a bad crash.

[pull-bot]

	Warnings
⚠️	📋 Release Notes - This PR may have incorrectly formatted Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

[facebook-github-bot]

@dryganets I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[pull-bot]

	Warnings
⚠️	📋 Release Notes - This PR may have incorrectly formatted Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

[facebook-github-bot]

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.