ArrayBufferInputStream doesn't handle buffers with offsets correctly

RESOLVED FIXED in Firefox 23

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

unspecified
mozilla23
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
I haven't sorted out the details yet, but I have an xpcshell test that clearly shows this behaviour.
(Assignee)

Comment 1

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

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
Myk, if you could try this patch that would be great.
(Assignee)

Comment 4

5 years ago
Created attachment 741851 [details] [diff] [review]
Separate the logical concepts of a starting offset and the current offset for ArrayBufferInputStream.
Attachment #741851 - Flags: review?(vladimir)
Attachment #741851 - Flags: review?(vladimir) → review+
(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.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/d8c4ca787e39
Assignee: nobody → josh
Keywords: checkin-needed
Whiteboard: [fixed-in-birch]
Backed out for xpcshell failures.
https://hg.mozilla.org/projects/birch/rev/10c026f436ed

https://tbpl.mozilla.org/php/getParsedLog.php?id=22285295&tree=Birch
Whiteboard: [fixed-in-birch]
(Assignee)

Comment 8

5 years ago
Created attachment 746473 [details] [diff] [review]
Separate the logical concepts of a starting offset and the current offset for ArrayBufferInputStream.

Now with test_tcpsocket.js passing.
(Assignee)

Updated

5 years ago
Attachment #741851 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26ed846519e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d26ed846519e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 13

5 years ago
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.

Updated

5 years ago
Attachment #746473 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/26fa56d0ab7f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d9cccca577a3
(Assignee)

Updated

5 years ago
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → fixed
status-firefox23: --- → fixed
You need to log in before you can comment on or make changes to this bug.