916199 - Worker - Support TCPSocket on workers

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Comment 1

[Josh Matthews [:jdm]]

This will necessitate a rewrite of TCPSocket.js in C++. I am saddened but resigned.

Comment 2

[Till Schneidereit [:till]]

Required for supporting RTMP in an OMTShumway world

Comment 3

[Josh Matthews [:jdm]]

Having considered further, I don't think rewriting in C++ will gain us anything. We're going to require proxying to the main thread anyways, since we use Necko for the actual socket stuff, so we might as well just provide an extremely basic worker-side C++ shim that just hides the proxying.

Comment 4

[Josh Matthews [:jdm]]

Attachment: WIP

wip

Comment 5

[Josh Matthews [:jdm]]

Attachment: Proxy all off-main thread TCPSocket usage to the main thread to support workers. WIP

This patch passes the included test. There are many things about it that make me uncomfortable, but it's a good start.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8349237 [details] [diff] [review]
Proxy all off-main thread TCPSocket usage to the main thread to support workers. WIP

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

Is keeping objects on the worker alive while the main thread is doing stuff one of the things making you uncomfortable? ;)

::: dom/bindings/Bindings.conf
@@ +824,5 @@
> +    'nativeType': 'mozilla::dom::TCPSocketStub',
> +    'headerFile': 'mozilla/dom/network/TCPSocketStub.h',
> +    'implicitJSContext': ['host', 'port', 'ssl', 'bufferedAmount',
> +                          'suspend', 'resume', 'close', 'send',
> +                          'readyState', 'binaryType']

since both entries match and you aren't using the worker binding in code, you don't need this workers entry

::: dom/network/src/TCPSocketStub.cpp
@@ +494,5 @@
>  {
> +  if (NS_IsMainThread()) {
> +    return mozTCPSocketBinding::Wrap(aCx, aScope, this);
> +  }
> +  return mozTCPSocketBinding_workers::Wrap(aCx, aScope, this);

using mozTCPSocketBinding::Wrap in both cases should work.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Proxy all off-main thread TCPSocket usage to the main thread to support workers. WIP

Now with slightly better worker-owned lifetime management. Still various usages of the JSAPI that need vetting, and the new test leaks a bit more than it used it.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

At some point we should use Bent's new off-main-thread-thread which allows not bouncing through the main thread in the child process. And if we take advantage of the fact that necko these days can deliver OnDataAvailable calls to threads other than the main thread, it would allow us to avoid having the transmitted data ever touching the main thread in any process.

But maybe it's better to rewrite to use that once we change up the API to match with the spec. I don't think the spec is quite stable enough for that to make sense yet.

Comment 9

[Josh Matthews [:jdm]]

"off-main-thread-thread"? Is that a typo, or is there some reading I can do that will help me understand better? Off-main-thread OnDataAvailable won't help us without rewriting at least some part of the implementation in C++, since we can't run the JS socket listener on the worker thread.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's not a typo. It's a mechanism for child processes to talk to the parent process without going through the main thread of either process. You'll want to talk to bent. I'm not sure how much of it has landed yet.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But yes, my point is that we'll need to rewrite the TCPSocket API in C++ at some point to take advantage of this. Aren't we already bouncing back and forth between C++ and JS a bunch in order to do IPC?

Comment 12

[Josh Matthews [:jdm]]

Note to self: the test in this patch currently leaks.

Comment 13

[Josh Matthews [:jdm]]

Attachment: Part 1: Add TCPSocket C++ shim.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers.

Comment 15

[Josh Matthews [:jdm]]

Attachment: Part 3: Make OOP TCPSocket work again.

Comment 16

[Josh Matthews [:jdm]]

Attachment: Part 4: Merge inproc/oop/worker TCPSocket tests.

Comment 17

[Josh Matthews [:jdm]]

This is very close. I need to clean up the many commented out lines in the test harness, but it's definitely good to checkpoint right now. Andrea, would you be interested in reviewing here?

Comment 18

[Josh Matthews [:jdm]]

Note to self: investigate the XXXjdm regarding permissions in TCPSocket.js.

Comment 19

[Andrea Marchesini [:baku]]

(In reply to Josh Matthews [:jdm] from comment #17)
> This is very close. I need to clean up the many commented out lines in the
> test harness, but it's definitely good to checkpoint right now. Andrea,
> would you be interested in reviewing here?

Sure! Reviewing

Comment 20

[Andrea Marchesini [:baku]]

Actually... all your attached patches are magically disappeared. Mark me as reviewer when they will come back :)

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm more than happy for Andrea to review this first (yay less work for me) but the worker thread proxy needs to be reviewed by bent or me before it lands too.

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 8389539 [details] [diff] [review]
Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers.

This is the patch with the interesting worker interactions.

Comment 23

[Gene Lian (I already quit Mozilla)]

Just a drive-by comment: bug 949325 (part 2-2 patch) already introduced the WorkerMainThreadRunnable which has a better naming than the SyncWorkerRunnable in your patch. You may work out patches on top of that.

Comment 24

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #23)
> Just a drive-by comment: bug 949325 (part 2-2 patch) already introduced the
> WorkerMainThreadRunnable which has a better naming than the
> SyncWorkerRunnable in your patch. You may work out patches on top of that.

It also has a better return type for MainThreadRun().

Comment 25

[Andrea Marchesini [:baku]]

Is this a temporary solution?
Do we want to rewrite TCPSocket in C++ and have 1 single implementation for workers and main-thread eventually?

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8389538 [details] [diff] [review]
Part 1: Add TCPSocket C++ shim.

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

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

update this UUID and the other for nsIDOMTCPSocketInternal

::: dom/network/src/TCPSocket.js
@@ +56,5 @@
>  /*
>   * Debug logging function
>   */
>  
> +let debug = true;

false... correct? :)

