We'll be adding support for WebSocket tests to mochitest, via the mod_pywebsocket library, http://code.google.com/p/pywebsocket/.

The most recent version of mod_pywebsocket, 0.5, http://code.google.com/p/pywebsocket/downloads/detail?name=mod_pywebsocket-0.5.tar.gz&can=2&q=, does not work with our WebSocket implementation. When you create a new WebSocket, the pywebsocket server throws an error because the Sec-WebSocket-Key1 header is not found. Our implementation does work with mod_pywebsocket 0.4.9.2. Shall I integrate with this older version, or will our implementation be updated?

(In reply to comment #1) > Our implementation does work with mod_pywebsocket 0.4.9.2. Shall I integrate > with this older version, or will our implementation be updated? Our older version is already reviewed and tested, so I think it should be the best one for now. I'm just to submit the v76 of the protocol, so if you can test it it would be so great.

v76 patch is in bug 562681.

The WebSocket part of this works well with mochitest. The difficulty I am having is with the WebSocket url's. We don't want hard-coded unmapped url's in tests (like ws://127.0.0.1:9999/foo), since this will break remote tests (like on android). For http/https, we solve this by using the PAC proxy config to dynamically select proxies for the requests. We can do the same thing for ws:// url's, but the problem is that when a proxy is used for ws:// url's, it switches to using HTTP CONNECT, which mod_pywebsocket definitely doesn't support. I can think of two ways to deal with this: 1 - add a way to dynamically generate ws:// url's for tests. One way would be to add a helper method to SimpleTest that reads the host:port for ws:// url's from a pref, set in automation.py. 2 - use a simple HTTP proxy that sits between the browser and mod_pywebserver and forwards requests; there seem to be a few python proxies and I could probably use one of these. Joel, Ted, any opinions on what the best approach might be? We also need a way to test WebSockets via SSL. I might be able to use ssltunnel for this in some fashion, forwarding to the mod_pywebsocket server for wss:// requests, either by adding some code to ssltunnel to handle this or running a concurrent instance just to handle WebSocket requests. Wellington, have you tested WebSockets with SSL and can you verify we support it?

maybe I don't understand this setup very well, but do we have a second webserver setup to support websocket? Is the HTTP CONNECT issue when using proxy auto config an issue with Firefox (bug or expected behavior)? The two solutions you have, we might be able to do #1 similar to this: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_prompt_async.html?force=1#91 (this will require some changes for e10s) I don't like the idea of yet another program running, so unless we can reuse the ssltunnel for all cases, I would lean towards the #1 solution.

httpd.js currently functions as both a proxy and a web server. ssltunnel also functions as a proxy, just because it has to. They both accept the HTTP CONNECT request and forward it on. I don't think httpd.js knows how to proxy things to a different local address, but that might be something we could add support for. It certainly sounds less complicated than adding more moving parts to our test harness.

> Wellington, have you tested WebSockets with SSL and can you verify we support it? Yes, I have. It worked in my tests.

This patch adds support for ws:// url's to mochitest. It passes on tryserver. SSL support will be added in a follow-up patch.

Olli, the patch in comment #8 contains 1 simple test; I put it in content/base since most of the code changes seem to be there, but I'm not really sure where it should go. Do you have a location preference? Olli/Wellington, do you have existing tests that can be converted to mochitest, that already use mod_pywebsocket?

content/base/test is exactly the right place for the tests. I have some tests on my linux machine. I could convert those to mochitest tomorrow or on Friday.

I see we have a dependency on md5. I believe this isn't supported by talos boxes, but if it passed on try that would indicate the regular tinderbox machines have the appropriate version and library set of python.

> Olli/Wellington, do you have existing tests that can be converted to mochitest, > that already use mod_pywebsocket? I have one test that uses mochitest in bug 546616. However I didn't write the server stuff there using mod_pywebsocket. Also I have some other better tests, but they don't use mochitest. I will convert them.

md5 is a standard Python module, so if Talos doesn't have it, then Talos has a broken Python...

And as far as I understand the new try server, it's running the talos slaves just like the normal m-c does, so we should be ok. Otherwise, try server is broken :)

Comment on attachment 451515 [details] [diff] [review] mochitest patch >diff --git a/build/automation.py.in b/build/automation.py.in >--- a/build/automation.py.in >+++ b/build/automation.py.in >@@ -53,16 +53,19 @@ import tempfile > > SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0]))) > sys.path.insert(0, SCRIPT_DIR) > import automationutils > > _DEFAULT_WEB_SERVER = "127.0.0.1" > _DEFAULT_HTTP_PORT = 8888 > _DEFAULT_SSL_PORT = 4443 >+_DEFAULT_WEBSOCKET_SERVER = "127.0.0.1" >+_DEFAULT_WEBSOCKET_PORT = 9999 >+_DEFAULT_WEBSOCKET_PROXY_PORT = 7777 Expanding the number of open ports needed kinda sucks. Can you file a followup on maybe making this better? We should be able to let most of these servers pick a free local port, and then report back what port they're using, and then just write that info to the PAC we stick in the profile. Not something you need to fix now, but this is going to be more of a pain if people have random daemons running on these ports. Also, I think we should just tie the web server and websocket server to the same address. Even in the remote test scenario nobody is going to want to run them on different machines. >@@ -370,34 +385,39 @@ function FindProxyForURL(url, host) > '(?:[^/@]*@)?' + > '(.*?)' + > '(?::(\\\\\\\\d+))?/'); > var matches = regex.exec(url); > if (!matches) > return 'DIRECT'; > var isHttp = matches[1] == 'http'; > var isHttps = matches[1] == 'https'; >+ var isWebSocket = matches[1] == 'ws'; > if (!matches[3]) > { > if (isHttp) matches[3] = '80'; > if (isHttps) matches[3] = '443'; >+ if (isWebSocket) matches[3] = '80'; if (isHttp || isWebSocket) Would it make more sense here to just treat ws:// URLs as http:// for the purpose of proxying? That way we'll be able to write WS mochitests using any of the http:// domains in server-locations.txt (and you wouldn't have to add any ws:// URLs to server-locations). I'm thinking something like: if (isWebSocket) matches[1] = 'http'; Do you know how WebSockets-over-SSL gets handled? Is there a different protocol scheme or what? >@@ -434,16 +454,18 @@ user_pref("camino.use_system_proxy_setti > > # Create head of the ssltunnel configuration file > sslTunnelConfigPath = os.path.join(profileDir, "ssltunnel.cfg") > sslTunnelConfig = open(sslTunnelConfigPath, "w") > > sslTunnelConfig.write("httpproxy:1\n") > sslTunnelConfig.write("certdbdir:%s\n" % certPath) > sslTunnelConfig.write("forward:127.0.0.1:%s\n" % self.httpPort) >+ sslTunnelConfig.write("proxy:%s:%s:%s\n" % >+ (self.webSocketProxyPort, self.webSocketServer, self.webSocketPort)) Hrm. So you need to listen on a separate port for these connections? Is there any reason ssltunnel can't just listen on the same port and distinguish by protocol? >diff --git a/content/base/test/test_websocket_hello.html b/content/base/test/test_websocket_hello.html >new file mode 100644 >--- /dev/null >+++ b/content/base/test/test_websocket_hello.html >@@ -0,0 +1,57 @@ >+var ws; >+var originalPref; >+ >+function testWebSocket () { >+ netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect"); >+ var prefs = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch); >+ originalPref = prefs.getBoolPref("network.websocket.enabled"); >+ prefs.setBoolPref("network.websocket.enabled", true); I wouldn't bother with this. If we're going to land this it should be with websockets enabled. >+ >+ ws = new WebSocket("ws://mochi.test/tests/content/base/test/file_websocket_hello"); >+ ws.onopen = function(e) { >+ ws.send("data"); >+ } >+ ws.onclose = function(e) { >+ } >+ ws.onmessage = function(e) { >+ is(e.data, "Hello world!"); >+ ws.close(); >+ netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect"); >+ prefs.setBoolPref("network.websocket.enabled", originalPref); >+ SimpleTest.finish(); >+ } You probably want an onerror here that fails the test and calls SimpleTest.finish(); >diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in >--- a/testing/mochitest/Makefile.in >+++ b/testing/mochitest/Makefile.in >@@ -44,16 +44,17 @@ relativesrcdir = testing/mochitest > include $(DEPTH)/config/autoconf.mk > > DIRS = MochiKit \ > static \ > dynamic \ > tests \ > chrome \ > ssltunnel \ >+ pywebsocket \ > $(NULL) Don't bother with sub-makefiles. Just put the rules to copy the python files in this Makefile, like: _PY_FILES = \ pywebsocket/standalone.py \ ... $(NULL) libs:: $(INSTALL) $(foreach f,$(_PY_FILES),"$f") $(DEPTH)/_tests/testing/mochitest/pywebsocket/ >diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in >--- a/testing/mochitest/runtests.py.in >+++ b/testing/mochitest/runtests.py.in >+class WebSocketServer(object): >+ "Class which encapsulates the mod_pywebsocket server" >+ >+ def __init__(self, automation, options, scriptdir): >+ self.port = options.webSocketPort >+ self._automation = automation >+ self._scriptdir = scriptdir >+ >+ def start(self): >+ script = os.path.join(self._scriptdir, 'pywebsocket/standalone.py') >+ cmd = ['python', script, '-p', str(self.port), '-w', self._scriptdir, '-l', os.path.join(self._scriptdir, "websock.log"), '--log-level=debug'] Use sys.executable, not 'python'. Would it make sense to just import the classes and run this on a separate thread, or is that overkill? >diff --git a/testing/mochitest/ssltunnel/ssltunnel.cpp b/testing/mochitest/ssltunnel/ssltunnel.cpp >--- a/testing/mochitest/ssltunnel/ssltunnel.cpp >+++ b/testing/mochitest/ssltunnel/ssltunnel.cpp >- void* c = PL_HashTableLookup(server_info->host_cert_table, token); >+ void* c = NULL; >+ if (server_info->host_cert_table) >+ c = PL_HashTableLookup(server_info->host_cert_table, token); Is there any reason not to just stick empty hash tables in these fields for your proxy server's server_info_t, to avoid having to NULL check them all the time? >@@ -292,16 +303,19 @@ bool ReadConnectRequest(server_info_t* s > } > > *result = 200; > return true; > } > > bool ConfigureSSLServerSocket(PRFileDesc* socket, server_info_t* si, string &certificate, client_auth_option clientAuth) > { >+ if (si->no_ssl) >+ return true; Seems odd that we'd even call this function if the server doesn't want SSL. Can't you just fix the two call sites? > /** >+ * This function examines the buffer for a S5ec-WebSocket-Location: field, >+ * and if it's present, it replaces the hostname in that field with the >+ * value in the server's original_host field. This function works >+ * in the reverse direction as AdjustHost(), replacing the real hostname >+ * of a response with the potentially fake hostname that is expected >+ * by the browser (e.g., mochi.test). >+ * >+ * @return true if the header was adjusted successfully, or not found, false >+ * if the header is present but the url is not, which should indicate >+ * that more data needs to be read from the socket >+ */ >+bool AdjustWebSocketLocation(relayBuffer& buffer, server_info_t *si) >+{ >+ assert(buffer.margin()); >+ buffer.buffertail[1] = '\0'; >+ >+ char* wsloc = strstr(buffer.bufferhead, "Sec-WebSocket-Location:"); >+ if (!wsloc) >+ return true; >+ wsloc += 29; Comment explaining this magic number? Is this to skip "Sec-WebSocket-Location: ws://" ? Is the amount of whitespace there guaranteed not to change? >+ char* wslocend = strstr(wsloc + 1, "/"); strchr? >+ if (!wslocend) >+ return false; >+ char *crlf = strstr(wsloc, "\r\n"); >+ if (!crlf) >+ return false; >+ if (si->original_host.empty()) >+ return true; >+ >+ memmove(wsloc, wsloc + (wslocend-wsloc), buffer.buffertail - wsloc); >+ buffer.buffertail -= (wslocend-wsloc); >+ >+ assert(si->original_host.length() <= buffer.margin()); >+ memmove(wsloc + si->original_host.length(), wsloc, buffer.buffertail - wsloc); >+ >+ memcpy(wsloc, si->original_host.c_str(), si->original_host.length()); >+ buffer.buffertail += si->original_host.length(); >+ return true; I commented about this on IRC, but you should be able to do this with one memmove. >+} >+ >+/** >+ * This function examines the buffer for a Host: field, and if it's present, >+ * it replaces the hostname in that field with the hostname in the server's >+ * remote_addr field. This is needed because proxy requests may be coming >+ * from mochitest with fake hosts, like mochi.test, and these need to be >+ * replaced with the host that the destionation server is actually running spelling: "destination" >+bool AdjustHost(relayBuffer& buffer, server_info_t *si) >+{ >+ assert(buffer.margin()); >+ >+ // Cannot use strnchr so add a null char at the end. There is always some >+ // space left because we preserve a margin. >+ buffer.buffertail[1] = '\0'; >+ >+ char* host = strstr(buffer.bufferhead, "Host:"); >+ if (!host) >+ return false; >+ host += 6; More magic numbers... >+ >+ char* endhost = strstr(host, "\r\n"); >+ if (!endhost) >+ return false; >+ >+ // Save the original host, so we can use it later on responses from the >+ // server. >+ si->original_host.assign(host, endhost-host); >+ >+ memmove(host, host + (endhost-host), buffer.buffertail - host); >+ buffer.buffertail -= (endhost-host); >+ >+ char newhost[40]; >+ PR_NetAddrToString(&si->remote_addr, newhost, sizeof(newhost)); >+ assert(strlen(newhost) < sizeof(newhost) - 7); >+ sprintf(newhost, "%s:%d", newhost, PR_ntohs(si->remote_addr.inet.port)); >+ >+ size_t hostlength = strlen(newhost); >+ assert(hostlength <= buffer.margin()); >+ >+ memmove(host + hostlength, host, buffer.buffertail - host); >+ memcpy(host, newhost, hostlength); >+ buffer.buffertail += hostlength; >+ return true; Seems like you could probably factor out the ugly memmove stuff into a common helper for these two functions, just pass in the start and end points of the original string, the buffer, and the new string. Is it worth it? >+ if (s == 0 && expect_request_start) >+ { >+ if (!ci->server_info->no_ssl) >+ expect_request_start = !AdjustRequestURI(buffers[s], &fullHost); >+ else >+ expect_request_start = !AdjustHost(buffers[s], ci->server_info); Might be easier to read if you invert the conditions in this if. Reading "if not no ssl" is a little weird. >+ if (!strcmp(keyword, "proxy")) >+ { >+ server_info_t server; >+ server.host_cert_table = NULL; >+ server.host_clientauth_table = NULL; >+ server.no_ssl = true; I wonder if this field would be better off as "http_proxy_only" or something similar like that. r=me with those fixes.

