190549 - Textarea with lots of text within refuses to submit

Status: RESOLVED FIXED

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

Description

[Dean Richard Benson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030118
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030118

Using the above URL (vipersoft.co.uk/form1.html) if I click on "Go" it stays on
the first page.  This is broken between the 20020116 and 20020118 builds.

Reproducible: Always

Steps to Reproduce:
1.Load URL (http://vipersoft.co.uk/form1.html)
2.Click Go
3.Does it stay on first page (form1) or go through to second page (form2)

Actual Results:  
stays on first page (form1)

Expected Results:  
should go to second page (form2)

Comment 1

[Boris Zbarsky [:bzbarsky]]

to darin... this seems to be the necko patch.  :(

Darin, Dean says he sees it on a CVS build from one hour ago (so it's not fixed
by the other thing's you've landed so far).  Other people are seeing this too,
so confirming.

Comment 2

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

more async I/O fallout.

Comment 3

[Andrew Schultz]

*** Bug 190595 has been marked as a duplicate of this bug. ***

Comment 4

[Boris Zbarsky [:bzbarsky]]

According to bug 190595 this is linux-only.  The bug also contains an http log...

Comment 5

[Per Ångström]

Vipersoft's testcase works fine in "Mozilla/5.0 (Windows; U; Win98; en-US;
rv:1.3b) Gecko/20030124".

Comment 6

[Dean Richard Benson]

This does appear to be a Linux only problem.

Also when there is > 11k chars (approx) - the amount of characters does vary but
is around the figure of 11,000.

Comment 7

[tenthumbs]

With a 2003-01-26 Linux trunk cvs build I see the same thing when using direct
access. However, it succeeds it I use my squid proxy. Interesting.

Comment 8

[Boris Zbarsky [:bzbarsky]]

tenthumbs, what are your http settings for "normal" browsing and for proxies? 
Does changing either set of settings affect the situation?

Comment 9

[tenthumbs]

Changing HTTP 1/[0-1], persistent connections, or pipelining has no effect.
Proxy always works, direct doesn't.

Comment 10

[tenthumbs]

I tried capturing packets on the default route and the loopback interface
simultaneously for a proxy connection but tcpdump and/or ethereal are giving me
troubles. On the loopback dump I can only find half of the POST but it's all
there on the default route. Since my squid doesn't cache POST requests I'm not
sure what's happening.

Comment 11

[tenthumbs]

Via my squid proxy, strace (with some struggle and some editing) shows this:

send(27, "POST http://vipersoft.co.uk/form"..., 515, 0) = 515
send(27, "Content-Type: application/x-www-"..., 49, 0) = 49
send(27, "Content-Length: 39491\r\n\r\n", 25, 0) = 25
send(27, "huge=123456789012345678901234567"..., 39491, 0) = 39491
recv(27, "HTTP/1.0 200 OK\r\nDate: Sun, 26 J"..., 4096, 0) = 333
recv(27, 0x852d422, 3994, 0)            = -1 EAGAIN (Resource temporarily
unavailable)
recv(27, "", 3994, 0)                   = 0
close(27)                               = 0


For a direct connection we get:

send(26, "POST /form2.html HTTP/1.1\r\nHost:"..., 487, 0) = 487
send(26, "Content-Type: application/x-www-"..., 49, 0) = 49
send(26, "Content-Length: 39491\r\n\r\n", 25, 0) = 25
send(26, "huge=123456789012345678901234567"..., 39491, 0) = 11510

close(26)                               = 0

It appears that mozilla is unhappy with the short count on send.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Does anyone here who's seeing this build, by any chance?  I'd like to know what
branch is followed in nsHttpConnection::OnReadSegment in the failure case....

Comment 13

[Andrew Schultz]

when I submit the form, it goes into that function 6 times:
NS_OK
NS_BASE_STREAM_CLOSED
NS_OK
NS_BASE_STREAM_CLOSED
NS_OK
NS_BASE_STREAM_CLOSED

Comment 14

[Andrew Schultz]

sorry, 7 times:
NS_OK
NS_BASE_STREAM_CLOSED
NS_OK
NS_BASE_STREAM_CLOSED
NS_OK
NS_BASE_STREAM_CLOSED
NS_OK

Comment 15

[Boris Zbarsky [:bzbarsky]]

Andrew, could you try commenting out that middle

482     else if (*countRead == 0)
483         mSocketOutCondition = NS_BASE_STREAM_CLOSED;

?  Does that fix this bug for you?  (not the real fix; just trying to narrow
down where the problem is....).

Comment 16

[Andrew Schultz]

with the NS_BASE_STREAM_CLOSED branch commented out, it still goes through 7
times, all NS_OK now, but does not change the beavhior of this bug (form still
not submitted)

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, I've finally managed to reproduce the bug (I tripled the amount of text in
the textarea and it seems that I can only get 50k bytes out in a single
write...).  Looking now...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ok.. So there are two bugs here.  Bug #1 is that the NS_WARNING on line 544 of
nsHttpConnection.cpp doesn't print anything to the terminal.  Perhaps this is
because we're not on the UI thread??  In any case, we're hitting that "else"
clause. Here's what's up:

We call ReadSegments on the multiplex input stream.  It returns NS_OK, but does
_not_ read all the available data (because the socket only accepted so much). 
The socket does not set mSocketOutCondition to NS_BASE_STREAM_WOULD_BLOCK
because it in fact did not block -- it read some data.  The multiplex input
stream interprets being unable to read all the data as meaning that no more
reading should be done; see the code in nsMultiplexInputStream::ReadSegCb that does:

295     if (rv == NS_BASE_STREAM_WOULD_BLOCK ||
296         (NS_SUCCEEDED(rv) && *aWriteCount < aCount && aCount != 0))
297         state->mDone = PR_TRUE;

(here rv is the return from the writer callback it got passed).

If I understand the contract correctly (could we _please_ document this in the
nsIInputStream IDL?? please??), ReadSegments is expected to continue calling the
writer until either the writer fails or there is nothing to read.  In this case,
the writer succeeds, there is more data to be read, so we are not done.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: possible patch

This fixes the bug for me...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Though now that I think about it, should that not just set mDone on _any_
failure?  Not just on blocking?

Comment 21

[Boris Zbarsky [:bzbarsky]]

The other interesting thing are those calls that pass an empty buffer and hence
get a read count of 0....  Those seem to be due to the ends of streams inside a
multiplex input stream. 

Comment 22

[Boris Zbarsky [:bzbarsky]]

Right; the end of string stream calls the writer func with a 0-byte buffer (see
ConstCharImpl::ReadSegments).  Should it just set *result to 0 and return NS_OK
without calling the writer, perhaps?

Comment 23

[Boris Zbarsky [:bzbarsky]]

So... I just looked at the blame for that line.  It used to look like this:

if (rv == NS_BASE_STREAM_WOULD_BLOCK ||  
    (NS_SUCCEEDED(rv) && *aWriteCount == 0 && aCount != 0))
        state->mDone = PR_TRUE;

The change to |*aWriteCount < aCount| was made in bug 126829.  So I guess we
need to test file submission with that patch.... 

Comment 24

[Darin Fisher]

Attachment: v2 patch

this patch cleans up the multiplex input stream error handling.  i've also
corrected the documentation on nsIInputStream/nsIOutputStream.	the return
value of the nsReadSegmentFun/nsWriteSegmentFun must never be propogated to the
caller of ReadSegments/WriteSegments.  doing do causes ambiguity when
responding to the return value of ReadSegments/WriteSegments.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 112809 [details] [diff] [review]
v2 patch

Please add a comment to the IDL something like:

"ReadSegments is expected to keep calling the writer until either there is
nothing left to read or the writer fails"

Similarly for WriteSegments.

+	 if (NS_FAILED(rv))
+	     break;

This depends on the fix to make the file stream not bogusly return
STREAM_CLOSED

The rest looks good.

Comment 26

[Darin Fisher]

Attachment: v2.1 patch

revised patch per bz's comments.  also, includes bug/typo fix in
nsStreamUtils.cpp pointed out by bz in the async-io review.

Comment 27

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

Comment on attachment 112881 [details] [diff] [review]
v2.1 patch

r=sicking. Much nicer now!

Please document what ::Read/::ReadSegments returning less then requested bytes
means.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 112881 [details] [diff] [review]
v2.1 patch

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 29

[Darin Fisher]

sicking: actually, all streams may return less than the specified count except
for blocking output streams.  only those streams must block until all given data
can be consumed (or an error occurs).  i'm documenting this fact.

consider a blocking pipe.  the reader may say give me 4096 bytes, but the pipe
may only have 10 bytes of data.  it should give the caller the 10 bytes.  it
should not block waiting for the next 4086 bytes.

Comment 30

[Darin Fisher]

fixed-on-trunk