For testing 441751 (and presumably others in the future), it would be useful to have access to an ssl server in xpcshell tests. I don't think we'd need anything fancy here, just "point https://localhost:8889/ at http://localhost:8888" or something like that.

This is base for ssl integration to xpc-shell tests. It has several problems: - ###!!! ASSERTION: mozHunspellDirProvider not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /mozilla/HG/src/extensions/spellcheck/hunspell/src/mozHunspellDirProvider.cpp, line 45 when opening outgoing socket nsCertOverrideService::Init calls NS_GetSpecialDirectory. - cert overrride service fails to add an exception (certOverrideService.rememberValidityOverride) probably because of missing database to import the certificate to - ssltunnel process could not be killed at the end because PR_KillProcess is not implemented on all platforms Thus, it seems we need a profile and thread safe directory provider to move with this.

A bit from finish. Issues left: - process killing doesn't work on windows - not yet tested on other platforms The patch is not dependent on providing a profile for xpc-shell tests (bug 459114) I renamed netwerk/test/httpserver/test to netwerk/test/httpserver/unit. This allows running a single test with check-one target. If there is serious issue with that I will revert it. Maybe createSecureServer should return some wrapper for https/http server with method stop() to be similar to 'normal' server. Opinions?

createSecureServer should return an object with the same API as createServer does, i.e. implementing nsIHttpServer. It's kind of useless for a lot of things, because otherwise you can't add new path->directory mappings, path handlers, or anything of that sort. The patch should also ideally write the cfg file to a temp file and pass its path when executing ssltunnel; it should be possible to run multiple secure servers at once, up to some limit, running through a preset list of ports and catching exceptions until a working one is found or the entire list is exhausted. It's possible to use SOLO_FILE to run the server tests, but it's slightly more complicated than the normal instructions. I have to recreate the commandline from the run_one.sh script every time I need to do it (rarely, actually).

That should be "If it doesn't, it's kind of...", of course.

Note that implementing the same API does mean you'll have to defer a number of operations by returning a closure, come to think of it; I hadn't considered that wrinkle until thinking about how normal server objects are created and used.

I don't agree or I don't understand. Why should createSecureServer return nsIHttpServer? That interface is not applicable to the secure server as the patch provides it. There is no way how to register path/dirs on it nor a way to add an identity because the secure server is just a "wrapper" for the http server and everything is still happening and can be driven on the http server. ssltunnel (actually, our implementation of the secure server) is just a stupid tool providing SSL overlap, it doesn't care about identities, path handlers at all. I would only understand to let the secure server instantiate its own http server and delegate all operations to it through that interface and let the http server be hidden (or somehow provided) to a user, but its nonsense to do this by default and as the only way because there are always situations we need both secure and insecure server with the same content. I agree (and I suggest it) to let createSecureServer return some kind of interface. Probably nsIHttpSecureServer or SecureWrapper or nsISSLTunnel (then also change name of the function) that takes http server instance and provides some functions like start(httpServer, securePort) and stop(). Probably nothing more. I don't see a point to have a port range or a list, unless we want to hack this way the process kill issue. Starting normal http server also takes just a single port number. I absolutely agree with the temp file for the config. Just have to figure out how to create it. Can you tell me how to use SOLO_FILE w/o moving the files? It's quit time-saving to run just a single file than all of them.

Er, oh. I misread createSecureServer and thought it was creating a new server rather than importing an existing one. Never mind me on that and most of the other issues! I'm not a fan of having to disconnect the process killing from the server stopping, but I guess it's not too difficult to make sure to do both. I still kind of like having it return its own server-like object, but if that means you'd have to add handlers to two different servers if you wanted to do side-by-side testing, it's not really going to work. find-waldo-now:~/moz/2 jwalden$ echo && make SOLO_FILE=test/test_host.js -C $OBJDIR/netwerk/test/httpserver/ check-one NATIVE_TOPSRCDIR='/Users/jwalden/moz/2' TOPSRCDIR='/Users/jwalden/moz/2' ../../../dist/bin/xpcshell -j -s -f /Users/jwalden/moz/2/tools/test-harness/xpcshell-simple/head.js -f ../../../_tests/xpcshell-simple/test_necko//test/head_utils.js -f ../../../_tests/xpcshell-simple/test_necko//test/test_host.js -f /Users/jwalden/moz/2/tools/test-harness/xpcshell-simple/tail.js -f /Users/jwalden/moz/2/tools/test-harness/xpcshell-simple/execute_test.js 2>&1 test_host.js: PASS

