81384 - multipart converter should be able to handle byteranges

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Doug Turner (:dougt)]

Comment 1

[Doug Turner (:dougt)]

multipart converter should be able to handle byteranges

Comment 2

[Doug Turner (:dougt)]

Attachment: patch to streamconv.

Comment 3

[Doug Turner (:dougt)]

Attachment: nsIByteRangeRequest interface - new file - netwerk/base/public/

Comment 4

[Doug Turner (:dougt)]

Attachment: patch to netwerk/build

Comment 5

[Judson Valeski]

- some comments in the new idl would be nice (as self explainatory as it is), 
both iface level and method level.
- while you're in here, please turn the PRInt8's into PRInt32's. PRInt8's are 
actually more expensive now as most arch's just build up a 32 bit int anyway and 
use trickery to make you think it's 8.

- It's not clear to me what you're doing here:
         mFirstOnData = PR_FALSE;
         NS_ASSERTION(!mBufLen, "this is our first time through, we can't have 
buffered data");
         const char * token = mToken;
+            
+        PRInt8 chars = 0;
+        if (*buffer == nsCRT::CR || *buffer == nsCRT::LF) {
+            if (buffer[1] == nsCRT::LF)
+                chars++;
+            chars++;
+        }
 
         if (bufLen < mTokenLen+2) {
             // we don't have enough data yet to make this comparison.
             // skip this check, and try again the next time OnData()
             // is called.
             mFirstOnData = PR_TRUE;
-        } else if (!PL_strnstr(buffer, token, mTokenLen+2)) {
+        } else if (!PL_strnstr(buffer + chars, token, mTokenLen+2)) {
             bufLen = read + mTokenLen + 1;
             char *tmp = (char*)nsMemory::Alloc(bufLen);
             if (!tmp) {

a) you should just use PushOverLine() instead of expanding that linefeed char* 
incrementor. b) that will negate the need for the modified else if stmt. but, I 
don't see why you're doing this at all.

- thanks for updateing w/ ignore-case. when this code was first written, we were 
lowercasing *all* headers.

- in theory you'll have a mPartChannel all the way up until you send an 
OnStopRequest() to the final listener, but, after that, we explicitly null out 
the mPartChannel to break a circular dependency (see SendStop()). I'm a bit 
concerned about passing "this" as the channel on down the chain. pointer 
compares are common practice and the thought of a consumer getting the same 
channel back in two seperate On*() chain callback sets seems wierd. maybe 
mPartChannel (created for each "part") should be the nsIByteRangeRequest, and 
you should set the attribs through a a constructor for the mPartChannel.? That 
would protect from null mPartChannel's (those macros just look evil) and ensure 
that different channels get propogated to the consumer (definately a requirement 
at one point, maybe not anymore???).
+    NS_DECL_NSIBYTERANGEREQUEST
+    
+    // XXX what if mPartChannel is null
+    NS_FORWARD_NSICHANNEL(mPartChannel->)
+    NS_FORWARD_NSIREQUEST(mPartChannel->)

I still can't believe there's a multipart/byteranges :-).

Comment 6

[benc]

qa to me

What would be good test case (theoretical or real world for this)?

Comment 7

[Doug Turner (:dougt)]

- some comments in the new idl would be nice (as self explainatory as it is), 
both iface level and method level.

simple enough - done.  I will attach a new interface header.

- while you're in here, please turn the PRInt8's into PRInt32's. PRInt8's are 
actually more expensive now as most arch's just build up a 32 bit int anyway and 
use trickery to make you think it's 8.

done.

- It's not clear to me what you're doing here: ...

the first time the multipart/byterange data comes through, it has a CRLF that I 
need to remove so that the strstr does not fail.  I was worried about using 
PushOverLine for no real reasons.  

- pointer compares are common practice and the thought of a consumer getting the 
same channel back in two seperate On*() chain callback sets seems wierd. 

Bad assumption!!  There is no contract that this happens.  

- maybe mPartChannel (created for each "part") should be the 
nsIByteRangeRequest, and you should set the attribs through a a constructor for 
the mPartChannel.? 

Can't.  mPartChannel is a generic class.  I don't want to add the 
nsIByteRangeRequest to it.  The forwarding macros should test for null if that 
is a problem.

- I still can't believe there's a multipart/byteranges :-).

I can't believe how easy it was to modify your converter to support 
byteranges!!! :-)

Comment 8

[Doug Turner (:dougt)]

Attachment: Same interface with some comments.

Comment 9

[Doug Turner (:dougt)]

Attachment: Patch to stream converter - w/ Jud Suggestions

Comment 10

[Judson Valeski]

thanks doug. r=valeski after the _FORWARD() macros do null checking;
mPartChannel can be null.

Comment 11

[Darin Fisher]

checked in the following diff to nsHttpChannel.cpp:

Index: nsHttpChannel.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v
retrieving revision 1.10
diff -u -r1.10 nsHttpChannel.cpp
--- nsHttpChannel.cpp   2001/05/17 21:38:06 1.10
+++ nsHttpChannel.cpp   2001/05/17 22:48:04
@@ -360,6 +360,7 @@
     switch (httpStatus) {
     case 200:
     case 203:
+    case 206:
         rv = ProcessNormal();
         break;
     case 300:

Comment 12

[Doug Turner (:dougt)]

I checked the interface in.  I will give the converter another round of testing, 
and I will check that in...

Comment 13

[Doug Turner (:dougt)]

Fix checked in.

Comment 14

[rpotts (gone)]

see bug #81937 for follow-up info...