[Android] Added progress updates for all XMLHttpRequest upload types

[allengleyzer]

Motivation

Previously, only form-data request bodies emitted upload progress updates. Now,
other request body types will also emit updates.

Addresses issues:
#15724
#11853

This is a bug fix for functionality that's missing on Android. These events are already working correctly on iOS.

Test Plan

Run the following code on Android, and ensure that events are being sent:

const fileUri = 'file:///my_file.dat';
const url = 'http://my_post_url.com/';
const xhr = new XMLHttpRequest();

xhr.upload.onprogress = (event) => {
    console.log('progress: ' + event.loaded + ' / ' + event.total);
}

xhr.onreadystatechange = () => {if (xhr.readyState === 4) console.log('done');} 

console.log('start');

xhr.open('POST', url);

// sending a file (wasn't sending progress)
xhr.setRequestHeader('Content-Type', 'image/jpeg');
xhr.send({ uri: fileUri });

// sending a string
xhr.setRequestHeader('Content-Type', 'text/plain');
xhr.send("some big string"); // sending a string (wasn't sending progress)

// sending form data
xhr.setRequestHeader('Content-Type', 'multipart/form-data');
const formData = new FormData(); formData.append('test', 'data');
xhr.send(formData); // sending form data (was already working)

Release Notes

[ANDROID] [BUGFIX] [XMLHttpRequest] - Added progress updates for all XMLHttpRequest upload types

Previously, only form-data request bodies emitted upload progress updates. Now,
other request body types will also emit updates.

Addresses issues:
#15724
#11853

[janicduplessis]

Nice, test plan and code looks good to me. Also CI is green 👍

[fengyibo]

when to shipit @janicduplessis @satya164

[janicduplessis]

@facebook-github-bot shipit

[facebook-github-bot]

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

[iamtekeste]

@janicduplessis Do you know on what version of RN this would be available?

[fengyibo]

Who knows on what version of RN this would be available?

[RSNara]

There was a surge in crashes with a recent release of one of our apps. We traced the crashes down to this diff and consequently reverted it. Once the next release ships, we'll know for sure if this diff was the cause.

@allengleyzer, The crash reports state that an IllegalStateException was raised. Do you have idea why the changes in this diff could raise it?

[allengleyzer]

@RSNara If I had to guess, it's probably happening while trying to read an uploaded file's size, since this is an IO operation that wasn't happening before. Looking around, it seems that if an InputStream gets closed unexpectedly (e.g. due to a loss in a user's internet connection), this exception might get thrown.

Do you allow users to upload certain forms of data in your app?

I'll investigate some more to confirm if this is the cause.

Thanks for the report, sorry about the issues you've been having.

[fred2028]

Hi @allengleyzer , yes, in Facebook Ads Manager, a user can upload photos and videos as part of their ad creation/edit process. It is definitely a possibility that a user can lose his or her Internet connection during this process.

[allengleyzer]

@RSNara @fred2028 I wasn't able to reproduce the issue. Disconnecting in the middle of an upload didn't raise any exceptions on my device, and I haven't experienced problems with letting uploads go through normally either. Tried in both release and debug builds (though, on a debug build, if I was also connected to the JS debugger, then disconnecting at any point usually resulted in an IllegalStateException, though this problem existed before this PR and is due to the JS debugging client, and so wouldn't happen in a release build).

Any chance you have more information, such as a stack trace or file/line number? Also, which RN version does this happen on, and is it isolated to a particular OS version, or does it happen on all versions? The app I'm working on is still in production, so I haven't been able to test this change on a large scale.

[allengleyzer]

I'm going to guess that if this PR was the cause, then most likely the app crashes for the same reason that #10423 was already happening. This PR might cause the issue to happen more frequently due to a ProgressRequestBody being created for all uploads containing a body, instead of just multipart form-data, as was the case before (and hence the bug this PR fixes).

The fix for the crash bug would probably just be to insert a try-catch handler on the write operation, to prevent the app from crashing, since there's no way to check to see whether the sink is open beforehand.

[LEEY19]

@janicduplessis @allengleyzer guys, are you sure that this commit has fixed the xhr.upload.progress issue? I have copied pasted your codes but issue still persisted.

[allengleyzer]

@LEEY19 These changes aren't in any release yet, and if you're using the latest master, these changes were recently reverted. I'm not sure what you're RN setup looks like, but that might be the issue?

[allengleyzer]

FYI, I figured out a way to reproduce the crash/exception that's in 10423, and was able to confirm that my changes would now cause this same crash to happen more often, since it wouldn't just be form-data bodies that cause it, but all requests that have a request body.

I'm opening a new PR with the changes made here, and an additional commit to fix the crash.

[janicduplessis]

👍 Thanks @allengleyzer for being on top of this.

@fred2028 Could you confirm that the crash reported in #10423 was the one you were seeing?

[LEEY19]

@allengleyzer I just modified the files NetworkingModule.java and NetworkingModuleTest.java by copy-pasting your code. Since I built my RN from source changes made to native code will be reflected.

      xhr.upload.onprogress = (evt) => {
        console.log('progress: ' + evt.loaded + ' / ' + evt.total);
      }

Nothing appeared in console for the above code on Android. It works as expected in iOS

[allengleyzer]

@LEEY19 Sorry about that, the updates don't send if you don't set the Content-Type header on the request. I've updated the test example with setting the Content-Type header.

Another issue I noticed, however, is that the progress updates don't work particularly well for strings/form data, as it usually just sends one progress update at the very end. Unfortunately, this is due in part with the implementation used for sending the updates and/or Okio's implementation for their buffers. There are some possible changes/workarounds, but I think it's out of the scope of this PR. Also, the main use case (IMO) of file updates is working well.

I made some additional changes in the new PR (referenced above), so you should try that one out, and if you have any issues, let's move the discussion to that one instead of here.