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)

Oops, that was missing TCPSocketEvent.cpp

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

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.

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.

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.

(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?

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.

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.

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.

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.

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

(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 :(

wip

wip C++ stub

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?

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

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

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.

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.

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.

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

(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.

What remains to be done here?

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

That sounds bad. What blocks fixing that?

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

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

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;

(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.

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?

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 .

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

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.

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).

Comment on attachment 8546741 [details] [diff] [review] Convert MozTCPSocket to WebIDL The efforts to maintain the JS implementation really no longer seem worth it.

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.

\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.

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?

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.)

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.

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.

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.

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.

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.

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.)

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.)

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.

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

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

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

(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.

(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.

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

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.

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?

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

Rebased and review comments addressed.

Rebased and review comments addressed.

Rebased and review comments addressed.

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.

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.

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....

Now including LegacyMozTcpSocket changes.

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.

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?

Folded with the interdiff.

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

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

Folder with the interdiff.

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.

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.

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.

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.

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

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.

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

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.

Interdiff for changes to patch 1.

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!

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

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

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