1087145 - Move mozTCPSocket/TCPSocket unit tests from xpcshell tests to mochitest-plain tests

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Bug 1082450 changed some XPConnect security stuff and broke TCPSocket but didn't notice because TCPSocket tests really only had meaningful coverage in chrome-privileged xpcshell, missing a lot of nuances.  Bug 1084245 fixed the problem.  This bug is about making the tests be mochitests.  (TCPSocket is a content-targeted API and so xpcshell tests are not particularly appropriate or helpful.)

I'm currently working on the tests.  To underscore the need for this change, it appears that in non-e10s mode the data event's event.data ArrayBuffer is actually not usable by content.  We get "TypeError: invalid arguments" when doing new Uint8Array(event.data) and just trying to stringify via concatenation results in "Error: Permission denied to access property 'valueOf'".  The problem doesn't happen when doing "mach mochitest-plain --e10s" to run the test.

It also appears that although nsIDOMTCPSocket.send claims that its second two args are optional, they are in fact not (at least for in-process).

Comment 1

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

Attachment: most xpcshell tests ported to plain mochitests, moot stuff deleted, some TCPServerSocket fixes v1

So TCPServerSocket was failing to hook up the e10s 'drain' mechanism correctly and was also failing to propagate our horrible "useWin" hack which led to its socket not being able to use received data in non-e10s.

In general this consolidates most of the existing TCPSocket family of tests into mochitests by having us create a TCPServerSocket and a TCPSocket and have them talk to each other and do the same things.  I've tried to clean things up and make the tests easier to read by using generators and helpers that use promises.  The original tests were largely written by me, so I can confidently say that no one's feelings should be hurt by the idiom change.

I did not port or remove the following tests from test_tcpsocket:
- badconnect: given that the test has historically had problems with flakiness and may require more control/simulation to do right, it didn't seem appropriate to move it
- childnotbuffered/childbuffered:  These test drain logic in e10s mode by messing with nsITCPSocketInternal.  This arguably isn't appropriate for a plain mochitest to do.

I removed the following test but did not particularly port it:
- buffertwice: The idea of this one was to write two big buffers in succession without yielding control of the event loop and making sure drain fired at least once (with potentially weird test failures happening if it fired twice).  Given the multithreading and multiprocess issues going on, I think I argue this test probably needs to be rethought if we are concerned about this specific implementation regressing.

Note that this bug contains a stop-gap implementation of add_task for mochitest-plain that can ideally be removed when bug 1078657 is addressed.  I'll attach the implementation there too in case it's good-enough and existing implementations aren't awesome.

I've run this locally with "mach mochitest-plain" with and without "--e10s".  I tried to push to try but my account was disabled because I mainly hang out in git land right now.

Comment 2

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

try server run just pushed:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=36d9e78273c1

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 8509252 [details] [diff] [review]
most xpcshell tests ported to plain mochitests, moot stuff deleted, some TCPServerSocket fixes v1

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

Does test_tcpsocket_client_and_server_basics.html get run as part of the mochitest-e10s suite? We really can't lose our IPC TCPSocket tests. In other news, this will make the test situation for bug 885982 and bug 916199 so much nicer, but will also necessitate implementing TCPServerSocket worker support. So it goes.

::: dom/network/TCPSocketParentIntermediary.js
@@ +69,5 @@
>                              .createInstance(Ci.nsITCPSocketParent);
>        var intermediary = new TCPSocketParentIntermediary();
> +
> +      let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal);
> +      // XXX why isn't appId being set?

We should set it.

::: dom/network/tests/add_task.js
@@ +16,5 @@
> +      catch (ex) {
> +        ok(false, 'Problem invoking generator func: ' + ex + ': ' + ex.stack);
> +        return;
> +      }
> +      var stepResolved = function(result) {

I admit that I don't know much about ES6 generators, but how does result ever get a value?

::: dom/network/tests/test_tcpsocket_client_and_server_basics.js
@@ +22,5 @@
> +      ok(false, comparingWhat + ' arrays differ at index ' + i +
> +         a[i] + ' versus ' + b[i]);
> +    }
> +  }
> +  ok(true, comparingWhat + ' arrays were equivalent.');

This is a little weird if any of the earlier comparisons failed.