@@ +400,5 @@
>                         .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
>      LOG("content process: " + (this._inChild ? "true" : "false"));
>  
> +    this.owner = aOwner;
> +    let window = aOwner.ownerGlobal;

I would check if this exists.

@@ +420,4 @@
>      this.innerWindowID = util.currentInnerWindowID;
>      LOG("window init: " + this.innerWindowID);
>      Services.obs.addObserver(this, "inner-window-destroyed", true);
> +    

extra space

@@ +420,5 @@
>      this.innerWindowID = util.currentInnerWindowID;
>      LOG("window init: " + this.innerWindowID);
>      Services.obs.addObserver(this, "inner-window-destroyed", true);
> +    
> +    if (!this._hasPrivileges && false /*XXXjdm*/) {

?!? :)

@@ +445,5 @@
>                                    this._binaryType, this.useWin, this.useWin || this);
>        return;
>      }
>  
> +    let transport = this._transport = this._createTransport(aHost, aPort, this._ssl);

why this extra 'transport' variable?

::: dom/network/src/TCPSocketStub.cpp
@@ +2,5 @@
> + * 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/. */
> +
> +#include "mozilla/dom/TCPSocketBinding.h"
> +#include "TCPSocketStub.h"

mozilla/dom/network/TCPSocketStub.h

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

MOZ_ASSERT(mSocket)

@@ +106,5 @@
> +  return mSocket->Close();
> +}
> +
> +NS_IMETHODIMP
> +TCPSocketStub::Send(JS::HandleValue data, uint32_t byteOffset, uint32_t byteLength, bool* aMoreHint)

aData, aByteOffset, aByteLength

