1057556 - TCPSocket doesn't work correctly if byteOffset and byteLength aren't set

Status: RESOLVED WORKSFORME

(Core :: DOM: Device Interfaces, defect)

Description

[Marco Castelluccio [:marco]]

const DATAARRAY = [0, 255, 254, 0, 1, 2, 3, 0, 255, 255, 254, 0],
      DATAARRAYBUFFER = new ArrayBuffer(DATAARRAY.length);

// This works correctly:

var socket = navigator.mozTCPSocket.open("www.mozilla.org", 80, { binaryType: "arraybuffer" });
socket.onopen = function() {
    console.log("Connection opened");
    console.log(socket.send(DATAARRAYBUFFER, 0, 5));
}
socket.ondata = function(event) {
    console.log(event.data.byteLength);
    socket.close();
}

// This doesn't work correctly:

var socket = navigator.mozTCPSocket.open("www.mozilla.org", 80, { binaryType: "arraybuffer" });
socket.onopen = function() {
    console.log("Connection opened");
    console.log(socket.send(DATAARRAYBUFFER, 0));
}
socket.ondata = function(event) {
    console.log(event.data.byteLength);
    socket.close();
}

// This doesn't work correctly:

var socket = navigator.mozTCPSocket.open("www.mozilla.org", 80, { binaryType: "arraybuffer" });
socket.onopen = function() {
    console.log("Connection opened");
    console.log(socket.send(DATAARRAYBUFFER));
}
socket.ondata = function(event) {
    console.log(event.data.byteLength);
    socket.close();
}

Comment 1

[Marco Castelluccio [:marco]]

They all work correctly if run in a privileged environment like about:newtab.

Comment 3

[Marco Castelluccio [:marco]]

Looks like this has been fixed.

Comment 4

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

:marco, can you confirm your testing method?  I don't think anything in TCPSocket actually changed to make this work, so unless XPConnect internals got changed, I don't know why it would start working.  Specifically, I think when I migrated the tests to be mochitests in bug 1087145 I think the 3-arg form was still required, although it might have been only in e10s that it was a problem...

Comment 5

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

I just re-ran the mochitests with the following patch applied, and they failed (they hang) in both e10s and non-e10s (on a tip debug build at least), so I'm reopening:

diff --git a/dom/network/tests/test_tcpsocket_client_and_server_basics.js b/dom/network/tests/test_tcpsocket_client_and_server_basics.js
--- a/dom/network/tests/test_tcpsocket_client_and_server_basics.js
+++ b/dom/network/tests/test_tcpsocket_client_and_server_basics.js
@@ -194,26 +194,26 @@ function* test_basics() {
 
   // -- Simple send / receive
   // - Send data from client to server
   // (But not so much we cross the drain threshold.)
   let smallUint8Array = new Uint8Array(256);
   for (let i = 0; i < smallUint8Array.length; i++) {
     smallUint8Array[i] = i;
   }
-  is(clientSocket.send(smallUint8Array.buffer, 0, smallUint8Array.length), true,
+  is(clientSocket.send(smallUint8Array.buffer), true,
      'Client sending less than 64k, buffer should not be full.');
 
   let serverReceived = yield serverQueue.waitForDataWithAtLeastLength(256);
   assertUint8ArraysEqual(serverReceived, smallUint8Array,
                          'Server received/client sent');
 
   // - Send data from server to client
   // (But not so much we cross the drain threshold.)
-  is(serverSocket.send(smallUint8Array.buffer, 0, smallUint8Array.length), true,
+  is(serverSocket.send(smallUint8Array.buffer), true,
      'Server sending less than 64k, buffer should not be full.');
 
   let clientReceived = yield clientQueue.waitForDataWithAtLeastLength(256);
   assertUint8ArraysEqual(clientReceived, smallUint8Array,
                          'Client received/server sent');
 
   // -- Perform sending multiple times with different buffer slices
   // - Send data from client to server

Comment 6

[Marco Castelluccio [:marco]]

You're right, sorry. I was testing in about:newtab.

Comment 7

[Josh Matthews [:jdm]]

I believe this should work as expected now that bug 885982 has merged.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Is this a dup to bug 885982 according to comment 7?

Comment 9

[Josh Matthews [:jdm]]

It wouldn't hurt for someone to actually verify my earlier statement.

Comment 10

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

Declaring this WFM based on my review back then and re-inspection here that we default byteOffset to 0:
https://hg.mozilla.org/mozilla-central/diff/bf8d9233a7bd/dom/webidl/TCPSocket.webidl#l1.137
and we automatically initialize to length if not passed:
https://hg.mozilla.org/mozilla-central/rev/bf8d9233a7bd#l6.646