838988 - Incorrect download status in nsIRequest::OnStopRequest

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[christophe.mouraud]

The aStatus parameter is incorrect (NS_OK instead of OnStopRequest) in method nsIRequest::OnStopRequest if the download (FTP or HTTP) is interrupted by the server shutdown. The bug is not systematic.

How to reproduce the bug with Firefox :
   - navigate into a directory of server (protocol FTP or HTTP),
   - start the download of a big file,
   - stop the server,
   - in some cases, no error is reported and the downloaded file is incomplete.

Correction in function nsInputStreamPump::OnStateTransfer() of file netwerk/base/src/nsInputStreamPump.cpp : 
517     if (NS_SUCCEEDED(mStatus)) {
518         if (NS_FAILED(rv))
519             mStatus = rv;
520         else if (avail) {
521             // if stream is now closed, advance to STATE_STOP right away.
522             // Available may return 0 bytes available at the moment; that
523             // would not mean that we are done.
524             // XXX async streams should have a GetStatus method!
525             rv = mAsyncStream->Available(&avail);
526             if (NS_SUCCEEDED(rv))
527                 return STATE_TRANSFER;
+               if (rv != NS_BASE_STREAM_CLOSED)
+                   mStatus = rv;
528         }
529     }
530     return STATE_STOP;
531 }

Comment 1

[Matthias Versen [:Matti]]

An attached Patch would it make easier to request a review but maybe Patrick can review the "patch as comment"

Comment 2

[Patrick McManus [:mcmanus]]

I would say attach a real patch and assign the review to jduell who probably knows that best.

Comment 3

[Jason Duell]

Attachment: v1

Actually I strongly suspect honza knows this better than I.  

Pushed to try for starters, as I suspect there's a lot of downstream code this change could affect.

  https://tbpl.mozilla.org/?tree=Try&rev=bce36533a378

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 711679 [details] [diff] [review]
v1

Review of attachment 711679 [details] [diff] [review]:
-----------------------------------------------------------------

I'm just curious why the first call to Available() doesn't detect this.  We should get one more call to OnInputStreamReady() for interrupt socket error, since according the code we wait after successful read.  I might be wrong either it could be a bug...  However, the second call to Available() to advance to STATE_STOP is already there, just missing rv/mStatus assignment.

r=honzab

This seems to me correct, nice and easy.

Please land after merge for Fx22 that happens in about one week.  This needs a full train coverage.

Comment 5

[Jason Duell]

> Please land after merge for Fx22 that happens in about one week.

will do.

Comment 6

[Jason Duell]

So I'm testing with the STR here with the patch applied and I'm not seeing any difference.  I.e. I start a download, then kill -9 the server process, and since the OS just closes the TCP socket's file descriptor at process exit, TCP just sends a regular FIN and we still see rv=NS_OK in the code here (and so that's what the download manager's OnStopRequest sees too).  From my reading of Richard Stevens' TCP books, that's exactly as expected, i.e. we don't get any way via TCP to tell if a server has crashed vs it called close() intentionally.  Am I missing something here?  

I'm no longer sure of the use case that we're trying to handle with this patch. Christophe, what scenario changes for you when you apply the fix?

Comment 7

[Jason Duell]

Does "stop the server" in the STR mean "shutdown/kill the operating system", not "kill the server process"?

Comment 8

[christophe.mouraud]

(In reply to Jason Duell (:jduell) from comment #6)
> So I'm testing with the STR here with the patch applied and I'm not seeing
> any difference.  I.e. I start a download, then kill -9 the server process,
> and since the OS just closes the TCP socket's file descriptor at process
> exit, TCP just sends a regular FIN and we still see rv=NS_OK in the code
> here (and so that's what the download manager's OnStopRequest sees too). 
> From my reading of Richard Stevens' TCP books, that's exactly as expected,
> i.e. we don't get any way via TCP to tell if a server has crashed vs it
> called close() intentionally.  Am I missing something here?  
> 
> I'm no longer sure of the use case that we're trying to handle with this
> patch. Christophe, what scenario changes for you when you apply the fix?

In my test, the apache server is stopped with command : "httpd -k stop".

The bug is not systematic : some time, we reach the transfer state "Stopped" after the first call to Available() (line 441) and the member mStatus is set correctly (line 520). Some time, the transfert state is set to "stopped" by the second call to Available() (line 526). In this case, the member mStatus is not set and the request is terminated with a NS_OK status while the download is not complete.

Sorry about my poor english level.

Comment 9

[Jason Duell]

Christophe,

I agree that your patch changes things so that we always set mStatus = rv if Available fails, so that's probably a good thing.  But I'm still wondering how we get to this state, since killing httpd gives me a FIN that means that Available returns NS_OK.  Perhaps we get a different result if we've been sent a RST?  Honza, do you know?  I'd just like to understand the use case here before we commit the patch.

Comment 10

[Patrick McManus [:mcmanus]]

If we get RST then we will return something other than OK.. The fin actually returns NS_ERROR_BASE_STREAM_CLOSED which we map into OK.. I can't remember exactly what the rst error is atm. And the RST is exceptionally racy in what can create it (it would normally be created in addition to the fin based on something happening - like ack traffic arriving after the process exited) and RST is an orderless control packet.. so we would honor it only if it arrived before we processed the fin (not necessarily arrived before the fin). tough to build an STR around :)

Comment 11

[Honza Bambas (:mayhemer)]

From my experience and knowledge I can confirm what Patrick says.  If we receive FIN prior to RST, there is no way to recognize a TCP error (i.e. non-graceful shutdown of a server or simply a connection interuption).  This could however be turned to an evang bug to make gracefully shutting down server deploy RST on active unfinished transmissions.

Note for me: perform those tests.

Also, we should make sure the server is not already behaving that way (since I would expect it).

If found Apache/others don't, for now, if we want let upper levels (like download manager or html renderer) know there were a resource download interruption while not having a RST in our hands, Content-length or chunks check must be made on level of HTTP channel.

Comment 12

[Matthias Versen [:Matti]]

Could this bug report the reason for bug 237623 ?

Comment 13

[Jason Duell]

> Could this bug be the reason for bug 237623?

That's what I thought initially, but this patch will only fail the download
manager for RSTs, and most aborted downloads end with the server sending FIN
instead of RST, in which we still send NS_OK to the download manager (it needs to use another mechanism to detect truncated download).  But it will be a small improvement.

   https://hg.mozilla.org/integration/mozilla-inbound/rev/17d25f2c3c38

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/17d25f2c3c38