When a devtools.debugger.remote-use-websockets pref is enabled, switch the remote debugging server to a WebSocket based one. Clients like debugger.html, which are running inside a browser and don't have access to raw TCP sockets, should be able to connect directly to the debugged Firefox instance.

This seems like a nice to have for debugger.html, since it allows us to get rid of the websocket-to-tcp that we've been using up until now. Since we can always fall back to that, I'm assigning P3 to this bug for now. @jlongster, @jlast: if you think this bug needs to be higher priority, feel free to up it.

First version of the WebSocket support that works with debugger.html. Bare bones WebSocket server: - can perform the server side of the handshake - supports only text frames (opcode=1), all others are ignored - no support for ping/pong - no support for correctly closing the connection (sending/receiving the close frames) WebSocketDebuggerTransport built on top of the WebSocket server: - supports only JSON packets (no bulk binary transport) - only server-side works, not client. It means that browser toolbox can't connect - the JSON packets are still prefixed with the length, which is already done by the WebSocket layer - will be removed soon Enable the devtools.debugger.remote-use-websocket pref to use the WebSocket transport. Works beautifully with debugger.html, if you use this PR: https://github.com/jlongster/debugger.html/pull/323

Stupid question: Could it possibly work to have devtools.html use a postMessage-based transport? That uses structured clone, right?