This patch addresses all of ted's comments, except the following: > Hrm. So you need to listen on a separate port for these connections? Is there > any reason ssltunnel can't just listen on the same port and distinguish by > protocol? We can do this, but it requires more surgery to ssltunnel. Both WebSocket and other traffic initiate a connection with an HTTP CONNECT, at which point it is impossible to distinguish the type of tunneled traffic, and ssltunnel immediately makes a connection to the remote server. In order to be able to split traffic based on protocol, we'll need to wait to do this until we've received the next complete set of headers. I'm nervous about breaking some other tests, so I'm going to land as-is and file a followup bug for this work. Also I'm splitting the test case into a separate patch which will land later, as the test infrastructure needs to land prior to the actual WebSocket code.

Test infrastructure pushed as http://hg.mozilla.org/mozilla-central/rev/c69b38b80f63

The simple mochitest misses Makefile.in changes. But even with those I can't get websockets working. I did enable network.websocket.enabled. Jonathan, is there anything special to get websockets running. Though, perhaps the problem is somewhere in the merge.

Using standalone pywebsocket locally does work.

The test framework is using the latest websocket protocol, so you need the patch from bug 562681. Were you using it?

Added missing Makefile changes

Ugh, my problem was that something is using port 9999.

Olli ran into some problems with this patch on his machine; it seems that port 9999 is used by default by something on fedora. I changed the default websocket server port to 9988 and pushed as http://hg.mozilla.org/mozilla-central/rev/3490334dbc71. I have a followup bug, bug 572572, to allow servers like this to select their ports dynamically.

http://hg.mozilla.org/mozilla-central/rev/403e9f69ec1a I pushed the test as an example how to write websocket tests.