24996 - Unable to download files via save as dialog

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Judson Valeski]

The affects both HTTP and FTP downloads. When you try to download a file, the 
progress dialog appears, sets itself to 100%, closes, then you crash. in 
nsStreamXferOp.cpp

Comment 1

[Judson Valeski]

Bill, I think we're creating and starting the StreamXferOp before the initial 
channel is canceled. XferOp::OnStopReq() is getting fired before anything really 
happens, thus the mInputStream get's zero'd out and we crash. I'm assuming the 
OnStopReq() get's hit because we're resetting the channel's Notifications to the 
XferOp *before* it's been canceled. Can you confirm?

Comment 2

[Chris McAfee]

marking dogfood

Comment 3

[Jon Granrose]

marking blocker.  cc'ing dougt.

Comment 4

[Judson Valeski]

here's the stack. it's an async callback so it's not too helpful.. but it shows 
the crash.

mInputChannel is null.

NS_IMETHODIMP
nsStreamXferOp::OnProgress(nsIChannel* channel, nsISupports* aContext,
                                     PRUint32 aProgress, PRUint32 aProgressMax) 
{
    nsresult rv = NS_OK;

    if (mContentLength < 1) {
        NS_ASSERTION(channel, "should have a channel");
        rv = mInputChannel->GetContentLength(&mContentLength);
        if (NS_FAILED(rv)) return rv;
    }

NTDLL! 77f7629c()
nsDebug::Assertion(const char * 0x02b7bedc 
??_C@_0DJ@KMGL@You?5can?8t?5dereference?5a?5NULL?5nsC@, const char * 0x02b7bf20 
??_C@_0N@NHHF@mRawPtr?5?$CB?$DN?50?$AA@, const char * 0x02b7bf30 
??_C@_0CE@LLDK@?4?4?2?4?4?2?4?4?2?4?4?2dist?2include?2nsCOMPt@, int 569) line 
189 + 13 bytes
nsDebug::PreCondition(const char * 0x02b7bedc 
??_C@_0DJ@KMGL@You?5can?8t?5dereference?5a?5NULL?5nsC@, const char * 0x02b7bf20 
??_C@_0N@NHHF@mRawPtr?5?$CB?$DN?50?$AA@, const char * 0x02b7bf30 
??_C@_0CE@LLDK@?4?4?2?4?4?2?4?4?2?4?4?2dist?2include?2nsCOMPt@, int 569) line 
278 + 21 bytes
nsCOMPtr<nsIChannel>::operator->() line 569 + 34 bytes
nsStreamXferOp::OnProgress(nsStreamXferOp * const 0x02ac00d8, nsIChannel * 
0x02ab0e70, nsISupports * 0x02ab0e70, unsigned int 137, unsigned int 0) line 289 
+ 18 bytes
nsHTTPChannel::OnProgress(nsHTTPChannel * const 0x02ab0e78, nsIChannel * 
0x02b604b4, nsISupports * 0x02ab0e70, unsigned int 137, unsigned int 0) line 627 
+ 60 bytes
XPTC_InvokeByIndex(nsISupports * 0x02ab0e78, unsigned int 3, unsigned int 4, 
nsXPTCVariant * 0x02b653f0) line 139
EventHandler(PLEvent * 0x02b65650) line 473 + 41 bytes
PL_HandleEvent(PLEvent * 0x02b65650) line 526 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00c927e0) line 487 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x03e306f0, unsigned int 49489, unsigned int 0, 
long 13182944) line 975 + 9 bytes
USER32! 77e71820()
00c927e0()

Comment 5

[Judson Valeski]

It looks like we're processing events differently (not necessarily incorrectly).

We reset the notification callbacks on the channel that was cancelled, *before* 
the cancel notification makes it back. So, the new notification handlers get hit 
w/ the OnStopRequest() that gets fired because of the previous Cancel().

Comment 6

[Judson Valeski]

I tried backing out last nights change to nsprpub/lib/ds/plevent.c to no avail. 
Something else is aggrivating this.

Comment 7

[Judson Valeski]

I think I've got this. re-building. looks like an nsifile change is trying to 
open the dest file as readonly.

Comment 8

[Judson Valeski]

scratch that. didn't have any affect. still in the dark.