@@ +250,5 @@
> +    // - Send "big" data from the client to the server
> +    is(clientSocket.send(bigUint8Array.buffer, 0, bigUint8Array.length), false,
> +       'Client sending more than 64k should result in the buffer being full.');
> +    is((yield clientQueue.waitForEvent()).type, 'drain',
> +       'The drain event should fire after a large send that returned true.');

The previous send returned false.

@@ +261,5 @@
> +    // - Send "big" data from the server to the client
> +    is(serverSocket.send(bigUint8Array.buffer, 0, bigUint8Array.length), false,
> +       'Server sending more than 64k should result in the buffer being full.');
> +    is((yield serverQueue.waitForEvent()).type, 'drain',
> +       'The drain event should fire after a large send that returned true.');

Previous send returned false.

@@ +291,5 @@
> +  clientQueue = listenForEventsOnSocket(clientSocket, 'client');
> +  is((yield clientQueue.waitForEvent()).type, 'open', 'got open event');
> +
> +  let connectedResult = yield connectedPromise;
> +  // destructuring assignment is not yet ES6 compliant, must manually unpack

Yet it was used higher up?

::: dom/network/tests/unit/test_tcpserversocket.js
@@ -201,5 @@
> -/**
> - * Connect the socket to the server after the server was closed.
> - * This test is added after test to close the server was conducted.
> - */
> -function openSockInClosingServer() {

I didn't see an equivalent of this test.

Comment 4

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

(In reply to Josh Matthews [:jdm] from comment #3)
> Does test_tcpsocket_client_and_server_basics.html get run as part of the
> mochitest-e10s suite? We really can't lose our IPC TCPSocket tests. In other
> news, this will make the test situation for bug 885982 and bug 916199 so
> much nicer, but will also necessitate implementing TCPServerSocket worker
> support. So it goes.

Yes, in the mochitest-e10s-log we have:
11:08:36     INFO -  396 INFO TEST-OK | /tests/dom/network/tests/test_tcpsocket_client_and_server_basics.html | took 739ms

(And I also ran both mochitest and mochitest-e10s locally for the file, and indeed the different code-paths were used, etc., as indicated by the non-e10s tests failing for TCPServerSocket until I addressed those bugs.)

Note that the try results indicate that I made add_task.js too aggressive about deciding that there are no "next tasks" and so many of the non-e10s runs experienced a race there and did not run the tests.  I'll clean up add_task to listen for the load events on subsequent scripts (or equivalent/superior).  I'll push a new try run when I do that.


> ::: dom/network/TCPSocketParentIntermediary.js
> @@ +69,5 @@
> >                              .createInstance(Ci.nsITCPSocketParent);
> >        var intermediary = new TCPSocketParentIntermediary();
> > +
> > +      let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal);
> > +      // XXX why isn't appId being set?
> 
> We should set it.

Okay, I'll look into plumbing this through.  This may require some additional minor IDL changes.  Looking at one of the other bugs, I realized I failed to update the IDL docs, so I'll also do that.

> ::: dom/network/tests/add_task.js
> @@ +16,5 @@
> > +      catch (ex) {
> > +        ok(false, 'Problem invoking generator func: ' + ex + ': ' + ex.stack);
> > +        return;
> > +      }
> > +      var stepResolved = function(result) {
> 
> I admit that I don't know much about ES6 generators, but how does result
> ever get a value?

stepResolved is used as the callback argument to a promise a la promise.then(stepResolved).  The required contract, which is consistent (except for returning generators) with the other add_task implementations, is that if the generator yields control flow, the values it yields is a promise.

> ::: dom/network/tests/test_tcpsocket_client_and_server_basics.js
> @@ +22,5 @@
> > +      ok(false, comparingWhat + ' arrays differ at index ' + i +
> > +         a[i] + ' versus ' + b[i]);
> > +    }
> > +  }
> > +  ok(true, comparingWhat + ' arrays were equivalent.');
> 
> This is a little weird if any of the earlier comparisons failed.

Yes, there should be a return.  I will address that, thank you.

> @@ +250,5 @@
> The previous send returned false.
> 
> @@ +261,5 @@
> Previous send returned false.

I got these booleans wrong initially; the risk of detailed strings and coding with a cold! ;)

> > +  let connectedResult = yield connectedPromise;
> > +  // destructuring assignment is not yet ES6 compliant, must manually unpack
> 
> Yet it was used higher up?

