342877 - Enhancements to the JS-implemented HTTP server (used in netwerk tests)

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Jeff Walden [:Waldo]]

There's an HTTP server in netwerk/test/unit/head_http_server.js for use in Necko unit tests.  It's small, does pretty much exactly what's required of it and absolutely no more, and isn't particularly useful outside of those tests.

I've taken the original server and beefed it up a bit -- added features such as the ability to serve a local directory, override files within that directory, shut down the server :-), etc.  There are still more changes I'd like to make, but the added features (particularly the ability to serve a local directory) would make it useful for a lot more code than just a few Necko unit tests, in particular for testing client application update code.  Consequently, I'd like to upload the heavily-modified server to tools/test-harness/ somewhere so it can be used by any tests that would want to use it.

I don't have time to finish up "enough" of the things I'd like to do with it this week, so look for a patch sometime next week.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Zip of JS file for server, brief README, IDL, and a small test script (run manually in browser)

This is what I have right now; somehow there needs to be a way to run what's effectively the code in test.js within any arbitrary testcase in the tree.  Network tests would definitely use this, toolkit updates code could use it for parts which require a network connection (although I don't know whether XMLHttpRequest works in xpcshell, which is a prerequisite for testing those aspects of updates), and I know sayrer would like it for testing charset handling with feeds.

The question now is where to put this (so I can make a proper patch for review).  I think having a small library of written-once utility scripts which could be used in tests would be a good thing (i.e., get local file objects for files adjacent to test scripts, file-to-DOM-tree functions, write-text-to-file functions, etc.), and I see this as being one such thing which could be placed there.  Maybe that's too far in the future, tho.

Anyways, where do we want this?

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Two newer versions (for comparison of the easy way vs. the hard way)

biesi doesn't like tempfiles, and while I don't exactly like them, I strongly prefer them to the code clutter created by using an async stream copier and a storage stream.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to comment #2)
> code clutter created by using an async stream copier and a storage stream.

biesi wants me to clarify that this is about using the stream copier, not really about storage streams.  I could just as easily have replaced the storage stream with a pipe for equivalent effect (including code clutter).  :-)

Also, while it should be noted that there may be further cleanup to do in the copier version (possibly removing Response.destroy), I believe the essential complaint about readability still holds.

Comment 4

[Jeff Walden [:Waldo]]

(moving to correct component so the right people see this; I think I filed back when my plans here were much less ambitious)

davel, where do we want this?  For testing I see its primary use case being from the xpcshell-simple system, but it's fully functional as an XPCOM component on both branch and trunk.

Also, how do we want to make it available to tests which want to use it?

Comment 5

[Dave Liebreich [:davel]]

I'm not sure yet where to put it.  I'm open to suggestions.  Right now, mozilla/tools/test-harness is checked out by default, and mozilla/testing is not.

Maybe mozilla/netwerk/test/server ?

I'll take a look at the code later today . . . 

Comment 6

[Jeff Walden [:Waldo]]

(In reply to comment #5)
> Maybe mozilla/netwerk/test/server ?

I'd really like to see it in a location fairly close to the test harness itself, or in a location where one would expect to find testing utility code.  Keeping it in netwerk/ makes it much less visible and much harder to discover if you don't already know it exists.  The only reason I happened to stumble upon it was because I wanted tests for nsUpdateService and darin pointed me at the existing netwerk test cases as examples (and because I spent the time to poke through the entire directory).  That wouldn't have happened if he hadn't worked on both update and netwerk code, either.

Granted, a fair portion of this problem can be solved with good/better docs, but I'd prefer that there be multiple ways to learn of the server's existence (location, docs, addition to nascent FAQs, use in existing netwerk/other tests, etc.).

> I'll take a look at the code later today . . . 

For the record, I plan to have biesi/darin review the code and final patch, since I hear they know a little bit about networking code.  ;-)

Comment 7

[Benjamin Smedberg]

