865652 - ArrayBufferInputStream doesn't handle buffers with offsets correctly

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Josh Matthews [:jdm]]

I haven't sorted out the details yet, but I have an xpcshell test that clearly shows this behaviour.

Comment 1

[Josh Matthews [:jdm]]

Actually all it shows is that sometimes three TCPSocket send calls aren't sent as part of one full packet. Back to square one.

Comment 2

[Josh Matthews [:jdm]]

Found the problem. Patch imminent.

Comment 3

[Josh Matthews [:jdm]]

Myk, if you could try this patch that would be great.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Separate the logical concepts of a starting offset and the current offset for ArrayBufferInputStream.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to Josh Matthews [:jdm] from comment #3)
> Myk, if you could try this patch that would be great.

I just built with it, and it works!  As in: repeatedly passing TCPSocket.send() the same buffer with different offsets successfully transfers a large dataset in chunks.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/d8c4ca787e39

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/projects/birch/rev/10c026f436ed

https://tbpl.mozilla.org/php/getParsedLog.php?id=22285295&tree=Birch

Comment 8

[Josh Matthews [:jdm]]

Attachment: Separate the logical concepts of a starting offset and the current offset for ArrayBufferInputStream.

Now with test_tcpsocket.js passing.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d26ed846519e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d26ed846519e

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 746473 [details] [diff] [review]
Separate the logical concepts of a starting offset and the current offset for ArrayBufferInputStream.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 831107
User impact if declined: TCPSocket is broken for sending data from slices of arraybuffers.
Testing completed: m-c, firefox simulator
Risk to taking this patch (and alternatives if risky): Quite small. It's an API with few consumers, and only some of them use the broken functionality. 
String or UUID changes made by this patch: None.

Comment 12

[Alex Keybl [:akeybl]]

(In reply to Josh Matthews [:jdm] from comment #11)
> User impact if declined: TCPSocket is broken for sending data from slices of
> arraybuffers.

Do we think this will mitigate known issues in v1.0.1 or v1.1? The possibility of breakage (as opposed to known impact) may not warrant taking any risk.

Comment 13

[Josh Matthews [:jdm]]

To my knowledge, the only B2G consumer is the email app, and I am not aware of any use of slices inside of it. I don't know what version of Gecko the Firefox simulator uses; Myk, can you answer that?

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

The e-mail app does use slicing.  Currently we use a 0-offset for the slicing which is probably why we are not affected by the bug at this time.  However, there is the potential that we will need to implement streaming (and do so in a way that curtails garbage generation) to address our memory usage explosion when sending large files.  I could see a request for uplifting that to v1.1 happening, but our unit tests would likely not get uplifted because of various factors, so we wouldn't notice the breakage from not having this patch and manual QA testing would be unlikely to pick up on the problem either.

As such, unless v1.1 is hard-frozen already, we should just take this patch on mozilla-b2g18 so we don't forget about it later.  Especially since this is a self-contained patch with unit tests and there's no semantic leakage.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to Josh Matthews [:jdm] from comment #13)
> To my knowledge, the only B2G consumer is the email app, and I am not aware
> of any use of slices inside of it. I don't know what version of Gecko the
> Firefox simulator uses; Myk, can you answer that?

The only part of the Simulator that would use slices is the part that is an addon to Firefox, which users then use in whatever version of the browser they're running, i.e. the current Nightlies through release builds.  And the B2G Remote Control addon does the same.  So those addons don't need this to be on B2G branches.

Comment 16

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/26fa56d0ab7f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d9cccca577a3