544097 - allow for tests to not require localhost:8888

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org

most tests are hardcoded for localhost:8888.  We need to remove this hardcoded special value and use a PAC value (I am using mochitest.org:8888).  This allows us to change the proxy value to a remote server and not depend on localhost or 127.0.0.1.

Changes also have to be made to httpd.js to support a dymanic ip.

For the xpcshell tests, there are a lot of changes required and in this case I have sent 'localhost' as the server which httpd listens on.  As it stands, I don't know of a way to load the xpcshell tests via a remote webserver, so this seems acceptable from a remote testing point of view.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (2)

update for bitrot

Comment 2

[Jeff Walden [:Waldo]]

We should not use a domain name like mochitest.org which might actually be used.  If you're going to replace like this (I've thought of doing so but never thought it worth the effort), please switch to a name that's reserved.  So far we've only used names which are reserved and which can't represent sites in the real world -- everything's under example.com, example.org, or an IDN version of example.test, and all these names are reserved for specific use outside the general naming system.  I'd prefer a name like example.com or example.org as a minimal name -- mochitest.org could refer to a real site (indeed I'm surprised it doesn't now).  Another possibility: RFC 2606 reserves .test, so you could use mochi.test for the name instead.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 426025 [details] [diff] [review]
patch to replace localhost with mochitest.org (2)

It is not acceptable to change httpd.js to accept non-loopback connections, not in a way that's not configurable when the server is created or started.  Exposing every user of the server to arbitrary inbound connections isn't acceptable, given that this code is probably being used by people in situations where they depend on controlling exactly what connections are made.

I don't think the way you set the server's address is acceptable, either -- typeof a name that gets randomly sourced in is not a clean solution.  If it's necessary to do something like this, modify (or extend, with the current API specifying a default) the server-start API to allow a server name to be specified.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

I agree that mochitest.org is not a good idea.  I will find a better 'domain name' to use.  

As for the non-loopback connnections, that is an easy fix.  The one thing I am struggling with is the host.  Right now mochitest and xpcshell are launched from python via a command line string.  There are 2 ways I can think of to get the host name:
1) via cli parameter '-e const SERVER=127.0.0.1'
2) via file; echo 127.0.0.1 > servername.txt

Adding a variable in the start function is an option, but we still need to get the dynamic ip from somewhere.  If the concern is around just checking if a variable is defined, what about doing something more along the lines of this:

if (typeof(SERVER) != "undefined") {
  //it equals a valid a.b.c.d ip address or 'localhost'
    gServerAddr = SERVER;
}

I am open to other ideas, but appreciate the feedback and concerns so far!

Comment 5

[Jeff Walden [:Waldo]]

The IP address can still be specified through the command line, that's not the problem.  However -- that value should be used in testing/mochitest/server.js and specified at server startup, something like so:

  // -e 'const SERVER_IP = "127.0.0.1";'
  var server = new nsHttpServer();
  server.start(4444, SERVER_IP);

server.js may infer an IP address in this way, but httpd.js should not.  It should require explicit specification of the IP address (possibly defaulting it to 127.0.0.1, but definitely not typeof-checking for the existence of a magical name).

Please do r? me on this, especially if I was insufficiently clear in the above explanation.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (3)

updated patch to use mochi.test and not have httpd.js get values from randomly undocumented variables.

Comment 8

[[e-mail]]

Your changes to some of the cookie tests are subverting the point of those tests: to make sure that loads from IP addresses and aliases such as 'localhost' work in an intended fashion.

Do you have an IP address that can be used in place of 127.0.0.1?

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I had posed this question originally and was not sure if I was invalidating any tests.  Are these cookie tests the 'extensions/cookie/test/*' files?

The majority of the changes I made are to postMessage() calls, but in the example of the extension/cookie/test/test_same_base_domain_*.html files I load file_localhost_inner and file_loopback_inner via the mochi.test method.  

ATM there is no other ip address.  One thought I had here was to identify all of these tests which are really testing 127.0.0.1 or localhost and ignore those tests when testing on remote systems.  

Is there something special about localhost with cookies, or can it just use a different domain name?  For example we have example.com that we use in many tests.  I can also add code to pull the real proxy ip address and use that so we can test with mochi.test and the raw proxy (I had to do this for: toolkit/components/passwordmgr/test/test_prompt_async.html)

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 427633 [details] [diff] [review]
patch to replace localhost with mochitest.org (3)

We need the ability to refer to *some* IP address numerically.  Whether it's 127.0.0.1 or 1.2.3.4 (but of course only an idiot would use that IP address) doesn't really matter.  It would be much preferable to have one canonical IP address to use for this stuff; I don't know if PAC intercepts attempts to directly load IP addresses, tho.  If it does, we should be able to map in 127.0.0.1 to whatever the actual address is.  Has that been tried?  At least on my system the connection settings dialog has a No proxy for: field filled with localhost and 127.0.0.1, so that suggests 127.0.0.1 is PAC-able.

Another test that relies on IP address access is dom/tests/mochitest/dom-level0/test_setting_document.domain_to_shortened_ipaddr.html.  Specifically, read dom/tests/mochitest/dom-level0/child_ip_address.html and actually try to understand it -- clearly, just changing from 127.0.0.1 to mochi.test completely subverts the intent of the test.

