:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 11 years ago

      No description provided.

	Reuben Morais [:reuben]
	Comment 1 • 11 years ago

WIP. This is leaking an nsSocketTransportService somewhere, I haven't yet managed to find where the leak is.

5 Mutex (120 bytes)
3 ReentrantMonitor (96 bytes)
2 nsAStreamCopier (288 bytes)
2 nsPipe (576 bytes)
2 nsSocketTransport (880 bytes)
1 nsSocketTransportService (216 bytes)
2 nsStringBuffer (16 bytes)

	Reuben Morais [:reuben]
	Comment 2 • 11 years ago

Oops, that was missing TCPSocketEvent.cpp

	Reuben Morais [:reuben]
	Comment 3 • 11 years ago

Using nsIVariant in nsIDOMTCPSocketEvent to be able to use generated events, and converted the multisend test.

	Reuben Morais [:reuben]
	Comment 4 • 11 years ago

Jonas, this changes the API to be more sane (no more navigator property with open method (on the same interface, nonetheless!), instead use a constructor).

Josh, the leak is still there, I'll see if I can find it but given that it's preexisting problem and I don't know much about this code, I appreciate any help.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 5 • 11 years ago

Comment on attachment 766964 [details] [diff] [review]
Move TCPSocket to WebIDL

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

sr=me as long as jdm reviews.

	Josh Matthews [:jdm] Assignee
	Comment 6 • 11 years ago

Comment on attachment 766964 [details] [diff] [review]
Move TCPSocket to WebIDL

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

By converting the tests to mochitests, we lose the IPC tests, right? That's totally not going to fly :< For example, they would catch that TCPSocketParentIntermediary still calls TCPSocket.open, which no longer exists. The rest of the changes look ok to me, but we've got to not regress the IPC situation.

::: dom/network/src/TCPSocket.js
@@ -56,1 @@
>  }

Revert these changes, please.

	Reuben Morais [:reuben]
	Comment 7 • 11 years ago

