User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20081001 Shredder/3.0b1pre Dragged an attachment from a message to the desktop. An icon for the attached file appears on the desktop, but it is 0 bytes in size. This does not appear to dependon file type: I've tried pdf and excel file attachments. Reproducible: Always Steps to Reproduce: 1.open message 2.drag file attachment icon from message window to desktop 3. Actual Results: File copied to desktop contains no data (0 bytes) Expected Results: File copied to desktop should be a copy of the actual attachment. Should produce same result as right clicking on attachment and saving, which does work.

WFM on Windows XP using the 10/2 nightly.

WFM on Windows Vista using the 10/2 nightly, as well.

(In reply to comment #2) > WFM on Windows Vista using the 10/2 nightly, as well. Tried again with 10/2 nightly, but still seeing same problem.

I get this bug using Thunderbird/3.0b2 on vista

I'm still seeing this bug in Vista SP1 now using TB3b2. It doesn't matter where I drag an attachment or what type of attachment it is, if I drag it to the desktop or to a Windows Explorer folder, the icon appears but has size 0 bytes. If I right click on the attachment and select "save as" then select a location from the dialog box, it will save without any problems. It's obviously more convenient to be able to drag and drop. I hope someone can pick up this bug and fix it. Thanks!

From bug 487180: Drag'n'drop of attachments from the message reader to the desktop or file folders (i.e. outside Shredder) results in zero length files. Right-clicking and choosing "Save As..." works fine. This happens every time with Shredder nightlies, but works fine in Thunderbird 2.0.x. BROKEN: 2008-05 This already happens in Thunderbird 3a1 from May 2008, with the difference that Windows displays a "moving..." status window with a progress bar, then a message in the windows task bar that Thunderbird was closed because of Data Execution Prevention, although that doesn't close Thunderbird. It still results in a zero length file. BROKEN: 2007-06-01 The 2007-06-01 trunk nightly displays the same "moving..." status window, but no message regarding to Data Execution Prevention. It results in no file at all. BROKEN: 2006-06-01 The 2006-06-01 trunk nightly (ugh, is that ugly!) drops a file called "INBOX4175" with zero length. BROKEN: 2006-03-02 "3.0a1" trunk nightly, same as before. BROKEN: 2006-02-28 "1.6a1" trunk nightly. WORKS: 2006-02-01 The 2006-02-01 "1.6a1" trunk nightly, called 1.6a1, works fine. Looks like the 1.6a1 nightlies are from the 1.9.0 cvs trunk indeed, since the version was directly changed to 3.0a1 on 2006-03-01: http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mail/config/version.txt&rev=HEAD&mark=1.6

BROKEN: 2006-02-21-10: crashes when dragging attachments just a few pixels. WORKS: 2006-02-20-11: drops the complete file as expected.

In that range is: Bug 267426 "an empty file when dragging images (files) out of the browser to the Windows desktop or Explorer or drives other than C:" That's funny. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=ThunderbirdTinderbox&branch=HEAD&date=explicit&mindate=2006-02-20+10%3A00&maxdate=2006-02-21+10%3A00

This is broken on Windows Vista, but works on Windows XP.

We need to do something about making attachment handling be less painful. It might be fixing this bug, or at least disabling this functionality on Vista, it might be something related bug 436555, or maybe even something else. Marking as blocking+ as a way of ensure that something doesn't get lost. Starting off in davida's hands, as he's the owner of bug 436555.

The reason this doesn't work is that we don't set the content length correctly in any of our channel/protocol code, and <http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDataObj.cpp#303> needs it. Setting it to an arbitrary value X causes a file of size X to be created. Somehow XP's explorer didn't need it, and Vista/7's explorer does. Taking, and ccing bienvenu for ideas -- I'll see what I can do about this.

> Somehow XP's explorer didn't need it, and Vista/7's explorer does. In fact, XP's explorer doesn't call ::Stat at all.

Well, this required a bit of a rework. I've moved the progress stuff into a maxProgress attribute, which allowed me to move that code to PRInt64s. (Removing old XXX comments is fun :) ) I've tested this a fair bit with all three kinds of messages/attachments, and it seems to work.

More 64-bit goodness.

Comment on attachment 379268 [details] [diff] [review] patch, v1.01 very nice! + nsresult rv = msgHdr->GetMessageSize(&messageSize); + if (NS_SUCCEEDED(rv)) + SetContentLength(messageSize); since we're dropping rv, we can just say if (NS_SUCCEEDED(msgHdr->GetMessageSize())) I'll try this patch out on Vista

> // this can happen when viewing a news://host/message-id url, so try > // getting the spec and using that Doh, this comment is wrong. We'll handle internal news:// URLs, i.e. ones with group and key params. I think we can have unit tests for the content length.

(In reply to comment #17) > > I think we can have unit tests for the content length. Great !

Addresses the review comment, fixes the comments, and adds unit tests. David -- I'm not sure what can be done about news servers that don't return sizes (like the fakeserver). Any ideas?

A few thoughts - first of all, we have to fetch the whole message, unlike w/ imap, in order for the user to start a drag drop of a news attachment. So we could keep track of the total msg size when fetching the message, and stick it in the db. Or, perhaps there's a way to get the message size out of the mem/disk cache entry for the news message, though that would fail if the mem cache or disk cache was turned off.

I tried the v1 patch with imap and local, and imap with a non-downloaded attachment, and it all worked fine - great job, Sid!

Comment on attachment 379355 [details] [diff] [review] patch, v2 I tried news and imap online/offline drag drop to vista desktop and it all worked fine. One nit - maybe remove the XXX from comments, since I don't think we're going to make changes to the code. + // so make an arbitrary decision based on the content length of the attachment + if (mMaxProgress != -1 && mMaxProgress > aBytesDownloaded * 2) this means if the first block is less than half the total, pop up the dialog? One other thing - I do get a strange error if the progress dialog does popup on a drag drop, with the following message: 'thunderbird.exe': Loaded 'C:\Windows\SysWOW64\actxprxy.dll' First-chance exception at 0x76ba8a2a in thunderbird.exe: 0xC0000005: Access violation reading location 0xfeeefef6. Windows has triggered a breakpoint in thunderbird.exe. This may be due to a corruption of the heap, which indicates a bug in thunderbird.exe or any of the DLLs it has loaded. This may also be due to the user pressing F12 while thunderbird.exe has focus. The output window may have more diagnostic information. Continuing on always works, but should probably figure out what's going on there. r=bienvenu, with those comments addressed.

(In reply to comment #22) > > + // so make an arbitrary decision based on the content length of the > attachment > + if (mMaxProgress != -1 && mMaxProgress > aBytesDownloaded * 2) > > this means if the first block is less than half the total, pop up the dialog? Yes. While I'm here I'll add a comment explaining this. > > One other thing - I do get a strange error if the progress dialog does popup on > a drag drop Are you talking about the Thunderbird progress dialog or the Explorer one? I'm unable to get the former to show up on a drag drop, and I don't see that exception happening with the latter. A trouble I have with drag/drop (but only with a debugger attached) is an access violation reading location 0xdddddde5, and that looks like a bug in backend code. Could you try changing this line: <http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDataObj.cpp#2009> to aSTG.pUnkForRelease = NULL; and see if that fixes your issue? It fixes mine.

Oh, and unlike yours, this access violation is actually fatal.

I was talking about the Explorer dialog (I believe). Adding that line seems to make it so the dialog does not come up, which fixes the issue. Is that what you meant?

Carrying forward r+ with comments addressed. I'll file a bug for the widget/ bug shortly.

(In reply to comment #26) > I'll file a bug for the widget/ > bug shortly. Bug 495075.

Comment on attachment 379894 [details] [diff] [review] patch, v3 I don't think you've answered comment 25 it would be nice to have just for the record. > if(mTransfer) > { >- mTransfer->OnProgressChange(nsnull, nsnull, mContentLength, mContentLength, mContentLength, mContentLength); >+ mTransfer->OnProgressChange64(nsnull, nsnull, mMaxProgress, mMaxProgress, mMaxProgress, mMaxProgress); > mTransfer->OnStateChange(nsnull, nsnull, nsIWebProgressListener::STATE_STOP | > nsIWebProgressListener::STATE_IS_NETWORK, NS_OK); > mTransfer = nsnull; // break any circular dependencies between the progress dialog and use > } Does this make its way back to our progress dialogs/status bar feedback? If so, I'm concerned about the swap to 64 - because our progress dialogs & status bars currently truncate the value to 32 bits. I guess this will only be a problem for very large files though > 4GB?

(In reply to comment #28) > (From update of attachment 379894 [details] [diff] [review]) > I don't think you've answered comment 25 it would be nice to have just for the > record. Sorry, I answered it over IRC. According to bienvenu, with the one line fix applied: - if the attachment's offline, the operation happens too quickly for a dialog to come up - if the attachment isn't offline, then the dialog does come up, but the exception doesn't occur. So the fix works in both cases. > If so, I'm concerned about the swap to 64 - because our progress dialogs & > status bars currently truncate the value to 32 bits. I guess this will only be > a problem for very large files though > 4GB? Yes. The current code wouldn't send progress correctly for file sizes over 2 GB, so this is an improvement. If you wish, I can file a followup bug for fixing the progress dialogs.

(In reply to comment #29) > (In reply to comment #28) > > If so, I'm concerned about the swap to 64 - because our progress dialogs & > > status bars currently truncate the value to 32 bits. I guess this will only be > > a problem for very large files though > 4GB? > > Yes. The current code wouldn't send progress correctly for file sizes over 2 > GB, so this is an improvement. If you wish, I can file a followup bug for > fixing the progress dialogs. Please make sure we have one on file. When running the tests in debug mode I'm getting: TEST-UNEXPECTED-FAIL | /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_mailnewslocal/unit/test_mailboxContentLength.js | test failed (with xpcshell return code: -6), see following log: ... ###!!! ASSERTION: Fatal ... bad message format : 'strncmp(start, "From ", 5) == 0', file /Users/moztest/comm/main/src/mailnews/local/src/nsLocalMailFolder.cpp, line 2396

(In reply to comment #30) > When running the tests in debug mode I'm getting: > > TEST-UNEXPECTED-FAIL | > /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_mailnewslocal/unit/test_mailboxContentLength.js > | test failed (with xpcshell return code: -6), see following log: Sorry, ignore this I hadn't spotted a bad merge (caused by qimportbz mangling line endings I think).

Comment on attachment 379894 [details] [diff] [review] patch, v3 >+ // Set max progress if we haven't already got it. >+ if (mMaxProgress == -1) >+ { >+ nsCOMPtr<nsIURI> uri; >+ channel->GetURI(getter_AddRefs(uri)); >+ nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl(do_QueryInterface(uri)); >+ if (mailnewsUrl) >+ mailnewsUrl->GetMaxProgress(&mMaxProgress); nit: the comment should say Get. >+++ b/mailnews/news/test/unit/test_nntpContentLength.js >+function run_test() { >+ // XXX The server doesn't support returning sizes! >+ return; Please file a follow-up bug on this and set in-testsuite? on it so that we can track the fact this test isn't run. sr=Standard8

Pushed to c-c. http://hg.mozilla.org/comm-central/rev/af67b109386e

(In reply to comment #32) > > >+++ b/mailnews/news/test/unit/test_nntpContentLength.js > >+function run_test() { > >+ // XXX The server doesn't support returning sizes! > >+ return; > > Please file a follow-up bug on this and set in-testsuite? on it so that we can > track the fact this test isn't run. > Filed bug 497080.

Filed bug 497087 about setting the content length of attachment URLs correctly.