121441 - unable to publish via http PUT (nsStorageStream should support nsISeekableStream)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Kathleen :Brade]

I am unable to publish via http PUT.

This is the relevant code snippet:
    nsCOMPtr<nsIUploadChannel> uploadChannel(do_QueryInterface(inputChannel));
    uploadChannel->SetUploadStream(inputstream, tmpFormat, -1);
    rv = inputChannel->AsyncOpen(this, nsnull);

I am trying to http publish with the username and password in the uri:
  http://brade:[e-mail]/users/brade/publish/testpub.html

The url loads fine in Composer but publishing results in writing a 0-length
file.  HTTP logging done; log file coming soon.

Comment 1

[Kathleen :Brade]

Attachment: http log file

This log file was reproduced by doing the following steps:
 * open location in Composer (http url with username and password)
 * make a change in composer 
 * load "www.ietf.org" in browser window
 * File | Publish
 * quit

You should skip past everything for ietf.org to get to the put command (very
end of log file).

Comment 2

[Kathleen :Brade]

clarification for any readers of this bug:  NO, my password isn't "password" :-P

Comment 3

[Darin Fisher]

does this "publish" example work for you in nav 4x?

Comment 4

[Kathleen :Brade]

4.x can publish this page to this server fine

Comment 5

[Darin Fisher]

-> 0.9.9 (critical)

Comment 6

[Darin Fisher]

the response from the server is a 401, and the username:password from the URL
are used to generate a new request.  this requires that the given nsIInputStream
be rewound, so it can be rewritten in the new request.  it appears that
nsStorageStream unfortunately does not support nsISeekableStream.  this is the
cause of this bug.  if authentication were not required, you'd find that
everything would just work.

Comment 7

[Darin Fisher]

Attachment: v1.0 patch

this patch adds support for nsISeekableStream to nsStorageInputStream.
brade: can you verify that this patch solves the HTTP PUT problem?

Comment 8

[Kathleen :Brade]

Comment on attachment 66247 [details] [diff] [review]
v1.0 patch

this patch didn't work until I changed this line to
NS_IMPL_THREADSAFE_ISUPPORTS2(nsStorageInputStream, nsIInputStream,
nsISeekableStream)

r=brade with that fix :-)

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 66247 [details] [diff] [review]
v1.0 patch

sr=dveditz with brade's change to the NS_IMPL_ macro

Comment 10

[Darin Fisher]

brade: thanks for catching my error :-)

fixed-on-trunk

Comment 11

[Aleksey Nogin]

nsStorageInputStream::Seek in this patch can pass an uninitialized value to Seek
if called with an unexpected value of aWhence. This, among other things,
produces a compiler warning.

Comment 12

[Kathleen :Brade]

I see a minor bug in nsStorageInputStream::ReadSegments as well:
  availableInSegment doesn't get reset when mReadCursor and mSegmentEnd are reset
This causes a needless iteration.
Adding "availableInSegment = mSegmentEnd - mReadCursor;" after mSegmentEnd is
reset should fix this.

Comment 13

[Darin Fisher]

brade: i don't follow... are you sure the current impl is flawed?

aleksey: thanks for catching that mistake... fixed.

Comment 14

[Kathleen :Brade]

Here is the snippet of code where I think the bug is with some comments tossed
in to explain what I saw:

 while (remainingCapacity) {    // remainingCapacity is 8192
   availableInSegment = mSegmentEnd - mReadCursor; // availableInSegment == 0
   if (!availableInSegment) {
     PRUint32 available = mStorageStream->mLogicalLength - mLogicalCursor;
     if (!available) // available is # of bytes to write (say 700)
       goto out;

     mReadCursor = mStorageStream->mSegmentedBuffer->GetSegment(mSegmentNum++);
     mSegmentEnd = mReadCursor + PR_MIN(mSegmentSize, available);
     // THIS is where I think we need to reset availableInSegment
     // since it is still 0
   }

   count = PR_MIN(availableInSegment, remainingCapacity);
   // count is 0 since that's the min of 0 and 8192
   writer(this, closure, mReadCursor, mLogicalCursor, count, &bytesConsumed);
   // this was called to write 0 bytes

looping again will reset availableInSegment as desired but we do an iteration
for writing 0 bytes when we don't need to

Comment 15

[Darin Fisher]

hmm... that makes sense.  i'm a bit worried about recalculating
availableInSegment, but i definitely think we should guard against calling the
writer to write zero bytes.

brade: how about filing a separate bug for this?  ... feel free to submit a
patch BTW :-)

Comment 16

[Darin Fisher]

marking FIXED again