The server code itself ought to live in netwerk/... we want code location to continue to directly relate to module ownership and maintenance. The fact that it is used by the testing framework is wonderful, but doesn't justify putting it in dedicated mozilla/testing directories (just as unit tests belong with the code they test).

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> The server code itself ought to live in netwerk/... we want code location to
> continue to directly relate to module ownership and maintenance. The fact that
> it is used by the testing framework is wonderful, but doesn't justify putting
> it in dedicated mozilla/testing directories (just as unit tests belong with
> the code they test).

I don't intend for this to only be used in netwerk tests; I want it to see used in significantly more places than just netwerk tests (update tests, XMLHTTPRequest tests, feeds testing, perhaps microsummaries, etc.).  Essentially, I'd like it to be a core component available for use in any test script which might want to use it.  Given that, I don't see how it's unreasonable to want it to be outside of netwerk.

As for maintenance, I plan to continue maintaining it regardless where we put it, so I don't think its location will have any meaningful relationship to module ownership or maintenance.

After a little more thought, I suggest testing/utils/httpserver for the location, which is more centralized and (whenever testing code is really consolidated into testing/) more discoverable.

Comment 9

[Benjamin Smedberg]

It doesn't matter how you *use* the code... it matters what the code *does*. In thise case it runs a HTTP server, and should be maintained by the network module owner/peers, so it should live under netwerk.

Comment 10

[Dave Liebreich [:davel]]

I agree with benjamin.  lots of stuff is copied to dist/ during the build, and most (but not all) of it is copied to the final install package.

Among the stuff that is not copied are the necko unit tests, the content unit tests, and the xpcshell-simple test harness files.

maybe the http server files can be copied to somewhere in dist, and excluded from the final packaging.  That would address discoverability (look for test utils in dist/...) and maintainability (keep original files grouped by functionality).

maybe we should put all the stuff-that-is-useful-during-build-and-test-but-should-not-be-part-of-the-final-package in one location, named something other than "dist".  but that's another issue.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Adds the HTTP server in netwerk/test/httpserver, fixes existing tests to use it

This patch adds the HTTP server and related files to netwerk/test/httpserver/ (with a few tests for it in netwerk/test/httpserver/test/), makes a few tweaks to the test harness so that it's possible to use the HTTP server from any test in the tree with a single line of code, and converts all existing necko unit tests to use it.  All new and modified tests pass.

For the first pass over this I'd mostly like feedback on the changes to the testing architecture and the build-fu changes.  Once I have and have acted upon that feedback, I'll ask for the appropriate code-level reviews.

Comment 12

[Dave Liebreich [:davel]]

Comment on attachment 238858 [details] [diff] [review]
Adds the HTTP server in netwerk/test/httpserver, fixes existing tests to use it

comments on the testing architecture and build-fu:

1) wrt bug 351968, I think all the test stuff is going to move out of dist/...  When the new structure of the test stuff is figured out, the build-fu and test architecture stuff in this patch will probably need to change :-)

2) this bug should land without waiting for bug 351968, or at least go under code-review.  Will you be around to make changes in the next few months as we figure stuff out?

3) adding DIST="$bin" to the test_all.sh invocation line and referencing it in do_import_script seems odd to me.  Is there already an env var that can be used, like MOZ_DIST_BIN, or DIST from make?  Or maybe modify xpcshell so that load() takes a PATH-like arg?

4) is there a reason the loading of httpd.js can't happen outside of run_test()?

5) (not build-fu) A lot fo the code in httpd.js, like toDateString(), looks like it belongs in a common library.  Is this code you wrote from scratch, or copied from somewhere else?

Comment 13

[Jeff Walden [:Waldo]]

(In reply to comment #12)
> 1) wrt bug 351968, I think all the test stuff is going to move out of dist/... 

Change is a constant.  The key here is that tests which use this don't actually care that they're in DIST or wherever -- they just have to put the files in the right place (a more Makefile.in-y change) and ensure a reasonably consistent and constant directory hierarchy for centralized testing scripts such as this one.  The interface tests see won't change, although how that works behind the scenes can.

> 2) this bug should land without waiting for bug 351968, or at least go under
> code-review.  Will you be around to make changes in the next few months as we
> figure stuff out?

