482418 - HTTP range handling code doesn't account for SJS files correctly

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jeff Walden [:Waldo]]

Range detection occurs before splitting control flow based on the type of the file.  This detection shouldn't happen for SJS files, which should handle that themselves (or more likely not handle it).  Here's one problem with the current setup:

1. Incoming request for valid range of data in an SJS file.
2. Request header processed, result set as 206 Partial Content.
3. SJS handler called, doesn't modify the status line, assumes default 200 OK.
4. Response has wrong status and a bogus Content-Range header.

Noted in passing while working on bug 396226, but I can't justify putting it in an already-huge patch when it's an entirely tangential bug.

Comment 1

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

Attachment: fix

As we discussed on IRC, we can just skip Range start/end processing for SJS files and let them handle it.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 374213 [details] [diff] [review]
fix

>diff --git a/netwerk/test/httpserver/httpd.js b/netwerk/test/httpserver/httpd.js
>+        metadata.hasHeader("Range") &&
>+        this._getTypeFromFile(file) != SJS_TYPE)

Make that === (equals-like-you-mean-it) as a general rule to avoid unexpected coercions creeping up when you least expect it.  They don't here, but...CONSTANT VIGILANCE! :-D  There may be contradictions to this rule in the rest of the file; I fix my mistakes as I notice them, at least so far.


>diff --git a/netwerk/test/httpserver/test/data/sjs/range-checker.sjs b/netwerk/test/httpserver/test/data/sjs/range-checker.sjs
>new file mode 100644
>--- /dev/null
>+++ b/netwerk/test/httpserver/test/data/sjs/range-checker.sjs
>@@ -0,0 +1,11 @@
>+function handleRequest(request, response)
>+{
>+  var expect = request.queryString.match(/^expect=(.*)$/);
>+  var range = request.getHeader("Range");
>+  if (range != expect[1]) {
>+    response.setStatusLine(request.httpVersion, 404, "Not Found");
>+  } else {
>+    response.setStatusLine(request.httpVersion, 200, "OK");
>+  }
>+  response.setHeader("Content-Type", "text/html");
>+}

This doesn't work, and if I qimport the patch, qref -X httpd.js, qnew -f more.diff, qpo, and rerun, test_sjs.js passes.  registerFile doesn't go through the buggy logic in _handleDefault that checks for range requests (it probably should as a separate bug), so the Range header you specify is ignored completely rather than erroneously throwing the 416 _handleDefault throws.  I'm also not sure why you have the queryString bit here; the handler only ever receives one request, with one fixed query string, so it seems superfluous.

The only differences between partial-content requests and normal requests of SJS handled as files nested within a directory of files being served, that I can tell, are that 1) the range interval is parsed (and thus error-checked) and used against the SJS file itself, 2) the generated response defaults to 206 Partial Content for the status line, and 3) the generated response has a preset Content-Range header.  I don't see how any of those are tested are tested here.  You should have one SJS file in which handleRequest is an empty function, and you should make several requests of it:

* one with Range: not-actually-bytes-equals-interval
* one with Range: bytes=-
* one with Range: bytes=1000000-

You should be able to share a single check function for all three that verifies (ch.responseStatus === 200 && ch.responseStatusText === "OK") and that ch.getResponseHeader("Content-Range") throws an exception (which is not specified, and I'd rather not set an example that some extension might follow which would then break in the extremely unlikely situation that we were ever to change to throw a different exception).

Please test a version of the patch with qref -X httpd.js to make sure everything fails appropriately without the httpd.js change.

Comment 3

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

I already spent a few hours today tracking down the bug in httpd.js, fixing it, then puzzling through the test code trying to figure out how to write a suitable unit test, and evidently failing due to (as you say) a separate bug. Would it be too much to ask you to meet me halfway and fix the test?

Comment 4

[Jeff Walden [:Waldo]]

(In reply to comment #3)
> I already spent a few hours today tracking down the bug in httpd.js, fixing it,
> then puzzling through the test code trying to figure out how to write a
> suitable unit test, and evidently failing due to (as you say) a separate bug.
> Would it be too much to ask you to meet me halfway and fix the test?

Sigh; I guess not.  It wasn't exactly the easiest thing to figure out why this wasn't working, just to note, it didn't occur to me for quite a while that the logic in _handleDefault doesn't run when a file is registered individually.

Comment 5

[Jeff Walden [:Waldo]]

Okay, http://hg.mozilla.org/mozilla-central/rev/6e1eeb511cf4 is the substantive change plus a series of tests for all the different possible ways the current code could go awry.  This will make its way over to branch when I get around to copying all the updates from m-c there, definitely after b4 is out...

Comment 6

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

Thank you!

Comment 7

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/07087b6e5fa7