r- at least until the IP address conundrum is resolved; we need to be able to refer to at least one canonical IP address the same way we can refer to mochi.test with this patch.  A statically-known IP address like 127.0.0.1 would be vastly preferable, but if it has to be a dynamically determined IP address I guess that'll have to do.

Comment 11

[[e-mail]]

(In reply to comment #9)
> I had posed this question originally and was not sure if I was invalidating any
> tests.  Are these cookie tests the 'extensions/cookie/test/*' files?

Yup.

> The majority of the changes I made are to postMessage() calls, but in the
> example of the extension/cookie/test/test_same_base_domain_*.html files I load
> file_localhost_inner and file_loopback_inner via the mochi.test method.  

Yeah. The feature this is testing is third-party cookie blocking, which compares the URI of the load to the URI of the toplevel content docshell. This logic has separate cases for handling things like "localhost" (a host alias that doesn't fit the typical site.tld form), IP addresses, and the usual site.tld.

Which means that disallowing 'localhost' in mochitests will mean said functionality is not tested, across the entire browser. This scares me. The fact that we're not testing any browser functionality here seems like a bad idea, given that we increasingly rely on our automated tests to catch regressions.

For the purposes of cookies, any alias not containing a '.' will do. Is this possible?

Comment 12

[Jeff Walden [:Waldo]]

Sure, we can map "test" as well, or really any name in RFC 2606 (although we really should steer clear of invalid for sanity purposes).

Comment 13

[[e-mail]]

Sounds good. I'd like to stamp the cookie bits once the patch is up, just to be sure.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

thanks guys.  I am looking into a 169.x.x.x address that we can use.

Comment 15

[Jeff Walden [:Waldo]]

I looked into numbering some, and 169.254.x.x is apparently intended for OS use, or something like that -- "should not be configured manually":

http://tools.ietf.org/html/rfc3927#section-1.6

I suspect you probably should grab something from one of the ranges in here:

http://en.wikipedia.org/wiki/Private_network

192.168 is probably dangerously collision-y with routers just based on personal experience, and 10. seems to be more used just in my experience, so I'd try something in 172.something, maybe 172.27.random.number.  But really, if 127.0.0.1 works we should absolutely use that, and I suspect it probably would work if we set the right preferences.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (4)

updated patch to support proxy to "moztest" and "127.0.0.1".  This appears to work in a simple test on my local machine and on a remote server.  I am running on tryserver now and a pass on a remote webserver to look for anything that I overlooked.

Basically I did a search for all tests that I removed "127.0.0.1" and made sure that I had the correct value in there.  For the most part I left the 127.0.0.1 alone now that it is in the pac file.

Once this passes my tests I will ask for review.  Feel free to comment if I am missing anything.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 428440 [details] [diff] [review]
patch to replace localhost with mochitest.org (4)

try server and local tests with remote server pass

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 428440 [details] [diff] [review]
patch to replace localhost with mochitest.org (4)

addl review of dwitte

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (4.1)

update to 2 test files for bitrot.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 428905 [details] [diff] [review]
patch to replace localhost with mochitest.org (4.1)

please review this.

Comment 21

[Jonathan Watt [:jwatt]]

There have been some efforts to make it easier to run our tests in other implementations in our drive for better interoperability in order to improve the health of the Web. Unfortunately this change will make it more difficult to run our tests in other browsers.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

That is a good thing to keep in mind.  All other browsers support .pac files and that is what is doing the magic tricks here.  With these changes we just need to setup the .pac file (which we already require without this patch for firefox mochitest) for a given browser and point it at a live webserver.  Maybe I am overlooking something.

Comment 23

[(not currently active) Ted Mielczarek]

(In reply to comment #21)
> There have been some efforts to make it easier to run our tests in other
> implementations in our drive for better interoperability in order to improve
> the health of the Web. Unfortunately this change will make it more difficult to
> run our tests in other browsers.

I don't think this is adding much more Mozilla-specific test features than we already have. We already have a number of tests that require httpd.js, the mochitest proxy PAC, ssltunnel, enablePrivilege, etc. I would certainly support an effort to make our tests easier to run in other browsers, but as Joel says, this doesn't really require any more work than they currently do, using the PAC.

Comment 24

[(not currently active) Ted Mielczarek]

And also, if I had to choose between "making our tests easier to run in other browsers" and "making our tests easier to run on our own mobile browser", I would pick the latter.

Comment 25

[[e-mail]]

Comment on attachment 428905 [details] [diff] [review]
patch to replace localhost with mochitest.org (4.1)

extensions/cookie bits look good. Would like to add more tests for the different cases at some point, but that's a different bug :)

r=me.

Comment 26

[cmtalbert]

Checked in as changeset b4372055f473

Comment 27

[cmtalbert]

Backed out due to orange Bd build on os x:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268007117.1268009100.24313.gz&fulltext=1

Comment 28

[(not currently active) Ted Mielczarek]

Hum. leaktest.py uses automation.initializeProfile:
http://mxr.mozilla.org/mozilla-central/source/build/leaktest.py.in#75

but it doesn't actually run a server with httpd.js, it just uses Python to serve a static HTML file. It fetches bloatcycle.html via localhost, which appears to be working, given the snippet from the log:
localhost - - [07/Mar/2010 16:20:48] "GET /bloatcycle.html HTTP/1.1" 200 -

I don't know what the actual problem here is. Joel or Clint: you should try running $objdir/_leaktest/leaktest.py in a build with this patch applied, and see what happens.

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (4.2)

fixed a small bug in leaktest.py which fails on OSX for xpconnect permissions.

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 431148 [details] [diff] [review]
patch to replace localhost with mochitest.org (4.2)

updated patch to have leaktest.py generate a small user.js prefs file in the profile.

Comment 31

[(not currently active) Ted Mielczarek]

Comment on attachment 431148 [details] [diff] [review]
patch to replace localhost with mochitest.org (4.2)

>diff --git a/build/leaktest.py.in b/build/leaktest.py.in
>--- a/build/leaktest.py.in
>+++ b/build/leaktest.py.in
>@@ -67,17 +67,34 @@ if __name__ == '__main__':
>             automation.log.info("Unable to open logfile " + opts[0][1] + \
>                                 "ONLY logging to stdout.")
> 
>     httpd = EasyServer(("", PORT), SimpleHTTPServer.SimpleHTTPRequestHandler)
>     t = threading.Thread(target=httpd.serve_forever)
>     t.setDaemon(True)
>     t.start()
> 
>-    automation.initializeProfile(PROFILE_DIRECTORY)
>+    # since this requires localhost, we cannot use automation.initializeProfile
>+    prefs = []
>+    pref = """
>+user_pref("capability.principal.codebase.p0.granted",
>+          "UniversalXPConnect UniversalBrowserRead UniversalBrowserWrite \
>+           UniversalPreferencesRead UniversalPreferencesWrite \
>+           UniversalFileRead");
>+user_pref("capability.principal.codebase.p0.id", "http://localhost:%(port)s");
>+user_pref("capability.principal.codebase.p0.subjectName", "");
>+    """ % {"port": PORT }
>+    prefs.append(pref)
>+
>+    # write the preferences
>+    prefsFile = open(PROFILE_DIRECTORY + "/" + "user.js", "w")
>+    prefsFile.write("".join(prefs))
>+    prefsFile.close()


You don't need to use an array or use "".join here, just .write the 'pref' string directly. Also, you need to mkdir PROFILE_DIRECTORY first.

r=me with those changes.

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (4.3)

updated patch with fixed nits for leaktest.py fix.

Comment 33

[cmtalbert]

Take two as changeset 8ce70abd7777
* http://hg.mozilla.org/mozilla-central/rev/8ce70abd7777

Comment 34

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix for 2 new tests added shortly before checking in

fix for 2 tests which were added in the last few hours:
1) browser chrome test
2) mochitest plain content/base/test/test_CSP_eval*

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix for 2 new tests added shortly before checking in (2)

ok, there is a 3rd file (build/pgo/profileserver.py) which needs the same fix as leaktest.py.  According to mxr there are no other files that use automation.initializeProfile()

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out due to test failures.

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=614a48536800

Comment 37

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix for 2 new tests added shortly before checking in (3)

secondary followup patch for fix profileserver.py and two tests.  No need to edit the existing reviewed patch.

Comment 38

[cmtalbert]

Comment on attachment 431360 [details] [diff] [review]
fix for 2 new tests added shortly before checking in (3)

These changes look good.
r=me

Comment 39

[(not currently active) Ted Mielczarek]

It would be nice (in a followup) to consolidate that code you added in leaktest and profileserver back into automation.py, maybe a createMinimalProfile, or add a param like createProfile(minimal=True).

Comment 40

[cmtalbert]

(In reply to comment #39)
> It would be nice (in a followup) to consolidate that code you added in leaktest
> and profileserver back into automation.py, maybe a createMinimalProfile, or add
> a param like createProfile(minimal=True).

We opened bug 551177 for this.

And third time is the charm!  Cross your fingers!
* 3d7c54b194bb
* 88529662c474

Comment 41

[Jeff Walden [:Waldo]]

Sigh, I was mostly out of contact (particularly, basically unable to read email) from last Wednesday through yesterday) -- looking at the patch...

Comment 42

[Jeff Walden [:Waldo]]

Comment on attachment 431216 [details] [diff] [review]
patch to replace localhost with mochitest.org (4.3)

>diff --git a/build/pgo/server-locations.txt b/build/pgo/server-locations.txt

> #
> # These are a common set of prefixes scattered across one TLD with two ports and
> # another TLD on a single port.
> #
>+http://127.0.0.1:80               privileged
>+http://127.0.0.1:8888             privileged
>+http://moztest:80                 privileded

You have a typo here.

My earlier comments must have been unclear; I was asking for http://test:80 here, or more generally for test instead of moztest.


>+http://mochi.test:80              privileged
>+http://mochi.test:8000            privileged
>+http://mochi.test:8080            privileged
>+http://mochi.test:8888            privileged
>+http://mochi.test:4444            privileged

Why did you add mochi.test:{4444,80,8000,8080} here?  Only localhost:8888 was in use before, and the others should be superfluous here.  It seems to me like none of these should have been added, and I don't see any uses of these in the patch (but I might just have missed them, I did skim, if somewhat closely!).


>diff --git a/content/base/test/file_CSP.sjs b/content/base/test/file_CSP.sjs

>     var xhr = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]  
>                    .createInstance(Components.interfaces.nsIXMLHttpRequest);
>     //serve the main page with a CSP header!
>     // -- anything served from 'self' (localhost:8888) will be allowed,
>     // -- anything served from other hosts (example.com:80) will be blocked.
>     // -- XHR tests are set up in the file_CSP_main.js file which is sourced.
>     response.setHeader("X-Content-Security-Policy", 
>-                       "allow 'self'",
>+                       "allow mochi.test",
>                        false);
>-    xhr.open("GET", "http://localhost:8888/tests/content/base/test/file_CSP_main.html", false);
>+
>+    var proxyURI = "http://" + query["proxy"];
>+    xhr.open("GET", proxyURI + "/tests/content/base/test/file_CSP_main.html", false);
>     xhr.send(null);
>     if(xhr.status == 200) {
>       response.write(xhr.responseText);
>     }