(In reply to Jim Blandy :jimb from comment #3) > Stupid question: Could it possibly work to have devtools.html use a > postMessage-based transport? That uses structured clone, right? The /devtools/shared/transport/transport.js module defines several debugger transports designed to communicate over various kinds of boundaries. The WorkerDebuggerTransport uses postMessage/onMessage. ChildDebuggerTransport uses nsIMessageSender. So it's definitely possible to use postMessage as transport. However, these two transports mentioned above don't support startBulkSend. Probably only because their users never needed it. I don't know if there is any message manager implementation that could talk over TCP or Unix socket. There probably isn't. The WebSocket API is very similar though - it uses a "message" (text or binary) as its unit of abstraction, unlike a TCP socket, which is a raw stream of bytes.

Added support for client handshake to WebSocketServer. This means that the "WebSocketServer" name is no longer appropriate. But except the handshake, there is no difference between the client and server. The debugger client now also supports WebSocket transport, which means that browser toolbox can work over WebSocket now, too. You might need to recreate the chrome_debugger_profile directory, because the prefs (d.d.remote-use-websocket in particular) are not copied if the directory already exists. It might be a good idea to use a different pref for browser toolbox - something like devtools.debugger.chrome-debugging-use-websocket.

I had to temporarily disable the call to _isInputAlive at [1], because it waits for data or error on the input first. This works on the classic RDP protocol, because the server is the first to send any data (the hello packet). But with WebSocket, it waits forever, because the hello packet is sent only after the WebSocket handshake is finished. And that's client-initiated, by sending a HTTP upgrade request. We'll need to figure out a better way how to check for a certificate error on the connection (that's the sole purpose of the _isInputAlive method). [1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/security/socket.js#170

Updated patch that changes the protocol: stopped using the length prefix for JSON packets and instead sending them as WebSocket text frames. This makes the actual WebSocketDebuggerTransport really simple - it leverages the WebSocket capabilities and leaves all the messy details to the WebSocket layer. No conversion between Unicode strings and UTF-8 bytes etc. The transport in debugger.html that uses the standard WebSocket API is very similar and equally simple: https://github.com/jsnajdr/debugger.html/blob/69b203ffd50b7f0cff1e60c5625a23a80ed3542b/public/js/lib/devtools-sham/transport/ws-transport.js

(In reply to Jarda Snajdr [:jsnajdr] from comment #4) > (In reply to Jim Blandy :jimb from comment #3) > > Stupid question: Could it possibly work to have devtools.html use a > > postMessage-based transport? That uses structured clone, right? > > The /devtools/shared/transport/transport.js module defines several debugger > transports designed to communicate over various kinds of boundaries. The > WorkerDebuggerTransport uses postMessage/onMessage. ChildDebuggerTransport > uses nsIMessageSender. So it's definitely possible to use postMessage as > transport. > > However, these two transports mentioned above don't support startBulkSend. > Probably only because their users never needed it. I was just thinking that, for the local case, it's a shame to be stringifying and parsing all the JSON, as necessary with WebSockets, when we're just passing objects between the UI window to chrome code. I was trying to think of Web-visible APIs that might make the communication with the server more efficient. If we could arrange for the UI window to be passed a "window" that the UI could exchange postMessage messages with, that might work nicely.

(In reply to Jim Blandy :jimb from comment #8) > I was just thinking that, for the local case, it's a shame to be > stringifying and parsing all the JSON, as necessary with WebSockets, when > we're just passing objects between the UI window to chrome code. For that purpose, correct me if I'm wrong, we use LocalDebuggerTransport, which doesn't have any such overhead. For JS objects, it does something like this: send = (packet) => this.other.emit("message", packet); And for bulk binary data, it creates a nsIPipe, writes the binary data into it, and lets the other side read from the other end. Worker/ChildDebuggerTransport are only used to cross "real" boundaries. For workers it's different threads (?) and for child it's IPC, where it would be serialized no matter what.

But are we still able to use LocalDebuggerTransport with devtools.html? I thought we were putting the UI in a non-privileged window, so it shouldn't be able to exchange objects with chrome code. If we're still running the UI as chrome code --- but simply not using any of the extra facilities available in chrome --- then LocalDebuggerTransport is clearly the right thing, and we should continue to use that.

If we're only using non-privileged windows for easy development (refresh the window instead of restarting Firefox), then that's great.

(In reply to Jim Blandy :jimb from comment #10) > But are we still able to use LocalDebuggerTransport with devtools.html? I > thought we were putting the UI in a non-privileged window, so it shouldn't > be able to exchange objects with chrome code. > > If we're still running the UI as chrome code --- but simply not using any of > the extra facilities available in chrome --- then LocalDebuggerTransport is > clearly the right thing, and we should continue to use that. There are a few layers to the story as I understand it: * We currently plan for the top-level document in the toolbox _when used in Firefox_ to be a privileged XUL document (as it is currently) so we can keep using features not yet available to the web, such as native context menus (with fallbacks for when that's not available) * The child documents inside this "helper" document for the toolbox UI, panels, etc. would be non-privileged content, with each portion of the UI incrementally making that transition as it's converted * When running the toolbox in other browsers, we'll have non-privileged content all the way down, since that's the only option available For local connections: * Non-Firefox servers generally only expose WebSockets for both local and remote debugging, so the local transport discussion only really applies to the local Firefox debugging itself case * Since we plan to maintain a privileged XUL document for the UI in local Firefox, I don't think there is an immediate need to move from LocalDebuggerTransport to postMessage, but we could explore it as another way to use more regular web APIs * We _could_ use a WebSocket connection for local Firefox debugging, but I am not sure we need / want that quite yet, and I don't think it is the focus of this bug For remote connections: * Firefox acting as a WebSocket server allows the client to run in a non-privileged document, such as a regular tab in the same browser or remotely from a separate browser * We could use a WebSocket transport to replace the remote Gecko <-> Gecko connections that use TCP today, such desktop Firefox <-> Firefox for Android, making them more like the way WebKit / Chromium derivatives work (even if it's still the Firefox _protocol_ for now) Summary: * I view WebSockets as initially interesting for the remote case, as a replacement for the cases where we use raw TCP today * It could be used locally too, but it's unclear whether we want that right now, so let's push it off to a separate bug

Improved compliance of the WebSocket client/server: - correct close handshake - ping/pong frames - fragmented frames It now passes most of the Autobahn WebSocket testing suite Next steps: - try to use typed array and ArrayBuffers for binary data (will they work with the XPCOM streams?) - startBulkSend - mask outgoing messages - fix the TLS handshake (the commented-out isInputAlive call)

(In reply to Jarda Snajdr [:jsnajdr] from comment #4) > (In reply to Jim Blandy :jimb from comment #3) > > Stupid question: Could it possibly work to have devtools.html use a > > postMessage-based transport? That uses structured clone, right? > > The /devtools/shared/transport/transport.js module defines several debugger > transports designed to communicate over various kinds of boundaries. The > WorkerDebuggerTransport uses postMessage/onMessage. ChildDebuggerTransport > uses nsIMessageSender. So it's definitely possible to use postMessage as > transport. > > However, these two transports mentioned above don't support startBulkSend. > Probably only because their users never needed it. Well, for a long time we actually wanted to use bulk mode across IPC, but there was no mechanism to do so. If there had been at the time, we'd likely have a lot more use of bulk mode, since it's a great perf improvement. Now that bug 1093357 is resolved, it might be possible to offer bulk mode across IPC, but now we're planning to move away from the Firefox RDP to Chrome RDP in the future, so there's not much reason to use bulk now. There are two existing usages of bulk mode that I know of: 1. Transferring heap snapshots from server to client 2. Uploading apps to B2G from client to server Case #2 can be ignored since B2G has been deprioritized. Case #1 is still valuable, so it's a nice to have for that case, but I don't think we need a bulk mode in a first version of WebSockets since it only has one specific use case today and it's unlikely we would add more given our future plans.

Comment on attachment 8771530 [details] [diff] [review] WebSocket support in remote debugger protocol server Still need some more time to think about the details here, putting this back in my queue.

Thanks very much for the explanation, Ryan. The big picture is a lot clearer to me now.

Comment on attachment 8771530 [details] [diff] [review] WebSocket support in remote debugger protocol server Review of attachment 8771530 [details] [diff] [review]: ----------------------------------------------------------------- Overall, I think this approach is heading in the right direction. I believe you've convinced me it is best to directly use WebSockets to send the messages like you are doing here (instead of stuffing the previous transport protocol into WebSockets as if it were TCP). I haven't looked through the server details at all, just the integration points so far. I'll leave that for an actual review. For the remaining features: * startBulkSend: I would just throw an error for now and we can keep thinking about it later. There isn't an efficient way to do it that I can see, but perhaps someday you could send a ReadableStream over a WebSocket. * Binary data: does this matter for DevTools? We're only sending JSON packets today once you forget the bulk mode. That's all for initial thoughts. Great to see this work! :) ::: devtools/shared/security/socket.js @@ +196,5 @@ > > let transport; > if (alive) { > + if (Services.prefs.getBoolPref("devtools.debugger.remote-use-websocket")) { > + transport = new WebSocketDebuggerTransport(input, output); What are the reasons for using your WS client code as opposed to DOM WebSockets on the client side? Is it only for the authentication flow here? If it's just the authentication flow, I think we should aim for DOM WebSockets so that we'll have a client code path that works in content. As we discussed on IRC, the authentication flow is only used with remote devices over WiFi, so it would be fine to return to that problem later as a follow up. It seems more valuable to the get most from WebSockets by allowing the client to work from content first. If there is a compelling reason to use your code on the client side, then let's have it provide a separate WebSocket object vs. WebSocketServer for creating client vs. server, even if internally you're just setting some "this one is a client" flag. @@ +619,5 @@ > _createTransport() { > let input = this._socketTransport.openInputStream(0, 0, 0); > let output = this._socketTransport.openOutputStream(0, 0, 0); > + > + if (Services.prefs.getBoolPref("devtools.debugger.remote-use-websocket")) { It would be more flexible to create a socket listener property for this, so that each call site creating a DevTools socket can choose what they want. This way we can leave remote devices using the current TCP protocol over WiFi until there is an auth story, for example. ::: devtools/shared/transport/ws-server.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Let's call this file "web-socket-server.js". Also it seems like it could live in just devtools/shared, as it's a general WebSocket impl and not specific to DevTools. If we agree to use DOM WebSockets for the client, then it could move to devtools/server since the client wouldn't need this file at all. ::: devtools/shared/transport/ws-transport.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Let's call this file "web-socket-transport.js". @@ +11,5 @@ > + EventEmitter.decorate(this); > + > + this.active = false; > + this.hooks = null; > + this._ws = new WebSocketServer(input, output); I think it would be more natural for the caller to create the `ws` object from new WebSocket[Server]() and then pass it into the transport. This transport shouldn't need to know about the XPCOM streams at all, it just wants something that implements the WebSocket interface. @@ +17,5 @@ > + this._ws.on("close", () => this.close()); > +} > + > +WebSocketDebuggerTransport.prototype = { > + clientHandshake(host, port) { I would expect the caller to trigger these handshakes as needed before creating the transport. The transport should be passed a "ready for sending" object that implements the WebSocket interface.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17) > What are the reasons for using your WS client code as opposed to DOM > WebSockets on the client side? Is it only for the authentication flow here? It's because I need to integrate the WebSocket transport into the existing code, which first connects a raw TCP socket (nsISocketTransport), then does cert validation on it, and only then it hands the raw socket to the transport. If I used the "new WebSocket(`ws://${host}:${port}`)" approach, I wouldn't have access to the underlying TCP socket. My goal now is to figure out how to do the TLS auth with DOM/Necko WebSocket objects and prepare the patch for review and landing. It seems that maybe I could use the Necko WebSocket support even for the server, getting rid of my custom and incomplete WebSocket implementation in JS.

(In reply to Jarda Snajdr [:jsnajdr] from comment #18) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17) > > What are the reasons for using your WS client code as opposed to DOM > > WebSockets on the client side? Is it only for the authentication flow here? > > It's because I need to integrate the WebSocket transport into the existing > code, which first connects a raw TCP socket (nsISocketTransport), then does > cert validation on it, and only then it hands the raw socket to the > transport. > > If I used the "new WebSocket(`ws://${host}:${port}`)" approach, I wouldn't > have access to the underlying TCP socket. > > My goal now is to figure out how to do the TLS auth with DOM/Necko WebSocket > objects and prepare the patch for review and landing. Hmm, from everything you've said so far, it sounds like it's only for getting the authentication flow to work. But like I said, that authentication flow (which is completely custom and specific to debugging Firefox devices over WiFi) seems less important than using regular DOM WebSockets so that it can work correctly from a content page. I agree that if you use DOM WebSockets, you don't have access to the TCP socket. For the client side, do you need that access if we ignore the authentication flow? > It seems that maybe I could use the Necko WebSocket support even for the > server, getting rid of my custom and incomplete WebSocket implementation in > JS. That's an interesting idea, I am curious what it would look like!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19) > I agree that if you use DOM WebSockets, you don't have access to the TCP > socket. For the client side, do you need that access if we ignore the > authentication flow? No, I only need the access for the authentication flow. More precisely, for extracting info from the TLS layer: about the client and server certificates and results of their validation. If we don't support encryption and authentication on the WebSocket transport (at least for now), it removes a major obstacle to using DOM WebSockets. > > It seems that maybe I could use the Necko WebSocket support even for the > > server, getting rid of my custom and incomplete WebSocket implementation in > > JS. > > That's an interesting idea, I am curious what it would look like! I found out that the flyweb HTTP server does that - their HTTP server only handles the handshake part, and then creates a Necko WebSocket transport (which also supports server mode). I'll try if the interfaces are also usable from JS and will try to convert the WebSocket server.

Comment on attachment 8773320 [details] [diff] [review] Part 1: WebSocket server for WebSocket remote debugger transport Simple WebSocket server implemented in Javascript on top of a nsISocketTransport socket. It passes the Autobahn test suite from [1] with a few exceptions: - it tests some invalid UTF-8 strings where protocol errors are expected, but the Mozilla UTF-8 decoder recovers without error - it expects some invalid codes in a close frame to be reported as protocol errors, but my code doesn't check them The server doesn't support any extensions like per-message-deflate. It would be great if we could use a nsIWebSocketChannel from Necko instead of this implementation. Some work has been done in bug 1272100 as part of the FlyWeb project, but it looks unfinished and I didn't see setServerParameters and friends really used anywhere in m-c. Jonas, could you provide some info about the status of this code? What we need is to create a server-side nsIWebSocketChannel on top of already connected nsISocketTransport. [1] https://github.com/crossbario/autobahn-testsuite

Comment on attachment 8773321 [details] [diff] [review] Part 2: WebSocket-based debugger transport WebSocket-based debugger transport that can work both with standard DOM WebSockets (used by debugger client) and our custom JS-implemented WebSocketServer. Supports only text-based JSON packets, doesn't support binary bulk send.

Comment on attachment 8773322 [details] [diff] [review] Part 3: Add WebSocket transport support to debugger client and server sockets Add support for "useWebSocket" option to DebuggerSocket.connect (client-side) and DebuggerSocket.createListener (server-side). It's not compatible with "encryption" option and the OOB cert authentication. Why? In _isInputAlive at [1], we detect if the verification of the server certificate failed or not, i.e., whether the connection is alive and healthy. This is done by waiting on the input stream until data are available or an error is thrown. This works well on the classic protocol, because the server is the first to send anything on the connection (the root actor's hello packet). But in the WebSocket case, the client talks first (by sending the HTTP upgrade request), so _isInputAlive waits forever. It turns out to be really difficult to retrieve TLS errors from the nsISocketTransport, because the errors are well hidden behind various pipes, happen async without a possibility to wait for them etc... I also removed the call to transport.ready at [2], because it's not needed. The ready() method is always called either: - after successful handshake, when ServerSocketListener.allow calls DebuggerServer._onConnection. See [2]. - by the authenticator when it needs to receive auth packets from the other side. See [3]. [1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/security/socket.js#293-316 [2] https://dxr.mozilla.org/mozilla-central/source/devtools/server/main.js#1165 [3] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/security/auth.js#376

Comment on attachment 8773323 [details] [diff] [review] Part 4: Add WebSocket transport option to debugger server Added a pref "devtools.debugger.remote-use-websocket". When set to true, the debugger server started with the --start-debugger-server CLI option will be a WebSocket one. I'm not sure whether the pref definition belongs to the devtools/client/preferences/devtools.js or to modules/libpref/init/all.js. As it is a server-only pref, it should be the latter, right?

Comment on attachment 8773324 [details] [diff] [review] Part 5: Add WebSocket transport option to chrome debugger server Added a pref "devtools.debugger.chrome-debugging-use-websocket". When used, the browser toolbox and the addon debugger launched from about:debugging will use the WebSocket transport. Again, is the devtools/client/preferences/devtools.js file the right one to define the pref? Also, I improved the error handling when creating the debugger connection in toolbox-process-window.js. If the calls to gClient.connect(), gClient.listAddons() or gClient.getProcess() fail, i.e., they return a rejected promise, the rejected promise was unhandled and forgotten. I rewrote this code to task.js style where every error causes an exception to be thrown from the task. The code that displays the error in the UI was improved to be compatible with the thrown exception being a non-Error object. This happens when a DebuggerClient.request fails. There's one place where DebuggerClient error handling sucks: the DebuggerClient.request method sometimes returns a rejected promise, sometimes it throws an exception (at [1]). The exception is then swallowed by DebuggerClient.requester being infallible at [2]. It would be better if the error reporting was consistent, i.e., return a rejected promise. [1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.js#691 [2] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.js#252

The websocket server implementation in nsIWebSocketChannel is done. However there are a couple of things that it does not provide. * It does not provide a server-socket. This is to enable sharing server-socket with other types of requests. * It does not enable detecting when an incoming request is a wellformed websocket request * It does not send the http-upgrade response. However after all of those steps have happened, the nsIWebSocketChannel implementation implements the full websocket protocol, including support for the compression extensions gecko supports. It should be quite possible to just implement the above parts in JS and then instantiate a nsIWebSocketChannel and call setServerParameters. Even better would be to write a scriptable helper class which covers the above parts since most of it is already implemented in C++ logic in nsWebSocketChannel.cpp Yet another stategy would be to use the HttpServer class since it handles both incoming HTTP requests and WebSocket connections. You'd need to make it scriptable, but that should be relatively easy.

Btw, setServerParameters is used here: https://dxr.mozilla.org/mozilla-central/source/dom/base/WebSocket.cpp#1765

Posting an updated version of the WebSocket server that doesn't have own implementation of the protocol, but uses the server-side support in nsIWebSocketChannel. In order to make it work, the nsITransportProvider needs to be scriptable, which means it cannot have any [notxpcom] method. So, what to do with the getIPCChild method? It seems that its sole purpose is to make sure that any nsITransportProvider passed to WebSocketChannelChild is always an instance of TransportProviderChild, and cast it safely to PTransportProviderChild. Jonas, is there any way to achieve this, without affecting the interface? For now I commented out the getIPCChild method and replaced its call with a static_cast (which will crash if wrong object is passed to it). Also, it would be nice if instead of instantiating the nsIWebSocketChannel, which doesn't have the standard API, I could somehow create a server-side DOM WebSocket from Javascript. That would require exposing the ConstructorCommon via webidl. It seems that it could be achieved by using [ChromeOnly] and I even can use XPCOM interface pointers in webidl. Is this the right way how to proceed?

WebSocket server using the nsIWebSocketChannel from Necko. The handshake is still implemented in Javascript, but then the socket is handed over to Necko, which does the rest. Rough first version, will require a lot of cleanup before it's ready for review.

Comment on attachment 8773905 [details] [diff] [review] Part 0: Make the nsITransportProvider interface scriptable Review of attachment 8773905 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/websocket/nsITransportProvider.idl @@ +31,5 @@ > > // This must be implemented by nsITransportProvider objects running > // in the child process. It must return null when called in the parent > // process. > + // [noscript, notxpcom] PTransportProviderChild getIPCChild(); We still need this function. Otherwise you'll break FlyWeb. But it can remain unimplemented when this interface is implemented in JS, as long as you are running in the parent process.

You should be able to leave the [notxpcom] function as-is, even if the interface is marked scriptable. This will work fine as long as you're running in the parent process since the [notxpcom] function shouldn't be called there.

And yeah, exposing a [ChromeOnly] WebSocket ctor which takes a nsITransportProvider and any other needed parameters seems like a reasonable way to go. Ideally this ctor would take care of encoding the http-upgrade response as well, but I'll leave that up to you and the WebSocket DOM owner to decide (:baku?)

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #38) > You should be able to leave the [notxpcom] function as-is, even if the > interface is marked scriptable. This will work fine as long as you're > running in the parent process since the [notxpcom] function shouldn't be > called there. Unfortunately, passing a JS object as a nsITransportProvider parameter fails with this error: [Parent 59758] WARNING: XPTCall will not implement interface nsITransportProvider because of [notxpcom] members.: file /Users/jsnajdr/src/gecko-dev/xpcom/reflect/xptcall/xptcall.cpp, line 59 If there is any method or parent interface with the [notxpcom] flag, it can't be implemented in JS.

Huh, I did not expect that. Maybe ask bholley or mrbkap for suggested workarounds.

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #41) > Huh, I did not expect that. Maybe ask bholley or mrbkap for suggested > workarounds. Ok, asking Bobby - is there a way how to implement an interface with a [notxpcom] method in Javascript? Jonas, if the only option was NOT having a [notxpcom] member, is there any other way how to implement the getIPCChild functionality? Making it a normal XPCOM method? Using QueryInferface or other safe way of casting the object to PTransportProviderChild? I don't understand the complicated IPDL dance implemented in bug 1263991, so I don't know what exactly the getIPCChild method needs to do.

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #39) > And yeah, exposing a [ChromeOnly] WebSocket ctor which takes a > nsITransportProvider and any other needed parameters seems like a reasonable > way to go. Seems that WebIDL doesn't support declaring a [Constructor] and a special [ChromeConstructor] at the same time - I'm getting an "Extended attributes differ on different overloads" error from the WebIDL parser. Andrea, is there any way how to do this? > Ideally this ctor would take care of encoding the http-upgrade response as > well, but I'll leave that up to you and the WebSocket DOM owner to decide > (:baku?) Yes, that'd be a very nice and simple thing once the other issues are resolved.

Maybe it would work if the function is marked as both [noscript] and [notxpcom]? If that doesn't work, maybe change the signature so that it uses an out-argument instead. That way it can be [noscript] *instead of* [notxpcom] Regarding chrome constructors. Look at what we do for the File interface. https://dxr.mozilla.org/mozilla-central/source/dom/webidl/File.webidl#15

(In reply to Jarda Snajdr [:jsnajdr] from comment #42) > (In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #41) > > Huh, I did not expect that. Maybe ask bholley or mrbkap for suggested > > workarounds. > > Ok, asking Bobby - is there a way how to implement an interface with a > [notxpcom] method in Javascript? No. [notxpcom] means that the calling convention is arbitrary. XPConnect has no way to know how to marshal the arguments to/from JS. You'd need to move the [notxpcom] member to some other interface or just on the concrete class.

> Yes, that'd be a very nice and simple thing once the other issues are > resolved. I didn't follow the full discussion, but yes, you can have a Chrome Constructor and a content Constructor implementing something custom. We did the same for File where there is 1 content CTOR and 3 chrome only CTORs just adding this check: if (!nsContentUtils::ThreadsafeIsCallerChrome()) { aRv.Throw(NS_ERROR_FAILURE); return nullptr; } https://dxr.mozilla.org/mozilla-central/source/dom/base/File.cpp#556 and expose all of these CTORs fully in the WebIDL: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/File.webidl#15 Important thing: please, don't throw NS_ERROR_FAILURE. If the CTOR is used by a content code, you should throw a TypeError. So, the actual Chrome-only File CTORs throw the wrong error code and I'll file a bug to fix it.

Comment on attachment 8773323 [details] [diff] [review] Part 4: Add WebSocket transport option to debugger server Review of attachment 8773323 [details] [diff] [review]: ----------------------------------------------------------------- f+ for now, would like to see this with updated version of other patches. ::: devtools/client/devtools-startup.js @@ +157,5 @@ > debuggerServer.allowChromeProcess = true; > > let listener = debuggerServer.createListener(); > listener.portOrPath = portOrPath; > + listener.useWebSocket = Hmm, probably just `webSocket` is fine, to match the other properties, some of which are also boolean? I think it will be easier to say for sure when the other patches are ready for review.

Comment on attachment 8773324 [details] [diff] [review] Part 5: Add WebSocket transport option to chrome debugger server Review of attachment 8773324 [details] [diff] [review]: ----------------------------------------------------------------- f+ for now, would like to see this with updated version of other patches.

Converted the nsITransportProvider::getIPCChild to normal XPCOM method, so that the interface can be scriptable. Having the [noscript] flag is not relevant - it says that the method cannot be called from JS, but we are interested in implementing the interface in JS, not calling it. Not sure if I should return NS_OK even from implementation that return nullptr and call MOZ_CRASH.

Added a new constructor with signature (url, protocols, transportProvider, negotiatedExtensions) to WebSocket.webidl. I need some help with how exactly this constructor should behave if called from non-chrome code. It should be indistinguishable from the situation when no such constructor exists, right? Then I cannot just throw a TypeError, because: - before the patch, new WebSocket(url, protocols, null, null) ignores the two extra arguments and calls the WebSocket(url, protocols) ctor - after the patch, the behavior must be exactly the same, i.e., no exceptions, extra args ignored I tried to achieve this, but not 100% successfully - the binding still checks if the third argument is an object and throws MSG_NOT_OBJECT. The negotiatedExtensions parameter is optional so that a constructor with three arguments is not rejected with MSG_MISSING_ARGUMENTS.

Websocket support code for the debugger transport. The only part of the WebSocket protocol I implemented myself is the server handshake, then I hand over to a standard DOM WebSocket for both client and server. Doesn't support negotiating any extensions. The permessage-deflate could be useful for the remote debugging protocol. Let's do it as a followup when the WebSocket transport starts to be really used.

Unchanged from previous versions, only has a different part number :)

Changed the 'useWebSocket' option to just 'useWebSocket'

Changed the 'useWebSocket' option to just 'webSocket'. What about the pref name? Is "devtools.debugger.remote-use-websocket" OK?

Changed the 'useWebSocket' option to just 'webSocket'. What about the pref name? Is "devtools.debugger.chrome-debugging-use-websocket" OK?

There's one thing in the WebSocketChannel I'm worried about: if I create a server side channel and pass a nsITransportProvider to it, is it safe to call the onTransportAvailable callback synchronously? I.e.: let transportProvider = { setListener(upgradeListener) { upgradeListener.onTransportAvailable(transport, input, output); } } In this case, setListener is called inside asyncOpen at [1], which causes onTransportAvailable [2] to be called synchronously (including things like StartWebSocketData), and AFTER this the rest of asyncOpen is executes. Jonas, is that safe and expected? [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#3395 [2] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#3628

I'd definitely recommend calling onTransportAvailable asynchronously. We're intentionally adding an asynchronous step elsewhere. This would certainly be good to get documented, or ideally asserted.

Comment on attachment 8775213 [details] [diff] [review] Part 2: Expose server-side WebSocket chrome-only constructor in WebIDL Review of attachment 8775213 [details] [diff] [review]: ----------------------------------------------------------------- I see 2 options here: 1. having a CTOR: Constructor(DOMString url, sequence<DOMString> protocols, any transportProvider, any negotiatedExtensions); 2. if this is just chrome-only, what about if you do: partial interface WebSocket { [ChromeOnly] static WebSocket createFlyWebWebSocket(DOMString url, sequence<DOMString> protocols, any transportProvider, any negotiatedExtensions); } Probably, the second approach is cleaner.

(In reply to Andrea Marchesini (baku - PTO 1/8 - 12/8) from comment #59) > I see 2 options here: > > 1. having a CTOR: Constructor(DOMString url, sequence<DOMString> protocols, > any transportProvider, any negotiatedExtensions); > 2. if this is just chrome-only, what about if you do: > > partial interface WebSocket { > [ChromeOnly] > static WebSocket createFlyWebWebSocket(DOMString url, sequence<DOMString> > protocols, any transportProvider, any negotiatedExtensions); > } > > Probably, the second approach is cleaner. There's also option to create a derived interface: [ChromeConstructor(DOMString url, sequence<DOMString> protocols, any transportProvider, any negotiatedExtensions)] interface ServerWebSocket : WebSocket { } And then create instances in Chrome code with "new ServerWebSocket(...)". Would it work? The option #2 with the static factory method looks nicer, though. By the way, who should I ask for review when you're away from Aug 1 to Aug 12?

> By the way, who should I ask for review when you're away from Aug 1 to Aug > 12? If you do it for today, I'll review it today! Or tomorrow max.

Introducing a new interface seems wrong actually. I prefer to have a static method.

Implemented chrome-only static factory method WebSocket.createServerWebSocket.

Use WebSocket.createServerWebSocket to create the server-side WebSocket.

Resolve conflicts with patch from bug 1285557.

Comment on attachment 8775974 [details] [diff] [review] Part 2: Add static chrome-only method to create server-side WebSocket to WebIDL Review of attachment 8775974 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/WebSocket.webidl @@ +68,5 @@ > +// devtools remote debugging server. > +interface nsITransportProvider; > + > +partial interface WebSocket { > + [ChromeOnly, Throws] NewObject

Added [NewObject] attribute to createServerWebSocket.

If you do a ctor, please be careful that it doesn't affect webpages. Putting a security check in the beginning of the function only makes sure that webpages can't cause security problems. However the ctor will still effect the webidl selection algorithm. So adding Constructor(DOMString url, sequence<DOMString> protocols, any transportProvider, any negotiatedExtensions) Means that a webpage which calls new WebSocket(url, [], 1, "b"); will get an exception. Whereas per spec it should be equivalent to new WebSocket(url); Since the extra arguments should be ignored. In short, don't add Constructor(DOMString url, sequence<DOMString> protocols, any transportProvider, any negotiatedExtensions) with a security check at the beginning of the function since it'll affect webpages.

Comment on attachment 8775221 [details] [diff] [review] Part 4: WebSocket-based debugger transport I am guessing you meant to request a review here as well.

Comment on attachment 8775975 [details] [diff] [review] Part 3: WebSocket server for WebSocket remote debugger transport Review of attachment 8775975 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this seems reasonable on its own. A few changes to make before it's ready to go. Please add some kind of test for this WebSocket server to verify that you can start a server and a DOM WebSocket client can connect and exchange a message. In an ideal world, it could be an XPCShell test, but you might need mochitest chrome so that there's a window to use to get the DOM WebSocket? Not sure if the hiddenDOMWindow works in XPCShell. ::: devtools/server/websocket.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Add an ESLint ignore exclusion for this file so that it will be linted. @@ +12,5 @@ > + > +const CryptoHash = > + CC("@mozilla.org/security/hash;1", "nsICryptoHash", "initWithString"); > + > +const WebSocket = Services.appShell.hiddenDOMWindow.WebSocket; (Do we want to use the loader?) @@ +16,5 @@ > +const WebSocket = Services.appShell.hiddenDOMWindow.WebSocket; > + > +const threadManager = Cc["@mozilla.org/thread-manager;1"].getService(); > + > +const HEADER_MAX_LEN = 8000; Is this an arbitrary number or does it come from a spec? Either way, add a comment. @@ +18,5 @@ > +const threadManager = Cc["@mozilla.org/thread-manager;1"].getService(); > + > +const HEADER_MAX_LEN = 8000; > + > +function readLine(input) { Some quick doc comments on these functions would help me scan the code to make sure it agrees with what you want it to do. @@ +54,5 @@ > +function writeString(output, data) { > + return new Promise((resolve, reject) => { > + let wait = () => { > + if (data.length === 0) { > + resolve(); It's good to cover the base case of writing nothing, but for the main case where there's something to write, it seems like it would be better to resolve in the same cycle as writing, instead of waiting one more time first. @@ +75,5 @@ > + wait(); > + }); > +} > + > +const readHttpHeaders = Task.async(function* (input) { Maybe `readHttpRequest` since it's the request line and headers together? @@ +76,5 @@ > + }); > +} > + > +const readHttpHeaders = Task.async(function* (input) { > + let firstLine = ""; The HTTP/1.1 spec calls this "request line"[1] which seems less mysterious. [1]: http://httpwg.org/specs/rfc7230.html#rfc.section.3.1.1 @@ +102,5 @@ > + > + return { firstLine, headers }; > +}); > + > +function sendHttpHeaders(output, headers) { Maybe `writeHttpResponse`? @@ +142,5 @@ > + if (!key) { > + throw new Error("The handshake request must have a Sec-WebSocket-Key header"); > + } > + > + return computeKey(key); It seems strange for `checkRequest` to also compute the key... Why not call `computeKey` right after `checkRequest`? @@ +187,5 @@ > + > + let transportProvider = { > + setListener(upgradeListener) { > + // Not sure if the onTransportAvailable callback can be called synchronously. > + // The code in WebSocketChannel.cpp looks rather unfriendly to this. Can you link to something more specific in the file, or explain this in more detail? @@ +188,5 @@ > + let transportProvider = { > + setListener(upgradeListener) { > + // Not sure if the onTransportAvailable callback can be called synchronously. > + // The code in WebSocketChannel.cpp looks rather unfriendly to this. > + handshakeDone.then(() => { What about making `setListener` Task.async and `yield handshakeDone`? @@ +194,5 @@ > + }); > + } > + }; > + > + return handshakeDone.then(() => new Promise((resolve, reject) => { What about making `accept` Task.async and `yield handshakeDone`? @@ +207,5 @@ > +/** > + * Establish a client connection to a given location. Returns Promise with a WebSocket > + * ready to send and receive messages. > + */ > +function connect(host, port) { To support devtools.html, we want to have a client code path that doesn't need chrome access. Since you aren't actually using chrome access here (it's just in a module that _does_ use it), we should break this client piece out into a separate module. It probably needs to live under /devtools/shared to make writing tests easier. Perhaps /devtools/shared/client/websocket.js? We also want the client code to get WebSocket from the loader if available. Something like require("WebSocket") and then we can let the various loaders supply the WebSocket. The browser loader can supply it from window, for example.

Comment on attachment 8775221 [details] [diff] [review] Part 4: WebSocket-based debugger transport Review of attachment 8775221 [details] [diff] [review]: ----------------------------------------------------------------- Seems good overall, just want to double check the next version. ::: devtools/shared/transport/websocket-transport.js @@ +19,5 @@ > + if (this.active) { > + return; > + } > + > + this.socket.onmessage = this._onMessage.bind(this); We typically use `addEventListener` style instead of these per-event properties. Let's switch to addEventListener for each event. If you use `addEventListener(<name>, this)` and define a `handleEvent` method that switches on event type, you can skip the work of binding functions (the `handleEvent` method will be called with the correct `this` value). @@ +48,5 @@ > + }, > + > + close() { > + this.active = false; > + this.socket.close(); You should also null out the event handlers. Leaving bound handlers attached can lead to leaks. Maybe null out the socket as well? Something like: if (this.socket) { <null handlers> <close> <null socket> } @@ +50,5 @@ > + close() { > + this.active = false; > + this.socket.close(); > + > + this.emit("onClosed"); Move this to the beginning of the function to match the order of operations other transports use. While looking at this, I noticed some transports emit "close" and others "onClosed". I filed bug 1290599.

Comment on attachment 8775225 [details] [diff] [review] Part 5: Add WebSocket transport support to debugger client and server sockets Review of attachment 8775225 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this looks reasonable, though I am still unsure about a few parts. Also, please add tests at this level that starts a WebSocket listener and connects to it. The existing tests in devtools/shared/security/tests/unit might give you some ideas (though you may need a mochitest once more to access DOM WebSocket). ::: devtools/shared/security/socket.js @@ +15,5 @@ > var promise = require("promise"); > var defer = require("devtools/shared/defer"); > var DevToolsUtils = require("devtools/shared/DevToolsUtils"); > var { dumpn, dumpv } = DevToolsUtils; > +loader.lazyRequireGetter(this, "WebSocket", This is a confusing name since it's not the same thing as the DOM WebSocket interface... Once you've split up the client path into it's own module, you'll probably have new names here and the problem will go away. @@ +111,3 @@ > * A possible DevTools transport (if connection succeeded and streams > * are actually alive and working) > * @return certError boolean Can you remove the comments about `certError` and `s` here? It seems they were copied from `_attemptTransport` by mistake, since this function only returns a transport. @@ +118,5 @@ > var _getTransport = Task.async(function* (settings) { > + let { host, port, encryption, webSocket } = settings; > + > + if (webSocket) { > + if (encryption) { Similar to the server flow, I think it would be nice to validate options / settings up front. (I guess I called them settings in the client and options in the server... :S Feel free to clean up or ignore, whatever you prefer...) Create a new function to check various client settings like this up front and call it in `connect` before we proceed further. @@ +629,5 @@ > let input = this._socketTransport.openInputStream(0, 0, 0); > let output = this._socketTransport.openOutputStream(0, 0, 0); > + > + if (this._listener.webSocket) { > + let webSocket = yield WebSocket.accept(this._socketTransport, input, output); Maybe just `let socket = ...` to parallel the client flow. @@ -616,5 @@ > onClosed: reason => { > this.deny(reason); > } > }; > - this._transport.ready(); I am still unsure if this is a safe change for the TLS code path. For your next try build, please include Firefox for Android builds so I can do some testing there. @@ +741,5 @@ > } > } > dumpn("Debugging connection denied on " + this.address + > " (" + errorName + ")"); > + if (this._transport) { I am not sure I follow why this is needed... Was `deny` called after `allow` or something?

Comment on attachment 8775977 [details] [diff] [review] Part 7: Add WebSocket transport option to chrome debugger server Review of attachment 8775977 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/preferences/devtools.js @@ +80,5 @@ > // Enable the Debugger > pref("devtools.debugger.enabled", true); > pref("devtools.debugger.chrome-debugging-host", "localhost"); > pref("devtools.debugger.chrome-debugging-port", 6080); > +pref("devtools.debugger.chrome-debugging-use-websocket", false); Might as well drop the "use" part, so "devtools.debugger.chrome-debugging-websocket".

Comment on attachment 8775228 [details] [diff] [review] Part 6: Add WebSocket transport option to debugger server Review of attachment 8775228 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/devtools-startup.js @@ +157,5 @@ > debuggerServer.allowChromeProcess = true; > > let listener = debuggerServer.createListener(); > listener.portOrPath = portOrPath; > + listener.webSocket = Since this is a CLI interface, a pref seems like the wrong way to go. It seems more flexible to let the CLI options control whether WebSockets are used. Not sure the best way to go... maybe check if `portOrPath` starts with "ws:" and turn it on if so? Maybe a separate CLI flag? Also, the help info should be updated, since it only says TCP right now.

Added a mochitest that creates a WebSocket based connection and send "echo" over it. Inspired by test_encryption.js. This test needs access to DOM API and to run in the parent process. Therefore, mochitest-chrome is the right one. xpcshell is not capable enough, as it doesn't provide access to DOM APIs, only to XPCOM. I added the test to /devtools/server/tests/mochitest, because there are existing chrome tests there. The right place would probably be /devtools/shared/security/tests/mochitest, but there are no mochitests there yet (only xpcshell-based unit tests) and I got really scared by having to create the testing boilerplate there (head.js, the test actors, ...).

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #69) > If you do a ctor, please be careful that it doesn't affect webpages. Putting > a security check in the beginning of the function only makes sure that > webpages can't cause security problems. However the ctor will still effect > the webidl selection algorithm. Yes, there is an ugly interaction between the ChromeOnly constructor and extra arguments to normal constructors that are supposed to be ignored. The cleanest solution turned out to be to add a static ChromeOnly factory method WebSocket.createServerWebSocket. See the "part 2" patch and our related conversation with :baku.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #71) > (Do we want to use the loader?) Yes, I added the WebSocket property to the loader. Makes the client code 100% compatible with the web page environment. > > +const HEADER_MAX_LEN = 8000; > > Is this an arbitrary number or does it come from a spec? Either way, add a > comment. This is an arbitrary number, as there is no standard on how big HTTP headers can be. I do this to prevent receiving unlimited amount of data on input and allocating a memory buffer for it. Added an explaining comment. > Some quick doc comments on these functions would help me scan the code to > make sure it agrees with what you want it to do. Added comments to all functions in this module. > It's good to cover the base case of writing nothing, but for the main case > where there's something to write, it seems like it would be better to > resolve in the same cycle as writing, instead of waiting one more time first. If by "cycle" you mean an event loop tick, then this function is already correct - if there are zero bytes to write, it returns immediately without ever waiting for the output stream. In case all writes have been written, it returns in the same event loop tick, without scheduling an extra (and useless) runnable. > Maybe `readHttpRequest` since it's the request line and headers together? Changed. > The HTTP/1.1 spec calls this "request line"[1] which seems less mysterious. Changed to requestLine. This was a leftover from the times when I implemented the client handshake, too. In that case, the firstLine was the response status line. > Maybe `writeHttpResponse`? Changed. > It seems strange for `checkRequest` to also compute the key... Why not call > `computeKey` right after `checkRequest`? Renamed the method to 'processRequest'. Its purpose is to go through the request headers, check them and extract any important information from them. Right now, it only computes the accept key. In future, it could also process the WebSocket extensions like permessage-deflate. > Can you link to something more specific in the file, or explain this in more > detail? See our discussion with Jonas in comment 56 and comment 57. I'll file a bug to add a check for synchronous calls to onTransportAvailable. > What about making `setListener` Task.async and `yield handshakeDone`? I'm calling onTransportAvailable in a DevToolsUtils.executeSoon wrapper in the latest version. > What about making `accept` Task.async and `yield handshakeDone`? Done. > To support devtools.html, we want to have a client code path that doesn't > need chrome access. Since you aren't actually using chrome access here > (it's just in a module that _does_ use it), we should break this client > piece out into a separate module. I moved the client code that creates a WebSocket connection directly to socket.js. The websocket.js module now only contains only server code and I renamed it to websocket-server.js.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #72) > > + this.socket.onmessage = this._onMessage.bind(this); > > We typically use `addEventListener` style instead of these per-event > properties. Let's switch to addEventListener for each event. If you use > `addEventListener(<name>, this)` and define a `handleEvent` method that > switches on event type, you can skip the work of binding functions (the > `handleEvent` method will be called with the correct `this` value). Cool, I didn't notice that WebSocket iface inherits from EventTarget and has addEventListener, too. I refactored the code as you describe. > You should also null out the event handlers. Leaving bound handlers > attached can lead to leaks. Done, nulled the handlers and the socket, too. > > + this.emit("onClosed"); > > Move this to the beginning of the function to match the order of operations > other transports use. Moved.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #73) > > +loader.lazyRequireGetter(this, "WebSocket", > > This is a confusing name since it's not the same thing as the DOM WebSocket > interface... Renamed to WebSocketServer. The module now has only the 'accept' function. > Can you remove the comments about `certError` and `s` here? It seems they > were copied from `_attemptTransport` by mistake, since this function only > returns a transport. Removed, and added a doc comment about the "webSocket" option. > > + if (webSocket) { > > + if (encryption) { > > Similar to the server flow, I think it would be nice to validate options / > settings up front. (I guess I called them settings in the client and > options in the server... :S Feel free to clean up or ignore, whatever you > prefer...) > > Create a new function to check various client settings like this up front > and call it in `connect` before we proceed further. There is only the check for (webSocket && encryption), so I didn't create new function, only moved the check to the beginning of the _getTransport function and separated it clearly from the rest of the code. Or are there other checks that should be perfomed? > Maybe just `let socket = ...` to parallel the client flow. Changed. > > - this._transport.ready(); > > I am still unsure if this is a safe change for the TLS code path. For your > next try build, please include Firefox for Android builds so I can do some > testing there. I'm explaining in comment 28 why I think this is right. It would break the TLS code path only if the TLS handshake was performed lazily, i.e., waited until someone actually tries to read or write from the stream. Which I hope is not the case. > @@ +741,5 @@ > > } > > } > > dumpn("Debugging connection denied on " + this.address + > > " (" + errorName + ")"); > > + if (this._transport) { > > I am not sure I follow why this is needed... Was `deny` called after > `allow` or something? 'deny' is called for any error thrown inside the task at [1]. Including errors in _createTransport, when the connection cannot be established. In that early case, this._transport remains undefined and the 'deny' function throws a ReferenceError. I was regularly seeing that error in console when working with browser toolbox, long before I started working on this bug. [1] http://searchfox.org/mozilla-central/source/devtools/shared/security/socket.js#597-602

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #75) > Since this is a CLI interface, a pref seems like the wrong way to go. It > seems more flexible to let the CLI options control whether WebSockets are > used. > > Not sure the best way to go... maybe check if `portOrPath` starts with "ws:" > and turn it on if so? Maybe a separate CLI flag? The options now are: --start-debugger-server Reads and uses the defaults from d.d.remote-port and d.d.remote-websocket. This is a change from the previous behavior, where d.d.remote-port was never consulted and value "6000" was hardcoded. Or was it intentional to not consult the pref? I remember being unpleasantly suprised when I wanted to check out debugger.html for the first time, changed the pref to 6080 and see it ignored. --start-debugger-server 6789 Start the non-WebSocket server on port 6789. (Or maybe it should look at the d.d.remote-websocket pref?) --start-debugger-server ws:6789 Start the WebSocket server on port 6789. --start-debugger-server ws: Start the WebSocket server on default port (taken from prefs, mostly 6000)

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #74) > Might as well drop the "use" part, so > "devtools.debugger.chrome-debugging-websocket". Done.

Comment on attachment 8776586 [details] [diff] [review] Part 3: WebSocket server for WebSocket remote debugger transport Review of attachment 8776586 [details] [diff] [review]: ----------------------------------------------------------------- This part looks ready feature wise. I would like to see a test at this layer though that connects a DOM WebSocket to your server module without the higher level DevTools transport machinery (the test in part 8 is at the higher level, which is great, but I'd like to also have a test at this lower level). ::: devtools/.eslintrc @@ +23,5 @@ > "uneval": true, > "URL": true, > "XMLHttpRequest": true, > "_Iterator": true, > + "WebSocket": true, Let's keep these sorted, so above XMLHttpRequest.

Comment on attachment 8775194 [details] [diff] [review] Part 1: Make the nsITransportProvider interface scriptable Review of attachment 8775194 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!

Comment on attachment 8776587 [details] [diff] [review] Part 4: WebSocket-based debugger transport Review of attachment 8776587 [details] [diff] [review]: ----------------------------------------------------------------- Great, this part looks ready to go. ::: devtools/shared/transport/websocket-transport.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Since this is a new file, add an ESLint ignore exclusion to force it to be linted.

Comment on attachment 8776588 [details] [diff] [review] Part 5: Add WebSocket transport support to debugger client and server sockets Review of attachment 8776588 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/shared/security/socket.js @@ +115,5 @@ > */ > var _getTransport = Task.async(function* (settings) { > + let { host, port, encryption, webSocket } = settings; > + > + if (webSocket && encryption) { I think we want to do these checks at the entry point of this module for the client, so that's in the `connect` function above. Additionally, we should also verify that authenticator.mode == "PROMPT" when `webSocket` is true, as the other authenticator won't work. @@ +399,5 @@ > throw new Error("Discovery only supported for TCP sockets."); > } > + if (this.encryption && this.webSocket) { > + throw new Error("Encryption not supported on WebSocket transport"); > + } We should also check the authenticator mode on the server side when used with WebSockets. @@ -616,5 @@ > onClosed: reason => { > this.deny(reason); > } > }; > - this._transport.ready(); In my initial local testing of the Android build from your try run, WiFi connections (which use encryption) do appear to work with this change applied. I am not still not confident that this change is correct for the TCP transport, though. You pointed out in comment 28 that the authenticator calls transport.ready()[1], but that particular call is the client side of the authenticator. The transport.ready() call we are discussing here is on the server side. At the moment, the server side authenticator doesn't call transport.ready() itself, I believe because it only sends data to the client over the transport (it doesn't receive over the transport). So, the reason for the transport.ready() here is to catch stream failure on the server during the initial connection and authentication steps before we hand off the transport in allow(). I believe the reason your build worked fine for me is because I did not experience a stream failure, so it did not matter during my test. But for correctness, I believe it's still needed. It's not clear to me whether the existing tests cover this precise failure type, unfortunately. Let's start with your try run, do the intermittents go away if you put this back? [1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/security/auth.js#376

Comment on attachment 8776589 [details] [diff] [review] Part 6: Add WebSocket transport option to debugger server Review of attachment 8776589 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this behavior looks good to me. ::: devtools/client/devtools-startup.js @@ +134,5 @@ > if (!this._isRemoteDebuggingEnabled()) { > return; > } > + > + let webSocket = false; Please add your description of the current behavior in comment 86 as a code comment for future readers. (The behavior itself seems logical to me.) @@ +139,3 @@ > if (portOrPath === true) { > + // Default to pref values if no values given on command line > + portOrPath = Services.prefs.getIntPref("devtools.debugger.remote-port"); It seems okay to use the pref value here. It's never been used as the server's port value in the past, but it doesn't seem like it would cause a problem to make this change.

Comment on attachment 8776591 [details] [diff] [review] Part 7: Add WebSocket transport option to chrome debugger server Review of attachment 8776591 [details] [diff] [review]: ----------------------------------------------------------------- Great, this part looks good to me.

Btw, if you want to support extensions, which I think you do, I suggest exposing the "ProcessServerWebSocketExtensions" function to JS. Easiest way to do that is likely to add another static [ChromeOnly] function to WebSocket.webidl. That way you can call ProcessServerWebSocketExtensions and pass in the requested extensions from the parsed upgrade request. Then use the resulting value in the upgrade response and when calling CreateServerWebSocket.

Comment on attachment 8776592 [details] [diff] [review] Part 8: Mochitest for WebSocket-based debugger transport Review of attachment 8776592 [details] [diff] [review]: ----------------------------------------------------------------- The test itself looks reasonable, but like you "feared"... ;) I think it belongs in security directory, so let's setup the infrastructure for that. Create a "tests/chrome" directory under security. For the head.js and test actors, you may be able to copy the more minimal versions I used in security/tests/unit. Feel free to copy whatever extra bits you need for from the devtools/server versions. I am not really worried about duplication for the moment, so just copy what's needed. You'll need to update the moz.build in security to point to the new chrome.ini manifest. Please ask if you're unsure how to do these steps, mostly it involves copying from other places. ::: devtools/server/tests/mochitest/test_websocket.html @@ +20,5 @@ > + Services.prefs.setBoolPref("devtools.debugger.remote-enabled", true); > + Services.prefs.setBoolPref("devtools.debugger.prompt-connection", false); > + > + function finish() { > + Services.prefs.clearUserPref("devtools.debugger.remote-enabled"); Use `SimpleTest.registerCleanupFunction` to clear these prefs, so we push the responsibility to the test harness. @@ +25,5 @@ > + Services.prefs.clearUserPref("devtools.debugger.prompt-connection"); > + SimpleTest.finish(); > + } > + > + Task.spawn(function* () { If believe if pull in the SpawnTask.js test helper[1], you can use add_task like can with other test types. [1]: https://dxr.mozilla.org/mozilla-central/rev/ffac2798999c5b84f1b4605a1280994bb665a406/dom/animation/test/chrome/test_restyles.html#7

I've put the list of globals in .eslintrc back into alphabetical order. That's the only interdiff for this patch. I'll add the low-level test for the WebSocket server into the Part 8 patch.

Added the websocket-transport.js to .eslintignore as exception. Other than that, no change.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #92) > > + if (webSocket && encryption) { > > I think we want to do these checks at the entry point of this module for the > client, so that's in the `connect` function above. OK, I did the following: - extract the check to function validateSettings - call it from 'connect' instead of '_getTransport' - added a 'validateSettings' method to the client authenticators. It check the encryption for the OOB auth. > Additionally, we should also verify that authenticator.mode == "PROMPT" when > `webSocket` is true, as the other authenticator won't work. The validateOptions/validateSettings methods take care of this: first, they forbid 'encryption' and 'webSocket' being set at the same time, and then check that 'encryption' is set. This rules out the 'webSocket' option for OOB authenticator. I'll file a followup bug to unify the options/setting naming inconsistency. > We should also check the authenticator mode on the server side when used > with WebSockets. This is already happening on server and the latest patch added it to client - see above. > > - this._transport.ready(); Alright, I added the line back, as it seems to be really necessary for the server to catch a stream failure. I posted an interdiff for this part (part 5) at https://gist.github.com/jsnajdr/266e4fea5d25a4ceb1afbed7395bf6eb Next time I'll use MozReview!

- added a long comment to handleDebuggerServerFlag method - fixed a bug where the option "ws:" (start WebSocket server on default port) didn't work

There are now two tests: 1. devtools/shared/security/tests/chrome/test_websocket-transport.html - moved from devtools/server/tests - it was super easy, no large copy&paste was needed! - uses SpawnTask and SimpleTest.registerCleanupFunction - tests the high-level DebuggerClient and DebuggerServer talking over WebSocket transport 2. devtools/server/tests/mochitest/test_websocket-server.html - tests the low-level WebSocket API - creates a WebSocket server and client and exchanges a message - we need to watch the try-run for leaks - I'm not 100% sure if I'm shutting down everything properly

Comment on attachment 8777270 [details] [diff] [review] Part 5: Add WebSocket transport support to debugger client and server sockets Review of attachment 8777270 [details] [diff] [review]: ----------------------------------------------------------------- Great, this part looks ready to go! (Thanks for posting the interdiff, it helps a lot!)

Comment on attachment 8777308 [details] [diff] [review] Part 8: Mochitest for WebSocket-based debugger transport Review of attachment 8777308 [details] [diff] [review]: ----------------------------------------------------------------- Tests look good to me, thanks for moving them around! ::: devtools/server/tests/mochitest/chrome.ini @@ +111,5 @@ > [test_styles-matched.html] > [test_styles-modify.html] > [test_styles-svg.html] > [test_unsafeDereference.html] > +[test_websocket-server.html] Let's try running this test with skip-if = false to override the default above. ::: devtools/shared/security/tests/chrome/chrome.ini @@ +1,3 @@ > +[DEFAULT] > +tags = devtools > +skip-if = buildapp == 'b2g' || os == 'android' I am guessing this was copied from the manifest in devtools/server. I'd like to start with a no skips first and see if that works. If it fails, feel free to add it back, but let's verify whether it's actually needed first. My guess is that the default skip in devtools/server is only actually needed for a few specific tests or something.

Jarda, thanks again for taking on this task! :D I am excited to see this land.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #105) > > +[test_websocket-server.html] > > Let's try running this test with skip-if = false to override the default > above. > > > +skip-if = buildapp == 'b2g' || os == 'android' > > I am guessing this was copied from the manifest in devtools/server. I'd > like to start with a no skips first and see if that works. I started a new try run with the changes you suggest. Also, exactly as I feared, the test_websocket-server.html test is consistently leaking objects, as the server connection is not closed quickly enough: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cb0b5cf8b30&selectedJob=25029756 I'll have to fix this before landing.

Fixed the leaks reported by tests. The input and output streams need to be closed explicitly when the WebSocket session is over. Otherwise, the stream copier from NS_AsyncCopy is leaking. I'll investigate the precise cause in a followup.

Run the websocket tests also on Android (and B2G) - removed the skip-if as described in comment 105.

Android tests are OK, leaks are fixed, going to land!

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/4c2e1433a462 Part 1: Make the nsITransportProvider interface scriptable r=sicking https://hg.mozilla.org/integration/fx-team/rev/03f498db6539 Part 2: Add static chrome-only method to create server-side WebSocket to WebIDL r=baku https://hg.mozilla.org/integration/fx-team/rev/05299f77fe38 Part 3: WebSocket server for WebSocket remote debugger transport r=jryans https://hg.mozilla.org/integration/fx-team/rev/7a112d070bbf Part 4: WebSocket-based debugger transport r=jryans https://hg.mozilla.org/integration/fx-team/rev/e5376bdccaa4 Part 5: Add WebSocket transport support to debugger client and server sockets r=jryans https://hg.mozilla.org/integration/fx-team/rev/0da07a12817c Part 6: Add WebSocket transport option to debugger server r=jryans https://hg.mozilla.org/integration/fx-team/rev/107fd4076b3c Part 7: Add WebSocket transport option to chrome debugger server r=jryans https://hg.mozilla.org/integration/fx-team/rev/bfb4f7467dd4 Part 8: Mochitest for WebSocket-based debugger transport r=jryans

https://hg.mozilla.org/mozilla-central/rev/4c2e1433a462 https://hg.mozilla.org/mozilla-central/rev/03f498db6539 https://hg.mozilla.org/mozilla-central/rev/05299f77fe38 https://hg.mozilla.org/mozilla-central/rev/7a112d070bbf https://hg.mozilla.org/mozilla-central/rev/e5376bdccaa4 https://hg.mozilla.org/mozilla-central/rev/0da07a12817c https://hg.mozilla.org/mozilla-central/rev/107fd4076b3c https://hg.mozilla.org/mozilla-central/rev/bfb4f7467dd4