831107 - Fix up mozTCPSocket's ArrayBuffer support

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Bug 733573 introduced mozTCPSocket, which is quite handy, even behind a pref.  However, its ArrayBuffer support has a numeber of problems:

1) It claims ArrayBuffer support, but send() really wants a Uint8Array.  The data read handler is passed a Uint8Array.  It should be fixed so that sending works with either raw ArrayBuffers or any of the typed array types, and that receiving gives an ArrayBuffer that the user can then wrap in whatever the appropriate type is for the API.

2) send() with the Uint8Arrays is horrible -- it converts the uint8array to a string, character by character, and then sends that!  We should fix that, as the comment at http://mxr.mozilla.org/mozilla-central/source/dom/network/src/TCPSocket.js#450 mentions

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

3) nsIBinaryInputStream needs an extension -- readArrayBuffer().  Right now onDataAvailable creates a Uint8Array, calls nsIBIS.readByteArray (which returns a JS array) and then calls set() with that array.  That causes a slow element-by-element copy

Comment 2

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

Perf traces showed we are wasting a lot of time in initial e-mail sync because of this bug.  We would very much like to fix this and uplift it to v1.0.1.  Requesting tef+.

Comment 3

[Faramarz Rashed]

major email improvements are gained by this and our partners have been screaming for email performance gains.  adding this to the list of TEF+.  please prioritize this and get it fixed in the next couple of days if possible.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Add general ArrayBuffer support to TCPSocket.

Comment 6

[Dietrich Ayala (:dietrich)]

Does this improve email startup perf? Or post-startup perf while syncing? Or both?

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 738546 [details] [diff] [review]
Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Benjamin

Comment 8

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

