443610 - Add body and PUT handling to httpd.js

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: v1 + test

This patch loads body request and when method is PUT it creates new path override on the server returning body of the request on GETs.

I am adding this enhancement because I didn't found any other way how to modify files on the server under the same path. It is needed for mochitest/unittest for offline application caches and its manifest/page updates. This change will be used in bug 443017.

Just a note, SJS is not the right way for me because it can be modified only by headers/request path or date included in response.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v2 + test [Checkin: Comment 7]

Added preserving of original path override.

Comment 2

[Honza Bambas (:mayhemer)]

Deletion of PUT data could probably be implemented with DELETE method instead of putting empty data.

Comment 3

[Robert Sayre]

(In reply to comment #2)
> Deletion of PUT data could probably be implemented with DELETE method instead
> of putting empty data.
> 

Please do this in a follow-up.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: Deletion by DELETE method [Checkin: Comment 8]

As suggested this is follow-up patch for using DELETE method. According to the HTTP RFC (if I understand correctly) when deleting non-existent content the response is 204 - No Content. Test updated according to it.

Comment 5

[Honza Bambas (:mayhemer)]

Please land "v2 + test" on trunk and don't close this bug, still one more patch waiting for review. Thanks.

Comment 6

[Honza Bambas (:mayhemer)]

Ignore comment 5, please land both patches in order as they appear on the list.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 328323 [details] [diff] [review]
v2 + test
[Checkin: Comment 7]

http://hg.mozilla.org/mozilla-central/rev/e5985246e421

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 331671 [details] [diff] [review]
Deletion by DELETE method
[Checkin: Comment 8]

http://hg.mozilla.org/mozilla-central/rev/28ff44649e9b

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Since this landed, bug 451664 has become much worse on Linux and is basically holding the tree closed. I think we should disable the tests in bug 451664 though, instead of backing this out.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Or should we backout this, because this all indicates that bug 451664 may have something to do with httpd.js.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe there's a bug in httpd.js, but this patch isn't it, since the failures in bug 451664 were happening before this patch. So I don't think backing this out will do anything other than make 451664 harder to reproduce.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 328323 [details] [diff] [review]
v2 + test
[Checkin: Comment 7]

So how does this handle large request bodies?
I'm not too familiar with httpd.js, but it seems that onInputStreamReady doesn't do the right thing if the state is READER_IN_BODY when the method in called.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

And indeed, I do get the following assertion (when DEBUG=true;):
###!!!
###!!! ASSERTION!
###!!! Stack follows:
###!!!   Error()@:0
###!!!   NS_ASSERT(false)@./httpd.js:74
###!!!   ([object XPCWrappedNative_NoHelper])@./httpd.js:1047
###!!!

The testcase contains huge header and body:
http://mxr.mozilla.org/mozilla-central/source//content/base/test/test_bug435425.html?raw=1
    { method: "POST", withUpload: large, testAbort: false, testRedirectError: false, testNetworkError: false,
      expectedEvents: [{target: XHR, type: "loadstart", optional: false},
                       {target: UPLOAD, type: "loadstart", optional: false},
                       {target: XHR, type: "uploadprogress", optional: true},
                       {target: UPLOAD, type: "progress", optional: true},
                       {target: UPLOAD, type: "load", optional: false},
                       {target: XHR, type: "progress", optional: true},
                       {target: XHR, type: "load", optional: false}]},

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This broke the ability to let sjs files handle DELETE and PUT requests. In my cross-site XMLHttpRequest tests I had to use XXDELETE and XXPUT to get things working.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to comment #14)
> This broke the ability to let sjs files handle DELETE and PUT requests. In my
> cross-site XMLHttpRequest tests I had to use XXDELETE and XXPUT to get things
> working.

Do you believe your work around is sufficient or we should figure out a correct solution? This feature (in this bug) is intended to allow override of actual content on the server, i.e. PUT data is tested before references to files including sjs. We could move PUT data AFTER it or add exception for sjs files not to be "overridable".

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I do think we still want the ability to use PUT and DELETE. Do you really need to override sjs files, or just static files, or even just non-existing files?

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 328323 [details] [diff] [review]
v2 + test
[Checkin: Comment 7]

I just noticed this in the revision log for httpd.js, sorry for not following more closely.  (I've filtered all bugmail to a folder which I more or less haven't opened since June; it's at ~22000 messages currently.  I don't know how much of that I'll actually read, to be honest -- some threads definitely, others maybe not at all, and it'll be awhile before I catch up no matter what.)  Component reorg also played havoc with my watching abilities such that I may not have gotten bugmail for this bug, either.

Check for PUT only if non-existent seems best to me.

Some comments on the first patch and changes (basically cosmetic, some substantial) that should happen (patch with review to me, I'm very nearly to the point of being able to do reviews again -- yay!), second comment will follow to address the second patch:

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

>+  this._contentLength = 0;

Need a comment saying what this value is, as with all other "private" properties.

>+      dumpn("_processRequestLine, Content-length="+this._contentLength);

Need spaces around the + operator, throughout the patch in numerous places.

>+  _processBody: function(input, count)

Where's the method documentation of functionality, arguments, and return value?

>+      dumpn("*** remainig body data len="+this._contentLength);

"remaining"

>@@ -1469,6 +1512,12 @@ RequestReader.prototype =
> 
>         // either way, we're done processing headers
>         this._state = READER_IN_BODY;
>+        try
>+        {
>+          this._contentLength = parseInt(headers.getHeader("Content-Length"));

Yippee, "015" for a Content-Length will be interpreted wrongly as octal, but the RFC clearly states the number is decimal (although it doesn't actually say anything about leading zeroes) -- please write a test for this and add a second argument of 10.

>+          dumpn("Content-Length="+this._contentLength);
>+        }
>+        catch (e) {}

Comment the empty block explaining why we're swallowing here.

>@@ -1917,10 +1971,55 @@ ServerHandler.prototype =
>     {
>       try
>       {
>-        // explicit paths first, then files based on existing directory mappings,
>-        // then (if the file doesn't exist) built-in server default paths
>-        if (path in this._overridePaths)
>+        if (metadata.method == "PUT")

This should go into a new function; it's a big chunk of code that makes this function hard to read.

>+        {
>+          // remotely set path override
>+          var data = metadata.body.purge();
>+          data = String.fromCharCode.apply(null, data.splice(0, data.length + 2));

I don't understand this splice -- doesn't it just copy data, and if so, why the + 2?

>+            this._putDataOverrides[path] =

Make a top-level function which returns a function doing this stuff -- avoid the naming ridiculousness, and don't close over unnecessary values preventing their collection by a GC that doesn't minimize binding capture (I think we may not, yet).

>+              function(ametadata, aresponse)
>+              {
>+                aresponse.setStatusLine(metadata.httpVersion, 200, "OK");

Er, shouldn't that be |ametadata| there?  The top-level thing should address this problem.

>+                aresponse.setHeader("Content-Type", contentType, false);
>+                dumpn("*** writting PUT data=\'"+data+"\'");

"writing"

We're supposed to send a 201 when a put override is added, not 200, right?  Easy enough to check and act accordingly, methinks.

>+          // PUT data overrides are priviledged before all
>+          // other overrides.

"privileged"

>+          dumpn("calling PUT data override for "+path);
>+          this._putDataOverrides[path](metadata, response);
>+        }
>+        else if (path in this._overridePaths)
>+        {
>+          // explicit paths first, then files based on existing directory mappings,
>+          // then (if the file doesn't exist) built-in server default paths
>+          dumpn("calling override for "+path);
>           this._overridePaths[path](metadata, response);
>+        }
>         else
>           this._handleDefault(metadata, response);

If you brace any part of an if-else if-else chain, all parts must be braced.

>+  /** Body data of the request */
>+  this._body = new LineData();

This is really an abuse of LineData, which was intended only for accessing CRLF-delimited data.  Followup issue, I guess...not to mention this not working for anything trying to use the server as an XPCOM component, probably want to use a StorageStream or Pipe for this.

>diff --git a/netwerk/test/httpserver/test/test_remotesetpath.js b/netwerk/test/httpserver/test/test_remotesetpath.js

Why use the old test format with manual gunk, rather than runHttpTests?  I'm not even going to try to read and understand this right now; the tests I originally wrote (including the one you copied to make these) were difficult enough to understand even immediately after I'd written them.  (I had a patch at one time to convert them to the new style that's probably half-bitrotted now, I think -- should get that in pronto to encourage easier-to-read tests.)

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 331671 [details] [diff] [review]
Deletion by DELETE method
[Checkin: Comment 8]

Actually, second patch seems reasonable enough, modulo the space-around-operators thing.

Comment 19

[Jeff Walden [:Waldo]]

Actually, after about 0.001s more thought...shurely there's some better way to get stateful responses than to add insta-XSS capabilities to the server?  I'm not sure what would be a clean way to extend the SJS system to allow some minimal state storage, offhand.

Comment 20

[Honza Bambas (:mayhemer)]

Jeff, I will response to all these comments after FF3.1b2 is done. I'd like to discuss how to enhance or change this new feature. It would be useful to override/set headers as well somehow.

Comment 21

[Jeff Walden [:Waldo]]

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