Comment 9

[Bill Law]

I've got yesterday's changes building now, should be done soon.

I can't get any of yesterday's or today's build's to install, but that's got to 
be another bug.

Comment 10

[Sean Richardson]

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

Comment 11

[Judson Valeski]

The problem is that we're trying to write the data to a file that doesn't exist. 
This problem was introduced during the nsIFile landing. I'm not sure whether 
NS_NewLocalFile() should create a non-existant file (probably should) or whether 
the file transport should create the file.

Comment 12

[Judson Valeski]

The problem was that we weren't creating the file to download to, before trying 
to write to it. Fix patch attatched.

Comment 13

[Judson Valeski]

Attachment: Fix.

Comment 14

[Judson Valeski]

I need a review for this patch.

Comment 15

[Warren Harris]

I'm not sure I like the fix. Shouldn't the check be in the file transport's 
nsLocalFileSystem::GetOutputStream? Or maybe in nsFileInputStream::Init?

Comment 16

[Judson Valeski]

I agree that the creation symantics should be built into channel usage. I tried 
putting it in both of the places you suggest (and the nsFileOutputStream) but 
those code paths aren't hit before we fail. You and doug should work out what 
the creation symantics should be. I want to push this through for now to get the 
tree open.

Comment 17

[Judson Valeski]

fix checked in. I opened http://bugzilla.mozilla.org/show_bug.cgi?id=25022 
regarding create symantics and the file transport.

Comment 18

[Warren Harris]

This fix is wrong because it forces the download code to create the 
destination file before writing. That should be done by the file transport. If 
we don't fix the file transport to do this, other clients will have the same 
problem.

I'm including a patch which I believe fixes the problem, but the fix is a little 
scary and I need several reviewers:

1. The changes to nsFileTransport.cpp are mostly trivial. They just don't bail 
if the file doesn't already exist. This is ok, because the code that creates the 
file comes along later (how it used to work before the nsIFile landing). [Jud 
should review this.]

2. The addition of mSource->Close() in nsFileTransport.cpp deals with a failure 
case that doesn't come up in normal operation. If for some reason the output 
file can't be created, the transport trying to do the AsyncWrite shuts down, but 
doesn't close the input stream. Later, http fills up the input stream and blocks 
waiting for someone to take the data away, but no one does. Deadlock! 

3. But... the addition of the Close call isn't enough since there seems to be a 
bug in nsPipe2.cpp that ignores the fact that the stream is closed when trying 
to do a Write operation. (Why? I don't know.) I took this out and it didn't 
seem to affect anything else, but it needs thorough testing. [The mail/news team 
(mscott) should run with this change for a while.]

4. I took out the code in nsStreamXferOp.cpp that Jud added to work around this 
problem.

5. One other weirdness in nsStreamXferOp.cpp -- In the failure case I mentioned 
above, the OnStopRequest method comes in and causes the mInputChannel to be 
dropped, and then later an OnStatus comes in and crashes because mInputChannel 
is null. However, mInputChannel should be the same as the channel pass in as an 
argument so I just use that one instead of the one that was nulled out. [Bill 
should review this one.]

Comment 19

[Warren Harris]

Reassigning to myself. Including patch.

Comment 20

[Warren Harris]

Attachment: the patch of course

Comment 21

[Judson Valeski]

1. The code to move on if the file DNE looks fine. However, where does the file 
actually get created?

Comment 22

[Judson Valeski]

I'm not sure why this bug we re-opened. The actual bug warren is fixing is 
http://bugzilla.mozilla.org/show_bug.cgi?id=25022 .

Comment 23

[leger]

Moving [DOGFOOD] to keywords field.

Comment 24

[Warren Harris]

I don't believe in closing the original bug when a hack/work-around is 
available, and opening a new bug for the real fix. I know it was necessary to 
get the hack into m13, but I think we should just check it in and move the bug 
to m14 at that point awaiting the real fix. Opening a new bug scatters the 
information about the problem.

Comment 25

[Judson Valeski]

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

Comment 26

[rickg]

Dogfood per the PDT.

Comment 27

[Warren Harris]

I checked in this patch now.

Comment 28

[Tom Everingham]

verified:
NT 2000020414