These changes appear to deviate from the intent of the test.  Can you ask Sid Stamm (:geekboy) to review this bit?  It looks to me like only the open line here should have been changed.  Content security policies should be transparent to proxies, as best as I understand.

Taking a little bit of guessing, if you want to know the identity of the server being asked from a handler, you should use request.host and request.port (I think, might be request.hostPort) to determine that.


>diff --git a/content/base/test/test_CSP.html b/content/base/test/test_CSP.html

> // save this for last so that our listeners are registered.
> // ... this loads the testbed of good and bad requests.
>-document.getElementById('cspframe').src = 'file_CSP.sjs?main=1';
>+
>+netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+var ios = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
>+var pps = Components.classes["@mozilla.org/network/protocol-proxy-service;1"].getService();
>+
>+var uri = ios.newURI("http://example.com", null, null);
>+var pi = pps.resolve(uri, 0);
>+var proxy = pi.host + ":" + pi.port;
>+document.getElementById('cspframe').src = 'file_CSP.sjs?main=1&proxy=' + proxy;

These tests quite possibly will be used by other browser engines when they get around to implementing CSP, and this sort of change will make that harder, if it's actually necessary.  Again, I'm extremely skeptical that this test needs to be modified with knowledge of the current proxy's identity.


>diff --git a/dom/tests/mochitest/ajax/offline/test_fallback.html b/dom/tests/mochitest/ajax/offline/test_fallback.html