(In reply to Josh Matthews [:jdm] from comment #6)
> Comment on attachment 766964 [details] [diff] [review]
> Move TCPSocket to WebIDL
> 
> Review of attachment 766964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> By converting the tests to mochitests, we lose the IPC tests, right? That's
> totally not going to fly :< For example, they would catch that
> TCPSocketParentIntermediary still calls TCPSocket.open, which no longer
> exists. The rest of the changes look ok to me, but we've got to not regress
> the IPC situation.

We run mochitests on the child on B2G (and maybe Android?). Is that not enough coverage?

	Josh Matthews [:jdm] Assignee
	Comment 8 • 11 years ago

It does make it much much harder to debug failures when they occur, so I would only want to accept that if there is literally nothing else we can do.

	Reuben Morais [:reuben]
	Comment 9 • 11 years ago

The idea here would be to create a <browser remote> and then load the test on the child, so we get IPC coverage on all platforms. Sadly, the way TCPSocket is implemented right now makes fixing this bug way harder than it should be.

	Alexandre Poirot [:ochameau]
	Comment 10 • 11 years ago

What is the state of this patch. Is the patch actually ok to be landed as-is except the test story?

Speaking about the tests, is the main issue the convertion of test_tcpsocket.js/xpcshell in child to test_tcpsocket.html/mochitest-chrome?
What about converting it to a mochitest-plain? That would allow testing the ipc codepath when running tests on b2g, and in-parent process on other platforms.

	Josh Matthews [:jdm] Assignee
	Comment 11 • 11 years ago

The way forward is to set up something like IndexedDB and DeviceStorage's test_ipc wrapper (http://mxr.mozilla.org/mozilla-central/source/dom/devicestorage/ipc/test_ipc.html?force=1), since that will run on all platforms and making debugging it much easier if it breaks.

	Josh Matthews [:jdm] Assignee
	Comment 12 • 11 years ago

Also I'm pretty sure the IPC codepath in the patch is broken right now, so it's not landable without further revision.

	Reuben Morais [:reuben]
	Comment 13 • 11 years ago

(In reply to Alexandre Poirot (:ochameau) from comment #10)
> What is the state of this patch. Is the patch actually ok to be landed as-is
> except the test story?

No. TCPSocketParentIntermediary uses XPCOM to create instances of TCPSocket, so we can't get rid of the XPIDL :(

	Josh Matthews [:jdm] Assignee
	Comment 14 • 11 years ago

wip

	Josh Matthews [:jdm] Assignee
	Comment 15 • 11 years ago

wip C++ stub

	Gene Lian (I already quit Mozilla)
	Comment 16 • 11 years ago

Comment on attachment 8346690 [details] [diff] [review]
WIP C++ stub instead of JS implementation.

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

Hi Josh, just a quick question. :)

::: dom/network/src/TCPSocketStub.cpp
@@ +29,5 @@
> +
> +TCPSocketStub::TCPSocketStub(nsPIDOMWindow* aWindow)
> +: nsDOMEventTargetHelper(aWindow)
> +{
> +  mSocket = do_CreateInstance("@mozilla.org/tcp-socket-impl;1");

It seems you instantiate a mSocket to associate to the JS implementation instead of using the inheritance way.

::: dom/network/src/TCPSocketStub.h
@@ +12,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class TCPSocketStub: public nsDOMEventTargetHelper,
> +                     public nsIDOMTCPSocket {

I'm curious about why do you make it inherit from nsIDOMTCPSocket?

	Josh Matthews [:jdm] Assignee
	Comment 17 • 11 years ago

I was young and foolish. I actually remove the nsIDOMTCPSocket inheritance in the patch in bug 916199.

	Josh Matthews [:jdm] Assignee
	Comment 18 • 11 years ago

This passes. It's a big copy and paste hack of the harness in dom/browserelement. I am not a proud man.

	Josh Matthews [:jdm] Assignee
	Comment 19 • 11 years ago

I'm not thrilled at all by the duplication between the inproc/oop test_tcpsocket and the worker test_tcpsocket (especially since the latter is split between the main-thread server and the worker thread socket in separate files), but I'm having trouble coming up with a way of reconciling the two.

	Andrea Marchesini [:baku]
	Comment 22 • 10 years ago

Comment on attachment 8389537 [details] [diff] [review]
Convert MozTCPSocket to WebIDL.

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

looks good but why # User Reuben Morais <reuben.morais@gmail.com> in the headers of the patch?
I guess you can get rid of nsIDOMTCPSocketEvent, do you?

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +18,1 @@
>  [scriptable, uuid(65f6d2c8-4be6-4695-958d-0735e8935289)]

change the UUID

::: dom/network/interfaces/nsIDOMTCPSocketEvent.idl
@@ +6,5 @@
> +#include "nsIDOMEvent.idl"
> +interface nsIVariant;
> +
> +/**
> + * nsITCPSocketEvent is the event object which is passed as the

nsIDOMTCPSocketEvent

@@ +13,5 @@
> + * and the data associated with the event (if any).
> + */
> +
> +[scriptable, builtinclass, uuid(0f2abcca-b483-4539-a3e8-345707f75c44)]
> +interface nsIDOMTCPSocketEvent : nsIDOMEvent {

Why this is a xpcom event and not a webidl?

::: dom/network/tests/test_tcpsocket.html
@@ +6,5 @@
> +<head>
> +  <title>Mozilla Bug 885982</title>
> +  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
> +  <script type="application/javascript">
> +  SimpleTest.waitForExplicitFinish();

why is this here? :) It's already at the bottom of the file.

::: dom/webidl/TCPSocket.webidl
@@ +15,5 @@
> +  "string"
> +};
> +
> +dictionary SocketOptions {
> +  boolean useSSL;

default value to false, I guess.

	Reuben Morais [:reuben]
	Comment 23 • 10 years ago

Comment on attachment 8389537 [details] [diff] [review]
Convert MozTCPSocket to WebIDL.

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

::: dom/network/interfaces/nsIDOMTCPSocketEvent.idl
@@ +13,5 @@
> + * and the data associated with the event (if any).
> + */
> +
> +[scriptable, builtinclass, uuid(0f2abcca-b483-4539-a3e8-345707f75c44)]
> +interface nsIDOMTCPSocketEvent : nsIDOMEvent {

FWIW, you no longer need to have both XPIDL and WebIDL for generated events.

	Josh Matthews [:jdm] Assignee
	Comment 24 • 10 years ago

Note to self: this patch should be updated to make the interface TCPSocket, not mozTCPSocket.

	Josh Matthews [:jdm] Assignee
	Comment 26 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #22)
> looks good but why # User Reuben Morais <reuben.morais@gmail.com> in the
> headers of the patch?

Reuben did most of this work; I just rebased it.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 27 • 10 years ago

What remains to be done here?

	Josh Matthews [:jdm] Assignee
	Comment 28 • 10 years ago

Nothing, but if we land this patch as it is, we lose all IPC tests.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 29 • 10 years ago

That sounds bad.  What blocks fixing that?

	Josh Matthews [:jdm] Assignee
	Comment 30 • 10 years ago

They're added as part of bug 916199. I've always been intending to land the patches from both bugs together.

	Josh Matthews [:jdm] Assignee
	Comment 31 • 9 years ago

Rebased on top of bug 1118063 and now with e10s support. This will now actually be able to land independently of bug 916199.

	Andrea Marchesini [:baku]
	Comment 32 • 9 years ago

Comment on attachment 8544401 [details] [diff] [review]
Convert MozTCPSocket to WebIDL

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

I need you to answer a couple of questions (read the review) before continuing. Thanks.

::: dom/network/TCPSocketParentIntermediary.js
@@ +37,5 @@
>      aParentSide.sendUpdateBufferedAmount(aBufferedAmount, aTrackingNumber);
>    },
>  
>    open: function(aParentSide, aHost, aPort, aUseSSL, aBinaryType, aAppId) {
> +    let win = Services.appShell.hiddenDOMWindow;

why do you use this hiddenDOMWindow? If you don't want to return this object to content, can you write a comment explaining why you are doing this?

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +19,2 @@
>  interface nsIDOMTCPSocket : nsISupports
>  {

Why do you want to keep this interface?
Can you port all these methods and mark them as [ChromeOnly]?

::: dom/network/tests/test_tcpsocket_default_permissions.html
@@ +13,5 @@
>  <script type="application/javascript">
>  
>  /** Test to ensure TCPSocket permission is disabled by default **/
>  
> +ok(!("mozTCPSocket" in window), "navigator.mozTCPSocket should not exist by default");

remove 'navigator.'

::: dom/network/tests/test_tcpsocket_enabled_no_perm.html
@@ +22,4 @@
>  
>  try {
> +  new mozTCPSocket('localhost', 80);
> +  ok(false, "Error: navigator.mozTCPSocket.open should raise for content that does not have the tcp-socket permission");

remove navigator.

::: dom/network/tests/test_tcpsocket_enabled_with_perm.html
@@ +14,2 @@
>  
>  /** Test to ensure TCPSocket permission being turned on enables 

just because you are touching this file, can you remove this extra space?

::: dom/webidl/TCPSocket.webidl
@@ +31,5 @@
> +interface mozTCPSocket : EventTarget {
> +  /**
> +   * The host of this socket object.
> +   */
> +  readonly attribute DOMString host;

Can this be USVString?

@@ +90,5 @@
> +   *         and the caller may wish to wait until the ondrain event
> +   *         handler has been called before buffering more data by more
> +   *         calls to send.
> +   */
> +  boolean send(any data, optional unsigned long byteOffset, optional unsigned long byteLength);

Instead any, can it be (USVString or ArrayBuffer) data?

::: dom/webidl/TCPSocketEvent.webidl
@@ +23,5 @@
> +   * of the error.
> +   *
> +   * In the other callbacks, data will be an empty string.
> +   */
> +  readonly attribute any data; // (ArrayBuffer or DOMString)

Have you tried to do this:
readonly attribute (ArrayBuffer or DOMString) data;

	Josh Matthews [:jdm] Assignee
	Comment 33 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #32)
> Comment on attachment 8544401 [details] [diff] [review]
> Convert MozTCPSocket to WebIDL
> 
> Review of attachment 8544401 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/webidl/TCPSocketEvent.webidl
> @@ +23,5 @@
> > +   * of the error.
> > +   *
> > +   * In the other callbacks, data will be an empty string.
> > +   */
> > +  readonly attribute any data; // (ArrayBuffer or DOMString)
> 
> Have you tried to do this:
> readonly attribute (ArrayBuffer or DOMString) data;

I'd like to defer this to a follow up, since it hits a Codegen.py assertion because the rooting for generated event classes doesn't support this yet.

	Andrea Marchesini [:baku]
	Comment 35 • 9 years ago

Comment on attachment 8546741 [details] [diff] [review]
Convert MozTCPSocket to WebIDL

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

looks good!

::: dom/network/TCPSocket.js
@@ +420,5 @@
> +    if (options) {
> +      if (options.useSSL) {
> +          this._ssl = 'ssl';
> +      } else {
> +          this._ssl = false;

_ssl is false by default. Do you really need to do this else statement?

::: dom/network/TCPSocketParent.cpp
@@ +335,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +TCPSocketParent::SetSocketAndIntermediary(nsITCPSocketInternal *socket,

aSocket, aIntermediary, aCx

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +20,5 @@
> + * Needed to account for multiple possible types that can be provided to
> + * the socket callbacks as arguments.
> + */
> +[scriptable, uuid(E79C5F3C-0B54-44B9-B0D7-6930EF9126DC)]
> +interface nsITCPSocketInternal : nsISupports {

what about renaming the file to nsITCPSocketInternal.idl ?

::: dom/network/interfaces/nsITCPSocketParent.idl
@@ +11,5 @@
>  
>  // Interface required to allow the TCP socket object (TCPSocket.js) in the
>  // parent process to talk to the parent IPC actor, TCPSocketParent, which
>  // is written in C++.
>  [scriptable, uuid(6f040bf0-6852-11e3-949a-0800200c9a66)]

change the UUID

::: dom/network/tests/test_tcpsocket_enabled_no_perm.html
@@ +11,5 @@
>  </div>
>  <pre id="test">
>  <script type="application/javascript">
>  
>  /** Test to ensure TCPSocket permission being turned on enables 

can you remove this extra space?

::: dom/webidl/TCPSocket.webidl
@@ +1,5 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Some spec URL?

::: dom/webidl/TCPSocketEvent.webidl
@@ +1,5 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

If we have any documentation/spec URL, would be nice to add it here.

@@ +23,5 @@
> +   * of the error.
> +   *
> +   * In the other callbacks, data will be an empty string.
> +   */
> +  //TODO: make this (ArrayBuffer or DOMString) after sorting out the rooting required.

follow UP bug ID?

	Josh Matthews [:jdm] Assignee
	Comment 36 • 9 years ago

Same spec issue; this is just a straight conversion of nsIDOMTCPSocket, which only superficially resembles http://www.w3.org/2012/sysapps/tcp-udp-sockets/#interface-tcpsocket .

	Josh Matthews [:jdm] Assignee
	Comment 37 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c855e6d0d1

	Josh Matthews [:jdm] Assignee
	Comment 38 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fecaaa3d1d97

	Josh Matthews [:jdm] Assignee
	Comment 39 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ede577f5ada

	Josh Matthews [:jdm] Assignee
	Comment 40 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/865e7bc208df

	Phil Ringnalda (:philor)
	Comment 41 • 9 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0c454540fc2b along with bug 1118063 since I wasn't sure what to back out to clear up https://treeherder.mozilla.org/logviewer.html#?job_id=5595830&repo=mozilla-inbound

	Josh Matthews [:jdm] Assignee
	Comment 42 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d36667d3ce17

	Josh Matthews [:jdm] Assignee
	Comment 43 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01644633ed8b

	Josh Matthews [:jdm] Assignee
	Comment 44 • 9 years ago

Brad, word to the wise - the changes in this bug are going to break http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/secondscreen/RokuApp.jsm#330 . I think it should be as simple as adding a `let global = this;` at the top and using `new global.mozTCPSocket` to invoke the constructor, rather than calling `this._baseSocket.open`, but I have no way of testing that change.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 45 • 9 years ago

jdm, here's a roku simulator you can run on your desktop https://people.mozilla.org/~rbarker/roku-sim.tgz so you can test the change without an actual roku device (it's compiled for OSX, lmk if your on a different platform).

	Josh Matthews [:jdm] Assignee
	Comment 50 • 9 years ago

Comment on attachment 8546741 [details] [diff] [review]
Convert MozTCPSocket to WebIDL

The efforts to maintain the JS implementation really no longer seem worth it.

	Josh Matthews [:jdm] Assignee
	Comment 51 • 9 years ago

I've stashed my current snapshot of a C++ reimplementation of the TCPSocket and TCPServerSocket interfaces. I still intend to do a line-by-line walkthrough of TCPSocket.js and TCPServerSocket.js to make sure I haven't left out anything (for example, there are no privilege checks right now). The patches here pass the basic client/server test in dom/network/tests in both e10s and non-e10s.

	Andrew Sutherland [:asuth] (he/him)
	Comment 52 • 9 years ago

\o/ Hooray!  It will be great to lose the JS part of the code which results in the buffer payloads going from reference-counted to GC-tracked.

Also, probably a good idea for the next rev of the patches is to have mMultiplexStream->AppendStream calls happen in a runnable that's dispatched to the necko I/O thread because of that implementation's assumptions about when AppendStream is called.  Unless that's still not good enough.

	Josh Matthews [:jdm] Assignee
	Comment 53 • 9 years ago

Andrew, any suggestion on what to do about test_tcpsocket.js? I tried exposing TCPSocket et al via System, but I get JSAPI errors because the globals don't provide stuff the JS value-creating bits related to DOM events require. It doesn't feel worth spending the time to isolate precisely what the problem is and whether it's possible to work around it; what do you think?

	Andrew Sutherland [:asuth] (he/him)
	Comment 54 • 9 years ago

I believe we've been operating under the understanding that supporting TCPSocket from chrome/non-DOM code is a non-goal and that chrome code should just use socket.jsm or other such things.  So I think it's fine to not support that use-case since it just increases maintenance burden in a non-trivial way.

In terms of the tests that remain in test_tcpsocket, they're glass-box verifications of the edge-cases where the parent and child processes can make different decisions about "are we now buffering" which had previously led to bug 932183 being filed and the test cases being introduced.  It's hard to create this disparity in reality but easy to create by the direct meddling the xpcshell test performs.  (Also, it seems like only "childbuffered" can happen, not "childnotbuffered" under our implementation.)

We only needed these test cases because the initial control flow implementation was buggy, likely because of the complexity of TCPSocket.js operating in both the parent and child with different conditional logic happening.  If the revised C++ logic has a more straightforward state-machine that's sufficiently explains how we ensure the 'drain' event is reliably generated in comments and can then be easily verified by human brain, we may not even need the tests.

And/or maybe we can just port the tests by having debug-build only hooks that let us induce the buffer edge-case by sending a notification in the observer-service or setting hacky debug-only preferences.  (Using either SpecialPowers or a chrome-mochitest that does the legwork of spinning up the content situation.)

	Andrew Sutherland [:asuth] (he/him)
	Comment 56 • 9 years ago

Comment on attachment 8583092 [details] [diff] [review]
Part 1: Convert TCPSocket to WebIDL and rewrite in C++

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

::: dom/webidl/TCPSocket.webidl
@@ +17,5 @@
> +  "string"
> +};
> +
> +dictionary SocketOptions {
> +  boolean useSSL = false;

Note that in bug 784816 we switched from useSSL to useSecureTransport, so this should be useSecureTransport.

	Randell Jesup [:jesup] (needinfo me)
	Comment 57 • 9 years ago

Note that we want this for bug 950660 (support TURN-TCP in e10s for media/mtransport (aka WebRTC)).  This is only a step towards what we really need, which is moving TCPSocket (at least for WebRTC) to PBackground like we did UDPSocket.  I.e. hitting Mainthread for network traffic (especially from non-JS sources) is BAD.

	Josh Matthews [:jdm] Assignee
	Comment 58 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=426aea14d2ab

	Josh Matthews [:jdm] Assignee
	Comment 59 • 9 years ago

This produces a black screen instead of mirroring the current tab. I've tried to replicate everything that TCPSocket does but can't find what's going wrong.

	Josh Matthews [:jdm] Assignee
	Comment 64 • 9 years ago

Comment on attachment 8632301 [details] [diff] [review]
Part 1: Convert TCPSocket to WebIDL and rewrite in C++

Andrew, you're it! I've done a line by line comparison against the JS implementation, and this collection of patches makes all existing tests pass. I split it up into these three main parts to make it clearer how e10s support changes the relatively simple in-process architecture.

	Josh Matthews [:jdm] Assignee
	Comment 65 • 9 years ago

Comment on attachment 8632305 [details] [diff] [review]
Part 3: Add e10s support to TCPSocket and TCPServerSocket

Honza, this is a set of patches that maintain feature parity with the existing C++/JS implementation. I figure your input would be useful on this patch in particular, which reimplements e10s support on top of a fairly simple C++ in-process base. There is no more intermediate JS object between the TCPSocket implementation and the TCPSocketParent; now TCPSocketParent and TCPSocket have direct pointers to each other (as do TCPSocketChild and TCPSocket), and there's a simple CreateAcceptedSocket API for creating a new TCPSocket given an existing socket-related object (either TCPSocketChild or nsISocketTransport). I removed the test_tcpsocket.js because I believe the conditions it's testing (cross-process buffering + ondrain event behaviour) is simple enough in this new design that it's not worth the extra effort to create the internal APIs to replicate it.

	Andrew Sutherland [:asuth] (he/him)
	Comment 66 • 9 years ago

Comment on attachment 8632301 [details] [diff] [review]
Part 1: Convert TCPSocket to WebIDL and rewrite in C++

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

::: dom/network/TCPSocket.cpp
@@ +182,5 @@
> +
> +#ifdef MOZ_WIDGET_GONK
> +  nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1");
> +  if (networkManager) {
> +    networkManager->GetAtiveNetwork(mActiveNetwork);

typo, missing the letter "c".

@@ +439,5 @@
> +  mReadyState = CLOSED;
> +
> +  if (NS_FAILED(status)) {
> +    // Convert the status code to an appropriate error message.  Raw constants
> +    // are used inline in all cases for consistency.  Some error codes are

nit: the comment about inline raw constants was a JS thing and mooted here in the C++ code since we're now using the actual constants.

@@ +585,5 @@
> +    return false;
> +  }
> +
> +  uint32_t byteLength;
> +  if (aData.IsUSVString()) {

I notice here that we're changing our send() behaviour so that it no longer depends on the mUseArrayBuffers/_binaryType which I affirm is a good idea since what the JS code was doing was sketchy and you had to issue that bugfix recently.

@@ +715,5 @@
> +
> +  rv = mInputStreamPump->Init(mSocketInputStream, -1, -1, 0, 0, false);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  uint64_t suspendCount = mSuspendCount;

good logic fix here! (The JS code mutated mSuspendCount down to 0.)

	Andrew Sutherland [:asuth] (he/him)
	Comment 67 • 9 years ago

Comment on attachment 8632302 [details] [diff] [review]
Part 2: Convert TCPServerSocket to WebIDL and rewrite in C++

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

::: dom/network/tests/test_tcpsocket_client_and_server_basics.js
@@ +174,2 @@
>    // - Start up a listening socket.
> +  let listeningServer = new mozTCPServerSocket(serverPort,

Could we use StaticClassOverride (https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Static_Members) in order to keep navigator.mozTCPSocket.listen() around for backwards-compatibility reasons?

(Specifically, I don't think it's a deal-breaker to change the API, but if WebIDL can easily let us keep the signature around, it seems friendlier to do that.)

	Andrew Sutherland [:asuth] (he/him)
	Comment 68 • 9 years ago

Comment on attachment 8632305 [details] [diff] [review]
Part 3: Add e10s support to TCPSocket and TCPServerSocket

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

::: dom/network/TCPSocket.cpp
@@ +1002,5 @@
> +{
> +  if (aTrackingNumber != mTrackingNumber) {
> +    nsPrintfCString debugMsg("UpdateBufferedAmount called with non-matching tracking number: "
> +                             "%u (parent) vs %u (child)", aTrackingNumber, mTrackingNumber);
> +    NS_WARNING(debugMsg.get());

We should lose this warning.  This is something that we expect to potentially happen during normal operation.  It's rare, but not unexpected and when we don't update in this case, we're doing the right thing.  So this is just spam.

	Andrew Sutherland [:asuth] (he/him)
	Comment 69 • 9 years ago

Comment on attachment 8632306 [details] [diff] [review]
Part 5: Remove all traces of JS implementation

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

Wooo!

	Honza Bambas (:mayhemer)
	Comment 70 • 9 years ago

Review in progress.  Looks good!  Fantastic work, it's now so much more readable and better written now :)

	Honza Bambas (:mayhemer)
	Comment 71 • 9 years ago

Comment on attachment 8632305 [details] [diff] [review]
Part 3: Add e10s support to TCPSocket and TCPServerSocket

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

Locally untested.  r+ but please:
- change some of the names as suggested to make the code even clearer (it's still complicated)
- add comments on members and methods, at least on those that are not obvious.

::: dom/network/TCPServerSocket.cpp
@@ +131,5 @@
>    nsRefPtr<TCPServerSocketEvent> event =
>        TCPServerSocketEvent::Constructor(this, aType, init);
>    event->SetTrusted(true);
>    bool dummy;
>    DispatchEvent(event, &dummy);

do we want the event be dispatched on the parent?  who will ever receive it?

::: dom/network/TCPServerSocket.h
@@ +51,5 @@
>  
>    IMPL_EVENT_HANDLER(connect);
>    IMPL_EVENT_HANDLER(error);
>  
> +  nsresult AcceptSocket(TCPSocketChild* aSocketChild);

AcceptSocketChild ?

@@ +60,4 @@
>    void FireEvent(const nsAString& aType, TCPSocket* aSocket);
>  
>    nsCOMPtr<nsIServerSocket> mServerSocket;
> +  nsRefPtr<TCPServerSocketChild> mServerBridge;

mServerBridgeChild

::: dom/network/TCPServerSocketParent.cpp
@@ +157,5 @@
> +  socket->SetAppIdAndBrowser(GetAppId(), GetInBrowser());
> +  nsRefPtr<TCPSocketParent> socketParent = new TCPSocketParent();
> +  socketParent->SetSocket(socket);
> +  socket->SetSocketBridgeParent(socketParent);
> +  SendCallbackAccept(socketParent);

some blank lines please

::: dom/network/TCPServerSocketParent.h
@@ +33,5 @@
>  
>    virtual bool RecvClose() override;
>    virtual bool RecvRequestDelete() override;
>  
> +  nsresult SendCallbackAccept(TCPSocketParent *socket);

should this be private?

::: dom/network/TCPSocket.cpp
@@ +1006,5 @@
> +    NS_WARNING(debugMsg.get());
> +    return;
> +  }
> +  mBufferedAmount = aBufferedAmount;
> +  if (!mBufferedAmount) {

could we experiment with mBufferedAmount < 64k here to get some pipelining?  Followup sufficient.

::: dom/network/TCPSocket.h
@@ +66,5 @@
> +  void SendWithTrackingNumber(const USVStringOrArrayBuffer& aData,
> +                              const Optional<uint32_t>& aByteOffset,
> +                              const Optional<uint32_t>& aByteLength,
> +                              const uint32_t& aTrackingNumber,
> +                              ErrorResult& aRv);

the tracking number (at least!) needs comments on what it is.

actually, the overall lack of comments tends me to r- this patch...

@@ +118,5 @@
>  
> +private:
> +  ~TCPSocket();
> +
> +  void InitWithBridge(TCPSocketChild* aBridge);

InitWithSocketBridgeChild or InitWithSocketChild

@@ +135,5 @@
>    nsString mHost;
>    uint16_t mPort;
>    bool mSsl;
>  
> +  nsRefPtr<TCPSocketChild> mSocketBridge;

mSocketBridgeChild please

@@ +162,2 @@
>    uint32_t mSuspendCount;
> +  uint32_t mTrackingNumber;

comment what these are (rich, please)

::: dom/network/TCPSocketChild.cpp
@@ +133,5 @@
>    } else if (aData.type() == CallbackData::TSendableData) {
>      const SendableData& data = aData.get_SendableData();
>  
> +    AutoJSAPI api;
> +    api.Init(mSocket->GetOwner());

Where the GetOwner comes from?  It's a WEBIDL thing?  (just asking from curiosity ;)

::: dom/network/TCPSocketParent.h
@@ +22,5 @@
>  namespace dom {
>  
> +class TCPSocket;
> +
> +class TCPSocketParentBase : public nsISupports

do we still need this Base dance? (I don't remember why this has been introduced, probably something IPDL derivation..)

applies to TCPSocketChildBase as well

	Josh Matthews [:jdm] Assignee
	Comment 72 • 9 years ago

(In reply to Andrew Sutherland [:asuth] from comment #67)
> Comment on attachment 8632302 [details] [diff] [review]
> Part 2: Convert TCPServerSocket to WebIDL and rewrite in C++
> 
> Review of attachment 8632302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/tests/test_tcpsocket_client_and_server_basics.js
> @@ +174,2 @@
> >    // - Start up a listening socket.
> > +  let listeningServer = new mozTCPServerSocket(serverPort,
> 
> Could we use StaticClassOverride
> (https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Static_Members) in order to keep
> navigator.mozTCPSocket.listen() around for backwards-compatibility reasons?
> 
> (Specifically, I don't think it's a deal-breaker to change the API, but if
> WebIDL can easily let us keep the signature around, it seems friendlier to
> do that.)

If that worked (haven't tried it), that would allow window.mozTCPServerSocket.listen(...), but navigator.mozTCPSocket would be undefined. I don't think it's worth going to the trouble of making that possible.

	Andrew Sutherland [:asuth] (he/him)
	Comment 73 • 9 years ago

(In reply to Josh Matthews [:jdm] from comment #72)
> If that worked (haven't tried it), that would allow
> window.mozTCPServerSocket.listen(...), but navigator.mozTCPSocket would be
> undefined. I don't think it's worth going to the trouble of making that
> possible.

Hm, so, looking at https://thecount.paas.allizom.org/#/listing/permission/tcp-socket (which may be out of date), I'm wondering whether we should actually make an effort to be backwards-compatible in both the client (navigator.mozTCPSocket.open) and server cases (navigator.mozTCPServerSocket.listen) in the interest of not being jerky to those who have bothered to actually write FxOS-specific apps.

Currently we're going to break every consumer of TCPSocket/TCPServerSocket without otherwise changing the API.  I presume the intent in comment 4 when initially doing this was that we would evolve to conform to http://www.w3.org/2012/sysapps/tcp-udp-sockets/#interface-tcpsocket.  But from mailing list discussions, :sicking has indicated it doesn't make sense for us to do that unless other implementations also conform.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 74 • 9 years ago

FWIW, I propose that we don't change the API at all, unless really necessary.

I think aligning with the W3C API is going to provide very few advantages in practice.

If we're going to change the API, I'd rather change it to the one in [1]. But that seems better done as a separate bug.

[1] http://plugins.cordova.io/#/package/org.chromium.socket

	Josh Matthews [:jdm] Assignee
	Comment 75 • 9 years ago

I thought I could do it by adding [NavigatorProperty="mozTCPSocket"] and a static open method to TCPSocket.webidl, but the first is only supported for JS-implemented WebIDL right now :( If I keep around nsIDOMTCPSocket for its navigator property, I think I'm still left with the problem of obtaining a window from an JS scope where none is available.

	Andrew Sutherland [:asuth] (he/him)
	Comment 76 • 9 years ago

What if we have a throwaway JS interface implementation to provide the back-compat for the main thread?  The constructor args would have to be redundantly defined, but I assume the returned socket types could be reused.  And on workers I think it's okay for the "new" style to be used.  (Get it?)

Window-wise, do you mean that we still need to use nsIDOMGlobalPropertyObserver because no one else will give us the window explicitly (and implicit_jscontext is an XPCOM thing)?  I think we'd at least escape inner-window-destroyed watching?

	Josh Matthews [:jdm] Assignee
	Comment 77 • 9 years ago

I've got a solution; I've tested and it appears to work, so there shouldn't need to be any API breakage. Hooray!

	Josh Matthews [:jdm] Assignee
	Comment 78 • 9 years ago

Rebased and review comments addressed.

	Josh Matthews [:jdm] Assignee
	Comment 79 • 9 years ago

Rebased and review comments addressed.

	Josh Matthews [:jdm] Assignee
	Comment 80 • 9 years ago

Rebased and review comments addressed.

	Josh Matthews [:jdm] Assignee
	Comment 81 • 9 years ago

Comment on attachment 8640040 [details] [diff] [review]
Part 1: Convert TCPSocket to WebIDL and rewrite in C++

Boris, could you review the WebIDL changes? Both the introduction of TCPSocket.webidl and the LegacyMozTCPSocket interface used for the Navigator changes.

	Josh Matthews [:jdm] Assignee
	Comment 82 • 9 years ago

Comment on attachment 8640041 [details] [diff] [review]
Part 2: Convert TCPServerSocket to WebIDL and rewrite in C++

Same here - introduction of TCPServerSocket.webidl and update to the legacy interface.

	Boris Zbarsky [:bzbarsky]
	Comment 83 • 9 years ago

Comment on attachment 8640040 [details] [diff] [review]
Part 1: Convert TCPSocket to WebIDL and rewrite in C++

>+    'implicitJSContext': [ 'send' ],

The method uses a mix of aCx and api.cx(); just pick one, please.

>+++ b/dom/webidl/TCPSocket.webidl
>+[Constructor(DOMString host, unsigned short port, optional SocketOptions options),
>+ Pref="dom.mozTCPSocket.enabled", CheckAnyPermissions="tcp-socket", Exposed=(Window)]

I would vaguely prefer that these extended attributes were one per line.

Also, just Exposed=Window, no need for parens unless it's a list of more than one exposure type.

>+interface mozTCPSocket : EventTarget {

Why is this not named MozTCPSocket?

>+   * Enable secure on channel.

I'm not sure what this means.  If you know what it means, please document more clearly.  Also, document what can cause this to throw?

>+   * The host of this socket object.

In what form?

>+   * Pause reading incoming data and invocations of the ondata handler until
>+   * resume is called.
>+   */
>+  void suspend();

Can you suspend() and already-suspended socket?  Is it a boolean or a counter?

>+  [Throws]
>+  void resume();

When does it throw?

>+   * @param data The data to write to the socket. If
>+   *             binaryType: "arraybuffer" was passed in the options
>+   *             object, then this object should be an ArrayBuffer instance.
>+   *             If binaryType: "string" was passed, or if no binaryType
>+   *             option was specified, then this object should be an
>+   *             ordinary JavaScript string.

And if the wrong thing is passed this will throw?  Document.

>+   * @param byteOffset The offset within the data from which to begin writing.
>+   *                   Has no effect on non-ArrayBuffer data.
>+   * @param byteLength The number of bytes to write. Has no effect on
>+   *                   non-ArrayBuffer data.

In that case, it may make sense to write this as two overloads: one that takes three args (last two optional) and an ArrayBuffer as first arg, one that has a string as the only arg.  Then you wouldn't need to MOZ_ASSERT things that JS code can do don't happen.

Also, it's generally considered a good idea to use BufferSource, not ArrayBuffer, for cases like this; that allows consumers to pass in an ArrayBufferView of their choice.... but this API already has the nasty manual offset/length bits, so maybe we should just leave that be.  :(

>+  boolean send((USVString or ArrayBuffer) data, optional unsigned long byteOffset, optional unsigned long byteLength);

Why USVString?  Isn't this being used to represent random bytes?  Shouldn't it be more like ByteString or something?  Or DOMString?

>+  readonly attribute DOMString readyState;

Any reason this is not an IDL enum?

>+   * The onopen event handler is called 

More precisely, the "open" event is dispatched, right?

> onerror will be called, instead.

"error" will be dispatched, instead.

>+   * ondrain will be called

The "drain" event will be dispatched?

>+   * The ondata handler will be called repeatedly and asynchronously after
>+   * onopen has been called

The "data" event will be dispatched ... after "open" has been dispatched.

>+   * The onerror handler will be called when there is an error. The data


The "error" event will be dispatched....

>+   * If onerror is called before onopen, the error was connection refused,
>+   * and onclose will not be called. If onerror is called after onopen,
>+   * the connection was lost, and onclose will be called after onerror.

And fix these bits too.

>+   * The onclose handler is called once the underlying network socket

And this.

>+   * If onerror was not called before onclose, then either side cleanly

And this.

>+++ b/dom/webidl/TCPSocketEvent.webidl
>+/**
>+ * TCPSocketEvent is the event object which is passed as the
>+ * first argument to all the event handler callbacks.

I assume it's rather the event that's dispatched for all the events TCPSocket describes.

>+   * The data related to this event, if any. In the ondata callback,

 ... if this event is a "data" event ...

and similar throughout here.

>+  //TODO: make this (ArrayBuffer or DOMString) after sorting out the rooting required.

You mean rooting in the event codegen?  Is there a bug filed?  Please reference it here.  Also, at least in some cases it's a DOMError?  Need to document that... and ideally not use DOMError here unless that's what the old code did and we're just keeping compat.

>+TCPSocket::OnDataAvailable(nsIRequest* aRequest, nsISupports* aContext, nsIInputStream* aStream,
>+    nsString wide_data = NS_ConvertUTF8toUTF16(data);

Why swould "data" be UTF-8, exactly?  Isn't it bytes?

You seem to have some cases (like in this method) where you're not checking the return value of AutoJSAPI::Init.  You need to fix that.

>+TCPSocket::FireDataEvent(const nsAString& aType, JS::Handle<JS::Value> aData)
>+  TCPSocketEventInit init;

This will fail the rooting hazard analysis, I'd hope.  Needs to be a RootedDictionary, since it holds a gcthing.

>+TCPSocket::Constructor(GlobalObject& aGlobal,
>+    aRv = rv;

aRv.Throw(rv) please.

I don't see a LegacyMozTCPSocket in here, which makes it rather hard to review....

	Josh Matthews [:jdm] Assignee
	Comment 84 • 9 years ago

Now including LegacyMozTcpSocket changes.

	Boris Zbarsky [:bzbarsky]
	Comment 85 • 9 years ago

Comment on attachment 8640041 [details] [diff] [review]
Part 2: Convert TCPServerSocket to WebIDL and rewrite in C++

>+'mozTCPServerSocket' : {

Again, why lowercase 'm'?

>+enum TCPServerSocketBinaryType {

Is there a reason to not just reuse TCPSocketBinaryType here?

>+ Pref="dom.mozTCPSocket.enabled", CheckAnyPermissions="tcp-socket", Exposed=(Window)]

Same comments as for the other patch.  For the event interface too.

>+   * The onconnect event handler is called when a client connection is accepted.

You know the drill with these eventhandler comments.  ;)

>+   * The onerror handler will be called when the listen of a server socket is aborted.

And here.

r=me with those nits.

	Boris Zbarsky [:bzbarsky]
	Comment 86 • 9 years ago

Comment on attachment 8640109 [details] [diff] [review]
Part 1: Convert TCPSocket to WebIDL and rewrite in C++

>+    'register': False,

Why not just make it [NoInterfaceObject] instead?  Seems more correct to me....

>+interface nsITCPSocketInternal;

This is now dead code and should go away, right?

>+  static mozTCPSocket open(DOMString host, unsigned short port, optional SocketOptions options);

I don't see how this can work.  "static" means the method appears on the interface object, but the getter on Navigator returns an instance of the interface.  It won't have an open() method.  Are there tests for this working?

	Josh Matthews [:jdm] Assignee
	Comment 88 • 9 years ago

Folded with the interdiff.

	Josh Matthews [:jdm] Assignee
	Comment 89 • 9 years ago

I accidentally didn't make an interdiff for this. Luckily the changes requested were few\!

	Josh Matthews [:jdm] Assignee
	Comment 90 • 9 years ago

Part 3 needed some significant changes to accommodate the comments for part 1, unfortuantely.

	Josh Matthews [:jdm] Assignee
	Comment 91 • 9 years ago

Folder with the interdiff.

	Josh Matthews [:jdm] Assignee
	Comment 92 • 9 years ago

The interdiff for part 3 is unfortunately terrible because I rebased and then had merge errors that couldn't be resolved easily due to the changes from part 1 that were already in place.

	Josh Matthews [:jdm] Assignee
	Comment 93 • 9 years ago

Comment on attachment 8645947 [details] [diff] [review]
Part 2: Convert TCPServerSocket to WebIDL and rewrite in C++

Sorry, forgot the interdiff here but the requested changes were really small.

	Nils Ohlmeier [:drno]
	Comment 94 • 9 years ago

Josh is part 4 missing on purpose? I'm mostly asking because part 5 does not apply cleanly for me on top of part 3.

	Josh Matthews [:jdm] Assignee
	Comment 95 • 9 years ago

Part 5 should be renumbered as part 4. I haven't bothered updated it since I originally wrote it, but it's strictly removing now-unused code.

	Boris Zbarsky [:bzbarsky]
	Comment 96 • 9 years ago

I'm sorry for the horrible lag here.  I'm hoping to get to this on Tuesday; it definitely won't happen before then.

	Boris Zbarsky [:bzbarsky]
	Comment 97 • 9 years ago

Comment on attachment 8645944 [details] [diff] [review]
Part 1 interdiff

Sorry for the lag; I had to page this stuff in.  :(

>+++ b/dom/webidl/TCPSocket.webidl
>+  boolean send(DOMString data);

Document how the DOMString will be converted to actual bytes on the network?  More on this below; I don't think it makes sense as implemented.

>+  boolean send(ArrayBuffer data, optional unsigned long byteOffset, optional unsigned long byteLength);

Document what happens with byteOffset/byteLength if not passed?

Looking at the implementation, you could do "optional unsigned long byteOffset = 0" in the IDL and make that part self-documenting, but byteLength will need documentation, no way around it.

>    * has been established. If the connection is refused, onerror will be

"the error event will be dispatched".  This was in my original review comments...

The documentation for TCPSocketEvent is still bogus in exactly the way it was in the patch I last reviewed.  Please fix it.

Does something document when TCPSocketErrorEvent instances are dispatched?

>+++ b/dom/network/TCPSocket.cpp
>+  if (NS_WARN_IF(!api.Init(mGlobal))) {
>+    return nullptr;

You need to throw on aRv.  Otherwise caller will crash with a null-deref...

>+TCPSocket::FireErrorEvent(const nsACString& aName, const nsACString& aType)

Is there a reason the caller can't pass in UTF-16 strings?  But anyway....

>+  init.mName = NS_ConvertUTF8toUTF16(aName);

  CopyUTF8toUTF16(aName, init.mName);

Similar for mMessage.

>+TCPSocket::Send(const nsAString& aData, ErrorResult& aRv)
>+  nsCString cstr = NS_ConvertUTF16toUTF8(aData);

This may not make sense.  Especially when the passed-in data is random binary as a JS string, not actual Unicode text...  Why is this a sensible thing to do?

>+  stream->SetData(cstr.get(), byteLength);

This _definitely_ doesn't make sense.  byteLength is the length of the original nsAString data in UTF-16 code units.  It has nothing to do with the length of anything in cstr, which may be either larger or smaller.  Please fix this!

>+TCPSocket::Send(const ArrayBuffer& aData,
>+  stream->SetData(value, byteOffset, byteLength, api.cx());

This happens to work because the callee never uses the cx compartment for anything, but it would be safer to enter the compartment of our object before making this call....

Also, perhaps assert if this call fails?

> class LegacyMozTCPSocket
>+  nsCOMPtr<nsIGlobalObject> mGlobal;

Don't you need to cycle-collect that?

>+++ b/dom/webidl/Navigator.webidl
>+/*partial interface Navigator {

Why is this commented out?  Is it getting uncommented in a later patch or something?

r=me assuming the above are sorted out, but I'd like to see the resulting interdiff for sure.

	Boris Zbarsky [:bzbarsky]
	Comment 98 • 9 years ago

Comment on attachment 8645947 [details] [diff] [review]
Part 2: Convert TCPServerSocket to WebIDL and rewrite in C++

>-/*partial interface Navigator {

Should this uncommenting happen here or in part 1?

>+++ b/dom/webidl/TCPServerSocket.webidl
>+ * mozTCPServerSocket

Drop the "moz"?

>+   * The "error" event will be dispatched when the listen of a server socket is aborted.

Bonus points for replacing "the listen" with something that is actually English?  ;)

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 99 • 9 years ago

Comment on attachment 8645952 [details] [diff] [review]
Interdiff for part 3

I assume here I'm mostly looking at the webidl-related changes, not at all the actual IPC stuff?

> TCPSocket::OnDataAvailable(nsIRequest* aRequest, nsISupports* aContext, nsIInputStream* aStream,
>+  nsString wide_data = NS_ConvertUTF8toUTF16(data);

What guarantees that the incoming bytes are UTF-8 text?

r=me with that explained or (more likely) fixed.

	Josh Matthews [:jdm] Assignee
	Comment 100 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f20131460ca5

	Josh Matthews [:jdm] Assignee
	Comment 101 • 9 years ago

Interdiff for changes to patch 1.

	Josh Matthews [:jdm] Assignee
	Comment 102 • 9 years ago

Comment on attachment 8663155 [details] [diff] [review]
Interdiff #2 for patch 1.

This bug is apparently blocking work that is supposed to land before merge day, so your expediency is appreciated!

	Josh Matthews [:jdm] Assignee
	Comment 103 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=881cd83c738f

	Boris Zbarsky [:bzbarsky]
	Comment 104 • 9 years ago

Comment on attachment 8663155 [details] [diff] [review]
Interdiff #2 for patch 1.

>-        errorType.AssignLiteral("SecurityProtocol");
>+        errorType.AssignASCII("SecurityProtocol");

AssignLiteral should work on nsString too; you should be able to undo this change and the others in this function.

> TCPSocket::Send(const ArrayBuffer& aData,
>+  MOZ_ASSERT(NS_SUCCEEDED(rv));

Why is that OK to assert?  Please at least document.  Though given this is a throwing method anyway, you could just throw if rv is failure...

>+++ b/dom/webidl/TCPSocket.webidl
>+   *                   Defaults to the byte length of the ArrayBuffer if not present.

Should probably also mention that it's clamped to (length - offset)?

>+   *  will be dispatched, instead.

Extra space before "will" there.

>+++ b/dom/webidl/TCPSocketEvent.webidl
>+ Exposed=(Window)]

Drop the parens.

r=me

	Josh Matthews [:jdm] Assignee
	Comment 105 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8d9233a7bd5ea2e7a1a7916dd1a28021bca7f8
Bug 885982 - Part 1: Convert TCPSocket to WebIDL and rewrite in C++. r=asuth,mayhemer,bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad05ac8aac349d23626add9ffd85cbd06bd4de5
Bug 885982 - Part 2: Convert TCPServerSocket to WebIDL and rewrite in C++. r=asuth,mayhemer,bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/7877ab6c64ca511513120de41818fc97162ddc5d
Bug 885982 - Part 3: Add e10s support to TCPSocket and TCPServerSocket. r=asuth,mayhemer,bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6895a6883048a7166d9ce5e3917da27f3fb67c7
Bug 885982 - Part 4: Remove all traces of JS implementation. r=asuth

	Ekanan Ketunuti
	Comment 106 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/bf8d9233a7bd
https://hg.mozilla.org/mozilla-central/rev/2ad05ac8aac3
https://hg.mozilla.org/mozilla-central/rev/7877ab6c64ca
https://hg.mozilla.org/mozilla-central/rev/e6895a688304