I'll be around and at least moderately active (say on a couple-times-a-week basis, bugmail responses sooner), but I don't think I can commit to regular, consistent activity if for no other reason than past experience has shown regular activity tends not to happen when I get too hosed.

> 3) adding DIST="$bin" to the test_all.sh invocation line and referencing it in
> do_import_script seems odd to me.  Is there already an env var that can be
> used, like MOZ_DIST_BIN, or DIST from make?  Or maybe modify xpcshell so that
> load() takes a PATH-like arg?

I added a |printObj(environment)| to head_utils.js in the server tests; looking through the output, the only interesting environment variables are MOZILLA_FIVE_HOME, ADDON_PATH, and PWD.  PWD actually isn't interesting (it's the path within the objdir at which the test is located), although I suppose you could determine the location of the mozilla/ directory from it if you wanted assuming no adversarial directory-naming.  ADDON_PATH is only set because I'm on Linux (it's in build/unix/run-mozilla.sh), so that's out.  This leaves us with MOZILLA_FIVE_HOME, which may not be reliable (assumption!) since code exists to set bin=`dirname $0` if MOZILLA_FIVE_HOME isn't defined.  Consequently, I think we have to pass in $bin in some way here, be it by export or command-local definition as it is currently.

Modifying load() in xpcshell is a bad idea, because its semantics have been constant for quite a while (and thus we'd break existing uses of it), and it already accepts and processes a variable number of arguments.  I'm not sure what using another function really gains us, tho -- could you elaborate?

> 4) is there a reason the loading of httpd.js can't happen outside of
> run_test()?

No, none; I can move it to the top of each file if you want (as already happens for head_utils.js, incidentally).

> 5) (not build-fu) A lot fo the code in httpd.js, like toDateString(), looks
> like it belongs in a common library.  Is this code you wrote from scratch, or
> copied from somewhere else?

No, it's all from scratch (modulo IS_TOKEN_ARRAY, which was copied from our HTTP implementation to reduce the chance of an errant typo, and modulo the XPCOM trappings, which I believe were from the update service code; however, the license is the same and the actual intellectual content of either bit of code is non-existent).  It's also all in the same file because that's how JS-implemented XPCOM components are, modulo build preprocessing, the subscript loader, or other ugly hacks.  As for toDateString, I blame brendan for not putting a method to do date->string with custom formatting in JS.  ;-)

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Latest edition, for code review while waiting on build/test-fu feedback

davel's suggested I not wait for infrastructure review before getting the code reviewed.

Changes from the last version include:

- merging .init(port) and .start() into .start(port)
- adding a TODO file listing some possible future improvements and current bugs
- changing IDL and impl so handler-registering methods accept null to remove an
  existing handler (also do the equivalent for setBasePath)
- handling an onStopRequest race between a channel listener and the async
  copier for that request in certain cases
- move do_import_script calls to top of testcases
- add a new test for setBasePath and registerPathHandler

Also, an idea: instead of passing in $bin, pass in $topsrcdir or the equivalent and then use "absolute" paths to import scripts?

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Add another test, fix some atomicity issues in setStatusLine

Comment 16

[Jeff Walden [:Waldo]]

Attachment: More pending-request fixes

Changes:
- make isStopped() check for pending requests which are only partly served
- make pending-request count modifications happen for every request, for
  simplicity
- remove ?quit=1 support for shutting down the server from the client
- along with this, handle a "hang" when the current request shuts down the
  server by decrementing _pendingRequests prior to calling .stop() on the
  server (which calls .close() on the socket); must decrement beforehand, or
  .stop() will wait forever for that one request to be removed from the
  _pendingRequests count
- clarify .stop() docs for pending-request behavior

Comment 17

[Doug Wright]

(In reply to comment #13)
>As for toDateString, I blame brendan for not
> putting a method to do date->string with custom formatting in JS.  ;-)

I'm assuming you don't know about Date.toLocaleFormat(), since it's not been documented anywhere in the 18 months since it got added to JS 1.6 (see bug 291494).

If you did know about it, but it isn't suitable, then I apologise in advance for the bugspam.
 

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