>     case 003:
>     case 103:
>       OfflineTest.is(fallbackIdentification, 2, "fallback for namespace1/sub/ in step " + gStep);
> 
>-      fallbackFrame.location = "HTTP://LOCALHOST:8888/tests/dom/tests/mochitest/ajax/offline/namespace1/sub/non-existing.html";
>+      fallbackFrame.location = "HTTP://mochi.test:8888/tests/dom/tests/mochitest/ajax/offline/namespace1/sub/non-existing.html";
>       // Invokes load of fallback2.html
>       break;
> 

This change of capitalization is, in my opinion, presumptively suspicious: if the original was capitalized, any change here which fails to preserve that is a bad change, unless the original author says so.  (The fact that this would mean case 103 is an exact duplicate of case 102 argues for this understanding.)  Please track down the original author of this test, or the original bug that changed it, and have the author verify that changing case here is okay?  If it is okay, I'd prefer if you also lowercased the protocol so future readers aren't led astray in this manner too -- or better, removed the case entirely (have that author review).



>diff --git a/dom/tests/mochitest/bugs/test_bug346659.html b/dom/tests/mochitest/bugs/test_bug346659.html

> function startSecondBatch() {
>-  var baseurl = window.location.href.replace(/localhost:8888/, "example.com");
>+  var baseurl = window.location.href.replace(/mochi.test:8888/, "example.com");
>   wins[7] = window.open(r(baseurl, 'bug346659-opener.html?{"load":7}'));
>   wins[8] = window.open(r(baseurl, 'bug346659-opener.html?{"write":8}'));
>   wins[9] = window.open(r(baseurl, 'bug346659-parent.html?{"load":9}'));
>   wins[10] = window.open(r(baseurl, 'bug346659-parent.html?{"write":10}'));
> }
> 
> function startThirdBatch() {
>-  var baseurl = window.location.href.replace(/localhost:8888/, "example.com");
>+  var baseurl = window.location.href.replace(/mochi.test:8888/, "example.com");
>   wins[11] = window.open(r(baseurl, 'bug346659-opener.html?{"load":11,"xsite":true}'));
>   wins[12] = window.open(r(baseurl, 'bug346659-parent.html?{"load":12,"xsite":true}'));
> }

I would prefer if you backslash-escaped the dot in each of these, just to be safe/explicit.


>diff --git a/dom/tests/mochitest/whatwg/test_postMessage_special.xhtml b/dom/tests/mochitest/whatwg/test_postMessage_special.xhtml

