SDK incorrectly sending entire buffer to XHR

[konklone]

So in nathanpeck/s3-upload-stream#20, I found that the buffer I was sending to aws-sdk was not the buffer that the browser was actually using to form a multipart upload part. s3-upload-stream was slicing a buffer, and passing a buffer "view" onto aws-sdk, but the browser was actually trying to upload the entire parent buffer.

I took this over to feross/buffer#46 (comment) and got some additional background -- namely that using the .buffer property is deprecated.

I'm not sure what browser support is like for this feature, but I do know that right now, the AWS SDK defaults to doing the older, deprecated thing:

if (httpRequest.body && typeof httpRequest.body.buffer === 'object') {
  xhr.send(httpRequest.body.buffer); // typed arrays sent as ArrayBuffer
} else {
  xhr.send(httpRequest.body);
}

I can confirm that changing xhr.send(httpRequest.body.buffer) to xhr.send(httpRequest.body); addresses the problem.

In the meantime, @nathanpeck has patched s3-upload-stream to copy the buffer instead of slicing it before handing it into aws-sdk, but this is less performant. It'd be better to be able to pass a buffer view into aws-sdk, so that patch can be removed.

[lsegal]

This was added for browser support. I forget which browser this was causing problems for, but looking at the commit history around that time (commit 5714c8d with log) it looks like there was compat work being done for Firefox. I just did some preliminary testing in the latest Firefox and it seems to work now, but I'd have to run this against the other browsers (as well as older Firefox versions to support any stragglers). It looks like IE10 is okay with this, but I want to test Android as well, as well as older Safari versions (I just upgraded to Yosemite so I want to try on an older Safari-- we technically try to support back to 5.1).

Browser compat is definitely something we have to worry about with these changes, so it might be that we just change the strategy for detecting non-compliant browsers. Maybe we try to send() first, catch errors, and fallback? That would future-proof us and shift the performance penalty off to the older browsers, which I'm okay with.

[konklone]

Maybe we try to send() first, catch errors, and fallback? That would future-proof us and shift the performance penalty off to the older browsers, which I'm okay with.

That approach sounds right to me, of shifting the penalty to older browsers. CCing @feross in case he knows a more graceful way of feature detecting this than catching errors, but yeah.

I don't even know how to install older versions of Firefox to test this on my Ubuntu machine. I just tried installing Firefox 11 from Mozilla's tarballs and it won't even boot. If you have any VMs you can point to, I'd be happy to backup-test the solution.

[nathanpeck]

This chart should help in figuring out which browsers have support for sending the ArrayBuffer:

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#Browser_Compatibility

I believe that what we are trying to do is send an ArrayBufferView. Right now it is basically defaulting to always sending the entire buffer, because the buffer property exists, so the ArrayBuffer isn't being sent as is even though the browser supports that functionality.

Unfortunately I still see a lot of question marks on there, so I suspect that this still isn't supported very well, however, there must be a better way to detect whether or not the browser supports sending ArrayBuffer and ArrayBufferView, I'm just not sure what that is.

[feross]

I would just do:

try {
  xhr.send(uint8array);
} catch (err) {
  xhr.send(uint8array.buffer);
}

[lsegal]

@feross the only problem is that there are other reasons .send() can throw an error, a lot of which might be legitimate (security exceptions). I don't know that we'd want to immediately resend which may raise a separate error and [potentially?] lose the original.

That is the general approach we should take, though.

[feross]

Another option would be to just ignore Chrome 9-21 and Firefox 9-19 and always send the TypedArrayView. Current Chrome is 38 and Firefox is 33. These unsupported browsers are many years old at this point. It's a small problem and it'll only get smaller because of autoupdating.

[nathanpeck]

According to the table from Mozilla that I linked above there seems to be some question about whether or not Safari and IE support sending the uint8array, so that is something that has to be considered as well.

[lsegal]

I'm actually most worried about Safari. IE10 works with ArrayViews. I'm going to try to find a few old Safari installs and test those. You're right, if it's just Firefox and Chrome, that makes the problem smaller. That said, it would technically be a breaking change in the SDK to change our list of minimum supported versions. Some users, for whatever reason, may be relying on a specific browser version-- and I don't want to break anyone with a minor version update. Ideally I'd like to avoid breaking anyone, period. :)

[lsegal]

Thanks for following up on this everyone-- we will have a fix going out with our next release.

[nathanpeck]

Awesome! Thanks a lot!

[konklone]

Thank you very much, @lsegal!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.