@@ +185,5 @@
> +TCPSocketStub::Send(JSContext* aCx,
> +                    JS::Rooted<JS::Value>& data,
> +                    const Optional<uint32_t>& byteOffset,
> +                    const Optional<uint32_t>& byteLength)
> +{

aDatam aByteOffset, aByteLength

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

MOZ_FINAL ?

@@ +12,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class TCPSocketStub: public nsDOMEventTargetHelper,
> +                     public nsIDOMTCPSocket {

{ in a new line

@@ +15,5 @@
> +class TCPSocketStub: public nsDOMEventTargetHelper,
> +                     public nsIDOMTCPSocket {
> +public:
> +  TCPSocketStub(nsPIDOMWindow* aWindow);
> +  virtual ~TCPSocketStub();

no virtual if MOZ_FINAL

@@ +26,5 @@
> +
> +  uint16_t Port();
> +  bool Ssl();
> +  uint32_t BufferedAmount();
> +  bool Send(JSContext* aCx, JS::Rooted<JS::Value>& data, const Optional<uint32_t>& byteOffset,

aData, ... and 80chars per line

@@ +42,5 @@
> +              const SocketOptions& aOptions, ErrorResult& aRv);
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  nsPIDOMWindow* GetParentObject() {

This is implemented by nsDOMEventTargetHelper. remove it. And then you have the parent window, you could use it.

Comment 27

[Andrea Marchesini [:baku]]

Comment on attachment 8389539 [details] [diff] [review]
Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers.

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

::: dom/network/src/TCPSocket.js
@@ +400,5 @@
>                         .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
>      LOG("content process: " + (this._inChild ? "true" : "false"));
>  
>      this.owner = aOwner;
> +    let window = aWindow;

Why this change? Cannot we get the window from the nsIDOMEventTarget?

::: dom/network/src/TCPSocketStub.cpp
@@ +131,5 @@
> +{
> +  TCPSocketStub* mSocket;
> +  nsString mType;
> +  nsString mErrorName;
> +  uint64_t* mData;

Use AutoStructuredCloneBuffer as internal member of the object.
Otherwise this object COULD leak mData.

@@ +184,5 @@
> +}
> +
> +DispatchEventRunnable::~DispatchEventRunnable()
> +{
> +  // Avoid leaking if this doesn't end up running

then remove this...

@@ +235,5 @@
> +  return NS_OK;
> +}
> +
> +mozilla::dom::EventTarget*
> +EventTargetProxy::GetTargetForDOMEvent() {

new line for {
here and everywhere.

@@ +241,5 @@
> +  return nullptr;
> +}
> +
> +mozilla::dom::EventTarget*
> +EventTargetProxy::GetTargetForEventTargetChain() {

can you make a base class for all of these?

@@ +268,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +nsresult
> +EventTargetProxy::DispatchDOMEvent(mozilla::WidgetEvent* aEvent, nsIDOMEvent* aDOMEvent, nsPresContext* aPresContext, nsEventStatus* aEventStatus)

80chars?

@@ +288,5 @@
> +  MOZ_ASSUME_UNREACHABLE("shouldn't be called on the main thread");
> +  return nullptr;
> +}
> +
> +class InitRunnable : public SyncWorkerRunnable

MOZ_FINAL ?

@@ +326,5 @@
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIDOMTCPSocket> socket = do_CreateInstance("@mozilla.org/tcp-socket-impl;1");
> +  mSocket = new nsMainThreadPtrHolder<nsIDOMTCPSocket>(socket);

MOZ_ASSERT(socket) before doing this

@@ +343,5 @@
>  {
> +  if (NS_IsMainThread()) {
> +    Init(this, GetOwnerGlobal(), aHost, aPort, aOptions);
> +  } else {
> +    mWorkerPrivate = GetWorkerPrivateFromContext(aCx);

There is an AssertIsWorkerThread() or something.
Or just MOZ_ASSERT(mWorkerPrivate)

@@ +680,5 @@
> +  , mDataLen(0)
> +  , mByteOffset(aByteOffset)
> +  , mByteLength(aByteLength)
> +  {
> +    JSAutoStructuredCloneBuffer buffer;

I would prefer buffer as internal member.

@@ +715,3 @@
>  bool
>  TCPSocketStub::Send(JSContext* aCx,
> +                    const JS::Rooted<JS::Value>& data,

aData, ..

::: dom/network/src/TCPSocketStub.h
@@ +40,2 @@
>              const Optional<uint32_t>& byteLength);
> +  bool Send(const JS::Handle<JS::Value>& data, uint32_t byteOffset, uint32_t byteLength);

aData,..

::: dom/network/tests/tcpsocket_worker.js
@@ +187,5 @@
> +
> +  /*server.onaccept = yayFuncs.serveropen;
> +  server.ondata = makeFailureCase('serverdata');
> +  server.onclose = makeFailureCase('serverclose');*/
> +    

extra spaces

@@ +337,5 @@
> +      "ondata2", BIG_TYPED_ARRAY_2, false, yays.ondata2);*/
> +  }
> +  pendingSynchronizeCallback = function() {
> +    pendingSynchronizeCallback = null;
> +    

extra spaces

::: dom/workers/SyncWorkerRunnable.h
@@ +11,5 @@
> +#include "mozilla/dom/WorkerRunnable.h"
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +// Base class for the URL runnable objects.

URL? :)

Comment 28

[Josh Matthews [:jdm]]

Attachment: Part 5: Convert TCPServerSocket to WebIDL.

Comment 29

[Josh Matthews [:jdm]]

Attachment: Part 5: Convert TCPServerSocket to WebIDL.

Comment 30

[Josh Matthews [:jdm]]

Finally all tests in dom/network/tests pass without leaks. I'll start addressing the prior feedback now; thanks!

Comment 31

[Josh Matthews [:jdm]]

Attachment: Part 1: Add TCPSocket C++ shim.

Comment 32

[Josh Matthews [:jdm]]

Attachment: Part 4: Merge inproc/oop/worker TCPSocket tests.

Comment 33

[Josh Matthews [:jdm]]

Attachment: Part 5: Convert TCPServerSocket to WebIDL.

Comment 34

[Josh Matthews [:jdm]]

So I have thoroughly rebased everything, and built them on top of patch 2-2 in bug 949325 as Gene suggested. I've locally addressed all review comments so far; Andrea, would you like me to upload my new part 3 or wait until the open review is complete?

Comment 35

[Josh Matthews [:jdm]]

Same question; should I upload my new version of part 2 or would that mess up the open review?

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I haven't looked at it yet.

Comment 37

[Andrea Marchesini [:baku]]

(In reply to Josh Matthews [:jdm] from comment #34)
> So I have thoroughly rebased everything, and built them on top of patch 2-2
> in bug 949325 as Gene suggested. I've locally addressed all review comments
> so far; Andrea, would you like me to upload my new part 3 or wait until the
> open review is complete?

Yes please.

Comment 38

[Josh Matthews [:jdm]]

Attachment: Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers.

Comment 39

[Josh Matthews [:jdm]]

Attachment: Part 3: Make OOP TCPSocket work again.

Comment 40

[Josh Matthews [:jdm]]

(In reply to Andrea Marchesini (:baku) from comment #27)
> ::: dom/network/src/TCPSocket.js
> @@ +400,5 @@
> >                         .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
> >      LOG("content process: " + (this._inChild ? "true" : "false"));
> >  
> >      this.owner = aOwner;
> > +    let window = aWindow;
> 
> Why this change? Cannot we get the window from the nsIDOMEventTarget?

No, in the case of passing the EventTargetProxy which doesn't have an ownerGlobal (it only implements nsIDOMEventTarget).
 
> @@ +241,5 @@
> > +  return nullptr;
> > +}
> > +
> > +mozilla::dom::EventTarget*
> > +EventTargetProxy::GetTargetForEventTargetChain() {
> 
> can you make a base class for all of these?

I don't understand this request. Do we intend to reuse this concept of a cross-thread event target proxy somewhere else?

Comment 41

[Andrea Marchesini [:baku]]

Comment on attachment 8395234 [details] [diff] [review]
Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers.

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

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +18,2 @@
>  
>  [scriptable, uuid(bcc2e6c4-0f10-4093-83f0-a40f71cc8631)]

update the UUID

::: dom/network/src/TCPSocketStub.cpp
@@ +73,5 @@
> +
> +EventTargetProxy::EventTargetProxy(WorkerPrivate* aWorker,
> +                                   TCPSocketStub* aSocket)
> +: mWorker(aWorker)
> +    , mSocket(aSocket)

indentation

@@ +153,5 @@
> +{
> +  AutoSafeJSContext cx;
> +  JS::Rooted<JS::Value> data(cx, aEvent->Data(cx));
> +  Maybe<JSAutoCompartment> ac;
> +  Maybe<JS::Rooted<JSObject*>> obj;

why do you need Maybe?

@@ +187,5 @@
> +    bool ok = mData.read(aCx, &data);
> +    MOZ_ASSERT(ok);
> +  } else {
> +    nsRefPtr<DOMError> error = new DOMError(nullptr, mErrorName);
> +    JS::Rooted<JSObject*> scope(aCx, aWorkerPrivate->GlobalScope()->GetGlobalJSObject());

Maybe you should enter in the compartment of this global JSObject. But I'm not 100% sure. khuey will know more.

@@ +191,5 @@
> +    JS::Rooted<JSObject*> scope(aCx, aWorkerPrivate->GlobalScope()->GetGlobalJSObject());
> +    data = JS::ObjectValue(*error->WrapObject(aCx, scope));
> +  }
> +
> +  TCPSocketEventInit init;

whouls be nice to have a constructor that takes mbuubles, mcancelable data, etc

@@ +557,5 @@
> +  bool mSsl;
> +};
> +
> +bool
> +TCPSocketStub::Ssl(JSContext *aCx)

const ?

@@ +572,2 @@
>  bool
>  TCPSocketStub::Ssl()

const?

::: dom/network/src/TCPSocketStub.h
@@ +34,1 @@
>    uint16_t Port();

why this?

@@ +34,2 @@
>    uint16_t Port();
>    bool Ssl();

and this?

Comment 42

[Andrea Marchesini [:baku]]

Comment on attachment 8395235 [details] [diff] [review]
Part 3: Make OOP TCPSocket work again.

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

::: dom/network/interfaces/nsITCPSocketParent.idl
@@ +12,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(868662a4-681c-4b89-9f02-6fe5b7ace265)]

UUID :)

::: dom/network/src/TCPSocketStub.h
@@ +68,5 @@
>    virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
>  
>    void Init(nsIDOMEventTarget* aEventTarget, nsIDOMWindow* aWindow, const nsAString& aHost,
> +            uint16_t aPort, const SocketOptions& aOptions, bool aAssumePrivilege);
> +  void Init(JSContext* aCx, const nsAString& aHost, uint16_t aPort, const SocketOptions& aOptions, bool aAssumePrivilege);

80chars

::: dom/network/tests/chrome.ini
@@ +8,3 @@
>  
>  [test_tcpsocket.html]
> +[test_tcpsocket_oop.html]

alphabetic order

Comment 43

[Andrea Marchesini [:baku]]

Comment on attachment 8395175 [details] [diff] [review]
Part 5: Convert TCPServerSocket to WebIDL.

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

::: dom/network/interfaces/nsIDOMTCPServerSocket.idl
@@ +18,5 @@
>     * The port of this server socket object.
>     */
>    readonly attribute unsigned short localPort;
>  
>    /**

some UUID change is missing.

::: dom/network/src/TCPSocketParentIntermediary.js
@@ +66,2 @@
>        // The members in the socket parent object are set with arguments,
>        // so that the socket parent object can communicate data 

extra space

Comment 44

[Josh Matthews [:jdm]]

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

Comment 45

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8395234 [details] [diff] [review]
Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers.

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

This seems very weird to me.  What is EventTargetProxy for?  Why don't you just add an event listener to the main thread TCP socket?

Comment 46

[Josh Matthews [:jdm]]

(In reply to Kyle Huey [:khuey] ([e-mail]) (Away 4/19-5/7) from comment #45)
> Comment on attachment 8395234 [details] [diff] [review]
> Part 2: Proxy all off-main thread TCPSocket usage to the main thread to
> support workers.
> 
> Review of attachment 8395234 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems very weird to me.  What is EventTargetProxy for?  Why don't you
> just add an event listener to the main thread TCP socket?

Given that I answered this question on IRC (it's necessary because the main thread TCPSocket object is not an event target and can't be), what's next?

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8395234 [details] [diff] [review]
Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers.

Unfortunately I'm not going to finish this before I go on PTO.

Comment 48

[Josh Matthews [:jdm]]

Note to self: make the test load a subworker; make close unpin; fix the fun tryserver results.

Comment 49

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #23)
> Just a drive-by comment: bug 949325 (part 2-2 patch) already introduced the
> WorkerMainThreadRunnable which has a better naming than the
> SyncWorkerRunnable in your patch. You may work out patches on top of that.

I just landed this. This bug can then use WorkerMainThreadRunnable.

Comment 50

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Josh Matthews [:jdm] from comment #48)
> Note to self: make the test load a subworker; make close unpin; fix the fun
> tryserver results.

Hey jdm, have you done this already? I'd rather wait until this looks good on try before reviewing.

Comment 51

[Josh Matthews [:jdm]]

Comment on attachment 8395234 [details] [diff] [review]
Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers.

That's fair. I'll re-flag you when that's done.

Comment 52

[Josh Matthews [:jdm]]

Rebased patches sent to try: https://tbpl.mozilla.org/?tree=Try&rev=fc988c15a67f

Comment 53

[Josh Matthews [:jdm]]

Test fixes: https://tbpl.mozilla.org/?tree=Try&rev=6735bab0616a

Comment 54

[Josh Matthews [:jdm]]

b2g build fix: https://tbpl.mozilla.org/?tree=Try&rev=b4160f0c768d

Comment 55

[Josh Matthews [:jdm]]

More test fixes: https://tbpl.mozilla.org/?tree=Try&rev=bdc8a5eb78eb

Comment 56

[Josh Matthews [:jdm]]

Attachment: Part 1: Add TCPSocket C++ shim

Comment 57

[Josh Matthews [:jdm]]

Attachment: Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers

Comment 58

[Josh Matthews [:jdm]]

Attachment: Part 3: Make OOP TCPSocket work again

Comment 59

[Josh Matthews [:jdm]]

Attachment: Part 4: Merge inproc/oop/worker TCPSocket tests

Comment 60

[Josh Matthews [:jdm]]

Attachment: Part 5: Convert TCPServerSocket to WebIDL

Comment 61

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8430276 [details] [diff] [review]
Part 1: Add TCPSocket C++ shim

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

::: dom/webidl/TCPSocket.webidl
@@ +20,5 @@
>    TCPSocketBinaryType binaryType = "string";
>  };
>  
>  [Constructor(DOMString host, unsigned short port, optional SocketOptions options),
> + Pref="dom.mozTCPSocket.enabled"]

Maybe you can use [CheckPermissions="tcp-socket"] and remove the hand-coded permission checking in TCPSocket.js?

Comment 62

[Josh Matthews [:jdm]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #61)
> Comment on attachment 8430276 [details] [diff] [review]
> Part 1: Add TCPSocket C++ shim
> 
> Review of attachment 8430276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/TCPSocket.webidl
> @@ +20,5 @@
> >    TCPSocketBinaryType binaryType = "string";
> >  };
> >  
> >  [Constructor(DOMString host, unsigned short port, optional SocketOptions options),
> > + Pref="dom.mozTCPSocket.enabled"]
> 
> Maybe you can use [CheckPermissions="tcp-socket"] and remove the hand-coded
> permission checking in TCPSocket.js?

Read the later patches; I add something similar in the serversocket one I believe.

Comment 63

[Josh Matthews [:jdm]]

Now with attempted fixes for the release race and invalid intermediary object: https://tbpl.mozilla.org/?tree=Try&rev=5f181f934726

Comment 64

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=4471962e2219 seems to have gotten rid of the various crashes in the mochitests. Now I just need to solve that mysterious TCPServerSocket test_interfaces.html failure that's only present on opt builds.

Comment 65

[Josh Matthews [:jdm]]

Comment on attachment 8430277 [details] [diff] [review]
Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers

All known problems that showed up on try are fixed, so a review would be useful.

Comment 66

[Josh Matthews [:jdm]]

Attachment: Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers

Whoops, didn't include the most recent fixes for wrong-thread refcounting.

Comment 67

[Josh Matthews [:jdm]]

Comment on attachment 8430281 [details] [diff] [review]
Part 4: Merge inproc/oop/worker TCPSocket tests

Patrick, Donovan has said that he does not have time to review this patch which changes the tcpsocket test harness. Is this something that you could look at? For context, bug 885982 turns the xpcshell tests into mochitests, then earlier patches here add OOP and worker tests by doing a lot of copying and pasting. This patch just consolidates all of those variations into something that resembles the browser element test harness.

Comment 68

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 8430281 [details] [diff] [review]
Part 4: Merge inproc/oop/worker TCPSocket tests

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

::: dom/network/tests/file_tcpsocket_child.js
@@ +17,5 @@
> +TYPED_DATA_ARRAY.set(DATA_ARRAY, 0);
> +
> +for (var i_big = 0; i_big < BIG_ARRAY.length; i_big++) {
> +  BIG_ARRAY[i_big] = Math.floor(i_big % 256);
> +  BIG_ARRAY_2[i_big] = Math.floor((i_big * i_big) % 256);

These are integers already, no need to get floor of them.

@@ +88,5 @@
> +      if (++gSuccessCount === names.length)
> +        run_next_test();
> +    };
> +  });
> +  return gFuncs;

As joint success functions are made global, I'd suggest that you access them through |gFuncs| instead of |yays| in each test case, since |yays| looks like a reference to a local object.

@@ +187,5 @@
> +  sock.onclose = makeFailureCase('close');
> +
> +  /*server.onaccept = yayFuncs.serveropen;
> +  server.ondata = makeFailureCase('serverdata');
> +  server.onclose = makeFailureCase('serverclose');*/    

|server| is not in this file anymore, we should just remove code.

@@ +331,5 @@
> +    //yays.ondata();
> +    /*server.ondata = makeExpectData(
> +      "ondata2", BIG_TYPED_ARRAY_2, false, yays.ondata2);*/
> +  }
> +  pendingSynchronizeCallback = function() {

This function isn't called, since the parent notifies the child with 'serverCallback' message, which doesn't make this function called.

@@ +348,5 @@
> +  server.ondata = makeExpectData(
> +    "ondata", BIG_TYPED_ARRAY, false, serverSideCallback);*/
> +
> +  sock.onclose = yays.clientclose;
> +  sock.ondrain = yays.ondrain;

The next big array is sent once server received the data, whether |ondrain| on client side is called or not. We should make sure that the first ondrain is called before starting to send next big array.

::: dom/network/tests/file_tcpsocket_parent.js
@@ +267,5 @@
> +  if (event.data.type == 'setupNextTest') {
> +    dump('setup: ' + event.data.name + '\n');
> +    if (event.data.name == 'connectSock') {
> +      server.reset();
> +      var yayFuncs = makeJointSuccess(['serveropen', 'clientopen']);

Since |makeJointSuccess| here just pass a message to notify child that one of the success functions is called, it doesn't check whether all of the functions are call on parent side, we should make name of |makeJointSuccess| reflect this fact and note that 'clientopen' isn't expected to be called on parent side (or just delete it).

Comment 69

[Josh Matthews [:jdm]]

Comment on attachment 8440774 [details] [diff] [review]
Part 2: Proxy all off-main thread TCPSocket usage to the main thread to support workers

Let us rid ourselves of this false pretense.

Comment 70

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

(In reply to Josh Matthews [:jdm] from comment #69)
> Let us rid ourselves of this false pretense.

So, I do find this pretty funny in a complicated multi-layered kind of way, but is there a plan going forward for TCPSocket?  Like are we mooting this because https://github.com/sysapps/tcp-udp-sockets is getting close to something new and fancy and we're just planning to re-do everything for that (so why review this)?

Comment 71

[Josh Matthews [:jdm]]

I just cancelled the review request because there's no real reason to review that very old patch. I have rebased patches which I'm planning to throw at him again.

Comment 72

[Josh Matthews [:jdm]]

Attachment: Add worker support for TCPSocket and TCPServerSocket. WIP

Checkpointing my current work. There are still problems with the server socket having to do some work on the main thread.

Comment 73

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Josh Matthews [:jdm] from comment #3)
> Having considered further, I don't think rewriting in C++ will gain us
> anything. We're going to require proxying to the main thread anyways, since
> we use Necko for the actual socket stuff, so we might as well just provide
> an extremely basic worker-side C++ shim that just hides the proxying.

Why not move this to PBackground (and c++) - network traffic (from non-MainThread) really shouldn't hit MainThread....  and right now it hits it on both sides I think.  We need it moved to MainThread anyways for supporting realtime/low-latency use in media/mtransport (for TURN-TCP in WebRTC)

Comment 74

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

PBackground didn't exist when any of this was written.

Comment 75

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #74)
> PBackground didn't exist when any of this was written.

Aha... Yeah I saw all the recent traffic but didn't check the date on comment 3.  Yeah, that was a while ago!

Comment 76

[Jens Stutte [:jstutte]]

This bug sounds either like a Workers or a Networking thing - if it is still an issue at all.

Comment 77

[Randell Jesup [:jesup] (needinfo me)]

TCPSocket is not really used by us that I know of; It was for B2G. perhaps they still use it? Maybe asuth knows. We do use UDPSocket (for WebRTC); that is on PBackground

Comment 78

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

The most recent interest for TCPSocket that would justify enhancements in-tree was the dweb project which has ended and which never addressed the security UX.

I'm seeing a use in the remote-debugging adb-socket but devtools' logic is usually main-thread-only, so the lack of worker support is unlikely to be a meaningful problem.

I believe Thunderbird has some uses but it's my impression that TB in general is 1) trying to move away from JS-implemented protocols to rust and 2) all the logic is main-thread-only and it would be non-trivial for them to move the logic off the main thread.

These are all privileged callers and presumably the introduction of DOM streams could allow those callers to glue TCPSocket to streams and/or to reimplement their TCP needs in terms of DOM streams.

Marking WONTFIX.