398185 - Add byte range request support to JS httpd

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Mook (songbird dead account)]

Attachment: half-assed partial patch

It'd be nice to add byte range requests (Range: bytes=0-99) support to httpd.js - turns out that one of our components depend on it for testing :/ (Since we already forked this, it's a local patch, but it would be nice upstream)

Attached patch sort of works well enough for us, but 1) it's not actually parsing the header correctly, 2) it ignores the end point and just serves to the end of the file.

Comment 1

[cajbir (:cajbir)]

Attachment: Updates patch to latest trunk

Byte range request support is required for testing video seeking in bug 449159.

Comment 2

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

Are you ready to request review here? Seems like sayrer is your best bet.

Comment 3

[Robert Sayre]

You have to add a test for it or Waldo will have my head.

Comment 4

[cajbir (:cajbir)]

Attachment: Adds tests

Comment 5

[cajbir (:cajbir)]

Attachment: Added error handling

The original patch caused an update test (test_0030_general.js) to fail - it used an out of range byte range request. This new patch checks for out of range and results in a 416 return code as per HTTP spec. I've added a test for the out of range case as well.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 340494 [details] [diff] [review]
Added error handling

So if I have a number anywhere in the Range header that gets used as a starting offset (no check for "bytes=") and the entire rest of the file is sent, no attempt to only send to a specified end byte if one was specified?  Not good enough.  I'll accept requiring something matching the following regular expression:

/^bytes=(\d+)-(\d+)?$/

...where the first is the start byte, the second if present is the end byte and if not sends the rest of the file, and both numbers are well-ordered and in-range.  If the syntax isn't that, return a 400, I think that's semantically okay, and if the range isn't satisfiable, do the 416 thing.  Also only do the range stuff if the HTTP version is great enough (I don't know if 1.0 had ranges).

These are just drive-by comments, but I think they cover everything I care about.  This shouldn't take too long to do, I hope.

Comment 7

[cajbir (:cajbir)]

Attachment: Address review comments

Review comments address. I also added the Content-Range header so file resumes, etc work.

Comment 8

[cajbir (:cajbir)]

Comment on attachment 342547 [details] [diff] [review]
Address review comments

There's a bug when start range == 0. Adding test and fix in a patch shortly.

Comment 9

[cajbir (:cajbir)]

Attachment: Adds test and fixes for additional range cases

A start position of zero wasn't handled correctly. Added fix and test.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 343035 [details] [diff] [review]
Adds test and fixes for additional range cases

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

>+    var start;
>+    var end;

Define both on one line, please.