>@@ -180,27 +180,27 @@ function getContents(description, respon
>     "  <title>about:blank</title>\n" +
>     "  <script type='application/javascript'>\n" +
>     "function receive(evt)\n" +
>     "{\n" +
>     "  var response = '" + responseText + "';\n" +
>     "\n" +
>     "  if (evt.source !== window.parent)\n" +
>     "    response += ' wrong-source';\n" +
>-    "  if (evt.origin !== 'http://localhost:8888')\n" +
>+    "  if (evt.origin !== \"http://mochi.test:8888\")\n" +
>     "    response += ' wrong-origin(' + evt.origin + ')';\n" +
>     "  if (evt.data !== 'from-opener')\n" +
>     "    response += ' wrong-data(' + evt.data + ')';\n" +
>     "\n" +
>-    "  window.parent.postMessage(response, 'http://localhost:8888');\n" +
>+    "  window.parent.postMessage(response, \"http://mochi.test:8888\");\n" +
>     "}\n" +
>     "\n" +
>     "function ready()\n" +
>     "{\n" +
>-    "  window.parent.postMessage('next-test', 'http://localhost:8888');\n" +
>+    "  window.parent.postMessage('next-test', \"http://mochi.test:8888\");\n" +
>     "}\n" +
>     "\n" +

Please don't change the quote style here; it's much more readable if you don't have to read through backslashes necessitated only by using double-quote rather than single-quote.


>diff --git a/netwerk/test/httpserver/httpd.js b/netwerk/test/httpserver/httpd.js
>diff --git a/testing/mochitest/server.js b/testing/mochitest/server.js

The changes here misinterpret the requests I made above.  I'll comment on them in a separate comment and get on with the rest of the search/replacing changes.


>diff --git a/toolkit/components/passwordmgr/test/test_prompt_async.html b/toolkit/components/passwordmgr/test/test_prompt_async.html

You should have dolske (:dolske) review this; it looks like it might be valid, but it's a substantive change whose correctness is not completely mechanical.


>             case 6:
>                 // Reload the frame from the previous step and let the proxy
>                 // authentication pass but WWW fail. We expect two dialogs
>-                // and an unathentiocated page content load.
>+                // and an unathenticated page content load.
>                 ok(true, "doTest testNum 6");

Done now, but generally best to leave changes like this out of mega-patches -- dump in bug spelling or fix in a separate (if perhaps simul-pushed) mini-changeset.

Comment 43

[Jeff Walden [:Waldo]]

Comment on attachment 431216 [details] [diff] [review]
patch to replace localhost with mochitest.org (4.3)

>+  server._start(SERVER_PORT, gServerAddress);

Calling implementation-internal methods named as such is a bad idea; a public method, declared in IDL, should be used instead.  The IDL for the start method is currently this:

>    /**
>     * Starts up this server, listening upon the given port.
>     *
>     * @param port
>     *   the port upon which listening should happen, or -1 if no specific port is
>     *   desired
>     * @throws NS_ERROR_ALREADY_INITIALIZED
>     *   if this server is already started
>     * @throws NS_ERROR_NOT_AVAILABLE
>     *   if the server is not started and cannot be started on the desired port
>     *   (perhaps because the port is already in use or because the process does
>     *   not have privileges to do so)
>     * @note
>     *   Behavior is undefined if this method is called after stop() has been
>     *   called on this but before the provided callback function has been
>     *   called.
>     */
>    void start(in long port);

I suggest a second version of the start method should be added, which looks like this:

>    /**
>     * Starts up this server, listening upon the given port, running on the given
>     * host.  If loopbackOnly is true, only connections from the machine on which
>     * this server is running are permitted; otherwise, connections from any
>     * machine will be accepted.
>     *
>     * @param port
>     *   the port upon which listening should happen, or -1 if no specific port is
>     *   desired
>     * @param host
>     *   the host on which this server is running
>     * @param loopbackOnly
>     *   if true, only connections made to this server from the machine on which
>     *   this server runs are permitted; otherwise, requests may be made of this
>     *   server from any machine
>     * @throws NS_ERROR_ALREADY_INITIALIZED
>     *   if this server is already started
>     * @throws NS_ERROR_NOT_AVAILABLE
>     *   if the server is not started and cannot be started on the desired port
>     *   (perhaps because the port is already in use or because the process does
>     *   not have privileges to do so)
>     * @note
>     *   Behavior is undefined if this method is called after stop() has been
>     *   called on this but before the provided callback function has been
>     *   called.
>     */
>    void start(in long port);

We then could document the original version of the start method as follows:

>    /**
>     * Equivalent to start(port, "localhost", true).
>     */
>    void start(in long port);

Then you could change the call to:

  server.start(SERVER_PORT, gServerAddress, false);

Aside from poking inside the implementation, the way you currently set it up equates a locally-running server with no external access.  There's nothing which should prevent a server from being started up on the local machine, perhaps with or without external-facing names, while still permitting external connections.

It should be possible to make this change by changing the previous lines in httpd.js like so (leading . added to unchanged lines only so it lines up with Thunderbird quoting mechanisms):

>-    start: function(port)
>+    start: function(port, host, loopbackOnly)
>.    {
>+      if (arguments.length === 1)
>+      {
>+        host = "localhost";
>+        loopbackOnly = true;
>+      }
>+      else if (arguments.length !== 3)
>+      {
>+        throw Cr.NS_ERROR_INVALID_ARG;
>+      }
>.      if (this._socket)
>.        throw Cr.NS_ERROR_ALREADY_INITIALIZED;

Now, I'm not sure this should really be the extent of it -- the baroque way that you're prevented from removing all server identities probably conflicts with this change some.  I'll work on a patch that fixes these concerns to my satisfaction, because even *I'm* not sure what I want here just now.

Comment 44

[Jeff Walden [:Waldo]]

Comment on attachment 431360 [details] [diff] [review]
fix for 2 new tests added shortly before checking in (3)

>diff --git a/content/base/test/file_CSP_evalscript.sjs b/content/base/test

>   if ("main" in query) {
>     var xhr = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]  
>                    .createInstance(Components.interfaces.nsIXMLHttpRequest);
>+
>     //serve the main page with a CSP header!
>-    // -- anything served from 'self' (localhost:8888) will be allowed,
>+    // -- anything served from 'self' (mochi.test:8888) will be allowed,
>     // -- anything served from other hosts (example.com:80) will be blocked.
>     // -- XHR tests are set up in the file_CSP_main.js file which is sourced.
>     response.setHeader("X-Content-Security-Policy", 
>                        "allow 'self'",
>                        false);
>-    xhr.open("GET", "http://localhost:8888/tests/content/base/test/file_CSP_evalscript_main.html", false);
>+    var proxy = "127.0.0.1";
>+    if ("proxy" in query) {
>+      proxy = query["proxy"];
>+    }
>+    xhr.open("GET", "http://" + proxy + "/tests/content/base/test/file_CSP_evalscript_main.html", false);
>     xhr.send(null);
>     if(xhr.status == 200) {
>       response.write(xhr.responseText);
>     }
>   }
> }
>diff --git a/content/base/test/test_CSP_evalscript.html b/content/base/test/test_CSP_evalscript.html
>--- a/content/base/test/test_CSP_evalscript.html
>+++ b/content/base/test/test_CSP_evalscript.html
>@@ -47,16 +47,27 @@ var checkTestResults = function() {
>   // ... otherwise, finish
>   SimpleTest.finish();
> }
> 
> //////////////////////////////////////////////////////////////////////
> // set up and go
> SimpleTest.waitForExplicitFinish();
> 
>+//need to allow for arbitrary network servers, when PAC doesn't work
>+netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+var ios = Components.classes["@mozilla.org/network/io-service;1"].
>+          getService(Components.interfaces.nsIIOService);
>+var pps = Components.classes["@mozilla.org/network/protocol-proxy-service;1"].
>+          getService();
>+
>+var uri = ios.newURI("http://example.com", null, null);
>+var pi = pps.resolve(uri, 0);
>+var mozproxy = pi.host + ":" + pi.port;
>+
> // save this for last so that our listeners are registered.
> // ... this loads the testbed of good and bad requests.
>-document.getElementById('cspframe').src = 'file_CSP_evalscript.sjs?main';
>+document.getElementById('cspframe').src = 'file_CSP_evalscript.sjs?main&proxy=' + mozproxy;
> 
> </script>
> </pre>
> </body>
> </html>
>diff --git a/docshell/test/browser/browser_bug503832.js b/docshell/test/browser/browser_bug503832.js
>--- a/docshell/test/browser/browser_bug503832.js
>+++ b/docshell/test/browser/browser_bug503832.js
>@@ -1,18 +1,18 @@
> /* Test for Bug 503832
>  * https://bugzilla.mozilla.org/show_bug.cgi?id=503832
>  */
> 
> function test() {
>     waitForExplicitFinish();
> 
>     var pagetitle = "Page Title for Bug 503832";
>-    var pageurl = "http://localhost:8888/browser/docshell/test/browser/file_bug503832.html";
>-    var fragmenturl = "http://localhost:8888/browser/docshell/test/browser/file_bug503832.html#firefox";
>+    var pageurl = "http://mochi.test:8888/browser/docshell/test/browser/file_bug503832.html";
>+    var fragmenturl = "http://mochi.test:8888/browser/docshell/test/browser/file_bug503832.html#firefox";
> 

This is more of the same as the one comment I made in the earlier comment -- :geekboy should look this over as well.

Comment 45

[Dão Gottwald [:dao]]

Apparently this has been backed out:

http://hg.mozilla.org/mozilla-central/rev/72f8d4f19dc7

And now test_CrossSiteXHR.html and test_postMessage_jar.html time out persistently...

Comment 46

[Dão Gottwald [:dao]]

(In reply to comment #45)
> And now test_CrossSiteXHR.html and test_postMessage_jar.html time out
> persistently...

Fixed by http://hg.mozilla.org/mozilla-central/rev/4e4cb03513ca

Comment 47

[cmtalbert]

(In reply to comment #43)
> (From update of attachment 431216 [details] [diff] [review])
> Now, I'm not sure this should really be the extent of it -- the baroque way
> that you're prevented from removing all server identities probably conflicts
> with this change some.  I'll work on a patch that fixes these concerns to my
> satisfaction, because even *I'm* not sure what I want here just now.

I'm looking forward to your patch here, and to getting more eyes on these changes to the tests.  I'm wondering how best to package this up.  Can we take your patch and any changes we have to the tests as a follow on bug to this monster or do we need to land this in one monolithic glob?

I definitely don't want us to change the behaviors of the tests, so I'm inclined toward monolithicness. But at the same time, this patch is a bear to keep up to date and to keep bitrot free.  

What are your thoughts?

Comment 48

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (4.4)

updated patch with feedback from Waldo.  Still waiting on review of CSP and passwordmgr/test_prompt_async tests.  Also the httpd.js cleanup need to be finalized (either in this bug or a followup).

Comment 49

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix for 2 new tests added shortly before checking in (4)

updated this patch to remove bitrot and redo the initializeProfile code in automation.py to resolve the issues with leaktest.py and profileserver.py that we have been seeing on tinderbox builds.

Comment 50

[Sid Stamm [:geekboy or :sstamm]]

Unless I'm misunderstanding the fundamental update you're doing, you should be able to just replace all string instances of 'localhost' with 'mochi.test' everywhere in the CSP test files, and shouldn't have to do any proxy-oriented modifications.

(In reply to comment #42)
> (From update of attachment 431216 [details] [diff] [review])
> >diff --git a/content/base/test/file_CSP.sjs b/content/base/test/file_CSP.sjs
> 
> >     var xhr = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]  
> >                    .createInstance(Components.interfaces.nsIXMLHttpRequest);
> >     //serve the main page with a CSP header!
> >     // -- anything served from 'self' (localhost:8888) will be allowed,
> >     // -- anything served from other hosts (example.com:80) will be blocked.
> >     // -- XHR tests are set up in the file_CSP_main.js file which is sourced.
> >     response.setHeader("X-Content-Security-Policy", 
> >-                       "allow 'self'",
> >+                       "allow mochi.test",

Please leave this line alone, it's not a host name, and changes the semantics of the test.  Do change the comment, however (s/localhost/mochi.test/g).

> >                        false);
> >-    xhr.open("GET", "http://localhost:8888/tests/content/base/test/file_CSP_main.html", false);
> >+
> >+    var proxyURI = "http://" + query["proxy"];
> >+    xhr.open("GET", proxyURI + "/tests/content/base/test/file_CSP_main.html", false);
> >     xhr.send(null);
> >     if(xhr.status == 200) {
> >       response.write(xhr.responseText);
> >     }
> 
> These changes appear to deviate from the intent of the test.  Can you ask Sid
> Stamm (:geekboy) to review this bit?  It looks to me like only the open line
> here should have been changed.  Content security policies should be transparent
> to proxies, as best as I understand.

I agree with waldo: I don't think we need to do proxying here, unless I misunderstand the purpose of the proxies.  Can you instead just substitute "mochi.test:8888" for "localhost:8888"?

> >diff --git a/content/base/test/test_CSP.html b/content/base/test/test_CSP.html
> 
> > // save this for last so that our listeners are registered.
> > // ... this loads the testbed of good and bad requests.
> >-document.getElementById('cspframe').src = 'file_CSP.sjs?main=1';
> >+
> >+netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> >+var ios = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
> >+var pps = Components.classes["@mozilla.org/network/protocol-proxy-service;1"].getService();
> >+
> >+var uri = ios.newURI("http://example.com", null, null);
> >+var pi = pps.resolve(uri, 0);
> >+var proxy = pi.host + ":" + pi.port;
> >+document.getElementById('cspframe').src = 'file_CSP.sjs?main=1&proxy=' + proxy;
> 
> These tests quite possibly will be used by other browser engines when they get
> around to implementing CSP, and this sort of change will make that harder, if
> it's actually necessary.  Again, I'm extremely skeptical that this test needs
> to be modified with knowledge of the current proxy's identity.

Agreed. I'm not convinced the changes here are necessary.

Comment 51

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #44)
> (From update of attachment 431360 [details] [diff] [review])
> >diff --git a/content/base/test/file_CSP_evalscript.sjs b/content/base/test
> 
> >   if ("main" in query) {
> >     var xhr = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]  
> >                    .createInstance(Components.interfaces.nsIXMLHttpRequest);
> >+
> >     //serve the main page with a CSP header!
> >-    // -- anything served from 'self' (localhost:8888) will be allowed,
> >+    // -- anything served from 'self' (mochi.test:8888) will be allowed,
> >     // -- anything served from other hosts (example.com:80) will be blocked.
> >     // -- XHR tests are set up in the file_CSP_main.js file which is sourced.
> >     response.setHeader("X-Content-Security-Policy", 
> >                        "allow 'self'",
> >                        false);
> >-    xhr.open("GET", "http://localhost:8888/tests/content/base/test/file_CSP_evalscript_main.html", false);
> >+    var proxy = "127.0.0.1";
> >+    if ("proxy" in query) {
> >+      proxy = query["proxy"];
> >+    }
> >+    xhr.open("GET", "http://" + proxy + "/tests/content/base/test/file_CSP_evalscript_main.html", false);
> >     xhr.send(null);
> >     if(xhr.status == 200) {
> >       response.write(xhr.responseText);
> >     }
> >   }
> > }
> >
> >diff --git a/content/base/test/test_CSP_evalscript.html b/content/base/test/test_CSP_evalscript.html
> >--- a/content/base/test/test_CSP_evalscript.html
> >+++ b/content/base/test/test_CSP_evalscript.html
> >@@ -47,16 +47,27 @@ var checkTestResults = function() {
> >   // ... otherwise, finish
> >   SimpleTest.finish();
> > }
> > 
> > //////////////////////////////////////////////////////////////////////
> > // set up and go
> > SimpleTest.waitForExplicitFinish();
> > 
> >+//need to allow for arbitrary network servers, when PAC doesn't work
> >+netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> >+var ios = Components.classes["@mozilla.org/network/io-service;1"].
> >+          getService(Components.interfaces.nsIIOService);
> >+var pps = Components.classes["@mozilla.org/network/protocol-proxy-service;1"].
> >+          getService();
> >+
> >+var uri = ios.newURI("http://example.com", null, null);
> >+var pi = pps.resolve(uri, 0);
> >+var mozproxy = pi.host + ":" + pi.port;
> >+
> > // save this for last so that our listeners are registered.
> > // ... this loads the testbed of good and bad requests.
> >-document.getElementById('cspframe').src = 'file_CSP_evalscript.sjs?main';
> >+document.getElementById('cspframe').src = 'file_CSP_evalscript.sjs?main&proxy=' + mozproxy;
> > 
> > </script>
> > </pre>
> > </body>
> > </html>
>
> This is more of the same as the one comment I made in the earlier comment --
> :geekboy should look this over as well.

Yeah, similar to my last comment, I'm not sure the proxying changes are necessary. Can you just change localhost:8888 to mochi.test:8888?

Comment 52

[Jeff Walden [:Waldo]]

(In reply to comment #47)
> I'm looking forward to your patch here, and to getting more eyes on these
> changes to the tests.  I'm wondering how best to package this up.  Can we take
> your patch and any changes we have to the tests as a follow on bug to this
> monster or do we need to land this in one monolithic glob?
> 
> I definitely don't want us to change the behaviors of the tests, so I'm
> inclined toward monolithicness. But at the same time, this patch is a bear to
> keep up to date and to keep bitrot free.  

The changes I think need to be made, with respect to httpd.js and server.js, are more putting different packaging/veneer in place than making fundamental changes.  It should be possible to make them in isolation of the rest of the changes here, more or less, so there's no reason for these tweaks to require a backout of everything here.

Comment 53

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (4.5)

forgot two .jar binary files after backout and readding patch to my tree.

Comment 54

[Joel Maher ( :jmaher ) (UTC -8)]

In regards to comment 50 and 51 and adding the raw proxy into the CSP .sjs get string, I originally tried just changing localhost to mochi.test and this produced:
*** error running SJS at /home/joel/mozilla/firefox_release/_tests/testing/mochitest/./tests/content/base/test/file_CSP.sjs: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIXMLHttpRequest.send]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: ./httpd.js :: handleRequest :: line 2639"  data: no] on line 31

This has always been (and is still with this patch) using 127.0.0.1 as an underlying proxy in the PAC file.  The difference was the primary domain was 'localhost' which at a system level maps to 127.0.0.1 and now we are using a fictitious name 'mochi.test' as the primary domain. 

If I leave the xhr.open(localhost:8888/...), I don't get an exception, instead I get 'unrecognized hostname' in httpd debug output.  

I have uploaded a log session of my test run with using mochi.test instead of the raw proxy while having httpd.js in debug mode: http://mozilla.pastebin.com/p3Kv6tCu.

Comment 55

[Sid Stamm [:geekboy or :sstamm]]

Attachment: mochi.test:8888 compatibility fixes for the CSP tests

I think the proxying was a symptom of my awful test structure; there's no reason to use XHR in most of those tests (the ones that need the proxy), and I have learned this since I wrote them.  :)   So we should be able to modify the tests to play nicer and not require the proxying.

Attached is a patch that removes the need for the proxies, as far as I can tell in my build with your patches, by making the tests more portable and cleaner.

Aside from the proxying stuff, I think my only concern was the first note in comment 50: make sure not to change the X-Content-Security-Policy header values in the tests.

Comment 56

[Joel Maher ( :jmaher ) (UTC -8)]

Just looking at this patch, I like it because we remove our dependency on the CSP .sjs files.

Comment 57

[Justin Dolske [:Dolske]]

Comment on attachment 431774 [details] [diff] [review]
patch to replace localhost with mochitest.org (4.5)

Looked at the passwordmgr changes, looks good  but a couple tiny nits...

>--- a/toolkit/components/passwordmgr/test/test_basic_form_pwonly.html
...
>-pwlogin1.init("http://localhost:8888", "http://localhost:1111", null,
>+pwlogin1.init("http://mochi.test:8888", "http://localhost:1111", null,
>     "", "1234", "uname", "pword");

Should probably change both strings to mochi.test.


>--- a/toolkit/components/passwordmgr/test/test_prompt_async.html
...
>+            //need to allow for arbitrary network servers, hardcoded for 8888, although PAC might h$

I think your cut'n'paste got chopped off here. :)

>-                // and an unathentiocated page content load.
>+                // and an unathenticated page content load.

O_o Can't anyone around here spell?! :)

Comment 58

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: patch to replace localhost with mochitest.org (4.6)

this patch is updated with all feedback, but requires the two additional patches in this bug to be complete.  We will address the cleanup and enhancements to httpd.js in a separate bug.

Thanks everybody for the reviews and feedback.

carryover the r+ as this is just a few nits that are fixed.

Comment 59

[cmtalbert]

Comment on attachment 431705 [details] [diff] [review]
fix for 2 new tests added shortly before checking in (4)

Tested these changes and they look good.

Comment 61

[cmtalbert]

Landed as:
# 2985e7efe8db
# 990568a068b1
# 29810a1281ee 

FIXED \o/

Comment 62

[Jeff Walden [:Waldo]]

I filed bug 552852 on adjusting httpd.js API, Mochitest's use of it, and whatever else needs to be adjusted internally to preserve a primary location, as I said I'd do in comment 43.  Not sure when I'll get to it, hopefully pretty soon tho...

Comment 63

[(no longer active)]

There are quite a few remaining instances of localhost:8888 in the tree.  Is that intentional?

<http://mxr.mozilla.org/mozilla-central/search?string=localhost%3A8888&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central>

Comment 64

[cmtalbert]

(In reply to comment #63)
> There are quite a few remaining instances of localhost:8888 in the tree.  Is
> that intentional?
> 
> <http://mxr.mozilla.org/mozilla-central/search?string=localhost%3A8888&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central>

Hey Ehsan, I spoke to Joel about this briefly, he's building a complete list.  But many of these do appear to be intentionally left as localhost.  He's going through the list to verify and will report back when he has a chance.

Thanks for raising the question!