Worker - Support TCPSocket on workers

NEW
Unassigned

Status

()

defect
6 years ago
a month ago

People

(Reporter: dougt, Unassigned)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 20 obsolete attachments)

34.01 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

6 years ago
This will necessitate a rewrite of TCPSocket.js in C++. I am saddened but resigned.
Assignee: nobody → josh
Summary: Worker - Support TCP Socket on workers → Worker - Support TCPSocket on workers
Required for supporting RTMP in an OMTShumway world
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Shumway:m2]
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.
Posted patch WIP (obsolete) — Splinter Review
wip
This patch passes the included test. There are many things about it that make me uncomfortable, but it's a good start.
Attachment #8346692 - Attachment is obsolete: true
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.
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.
Attachment #8349237 - Attachment is obsolete: true
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.
"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.
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.
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?
Note to self: the test in this patch currently leaks.
Whiteboard: [Shumway:m2] → [shumway]
Attachment #8350158 - Attachment is obsolete: true
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?
Flags: needinfo?(amarchesini)
Note to self: investigate the XXXjdm regarding permissions in TCPSocket.js.
(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
Flags: needinfo?(amarchesini)
Actually... all your attached patches are magically disappeared. Mark me as reviewer when they will come back :)
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.
Attachment #8389541 - Flags: review?(amarchesini)
Attachment #8389539 - Flags: review?(amarchesini)
Attachment #8389538 - Flags: review?(amarchesini)
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.
Attachment #8389539 - Flags: review?(khuey)
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.
(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().
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 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.
Attachment #8389538 - Flags: review?(amarchesini) → review+
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? :)
Attachment #8389539 - Flags: review?(amarchesini)
No longer blocks: 949325
Attachment #8393968 - Attachment is obsolete: true
Finally all tests in dom/network/tests pass without leaks. I'll start addressing the prior feedback now; thanks!
Attachment #8389538 - Attachment is obsolete: true
Attachment #8389542 - Attachment is obsolete: true
Attachment #8394340 - Attachment is obsolete: true
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?
Flags: needinfo?(amarchesini)
Same question; should I upload my new version of part 2 or would that mess up the open review?
Flags: needinfo?(khuey)
I haven't looked at it yet.
Flags: needinfo?(khuey)
(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.
Flags: needinfo?(amarchesini)
Attachment #8389539 - Attachment is obsolete: true
Attachment #8389539 - Flags: review?(khuey)
Attachment #8389541 - Attachment is obsolete: true
Attachment #8389541 - Flags: review?(amarchesini)
Attachment #8395235 - Flags: review?(amarchesini)
Attachment #8395234 - Flags: review?(amarchesini)
Attachment #8395234 - Flags: review?(khuey)
(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?
Attachment #8395175 - Flags: review?(amarchesini)
Attachment #8395174 - Flags: review?(dpreston)
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?
Attachment #8395234 - Flags: review?(amarchesini)
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
Attachment #8395235 - Flags: review?(amarchesini) → review+
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
Attachment #8395175 - Flags: review?(amarchesini) → review+
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?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (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?
Attachment #8395174 - Flags: review?(dpreston)
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.
Attachment #8395234 - Flags: review?(khuey) → review?(bent.mozilla)
Note to self: make the test load a subworker; make close unpin; fix the fun tryserver results.
(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.
(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 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.
Attachment #8395234 - Flags: review?(bent.mozilla)
Attachment #8394372 - Attachment is obsolete: true
Attachment #8395234 - Attachment is obsolete: true
Attachment #8395235 - Attachment is obsolete: true
Attachment #8395174 - Attachment is obsolete: true
Attachment #8395175 - Attachment is obsolete: true
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?
(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.
Now with attempted fixes for the release race and invalid intermediary object: https://tbpl.mozilla.org/?tree=Try&rev=5f181f934726
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 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.
Attachment #8430277 - Flags: review?(khuey)
Whoops, didn't include the most recent fixes for wrong-thread refcounting.
Attachment #8440774 - Flags: review?(khuey)
Attachment #8430277 - Attachment is obsolete: true
Attachment #8430277 - Flags: review?(khuey)
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.
Attachment #8430281 - Flags: review?(pwang)
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).
Attachment #8430281 - Flags: review?(pwang)
No longer blocks: shumway-1.0
Whiteboard: [shumway]
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.
Attachment #8440774 - Flags: review?(khuey)
(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)?
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.
Checkpointing my current work. There are still problems with the server socket having to do some work on the main thread.
Attachment #8430276 - Attachment is obsolete: true
Attachment #8430278 - Attachment is obsolete: true
Attachment #8430281 - Attachment is obsolete: true
Attachment #8430283 - Attachment is obsolete: true
Attachment #8440774 - Attachment is obsolete: true
(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)
Flags: needinfo?(josh)
PBackground didn't exist when any of this was written.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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!
Flags: needinfo?(josh)

Updated

3 years ago
See Also: → 1247628
Assignee: josh → nobody
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.