>+    if (metadata._httpVersion.atLeast(nsHttpVersion.HTTP_1_1) &&
>+        metadata.hasHeader("Range")) {

Braces always on a new line, aligned with "if" and "else", throughout the patch.

>+      response.setStatusLine(metadata.httpVersion, 206, "Partial Content");
>+      var rangeMatch = metadata.getHeader("Range").match(/^bytes=(\d+)?-(\d+)?$/);
>+      if (!rangeMatch)
>+        throw HTTP_400;
>+
>+      if (rangeMatch[1] != null)
>+        start = parseInt(rangeMatch[1]);
>+
>+      if (rangeMatch[2] != null)
>+        end = parseInt(rangeMatch[2]);

A not-included parentheses match is the value |undefined|, not |null|.  Make that !== as well at the same time, also for all such subsequent comparisons.  Second, parseInt on a string starting with "0" assumes octal, but it seems reasonable to believe that if "bytes=011-33" is not malformed (HTTP says nothing about it that I can find), it doesn't mean bytes 9-33; specify an explicit 10 radix to correct this.  Also add a test to verify this works correctly.

>+      if (start == null && end == null)
>+        throw HTTP_400;

Add a test for this case where "bytes=-".

>+      if (start != null && end != null && end < start)
>+        throw HTTP_400;

So on a more careful read of the RFC, I see:

"If the last-byte-pos value is present, it MUST be greater than or equal to the first-byte-pos in that byte-range-spec, or the byte- range-spec is syntactically invalid. The recipient of a byte-range- set that includes one or more syntactically invalid byte-range-spec values MUST ignore the header field that includes that byte-range- set."

So what we have with bytes=N-M where N > M is a syntactically invalid range, so we should actually just send the full file with a 200 response, it seems, on a closer read; I'd only cursorily glanced at this part of the RFC before this.  Do you agree?

>+      // start and end are inclusive
>+      if (end == null || end >= file.fileSize)
>+        end = file.fileSize - 1;
>+
>+      // No start given, so the end is really the count of bytes from the
>+      // end of the file.
>+      if (start == null) {
>+        start = file.fileSize - end;
>+        end   = file.fileSize - 1;
>+      }

I /think/, if I'm reading this right, that bytes=-500 on a 10-byte file would actually omit the first byte, and I /think/ that switching the order of these two if statements would address the problem, if you clamp |start| to be non-negative after setting it in the new first if.  Add a test for this while you're at it.

>+    else {
>+      start = 0;
>+      end = file.fileSize-1;

Spaces around the -.

>+    dumpn("*** handling '" + path + "' as mapping to " + file.path + " from " + start + " to " + end + " inclusive");
>+    this._writeFileResponse(metadata, file, response, start, (end-start)+1);

Don't parenthesize |end-start|, and add spaces around the operator.

>+   * @param offset: integer
>+   *   the byte offset to skip to when writing
>+   * @param count: integer
>+   *   the number of bytes to write

I believe I've been using PRUint32 for the type here, not integer, since we're interacting with an API that expects a PR* type.

>       var fis = new FileInputStream(file, PR_RDONLY, 0444,
>                                     Ci.nsIFileInputStream.CLOSE_ON_EOF);
>-      response.bodyOutputStream.writeFrom(fis, file.fileSize);
>+      offset = offset || 0;
>+      count  = count || file.fileSize;
>+      if (offset != 0) {
>+        if(offset >= file.fileSize) {
>+          dumpn("*** offset of " + offset + " was greater than file size of " + file.fileSize);
>+          throw HTTP_416;
>+        }

Is it even possible to hit this?  Don't we check this in the method that calls this?  Add an assert that offset is < the file size.

>+        else if(count <= 0) {

Need a space after "if".

>+          dumpn("*** length of range, " + count + ", was less than zero");
>+          throw HTTP_416;

I don't see how this can be hit either, augment the above-requested assert with this.

>+        }
>+        else if (fis instanceof Ci.nsISeekableStream) {
>+          fis.seek(Ci.nsISeekableStream.SEEK_SET, offset);
>+        }
>+        else {
>+          dumpn("*** file stream is not seekable, failed to seek to offset " + offset);
>+          throw HTTP_416;

Would just 200 returning the entire thing maybe make more sense here?  Alternately, perhaps it makes sense to just read offset bytes from a scriptable wrapper around the stream and not bother seeking.  It's less efficient but more reliable, I'd think, and I don't think we're that concerned about efficiency to care.

>+    416: function(metadata, response)

I can't tell if you're following the 80-character line limit or not here on this computer; please make sure you are.

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

>+var tests =
>+  [
>+   new  Test(PREFIX + "/normal-file.txt",
>+             init_byterange, start_byterange, stop_byterange),

Only a single space between new and Test, throughout.

>+   new  Test(PREFIX + "/normal-file.txt",
>+             init_byterange2, start_byterange2, null),

Just omit if not needed, I should fix the comment in head_utils.js about that, really.

>+  var dir = do_get_file("netwerk/test/httpserver/test/data/name-scheme/");

Can you add a ranges/ directory with a similar file in it?  I'd rather not reuse files across functionality tests when a change in the file for one set of tests might affect other unrelated sets of tests.

>+function start_byterange8(ch, cx)
>+{
>+  do_check_eq(ch.responseStatus, 206);
>+}

Check for the proper Content-Range response header here, since it's not the same numerically as the range specified by the Range request header.

This time I actually referred to the RFC rather than just drivebying, so I think these actually are all the comments that need to be addressed, rather than "probably all".

Comment 11

[cajbir (:cajbir)]

> Add an assert that offset is < the file size.

I can't find any usages of assert anywhere else in httpd.js, nor can I find documentation on assert. Help an xpcshell newbie out here, When you say 'add an assert' what exactly do you mean?

Comment 12

[cajbir (:cajbir)]

Attachment: Address review comments

Address review comments. For the request to add an assertion I check and throw a string, following the examples of similar in the file.

I removed the use of a seekable stream and create a scriptable wrapper and read/discard the offset. 

I notice in httpd.js that there is a usage of parseInt to get the content-length that doesn't restrict the radix btw.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 343377 [details] [diff] [review]
Address review comments

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

>+      if (start == undefined && end == undefined)

>+      if (start == undefined)

Use === and !== throughout when comparing to undefined, please -- I stated only the latter but assumed the former was implied as well.

We can common the start comparison and make the end comparison simply a nested if, would be slightly clearer methinks.

>+    dumpn("*** handling '" + path + "' as mapping to " + file.path + " from " + start + " to " + end + " inclusive");
>+    this._writeFileResponse(metadata, file, response, start, end - start + 1);

Overlong line(s?) alert!

>+      if (offset >= file.fileSize)
>+        throw "bad offset";

When I said assert I meant use NS_ASSERT, defined earlier in httpd.js -- these tests run with DEBUG = true, so if it were hit a stack trace would be printed.

>+      if (count <= 0)
>+        throw "bad count";

I think you can only do < here; add a test serving an empty file with no Range header (only this situation will hit this code, if I'm not mistaken, as any range must have at least one byte) to be sure.  The previous structuring of the code guarded this with an |offset != 0| check and thus didn't have this problem.  I guess that makes the overlong dumpn kind of a lie sometimes, but I can't think of a super-simple way to make it fully accurate for the empty-file edge case -- whatever.

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

Beautiful.

(In reply to comment #12)
> I notice in httpd.js that there is a usage of parseInt to get the
> content-length that doesn't restrict the radix btw.

Yeah, I just noticed that reviewing the applied patches from bug 443610, we'll get it there.

Thanks for being patient with my anality over non-shipping code!

Comment 14

[:Mook]

(In reply to comment #13)
> (From update of attachment 343377 [details] [diff] [review])
> >diff --git a/mozilla/netwerk/test/httpserver/httpd.js b/mozilla/netwerk/test/httpserver/httpd.js
> 
> >+      if (start == undefined && end == undefined)
> 
> >+      if (start == undefined)
> 
> Use === and !== throughout when comparing to undefined, please -- I stated only
> the latter but assumed the former was implied as well.

drive-by: would typeof(start) == "undefined" be better?  It's possible (but completely stupid) to say var undefined = 42;

Comment 15

[cajbir (:cajbir)]

Attachment: Address remaining comments

I changed the asserts to NS_ASSERT and added the test as suggested which triggered the assert with offset being equal to the filesize. So I modified the asset to allow zero offsets. Thanks for the review!

Comment 16

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/3a90ec0f5027

Comment 17

[Jeff Walden [:Waldo]]

(In reply to comment #14)
> drive-by: would typeof(start) == "undefined" be better?  It's possible (but
> completely stupid) to say var undefined = 42;

First, if someone does that in their code and then sources httpd.js, I have no sympathy for them.  I consider undefined not being a keyword and literal value to be a bug in JavaScript, and I don't agree with adjusting code to accommodate this design flaw.

Second, the suggested comparison is harder to read and more indirect than the one used here.  I'm testing that a value is equal to a particular value, not that the type of a value is a particular type.  To understand typeof you have to first understand that you're checking the value is of the "undefined" type, and second you have to realize that the "undefined" type has only one member.  Any JavaScript author worth his salt will instinctively know this, true, but it's the same argument against doing |42 == foo| instead of |foo == 42| -- humans think the one way and not the other, so the other is less readable.  (Come to think of it, that also works around a design flaw in a different language -- more similarity than I thought.)  Put simply, readability trumps handling developer stupidity.

Comment 18

[Jeff Walden [:Waldo]]

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