an HTTP server can't use a locale-dependent date format.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Latest version, 95%-complete support for arbitrary directory mappings

In response to comments in m.d.quality requesting support for mapping /dir1/ -> /local/path/number/1 and /dir2/ -> /local/path/number/2 simultaneously, I've mostly added support for this feature.  This should allow use of the server in situations where you'd want to serve files from disparate locations without needlessly moving the directories around to match server expectations, doing lots of copying and deleting of temporary files, or relying upon OS support for symlinks or other hackery.

This patch as written works for me.  All the (new and modified) tests in it pass, and when I try mapping a local directory onto a non-root directory path I can browse it successfully (and get 404s when viewing pages above that directory).  The only thing that's missing is some comprehensive testing of how multiple and overlapping mappings work (i.e., / and /foo/ are both mapped at once, also with file reference overlaps).  The current test for it is just the old test_setbasepath.js renamed with minor API use changes to match, and as such it's insufficient to completely test the new behavior.

This patch is arguably ready for review.  However, I'm uncomfortable with the incompleteness of the testing of the new API, so as such this is being posted without a review request.  Also, since I'm fairly busy at the moment and am about halfway through a complete system wipe and reinstallation of the new FC6, I have no tree to test changes I'd still like to make.  Once I've gotten my system back to a stable state and have time to complete the last 5% of changes to the patch by writing a more complete set of tests for the new API, I'll submit a fully up-to-date version for review.

Comment 20

[Jeff Walden [:Waldo]]