(In reply to Dietrich Ayala (:dietrich) from comment #6)
> Does this improve email startup perf? Or post-startup perf while syncing? Or
> both?

Post-startup perf, unless you include online sync as part of startup.

Main-thread CPU utilization while synchronizing IMAP folders will be improved.  Since it's main thread, any wasted time affects our UI responsiveness.

This is most critical during initial sync when you first add an account where we were becoming unresponsive for ~1 second.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Comment 10

[Josh Matthews [:jdm]]

Attachment: Part 2: Add general ArrayBuffer support to TCPSocket.

Benjamin, can you review the new input stream that reads from an ArrayBuffer?

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 738830 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

Donovan, are you good to review the TCPSocket changes?

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 738830 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

Review of attachment 738830 [details] [diff] [review]:
-----------------------------------------------------------------

There is one unfortunate fallout of this change -- it's not possible to slice an ArrayBuffer (e.g. create a second ArrayBuffer that refers to a subregion of the original one).  ArrayBuffer.slice() copies, so that's not useful.  This can create a problem when trying to send, for example, a Uint8Array that refers to a portion of an arrayBuffer.  e.g.:

var buf = new ArrayBuffer(100);
var portion = new Uint8Array(buf, 10, 25);  // refers to the 25 bytes starting at index 10

At this point, portion.buffer === buf, which is the entire 100-byte buffer; there is no way to just pass the bytes covered by "portion" as an ArrayBuffer.

A solution is to extend the interface to allow sending an explicit offset and length, e.g.:

  socket.send(portion.buffer, portion.byteOffset, portion.byteLength);

::: dom/network/src/TCPSocket.js
@@ -94,5 @@
> -  // is no need for a wrapper to exist around the typed array from the
> -  // perspective of content.  (The wrapper is bad because it only lets content
> -  // see length, and forbids access to the array indices unless we excplicitly
> -  // list them all.)  We will then access the array through a wrapper, but
> -  // since we are chrome-privileged, this does not pose a problem.

Should we just get rid of useWin if this is no longer a problem?

Comment 13

[Josh Matthews [:jdm]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #12)
> ::: dom/network/src/TCPSocket.js
> @@ -94,5 @@
> > -  // is no need for a wrapper to exist around the typed array from the
> > -  // perspective of content.  (The wrapper is bad because it only lets content
> > -  // see length, and forbids access to the array indices unless we excplicitly
> > -  // list them all.)  We will then access the array through a wrapper, but
> > -  // since we are chrome-privileged, this does not pose a problem.
> 
> Should we just get rid of useWin if this is no longer a problem?

I wanted to, but it's still used for IPC.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Tiny update to use new when constructing the typed array.

Comment 15

[Josh Matthews [:jdm]]

Attachment: Part 2 for mozilla-b2g18

Comment 16

[Benjamin Smedberg]

Comment on attachment 738830 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

I am not a good reviewer for this code, please talk to somebody from networking.

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 738999 [details] [diff] [review]
Part 2 for mozilla-b2g18

I lied, this doesn't build :/

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 738993 [details] [diff] [review]
Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Review of attachment 738993 [details] [diff] [review]:
-----------------------------------------------------------------

This looks totally fine, but please upload a version of the patch with the RootedObject change; too important for a local fixup.

::: xpcom/io/nsBinaryStream.cpp
@@ +721,5 @@
>  NS_IMETHODIMP
> +nsBinaryInputStream::ReadArrayBuffer(uint32_t aLength, JSContext* cx, JS::Value* _rval)
> +{
> +    JSAutoRequest ar(cx);
> +    JSObject* buffer = JS_NewArrayBuffer(cx, aLength);

Use RootedObject here, otherwise you're relying on the newly-constructed-object root to save you until you assign to _rval (which I assume is a gc root itself).  Read() might end up calling down in to JS for a JS-implemented stream though, and badness will result.

Comment 19

[Benjamin Smedberg]

Comment on attachment 738830 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

In fact, unless there is a particular reason why ArrayBufferInputStream needs to live in XPCOM, I'd like that to live under netwerk as well.

Comment 20

[Josh Matthews [:jdm]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #18)
> Comment on attachment 738993 [details] [diff] [review]
> Part 1: Allow TCPSocket consumers to receive typed array data without
> converting it to a byte string first.
> 
> Review of attachment 738993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks totally fine, but please upload a version of the patch with the
> RootedObject change; too important for a local fixup.
> 
> ::: xpcom/io/nsBinaryStream.cpp
> @@ +721,5 @@
> >  NS_IMETHODIMP
> > +nsBinaryInputStream::ReadArrayBuffer(uint32_t aLength, JSContext* cx, JS::Value* _rval)
> > +{
> > +    JSAutoRequest ar(cx);
> > +    JSObject* buffer = JS_NewArrayBuffer(cx, aLength);
> 
> Use RootedObject here, otherwise you're relying on the
> newly-constructed-object root to save you until you assign to _rval (which I
> assume is a gc root itself).  Read() might end up calling down in to JS for
> a JS-implemented stream though, and badness will result.

The conservative stack scanner is still in effect, so I don't think this is as unsafe as you think it is. Regardless, I'll make the change.

Comment 21

[Donovan Preston [:fzzzy]]

Part 2 of this patch isn't applying cleanly for me, can you double check it? Thanks.

Comment 22

[Josh Matthews [:jdm]]

Attachment: Part 2: Add general ArrayBuffer support to TCPSocket.

mozilla-b2g18 patch.

Comment 23

[Josh Matthews [:jdm]]

Attachment: Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

mozilla-b2g18 patch.

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 739119 [details] [diff] [review]
Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Looks good

Comment 25

[Josh Matthews [:jdm]]

The API semantics I went with is that the new byteOffset and byteLength arguments are optional (and unused if the binary type is not arraybuffer), with byteLength defaulting to the length of the array buffer if it is 0.

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 739118 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

Review of attachment 739118 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good as well.  Thanks for jumping on this!

::: dom/network/src/TCPSocketChild.cpp
@@ +196,5 @@
>      if (!data) {
>        return NS_ERROR_OUT_OF_MEMORY;
>      }
>      FallibleTArray<uint8_t> fallibleArr;
> +    if (!fallibleArr.InsertElementsAt(0, data + aByteOffset, nbytes)) {

Damn, too bad we have to copy the data here -- would be nice if we could just SendData<uint8_t[]>(data + aByteOffset, nbytes)

Comment 27

[Donovan Preston [:fzzzy]]

Comment on attachment 739118 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

Review of attachment 739118 [details] [diff] [review]:
-----------------------------------------------------------------

Looks correct to me. Tests pass. Thanks for taking this!

Comment 28

[Josh Matthews [:jdm]]

https://hg.mozilla.org/projects/birch/rev/3494269ba9fb
https://hg.mozilla.org/projects/birch/rev/6a1f26c76aec

Comment 29

[Josh Matthews [:jdm]]

Attachment: Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Patch for mozilla-b2g18.

Comment 30

[Josh Matthews [:jdm]]

Attachment: Part 2: Add general ArrayBuffer support to TCPSocket.

Patch for mozilla-b2g18.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a1f26c76aec
https://hg.mozilla.org/mozilla-central/rev/3494269ba9fb

Comment 32

[Josh Matthews [:jdm]]

remote:   https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/12dcfbb09305
remote:   https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/93c002935f00

remote:   https://hg.mozilla.org/releases/mozilla-b2g18/rev/baafe3423590
remote:   https://hg.mozilla.org/releases/mozilla-b2g18/rev/b72ffc9ec174

Comment 33

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c3ee6b445cf5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/82fdff82a2d0

Build fix.

Comment 34

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

This broke the Firefox OS Simulator <https://github.com/mozilla/r2d2b2g/issues/459> and B2GRemote, which both use nsIDOMTCPSocket to communicate with FxOS devices using ADB.

Comment 35

[Josh Matthews [:jdm]]

Sorry, I was assured that the email app was the only client. Luckily the changes should be particularly invasive; where you previously passed Uint8Array values, you can now pass ArrayBuffer instead.

Comment 36

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

Apologies too for giving those assurances.  My mental checklist didn't include chrome-space code that I assumed would be using something more like socket.jsm.

Comment 37

[:gerard-majax]

Would it also explain total breakage of the gaia email client like this?

E/GeckoConsole(  601): [JavaScript Error: "[Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (DataCloneError)"  location: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js Line: 2536"]" {file: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js" line: 2536}]
E/GeckoConsole(  601): [JavaScript Error: "[Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (DataCloneError)"  location: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js Line: 2536"]" {file: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js" line: 2536}]
E/GeckoConsole(  601): [JavaScript Error: "[Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (DataCloneError)"  location: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js Line: 2536"]" {file: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js" line: 2536}]
E/GeckoConsole(  601): [JavaScript Error: "[Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (DataCloneError)"  location: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js Line: 2536"]" {file: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js" line: 2536}]

Comment 38

[Donovan Preston [:fzzzy]]

(In reply to Alexandre LISSY :gerard-majax from comment #37)
> Would it also explain total breakage of the gaia email client like this?

Make sure you have a version of gaia that has the updated email client. I saw the commits go through to update to the interface provided by this bug, so your gaia is probably out of date.

Comment 39

[:gerard-majax]

(In reply to Donovan Preston from comment #38)
> (In reply to Alexandre LISSY :gerard-majax from comment #37)
> > Would it also explain total breakage of the gaia email client like this?
> 
> Make sure you have a version of gaia that has the updated email client. I
> saw the commits go through to update to the interface provided by this bug,
> so your gaia is probably out of date.

Thanks but I did ensure. However, it was a real bug, see bug 864775.

Comment 40

[Donovan Preston [:fzzzy]]

Ah, I see, sorry.

Comment 41

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

(In reply to Josh Matthews [:jdm] from comment #35)
> Sorry, I was assured that the email app was the only client. Luckily the
> changes should be particularly invasive; where you previously passed
> Uint8Array values, you can now pass ArrayBuffer instead.

I'm guessing you meant they should *not* be particularly invasive.  Which makes sense.  But I'm having trouble making them anyway.  I have what I think is the right set of changes, and debug output suggests I'm sending identical bits over the wire, plus some simple transactions work; but pushing an app to a device fails.  Is there any chance I'm doing something obviously wrong?

https://github.com/mozilla/r2d2b2g/pull/465/files

Comment 42

[Josh Matthews [:jdm]]

Looks like sockSend needs updating, which would explain why pushing a file fails.

Comment 43

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #41)
> I have what I think is the right set of changes, and debug output suggests
> I'm sending identical bits over the wire

More precisely: passing identical bits to the socket object's send() method.  Unclear whether they're still identical over the wire.


(In reply to Josh Matthews [:jdm] from comment #42)
> Looks like sockSend needs updating, which would explain why pushing a file
> fails.

It isn't obvious from the patch view, but sockSend is updated with the change to line 441:

https://github.com/mozilla/r2d2b2g/pull/465/files#L0L441

This is clearer in the view of the file with the change applied:

https://github.com/mykmelez/r2d2b2g/blob/c6c268eaa995a01606c4fbd81fb56ed9ac9fd5f2/addon/lib/adb.js#L417-L442

Comment 44

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

After further research, it turns out that TCPSocket.send() now chokes when passed an ArrayBuffer it was previously passed.  And the Simulator/B2GRemote's ADB module reuses some ArrayBuffer objects when it sends a file to a device.

It even modifies one of them in the process (a Uint32Array(1) that it uses to send several values), which I thought was perhaps the problem.  But even when it doesn't modify the object, as with its fileData array, which it sends in chunks, calling nsIDOMTCPSocket.send() twice with the same ArrayBuffer still causes the other side to get the wrong data.

Presumably the problem (or intended limitation?) is really in ArrayBufferInputStream, which TCPSocket now uses.  In any case, I updated the ADB module to always send a new ArrayBuffer, including creating one for each file chunk, and it now works with the new API.

Comment 45

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

chokes = explodes, or chokes = since it holds onto your buffers and you might mutate them after you give it to them, it sends the mutated data rather than the data you originally put in?

Comment 46

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

I thought that was the problem: the caller was mutating buffers, and TCPSocket was holding onto them and then sending the mutated data in place of the original data.  But the caller doesn't mutate its file data buffer, it just sends different chunks of it by twiddling the offset and length.  Yet that too triggers a failure.

I'm flying somewhat blind at the moment, because I don't have a view into the data the device receives, so I only know whether or not it thinks it received it successfully and installs the app I push to it.  But that was sufficient for me to determine that replacing that mutating buffer was sufficient to get things working when the caller sends the file data in one chunk; while larger files that the caller sends in multiple chunks also need unique buffers for each chunk.

Comment 47

[Josh Matthews [:jdm]]

I've reproduced the problem in an xpcshell test by sending the buffer from an encoded "OK" followed by some unrelated buffer followed by the first buffer again. If the duplicate buffers are uninterrupted, the server receives the data correctly. If they're sent around some other data, only the first buffer is received, with no other data to follow.

Comment 48

[Josh Matthews [:jdm]]

Interestingly enough, the same results occur when using a non-arraybuffer socket and sending a duplicate string value. Internally, we use nsStringInputStream to read the data, which conveniently is what I based ArrayBufferInputStream on, leading me to suspect that both implementations of Read or ReadSegments are not quite right. I've filed bug 865652 for this; let's leave this resolved bug alone now.