I created a cover class that drives the secure server similar to the http server, has start(port) and stop() methods and takes the existing http server in the constructor to overlay it. Added port r/o attribute to nsIHttpServer interface, quit useful. Config file for ssltunnel now contains the port number, so there can be more then just a single secure server run.

Comment on attachment 353137 [details] [diff] [review] v1 Hmm, so you're only adding stuff to head_utils.js -- and I definitely don't want to have to coordinate testing utility function changes to that file with tests across the codebase. Could you move all the https-wrapping stuff into a netwerk/test/httpserver/httpsd.js which includes a do_import_script("netwerk/test/httpserver/httpd.js")? Sorry for the delay on this, hopefully the next review iteration will go faster...

Must be landed after bug 468087.

Updated, merged and again tested patch.

Comment on attachment 356558 [details] [diff] [review] v2 >diff --git a/netwerk/test/httpserver/test/head_utils.js >+var isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes); This declaration needs to move into httpsd.js - otherwise it's undefined in non-httpserver tests.

(In reply to comment #12) > (From update of attachment 356558 [details] [diff] [review]) > >diff --git a/netwerk/test/httpserver/test/head_utils.js > >+var isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes); > > This declaration needs to move into httpsd.js - otherwise it's undefined in > non-httpserver tests. I know, I forget to move it, will create a new patch. But joke is that it work as is.

Just fixed the isWindows declaration position. Otherwise the same as v2.

Jeff, any chance to get this reviewed? Could someone else do it? The patch gets old.

Comment on attachment 360551 [details] [diff] [review] v2.1 Ted, could you please take a look at this patch? It's very old, probably will need refresh but it would be good have this in finally.

Comment on attachment 360551 [details] [diff] [review] v2.1 I'll steal, after all I dropped the ball on this long ago...

Jeff, thanks, I'll probably need it to create some ssltunnel specific tests that could not be done by mochitests.

Comment on attachment 360551 [details] [diff] [review] v2.1 Hate hate hate Bugzilla, I accidentally shut down my browser two-thirds of the way through this and lost everything, this is what I remember of what I wrote... >diff --git a/netwerk/test/httpserver/httpd.js b/netwerk/test/httpserver/httpd.js >+ get port() >+ { >+ return this._port; >+ }, Let's move this to the non-XPCOM public API section -- I'm not prepared to expose this more broadly, and it seems somewhat unnecessary, really. >diff --git a/netwerk/test/httpserver/httpsd.js b/netwerk/test/httpserver/httpsd.js >+ * // First, create HTTP server as usual Avoid sentence fragments in comments except when the comment is very short (single word or phrase, usually). Also, you need "the" after "create". >+ * // Now, overlay it with a secure server, pass the HTTP server as an argument, >+ * // all trafic will forward to it. "Create a secure server wrapping the non-secure server; all traffic to the secure server will be proxied to the insecure server." (note "traffic" and not "trafic") >+ * var secsrv = createSecureServer(srv); >+ * secsrv.start(4433); >+ * >+ * (... do a stuff, you can now connect "https://localhost:4433/something" ...) // Do stuff with the servers... "do a stuff" isn't grammatically correct; you'd want "do stuff" if you were to leave it as-is. >+ * // Stop both servers in reverse order i.e. make sure you stop >+ * // the secure server first! You should add some assertions to verify this. I don't think this requires any new code to be added to httpd.js. >+ * secsrv.stop(); >+ * srv.stop(); The server-stop API has changed, so this has bitrotted some. For that reason I'd prefer if the secure-server stop API functioned the same way the normal one does, in that execution should continue from a callback rather than synchronously on return. >+function nsHttpsServerOverlay(httpServer) >+{ >+ this.httpServer = httpServer; Needs /** docs */ inline like the other constructors in httpd.js have. >+nsHttpsServerOverlay.prototype = >+{ >+ _sslTunnelProcess: null, >+ _securePort: 0, >+ _configFile: null, These need /** docs */, as do all the methods here. >+ httpServer: null, This is unnecessary and indeed even confusing -- keep the per-instance property, lose the one on the prototype (one that never gets used, even). >+ start: function(httpsPort) >+ { The methods here could all use some dumpn("*** ...") method calls to make it easier to debug this functionality when looking at xpcshell test results (or other tests, if debug output is enabled). >+ // Make sure the underlying server port number is different from >+ // a secure port we want to bind. the secure port >+ // remeber the secure server port number >+ this._securePort = httpsPort; This can just be _port; the secure bit is redundant. Also: comment should be a complete sentence. >+ // Initiate the personall security manager first >+ var psm = Components.classes["@mozilla.org/psm;1"] >+ .getService(Components.interfaces.nsISupports); >+ do_check_neq(psm, null, "Could not initialize PSM"); personal This never gets used anywhere -- is it truly necessary? Why isn't some other step along the way responsible for initializing the PSM? This feels like the wrong place to be doing this. >+ // Get the current working directory, it is obj-dir/dist/bin >+ var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"] >+ .getService(Components.interfaces.nsIProperties); Style: align dots when doing things like this. Even better, use Cc/Ci and align the dots with the [ after Cc when you do so. (Bonus: use Cr rather than Components.results as well, in other places in the patch.) >+ var file; >+ >+ // Locate the xpc-shell secure server certificate database >+ file = do_get_file("tools/test-harness/xpcshell-simple"); Unify the declaration and the definition. Also, sentence fragment. But really, you don't need this here anyway -- just use do_get_file(...).path instead of using the file variable yet. >+ // Generate name of the ssltunnel program configuration file, >+ // it should be located in obj-dir/_tests/xpcshell-simple directory. Run-on sentence; you should use ';' rather than ',' to separate the two parts of the sentence. >+ // There can be stated multiple servers each on a separate port number. >+ this._configFile = DIST_BIN.parent.parent; >+ this._configFile.append("_tests"); >+ this._configFile.append("xpcshell-simple"); >+ this._configFile.append(".ssltunnel"+httpsPort+".cfg"); Style: need a space on each side of a binary operator like '+' (and elsewhere below). >+ // Fill the config file content >+ configFileStream = Components.classes["@mozilla.org/network/file-output-stream;1"] >+ .createInstance(Components.interfaces.nsIFileOutputStream); >+ configFileStream.init(this._configFile, -1, -1, 0); This is a global variable -- needs var! >+ sslConfig = >+ "listen:*:" + this._securePort + ":xpc-shell secure server\n"+ >+ "forward:127.0.0.1:" + this.httpServer.port + "\n"+ >+ "certdbdir:" + ssltunnelCertDB + "\n"; Another global variable! >+ // Locate ssltunnel binary >+ file = DIST_BIN; >+ if (isWindows) >+ file.append("ssltunnel.exe"); >+ else >+ file.append("ssltunnel"); The comment seems unnecessary. Also, you should clone DIST_BIN (you're mutating it, now). Also, this is clearer and less verbose: file.append(isWindows ? "ssltunnel.exe" : "ssltunnel") >+ // BadCertListener class used to automatically add an exception for >+ // the secure server self signed certificate >+ function badCertListener() As the comment says, yes, it really should be BadCertListener, not badCertListener. At a glance this entire "class" looks self-contained: it doesn't use any variables defined in its enclosing scope from this invocation of start(). SpiderMonkey shouldn't penalize you for doing it this way any more (a 3.5 fix), but it's unnecessary confusion; you should move this all to top level. But at a level above that: should we really be relying on this bad-cert behavior? It seems destined to cause problems eventually, and I can imagine it might mask problems that don't occur in the bad-cert case. What if the bug only occurs when not using the bad-cert listener, and worse, what if the test author doesn't know that? There must be a better way here to not have to hack over SSL connection issues this way. >+ badCertListener.prototype = Needs /** docs */ comments by all the members. >+ getInterface: function (aIID) Extra space between function and (. >+ QueryInterface: function(aIID) >+ { >+ if (aIID.equals(Components.interfaces.nsIBadCertListener2) || >+ aIID.equals(Components.interfaces.nsIInterfaceRequestor) || >+ aIID.equals(Components.interfaces.nsISupports)) >+ return this; My style preferences have changed over time; httpd.js is inconsistent about it, but I prefer bracing the body of an if (or an else) if the condition, body, or any subsequent bodies (in an if-else if-...-else chain) are multiline. >+ notifyCertProblem: function(socketInfo, sslStatus, targetHost) >+ { >+ this.connected = true; >+ >+ var cert = sslStatus >+ .QueryInterface(Components.interfaces.nsISSLStatus) >+ .serverCert; >+ >+ var certOverrideService = Components.classes["@mozilla.org/security/certoverride;1"] >+ .getService(Components.interfaces.nsICertOverrideService); >+ certOverrideService.rememberValidityOverride( >+ "localhost", >+ httpsPort, >+ cert, >+ certOverrideService.ERROR_UNTRUSTED, >+ true); You don't lose anything with a much shorter name like "cos" here, and you can avoid this ugly wrapping/indentation of method arguments. >+ // Get override service first here on the main thread to let it be >+ // initialized. If we get it the first time in bad cert listener it would >+ // access some non-thread safe services on the ssl thread. >+ var certOverrideService = Components.classes["@mozilla.org/security/certoverride;1"] >+ .getService(Components.interfaces.nsICertOverrideService); Oh, yuck. Are you saying this code runs on a thread other than the main thread? That's going to cause GC to occur on a thread other than the main thread sometimes, and that's bad -- assertions at a minimum, and even worse consequences if you get unlucky. >+ // Let's try to connect the secure server 20 times. When connection succeeds >+ // ssltunnel process is up and we can try to add an exception for its certificate. Oh gag. There must be a better way. Also note that 20x is probably not a sufficiently long wait -- debug builds sometimes time out on server startup after 45s, and I can easily believe they'll overrun this 20x limit. >+ var certListener = new badCertListener(); >+ for (var retry = 0; >+ retry < 20 && !certListener.connected; >+ ++retry) >+ { >+ // Connect the ssl server and add an exception for its certificate Capitalize ssl when it's in a comment, please. >+ if (certListener.done) >+ // We successfully added the server cert exception what indicates >+ // the server is up and we are ready to use it. Add this new server >+ // as an identity to the underlying http server to let it accept >+ // connections to the secure server. >+ this.httpServer.identity.add("https", "localhost", this._securePort); >+ else >+ this.stop(); Both parts of the if should be braced, because the body for the if is multiline. >+ stop: function() >+ { >+ // Send SSLTUNNELSHUTDOWN command to the ssltunnel server to indicate >+ // we want to shut it down. Because ssltunnel is configured for a direct >+ // forwarding we have to send a secure request with method as a command >+ // string. This seems like it should be handled at the same time CONNECT is handled in ssltunnel -- not once the connection has been wrapped and handshaked. The ssltunnel.cpp changes seem to implement that -- it looks like if I did 'echo "SSLTUNNELSHUTDOWN / HTTP/1.1" | nc localhost:4443' or somesuch I could shut down the server that way, before any handshaking occurred. This comment, and the URL used with the XHR, suggest otherwise. What am I missing? >diff --git a/netwerk/test/httpserver/nsIHttpServer.idl b/netwerk/test/httpserver/nsIHttpServer.idl >+ /** >+ * Gets the port number this server listens to new connections on. It is the >+ * port number passed to start function. >+ */ >+ readonly attribute long port; As mentioned earlier, I don't think this should be added yet. >diff --git a/netwerk/test/httpserver/test/head_utils.js b/netwerk/test/httpserver/test/head_utils.js >+/** >+ * Creates a secure HTTPS server as overlay of an already established >+ * and runnign HTTP server. For now it is just an inline script. >+ * See netwerk/test/httpserver/httpsd.js for example and help how to >+ * use and establish the secure server. >+ */ >+function createSecureServer(httpServer) >+{ >+ return new nsHttpsServerOverlay(httpServer); > } The second sentence doesn't seem necessary, indeed seems to expose implementation details more than anything else. Don't use the 'ns' prefix. Someday I'm going to get around to removing it from HttpServer, actually, although that's going to be a bit tedious, I'm sure. >diff --git a/netwerk/test/httpserver/test/test_secure_server.js b/netwerk/test/httpserver/test/test_secure_server.js >+ function done() >+ { >+ srv.stop(); >+ } >+ >+ runHttpTests(tests, done); Here's an obvious bit of bitrotting. Anyway, sorry for the long delay, just takes a long time to get into this stuff, and the right way isn't as quickly apparent here as it is for some other things, which makes reviews more tricky.

Looks like this stalled - how much work is left here? Would be nice to get this fixed.

(In reply to comment #20) > Looks like this stalled - how much work is left here? Would be nice to get this > fixed. I have to merge the patch, not sure at the moment how much work is needed. I probably won't get to this sooner then in weeks.

Honza, I would like to take this over as I need it for my other work. EKR is also working on testing things that need DTLS support, which is a somewhat similar problem. I believe that we can make this all work without stunnel, since we already have the server-side SSL functionality in libssl and we'll be using that server-side SSL functionality for WebRTC anyway. I have already started an NSS-level in-process testing framework for libssl in bug 702322. My plan is to try to extend that libssl loopback framework so that we can run all the NSS loopback tests through our extensions to libssl (nsNSSIOLayer and our custom certificate verification cod). If that is too complicated, then I will do something simpler. One important question: Do we really need this to be available for xpcshell, or is it acceptable to require that SSL xpschell tests to be native C++ tests?

(In reply to Brian Smith (:bsmith) from comment #22) > One important question: Do we really need this to be available for xpcshell, > or is it acceptable to require that SSL xpschell tests to be native C++ > tests? Thunderbird performs its network unit tests from xpcshell. They are backed by "fake" server implementations, currently without SSL (although bug 662180 wants to introduce SSL support and tried using stunnel). Higher level tests (like autoconfiguration) are run using mozmill which is somewhat like mochitest (Thunderbird runs with some test harness logic up in its business) and so would benefit from scriptable XPCOM access to the mechanism. Being able to realistically test SSL and the failure modes of bad certificates would be desirable. The B2G e-mail client is in a similar boat, although xpcshell is probably right out. Having said that, those testing issues could also probably be dealt with by using JS mock objects that can imitate the key network features under test (startTLS) and certificate failure modes. The downside is the potential divergence between what is being simulated and the necko reality would not be caught by the automated tests.

I suspect it will not be TOO problematic to build scriptable tests (xpcshell or whatever) on top of the SSL loopback test framework. I will try to make it so.

Note that none of the work in this bug uses *stunnel* at all. The patch here, along with our existing Mochitest harness, uses *ssltunnel*, a simple SSL proxy that I cobbled together that uses NSS: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/ssltunnel/

After talk with Brian during the Necko meeting today I'm returning to finish this as a short term solution to allow at least some test for ssl in xpcshell ASAP.

This is a WIP of the new version: - fixes ssltunnel to work again in non-proxy mode (will be moved to a separate bug) - reintroduces ssltunnel shutdown by a simple process kill (seems to work now in windows on my machine) - is untested and probably doesn't work in remote env (needs to be finished in this bug) - has a simple workaround for bug 760608 - I add the server's cert CA cert as a trusted cert to the profile nss db (no bad cert hook hacking anymore) So, this works and can be used for basic testing of ssl in xpcshell tests.

Full patch. Try https://tbpl.mozilla.org/?tree=Try&rev=22c69c572f75 seems to be passing :)

Happy to early... the secure server test fails on OS X 10.8 optimized build. Will try to reproduce locally. HTTPSD-INFO | ssltunnel path: /builds/slave/talos-slave/test/build/FirefoxNightly.app/Contents/MacOS/ssltunnel HTTPSD-INFO | Listening to new SSL connections on port 4443, pid=3118 ... TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/netwerk/test/httpserver/test/test_secure_server.js | 0 == 2 - See following stack: JS frame :: /builds/slave/talos-slave/test/build/xpcshell/tests/netwerk/test/httpserver/test/test_secure_server.js :: do_check_security_state :: line 109 JS frame :: /builds/slave/talos-slave/test/build/xpcshell/tests/netwerk/test/httpserver/test/test_secure_server.js :: onsecstart :: line 159

I *think* we are dep on bug 766166. OSX is slower with starting ssltunnel (better said: starting the server socket) and thus we get immediate RST on the necko level since nothing listens on the given port. Linux errors are caused by other server already running on 4443 port.

Comment on attachment 669746 [details] [diff] [review] v3 (full) Jeff, changing the request to just a feedback. I would like to know from you whether you like the overall approach.

Comment on attachment 669746 [details] [diff] [review] v3 (full) Review of attachment 669746 [details] [diff] [review]: ----------------------------------------------------------------- Honza, this is great. I am looking foward to getting this r+d and checked in. ::: netwerk/test/httpserver/httpsd.js @@ +1,4 @@ > +/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim:set ts=2 sw=2 sts=2 et: */ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 Wrong license. @@ +53,5 @@ > + * secsrv.start(4433); > + * > + * // You can now connect "https://localhost:4433/something". > + * > + * // Stop the secure server only, it will shut the HTTP server down it self. s/it self/itself/ @@ +85,5 @@ > + this._port = httpsPort; > + > + // This is pregenerated CA certificate we add here as trusted to the > + // certificate database. ssltunnel server is configured with a cert > + // singned with this certification authority cert. We will need to be able to choose a different cert sometime. It would be better to load the cert's DER encoded file off of disk, because having the base64-encoded version here makes it very difficult to inspect the contents of the test certificate. I would avoid having a default certificate. I would instead require that a certificate be required to be passed in during initialization. ::: netwerk/test/httpserver/test/test_secure_server.js @@ +15,5 @@ > + new Test("https://localhost:" + SECUREPORT + "/securetest", initchannel, onsecstart, onsecstop) > +]; > + > +var test2 = [ > + new Test("https://localhost:" + SECUREPORT + "/securetest", initchannel, null, onfailstop) We should have a test with a non-localhost hostname and with a non-default certificate. @@ +27,5 @@ > + srv = createServer(); > + srv.registerPathHandler("/securetest", handler); > + srv.start(PORT); > + > + secsrv = createSecureServer(srv); createSecureServerTerminator?

Using the control service to wait for an actual ssltunnel servers start. More for a reference here, Brian, I will address your comments after this gets tuned. Try: https://tbpl.mozilla.org/?tree=Try&rev=7e49e50e189f

This is the one (qfold'ed to interdiff).

(In reply to Brian Smith (:bsmith) from comment #32) > We will need to be able to choose a different cert sometime. > > It would be better to load the cert's DER encoded file off of disk, because > having the base64-encoded version here makes it very difficult to inspect > the contents of the test certificate. > > I would avoid having a default certificate. I would instead require that a > certificate be required to be passed in during initialization. Are you OK to this in a followup? > We should have a test with a non-localhost hostname and with a non-default > certificate. Why?

(In reply to Honza Bambas (:mayhemer) from comment #35) > > I would avoid having a default certificate. I would instead require that a > > certificate be required to be passed in during initialization. > > Are you OK to this in a followup? Sure. > > We should have a test with a non-localhost hostname and with a non-default > > certificate. > > Why? First, it is a long-term goal of ours to not trust certificates for "localhost" at all, because "localhost" doesn't actually identify any particular server. That means eventually all tests that use "localhost" or any unqualified name should fail. But, we can fix this when we get to it.

Try: https://tbpl.mozilla.org/?tree=Try&rev=4f888e3be240 - new createSecureServerTerminator function wrapping http server that returns an object that handles all ssltunnel configuration and instantiation - traffic is redirected to an http server we already have (similar system as mochitest has now) - for now there is just a single "localhost" certificate, everything is placed in a single fixed database ; this should change in a followup bug to support custom certificates - start-up needs to be async since ssltunnel process has a slow start ; the ssltunnel control service protocol is used to achieve that - ssltunnel is killed (now works on all platform) ; can be changed in a followup to use the service control protocol to do the shutdown - has an automated test that checks we are able to connect the server after it started w/o any errors and that we are not able to connect it when the server is to be shut down

Comment on attachment 681474 [details] [diff] [review] v4.1 Review of attachment 681474 [details] [diff] [review]: ----------------------------------------------------------------- I'm not really wild with copying the method we use to get httpd.js, since it's kind of crappy. Is there a cleaner way to do this? (If not I'll allow it, but httpd.js is the way it is simply for historical reasons.) ::: netwerk/test/httpserver/Makefile.in @@ +23,1 @@ > $(NULL) Can you change the indent to two spaces while you're here? ::: netwerk/test/httpserver/test/head_utils.js @@ +8,5 @@ > * Loads _HTTPD_JS_PATH file, which is dynamically defined by > * <runxpcshelltests.py>. > */ > load(_HTTPD_JS_PATH); > +load(_HTTPSD_JS_PATH); I'd rather not propogate more magic constants here. ::: testing/xpcshell/head.js @@ +811,5 @@ > > let command = > "const _HEAD_JS_PATH='" + _HEAD_JS_PATH + "'; " > + "const _HTTPD_JS_PATH='" + _HTTPD_JS_PATH + "'; " > + + "const _HTTPSD_JS_PATH='" + _HTTPSD_JS_PATH + "'; " Can we make this a testing module or something less hacky? The httpd.js support here is pretty terrible.

Honza, will you have time to finish this up soon? Is there anything I can do to help?

(In reply to Brian Smith (:bsmith) from comment #39) > Honza, will you have time to finish this up soon? Is there anything I can do > to help? It's funny like this bug is priority only for short time and then everybody forget about it. If you need it, take it and finish it or tell me directly this is a priority right now and I'll jump on it. I have more priority things to do right now, but still this is in the main task list I want to work on.

Until someone expresses a strong desire to add this support I'm releasing this bug.