Yeah, this is weird.  Higher up "let" was present, however, and that seems to take us down a different parser path.  I got a hard syntax error here.  It's possible I'm missing some nuances here, but MDN claims we're not yet ES6-compliant for "let" or destructuring assignment, so I figured I'd just comment and move on.

 
> ::: dom/network/tests/unit/test_tcpserversocket.js
> @@ -201,5 @@
> > -/**
> > - * Connect the socket to the server after the server was closed.
> > - * This test is added after test to close the server was conducted.
> > - */
> > -function openSockInClosingServer() {
> 
> I didn't see an equivalent of this test.


I screwed up here, thanks for the attention to detail on the removed tests.  I will reimplement this test.  Our 'bad connect' tests have also had problems on OS X in general, and this conceptually falls into the same bucket, so I'll make sure to run the mochitest-3 tests multiple times on OS X to try and shake out any potential intermittents.  (I'll also do some extra mochitest-3 runs on other platforms too.)

I'll run with the conditional r+ and land either today or over the weekend assuming I don't run into any trouble propagating the appId and all tests are super-green.

Comment 5

[Josh Matthews [:jdm]]

I would also be willing to punt the appId to a followup; it's only used for network stats.

Comment 6

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

https://tbpl.mozilla.org/?tree=Try&rev=c2b098d61c2a

Comment 7

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

Ha, so I'm seeing a somewhat hilarious interaction between the event objects and the implementation of mozilla::dom::Promise::ResolveInternal in debug builds.  Specifically, the test resolves a promise with the event object.  Promise::ResolveInternal does JS_GetProperty(aCx, valueObj, "then", &then) which cascades through JS_GetPropertyById, JS_ForwardGetPropertyTo, js::Proxy::get, js::AutoEnterPolicy::AutoEnterPolicy, and xpc::FilteringWrapper which then results in xpc::ExposedPropertiesOnly::deny and xpc::ReportWrapperDenial giving us this:

12:46:13     INFO -  [Parent 991] WARNING: Silently denied access to property |then|: Access to privileged JS object not permitted (@http://mochi.test:8888/tests/dom/network/tests/test_tcpsocket_client_and_server_basics.js:44): file /builds/slave/try-osx64-d-000000000000000000/build/js/xpconnect/wrappers/XrayWrapper.cpp, line 197

I've addressed this problem by having the wrapper expose "then" with a value of undefined and put a comment pointing at bug 882123 which is where we properly use nsIEventTarget and all that.  It's ugly, but it arguably seems superior than having debug builds print out confusing and scary warnings.


I'm about to push another try build that should be super-green.  Note that when running "mach mochitest-plain dom/network/tests/test_tcpsocket_client_and_server_basics.html --e10s" I need to have the fix for bug 1088148 applied to avoid a (not our fault segfault) that occurs before the test even starts running.

The push addresses the appId stuff and all other outstanding action items.  Note that there was an xpcshell hiccup in the preceding try run because I removed the badconnect test from there since we now have that coverage from the "initiate client connect after closing the listening server".  That potentially made the cleanup code angry, so now that xpcshell test may run in a sort-of no-op mode that doesn't complain.

Comment 8

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

https://tbpl.mozilla.org/?tree=Try&rev=e745934bcd21

Comment 9

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

Attachment: Landing version: Move mozTCPSocket/TCPSocket unit tests from xpcshell tests to mochitest-plain tests

Most of the TCPSocket and TCPServerSocket coverage was implemented exclusively
in Chrome-privileged xpcshell tests.  This failed to provide coverage for the
key use case of content-privileged code using TCPSocket.

This cleans up the test implementation and migrates them to mochitests.
Coverage is improved as evidenced by two tested TCPServerSocket issues that were
addressed in this patch:
- ArrayBuffers weren't being created in the content page's context, so
  exceptions would be thrown when accessed.
- 'drain' notifications were not being hooked up.

The following fix that lacks coverage that notices the fix was implemented:
- TCPServerSocket now properly propagates the appId for network usage tracking.

Comment 10

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

Comment on attachment 8513962 [details] [diff] [review]
Landing version: Move mozTCPSocket/TCPSocket unit tests from xpcshell tests to mochitest-plain tests

This is the landing version after super-happy TBPL.  Carrying forward r=jdm

Comment 11

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

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

Comment 12

[Carsten Book [:Tomcat]]

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