Josh Matthews [:jdm] Assignee
	Description • 10 years ago

So as to not regress the ability for privileged content to create TCP servers when we convert TCPSocket to WebIDL, we need to convert TCPServerSocket at the same time.

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

Comment on attachment 8544360 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

Andrew, you should probably take a look at these changes. In particular, note that the onconnect/onerror interface must change by necessity (we weren't passing Event objects to the callbacks previously).

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 10 years ago

Comment on attachment 8544360 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

>+TCPServerSocketParent::SocketEnabled(JSContext* aCx, JS::Handle<JSObject*> aGlobal)
>+{
>+  const char* permissions[] = {"tcp-socket", nullptr};
>+  return CheckPermissions(aCx, aGlobal, permissions) ||
>+      nsGlobalWindow::IsChromeWindow(aCx, aGlobal);
Nit, use 2 spaces for indentation (unless the whole file uses wrong indentation for some reason).


>   listen: function(aTCPServerSocketParent, aLocalPort, aBacklog, aBinaryType, aAppId) {
>-    let baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
>-    let serverSocket = baseSocket.listen(aLocalPort, { binaryType: aBinaryType }, aBacklog);
>-    if (!serverSocket)
>-      return null;
>+    let win = Services.appShell.hiddenDOMWindow;
Do we really need this part? Couldn't we live without a window here?
Couldn't you make the *Event interface exposed=(Window,System) or so if that is the reason you need a global?

>+    'TCPServerSocket.webidl',
>+    'TCPServerSocketEvent.webidl',
So these files are missing from the patch.

	Andrew Sutherland [:asuth] (he/him)
	Comment 4 • 10 years ago

Comment on attachment 8544360 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

(clearing the review request against me until the webidl files show up)

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

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8544360 [details] [diff] [review]
> Convert TCPServerSocket to WebIDL
> >   listen: function(aTCPServerSocketParent, aLocalPort, aBacklog, aBinaryType, aAppId) {
> >-    let baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
> >-    let serverSocket = baseSocket.listen(aLocalPort, { binaryType: aBinaryType }, aBacklog);
> >-    if (!serverSocket)
> >-      return null;
> >+    let win = Services.appShell.hiddenDOMWindow;
> Do we really need this part? Couldn't we live without a window here?
> Couldn't you make the *Event interface exposed=(Window,System) or so if that
> is the reason you need a global?

I got excited about this prospect, but it appears to be impossible to avoid using a real window object. You are correct that we need a global that contains the relevant event constructor, but ConstructJSBinding in BindingUtils.cpp also fails if the used global is not a window. This means that the TCPSocketParentIntermediary component that is responsible for constructing the TCPServerSocket object on the parent side for an IPC server socket is unable to do so unless it uses the hidden window.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 10 years ago

(In reply to Josh Matthews [:jdm] from comment #5)
> I got excited about this prospect, but it appears to be impossible to avoid
> using a real window object. You are correct that we need a global that
> contains the relevant event constructor, but ConstructJSBinding in
> BindingUtils.cpp also fails if the used global is not a window. 
Even now that Bug 1117167 landed?

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

That was convenient timing, I suppose. Unfortunately we can't pass around nsIGlobalObject in IDL, so that's why there are some nsISupports arguments in the new changes.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 10 years ago

Comment on attachment 8546691 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

>   callListenerError: function tss_callListenerError(message, filename, lineNumber, columnNumber) {
>-    if (this._onerror) {
>-      var type = "error";
>-      var error = new Error(message, filename, lineNumber, columnNumber);
>-
>-      this["onerror"].call(null, new TCPSocketEvent(type, this, error));
>-    }
>+    var error = new Error(message, filename, lineNumber, columnNumber);
>+    let init = {
>+      message: message,
>+      filename: filename,
>+      lineno: lineNumber,
>+      colno: columnNumber,
>+      error: error
>+    };
>+    this.__DOM_IMPL__.dispatchEvent(new this.useWin.ErrorEvent("error", init));
This is weird. Could we just have error: null; since ErrorEvent itself has all the information.

>+++ b/dom/webidl/TCPServerSocketEvent.webidl
>@@ -0,0 +1,15 @@
>+/* 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/. */
>+
>+interface nsIDOMTCPSocket;
>+
>+[Constructor(DOMString type, optional TCPServerSocketEventInit eventInitDict),
>+ Func="TCPServerSocketParent::SocketEnabled", Exposed=(Window,System)]
>+interface TCPServerSocketEvent : Event {
>+  readonly attribute nsIDOMTCPSocket socket; // mozTCPSocket
>+};
>+
>+dictionary TCPServerSocketEventInit : EventInit {
>+  nsIDOMTCPSocket? socket = null;
>+};
r+ to this interface


I'm not familiar enough with the code to review it.

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

Comment on attachment 8546751 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

Honza, given that you've reviewed previous changes to the TCPServerSocket code, could you review these too? Smaug should be able to review any bits related to WebIDL/DOM things that you don't feel comfortable reviewing.

	Andrew Sutherland [:asuth] (he/him)
	Comment 12 • 10 years ago

Comment on attachment 8546751 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

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

I still had some feedback ;).  Assuming you have review deferral rights you may be able to promote this to a review if you like.  The main potential gap is that I'm not an expert about the JS-implemented WebIDL stuff, but I did check out the MDN page.

::: dom/network/TCPServerSocket.js
@@ +65,5 @@
>    },
>  
>    _callListenerAcceptCommon: function tss_callListenerAcceptCommon(socket) {
> +    try {
> +      this.__DOM_IMPL__.dispatchEvent(new this.useWin.TCPServerSocketEvent("connect", {socket: socket}));

Doh.  So, the change in behaviour where onconnect goes from returning a socket to an event with "socket" on it seems like a problem for existing consumers.  The good news is the IDL and https://developer.mozilla.org/en-US/docs/Web/API/TCPServerSocket.onconnect already claimed this was what we were doing.

The makes-this-okay news is that https://thecount.paas.allizom.org/#/listing/permission/tcp-socket tells us all the apps that use tcp-socket.  I just went through them, and it looks like only the "WebServer" app at https://thecount.paas.allizom.org/#/app/506837 uses TCPSserverSocket.  (I looked at what the apps said they did in their general description, plus what they said in the manifest explaining why they needed the permission.)

I think reaching out to that person and updating the MDN page to say something about the earlier versions being broken and needing to do "socket = event.socket || event;" is probably sufficient.

@@ +75,5 @@
>    init: function tss_init(aWindowObj) {
>      this.useWin = aWindowObj;
> +
> +    if (aWindowObj) {
> +      Services.obs.addObserver(this, "inner-window-destroyed", true);

Does this mean previous versions of this API could eternally leak a listening socket?  Doh!

(Aside: it's sad that we still need to use this inner-window-destroyed chaining.  I looked in DOMRequestHelper.jsm and it appears this must still be the state of the art because it uses it too.  I know it's unlikely to be as bad as it seems, but it freaks out the lizard part of my brain that loves premature optimization.)

::: dom/network/TCPServerSocketParent.cpp
@@ +27,5 @@
> +    return true;
> +  }
> +
> +  const char* permissions[] = {"tcp-socket", nullptr};
> +  return CheckPermissions(aCx, aGlobal, permissions) ||

I was going to say something about maybe checking the chrome window thing first and only then the permission to avoid misleading check failures.  But the call to CheckPermissions seems like it will actually hit a check for IsSystemPrincipal inside nsPermissionManager::CommonTestPermission at https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1250 so I think the IsChromeWindow is probably not needed.

	Honza Bambas (:mayhemer)
	Comment 13 • 10 years ago

Comment on attachment 8546751 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

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

Better a DOM peer should r+ this.  I'm not deeply familiar with webidl and these are mostly a mechanical changes.  If anything breaks, tests should catch it, not my eyes.

::: dom/network/TCPSocketParentIntermediary.js
@@ +90,5 @@
>  
>          aTCPServerSocketParent.sendCallbackError(error.message, error.filename,
>                                                   error.lineNumber, error.columnNumber);
>      };
> +    return serverSocketInternal;

why internal?

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

Comment on attachment 8546751 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

Andrea, could you review this one too? Neither smaug nor mayhemer felt confident with it.

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

Comment on attachment 8546751 [details] [diff] [review]
Convert TCPServerSocket to WebIDL

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

Talking just about the WebIDL part, it looks ok.

::: dom/network/TCPServerSocket.js
@@ +73,4 @@
>      }
>    },
>    init: function tss_init(aWindowObj) {
>      this.useWin = aWindowObj;

rename useWin to useGlobal or something else. This is not exactly a window all the time.

@@ +86,5 @@
> +      LOG("window init: " + this.innerWindowID);
> +    }
> +  },
> +
> +  __init: function(port, options, backlog) {

1. This double init/_init, confuses a bit. can you rename this one in something better?
2. plus, I don't see where this method is used.

@@ +91,5 @@
> +    this.listen(port, options, backlog);
> +  },
> +
> +  initWithGlobal: function(global) {
> +    this.useWin = global;

for instance, useWin here is a global.

@@ +95,5 @@
> +    this.useWin = global;
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    if (aTopic == "inner-window-destroyed") {

In theory you should not be able to receive anything else but inner-window-destroyed.

@@ +96,5 @@
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    if (aTopic == "inner-window-destroyed") {
> +      let wId = aSubject.QueryInterface(Ci.nsISupportsPRUint64).data;

Check if the QueryInterface fails.

@@ +157,5 @@
> +      lineno: lineNumber,
> +      colno: columnNumber,
> +      error: error
> +    };
> +    this.__DOM_IMPL__.dispatchEvent(new this.useWin.ErrorEvent("error", init));

try/catch ?

@@ +207,4 @@
>      Ci.nsITCPServerSocketInternal,
> +    Ci.nsIDOMGlobalPropertyInitializer,
> +    Ci.nsISupportsWeakReference,
> +    Ci.nsIObserver,

extra ,

::: dom/network/interfaces/nsIDOMTCPServerSocket.idl
@@ +18,1 @@
>  interface nsITCPServerSocketInternal : nsISupports 

extra space.

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +230,5 @@
>    // 
>    // @param transport
>    //        The accepted socket transport.
>    // @param binaryType
>    //        "arraybuffer" to use ArrayBuffer instances 

a couple of extra spaces... not yours, but you can remove them.

::: dom/network/interfaces/nsITCPServerSocketParent.idl
@@ +8,2 @@
>  
>  /** 

extra space

::: 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/webidl/TCPServerSocket.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/. */
> +

Do we have a spec for this file?

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

(In reply to Andrea Marchesini (:baku) from comment #15)
> Comment on attachment 8546751 [details] [diff] [review]
> Convert TCPServerSocket to WebIDL
> 
> @@ +86,5 @@
> > +      LOG("window init: " + this.innerWindowID);
> > +    }
> > +  },
> > +
> > +  __init: function(port, options, backlog) {
> 
> 1. This double init/_init, confuses a bit. can you rename this one in
> something better?
> 2. plus, I don't see where this method is used.

This is called by the bindings - https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Implementing_a_WebIDL_object_in_JavaScript
 
> @@ +157,5 @@
> > +      lineno: lineNumber,
> > +      colno: columnNumber,
> > +      error: error
> > +    };
> > +    this.__DOM_IMPL__.dispatchEvent(new this.useWin.ErrorEvent("error", init));
> 
> try/catch ?

I believe the bindings will deal with this appropriately if it throws an error; there's no need to introduce our own custom logging.

> ::: dom/webidl/TCPServerSocket.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/. */
> > +
> 
> Do we have a spec for this file?

Nope. It's just a straight conversion of the nsIDOMTCPServerSocket interface, which is only somewhat related to http://www.w3.org/2012/sysapps/tcp-udp-sockets/#interface-tcpserversocket .

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ccc86f7429e

	Phil Ringnalda (:philor)
	Comment 19 • 10 years ago

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

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

I admit, I have no idea why these messages appear on the opt b2g emulator alone:

23:27:59     INFO -  JavaScript error: jar:file:///system/b2g/omni.ja!/components/TCPSocketParentIntermediary.js, line 62: TypeError: global.mozTCPServerSocket is not a constructor
23:27:59     INFO -  JavaScript error: jar:file:///system/b2g/omni.ja!/components/TCPSocketParentIntermediary.js, line 44: TypeError: global.mozTCPSocket is not a constructor

	Andrew Sutherland [:asuth] (he/him)
	Comment 21 • 10 years ago

(In reply to Josh Matthews [:jdm] from comment #20)
> I admit, I have no idea why these messages appear on the opt b2g emulator
> alone:

Could it be some combination of:
- That thing where for JS modules in B2G where they all live in the same compartment with some weird voodoo applied instead of getting their own compartment / globals?  (I realize the file should probably be being loaded in a JS XPCOM component kind of way, but maybe that's the same thing?)
- Maybe the B2G emulator is the only OOP set of test runs we have for automated b2g builds, so this is the only case where that file is actually loaded in a JS context with the other module shenanigans going on?  (Guess, but I think we are still waiting on :qdot to land some stuff before b2g-desktop on (desktop) linux is OOP?)

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

This is now covered by the rewrite in bug 885982.