Closed Bug 865652 Opened 7 years ago Closed 7 years ago

ArrayBufferInputStream doesn't handle buffers with offsets correctly

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jdm, Assigned: jdm)

Details

Attachments

(1 file, 1 obsolete file)

I haven't sorted out the details yet, but I have an xpcshell test that clearly shows this behaviour.
Actually all it shows is that sometimes three TCPSocket send calls aren't sent as part of one full packet. Back to square one.
Found the problem. Patch imminent.
Summary: ArrayBufferInputStream and nsStringInputStream break when used in a multiplex stream in the presence of duplicate input → ArrayBufferInputStream doesn't handle buffers with offsets correctly
Myk, if you could try this patch that would be great.
(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.
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/d8c4ca787e39
Assignee: nobody → josh
Keywords: checkin-needed
Whiteboard: [fixed-in-birch]
Attachment #741851 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d26ed846519e
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.
Attachment #746473 - Flags: approval-mozilla-b2g18?
(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.
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 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.
(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.
Attachment #746473 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
You need to log in before you can comment on or make changes to this bug.