(In reply to comment #17)
> I'm assuming you don't know about Date.toLocaleFormat(), since it's not been
> documented anywhere in the 18 months since it got added to JS 1.6 (see bug
> 291494).

No, I didn't know about it.  I very briefly started documenting it on <http://developer.mozilla.org/en/docs/User:Waldo:Date.toLocaleFormat>, to be completed and moved to the right place as and when I have time; help is appreciated.

(In reply to comment #18)
> an HTTP server can't use a locale-dependent date format.

It's good to know my understanding of how it works is correct; I had hopes that some part of the strftime API could do the job, but those hopes were not to be.  :-\

Comment 21

[Dave Liebreich [:davel]]

Comment on attachment 238858 [details] [diff] [review]
Adds the HTTP server in netwerk/test/httpserver, fixes existing tests to use it

just cleaning up my queue, since I believe later attachments obsolete this patch

Comment 22

[Robert Sayre]

> [e-mail] 
> 2006-09-22 20:18:17 PST
> review?([e-mail])

biesi, are you going to have time to review this soon?

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 239757 [details] [diff] [review]
More pending-request fixes

OK, first, my apologies for the really long delay here :/

not sure which attachment you want me to review now, picking the one with the request

README:
+- you can pass in a handle function as a request handler to registerPathHandler
+  and registerErrorHandler without wrapping it in an object implementing the
+  proper interfaces

if you marked nsIHttpRequestHandler  with the [function] attribute, you'd get the same feature even as a component.

+- more to come when and as it makes sense...

I'm not sure that you should check this line in

httpd.js:
+/** Constructs an HTTP error object. */
+function httpError(code, description)

would be nice to have constructors start with an uppercase character (i.e. HttpError)

+    trans = trans.QueryInterface(Ci.nsITransport);

why the QI? nsIServerSocket inherits from nsITransport...

+    this._processConnection(serverSocket.port, input, output);

what's the port for? if this is for redirects, shouldn't it just use the host: header? (I guess my code should've done that too, but I didn't originally parse the headers...)

+    if (this._socket)
+      throw Cr.NS_ERROR_FAILURE;

not NS_ERROR_ALREADY_INITIALIZED?

+                  .getService(Ci.nsIThreadManager)

you shouldn't need the interface name here FWIW


stopping here:
+  // see nsIHttpServer.registerFile
will continue later

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Addresses comments so far, adds test for registerPathWithDirectory

I took the time to finish up writing the tests I hadn't written for the registerPathWithDirectory API, so this version with arbitrary virtual-location directory mapping should be ready to go.  (Well, ready to go modulo a certain change to a certain existing test, which will be obvious when you reach it.  :-) I needed the netwerk tests to actually run when I was testing these changes.)  As noted in comment 19, the attachment before this wasn't quite ready for review, in my opinion, because it didn't have tests for the new functionality rPWD provided.

Speaking of rPWD, the method name is way too long -- suggestions for a better name are highly appreciated!  Maybe even just registerDirectory?


(In reply to comment #23)
> not sure which attachment you want me to review now, picking the one with the
> request

You have chosen wisely.  :-)


> if you marked nsIHttpRequestHandler  with the [function] attribute, you'd get
> the same feature even as a component.

Interesting, didn't know about that...added in this patch, but so far not actually tested in a component/extension pair.


> would be nice to have constructors start with an uppercase character (i.e.
> HttpError)

Fixed this one, will probably need to change others further down; for now, not changing them because I'm not sure exactly how you'll want them changed.


> +    this._processConnection(serverSocket.port, input, output);
> 
> what's the port for? if this is for redirects, shouldn't it just use the host:
> header? (I guess my code should've done that too, but I didn't originally
> parse the headers...)

It's mostly an extra bit of metadata to pass to handlers; I could look in the host line, but it seems dodgy to be trusting incoming values that way.  That might or might not break with proxies or other somewhat-unusual situations; let me know if it does.  If it does, I'll probably just leave off port information for now and make that followup work.

(Incidentally, redirects are another piece of followup work to handle -- there's a hack in ServerHandler.prototype._getFileForPath whose proper fix requires support for redirection, and I'm sure I'll need such functionality in other places as well.)


> not NS_ERROR_ALREADY_INITIALIZED?

Didn't remember it, nice...


> +                  .getService(Ci.nsIThreadManager)
> 
> you shouldn't need the interface name here FWIW

Why not?  I (incorrectly) thought .getService() returned an nsISupports that had to be QI'd to be usable, but since it works fine in testing, changed accordingly.


Looking forward to further feedback...

Comment 25

[James Ross]

The thread manager implements nsIClassInfo, and all interfaces returned by that are immediately available without further QIing by the caller.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

Some further comments:

+    if ("@mozilla.org/thread-manager;1" in Cc)

Why this if?

+  registerPathWithDirectory: function(path, directory)
+  {
+    // XXX true path validation!
+    if (path.charAt(0) != "/" ||

Why validate this path but not registerFile's?


+      metadata.port = port;
+      metadata.init(inStream);

Why not pass the port to the constructor function? (Could pass the stream
too, but a constructor that reads the entire stream passed to it seems sort
of strange to me, so probably not)

+      var error = this._handler.handleRequest(outStream, metadata);
+      if (error !== EXIT)
+        throw new Error("handleRequest returned a non-EXIT value!");

OK, so... what's the point of the return value here, if there is only one
allowed value?


+   *   the nsILocalFile representing the file to be served; must be a directory
+   */
+  registerFile: function(path, file)

Must not be a directory, right? ;)

It'd be nice to share the code between the anonymous function in
registerFile and _handleDefault


nsIHttpServer.idl:
+#include "nsIServerSocket.idl"
+#include "nsISimpleEnumerator.idl"
+#include "nsILocalFile.idl"
+#include "nsIOutputStream.idl"

Why do you need to include anything other than nsIServerSocket?

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Addresses latest comments

(In reply to comment #26)
> +    if ("@mozilla.org/thread-manager;1" in Cc)
> 
> Why this if?

This code works with very little effort in both 1.8 and 1.9, mostly related to thread stuff.  This is one such place where a hack is necessary, and the IDL docs for .stop() explicitly mention that some implementations can't make a guarantee of full shutdown completion when the method completes.  It's not perfect, but a 95%-working server for 1.8 code which only fails at shutdown (if at all, depending on how the shutdown occurs) is better than none.


> +  registerPathWithDirectory: function(path, directory)
> +  {
> +    // XXX true path validation!
> +    if (path.charAt(0) != "/" ||
> 
> Why validate this path but not registerFile's?

The reason for this is that if you look further on in code, we consider the URL /foo/bar/baz in order as /foo/bar/baz, /foo/bar/, /foo/, and / so that we select the most recent directory mapping (provided via rPWD).  For example, if /foo/ and /foo/bar/ are mapped, we should use the mapping provided by /foo/bar/.  The method by which the mapping occurs is via _pathDirectoryMap, which is a hash of string->nsILocalFile.  The strings used as keys are of the form "foo/bar/baz/", where the leading slash is stripped and the trailing slash is only present if the path being examined references a directory and we are examining the most derived path, e.g. "foo/bar/" for the examining the most-derived component of /foo/bar/ (as opposed to a less-derived component like / or /foo/).  The way in which we consider portions in order is via the line |tmp = tmp.substring(0, tmp.lastIndexOf("/"));|.  The problem is that "/foo".substring(0, 1) == "/", "/foo".substring(0, 1).substring(0, 1) == "/", etc. -- either we need explicit logic to abort the loop for the single-character string "/" or we eliminate the leading "/" such that eventually we hit the empty string, rather than testing for tmp == "/".  I prefer the empty check, because I prefer the way that the string converges to a final value if no mappings exist.

Now consider if the user passes in a value like "f/g" for a path.  If we strip the "f", we have "/g" as the string, and if there's no mapping, it'll never converge and we'll loop forever.  The problem if a bogus file path is passed in is less severe; we'll just ignore the file.  (I'd like to validate when I've implemented real path validation, but until then some level of checking doesn't provide enough bang for the buck, in my opinion.)

I agree that this is somewhat confusing.  Note that there's a comment (perhaps too brief?) on this in _getFileForPath regarding how mapping-checks progress for a multi-level URL to help explain this, and _pathDirectoryMap also has an explanatory comment regarding its structure in the ServerHandler constructor.  I can add more if necessary.


> +      metadata.port = port;
> +      metadata.init(inStream);
> 
> Why not pass the port to the constructor function?

Sure.


> +      var error = this._handler.handleRequest(outStream, metadata);
> +      if (error !== EXIT)
> +        throw new Error("handleRequest returned a non-EXIT value!");
> 
> OK, so... what's the point of the return value here, if there is only one
> allowed value?

Originally it was because of some concerns about handlers, which originally returned a Response or something like that, and I needed to verify that the handler worked correctly by returning a non-spoofable return value.  I really don't recall the exact situation any more, and since it doesn't seem necessary, it's gone.


> Must not be a directory, right? ;)

Right.  :-)


> It'd be nice to share the code between the anonymous function in
> registerFile and _handleDefault

Done.


> Why do you need to include anything other than nsIServerSocket?

No reason other than needing forward declarations (I switched to them).


We're making progress here, I like it!

Comment 28

[Jeff Walden [:Waldo]]

Attachment: rPWD -> registerDirectory

Changes:
- rename registerPathWithDirectory to registerDirectory
- add more docs mentioning .stop()'s brokenness on 1.8 and workingness on 1.9

Comment 29

[Jeff Walden [:Waldo]]

Attachment: Robustify behavior of handlers which throw exceptions

Changes:
- Makes handling of exceptions thrown by handlers a bit more robust -- fallback
  from original handler to 500, or from original to X00 to 500 if handler was
  an error handler
- Clarified docs on this fallback process so it's clear how the process works
  (might want to be README material since it's something we might want to be
  able to change -- thoughts on that?)
- Actually define a constant used in serving requests for local files
- Fix problem that |response.destroy(); reponse = new Response();| invalidates
  the response but doesn't reset it or allow further changes to be propagated
  outside of the method where it's executed (in such cases later changes
  wouldn't be saved)
- Test for these behaviors (when existing tests didn't cover them)

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

+  init: function(input)
+  {
+    // XXX this is incredibly un-RFC2616 in every possible way:

That's really in the functions it calls, not in this function itself. but ok.

+    while (lis.readLine(line), line.value == "")

If the response only contains empty lines, this'll loop forever, right? (note: I don't mind ;) )

+      // we support HTTP/1.0 and HTTP/1.1 only
+      this.errorCode = 400;

not 501 Not Implemented? :)

+    } // lis.readLine(line)

well it's actually a while(true)..

+  var thread = Cc["@mozilla.org/thread-manager;1"]
+                 .getService(Ci.nsIThreadManager)
+                 .currentThread;

please be consistent in specifying/omitting the interface

netwerk/test/httpserver/nsIHttpServer.idl
+ *   script-defined function for a full nsIHttpRequestHandler object (and
+ *   thus avoid having to wrap the function in an object and give it a
+ *   QueryInterface implementation).

You wouldn't need a QI impl in any case. This entire note seems superfluous to me, do you want to put such a note on all interfaces that use this attribute?


+  void setStatusLine(in string httpVersion, in unsigned short statusCode, in string description);

please limit lines to 80 chars

netwerk/test/httpserver/test/head_utils.js
+function makeBIS(stream)

might make sense to use Components.Constructor instead, then you could use bis = new BinaryInputStream(stream) to wrap the stream.


Why are you in some tests using new nsHttpServer when in others you use createHttpServer()?

Comment 31

[Jeff Walden [:Waldo]]

(In reply to comment #30)
> If the response only contains empty lines, this'll loop forever, right?

Yes.  I'll file a followup bug to deal with that after committing this.


> +      // we support HTTP/1.0 and HTTP/1.1 only
> +      this.errorCode = 400;
> 
> not 501 Not Implemented? :)

Sure.


> netwerk/test/httpserver/nsIHttpServer.idl
> You wouldn't need a QI impl in any case. This entire note seems superfluous to
> me, do you want to put such a note on all interfaces that use this attribute?

The QI note is somewhat wrong as noted, so I removed it.  As for [function], in the absence of good, easily discoverable documentation for it in XPIDL docs I think it's worth mentioning here, because otherwise no one will know about it or use it as it can (and is intended to) be used.


> netwerk/test/httpserver/test/head_utils.js
> +function makeBIS(stream)
> 
> might make sense to use Components.Constructor instead, then you could use bis
> = new BinaryInputStream(stream) to wrap the stream.

Sure, followup.


> Why are you in some tests using new nsHttpServer when in others you use
> createHttpServer()?

The server tests test the server itself.  I'd eventually like to run the server tests both when the server is an inline script (as occurs now) and when the server is used as a component, to catch when functionality unintentionally diverges between the two methods of use.  Using createHttpServer means choosing between the two is a matter of redefining one function (eventually, once there's a mechanism to load an XPCOM component in the build, test it, and remove it).

The netwerk tests, on the other hand, are meant to test networking code, not the server.  Re-running non-server-specific tests as a component in addition to as a script seems like the wrong way to use resources.  If there are component/script differences, they should be handled in server tests only.


I'll file followup bugs and upload and commit a final version of the patch after I've completed the various final projects I have due this week.

Comment 32

[Jeff Walden [:Waldo]]

Attachment: Patch as checked in

(In reply to comment #31)
> > If the response only contains empty lines, this'll loop forever, right?
> 
> Yes.  I'll file a followup bug to deal with that after committing this.

Actually, I just changed the condition and replaced |,| with |&&|.  The reason I hadn't done this before was that the docs for nsILineInputStream as displayed on XULPlanet didn't say what the return value was, and I assumed the docs just didn't say.  As it turns out they do say, but they use @retval instead of @returns to do so.  I filed bug 363902 to fix this.


> > might make sense to use Components.Constructor instead, then you could use
> > bis = new BinaryInputStream(stream) to wrap the stream.
> 
> Sure, followup.

See bug 363904.

Comment 33

[Jeff Walden [:Waldo]]

Patch checked in with all test-running tinderboxen on <http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTest> passing them according to logs, so marking FIXED.  Huge thanks to biesi for the review, and on to further improvements!

Comment 34

[Jeff Walden [:Waldo]]

Moving httpd.js bugs to the new Testing :: httpd.js component; filter out this bugmail by searching for "ICanHasHttpd.jsComponent".

Comment 35

[Jeff Walden [:Waldo]]

...and changing the QA contact as well.  Filter on the string "BugzillaQAContactHandlingIsStupid".