[Vista only] Drag'n'drop attachment out of Shredder (desktop or other folder) creates 0 byte file

RESOLVED FIXED in Thunderbird 3.0b3

Status

MailNews Core
Attachments
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Magritte, Assigned: sid0)

Tracking

(Depends on: 2 bugs, {regression})

1.9.1 Branch
Thunderbird 3.0b3
x86
Windows Vista
regression
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Vista only])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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.

Comment 1

10 years ago
WFM on Windows XP using the 10/2 nightly.

Comment 2

10 years ago
WFM on Windows Vista using the 10/2 nightly, as well.
(Reporter)

Comment 3

10 years ago
(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.

Comment 4

9 years ago
I get this bug using Thunderbird/3.0b2 on vista
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Attachments
Product: Thunderbird → MailNews Core
QA Contact: general → attachments
Version: unspecified → 1.9.1 Branch
(Reporter)

Comment 5

9 years ago
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!
Flags: wanted-thunderbird3?

Updated

9 years ago
Duplicate of this bug: 487180

Comment 7

9 years ago
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
Flags: blocking-thunderbird3?
Keywords: regression

Comment 8

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

Comment 9

9 years ago
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

Comment 10

9 years ago
This is broken on Windows Vista, but works on Windows XP.
Whiteboard: [Vista only]

Updated

9 years ago
Blocks: 267426

Updated

9 years ago
Summary: Dragging attachment to desktop creates 0 byte file. → Drag'n'drop attachment out of Shredder (desktop or other folder) creates 0 byte file.

Updated

9 years ago
Summary: Drag'n'drop attachment out of Shredder (desktop or other folder) creates 0 byte file. → [Vista only] Drag'n'drop attachment out of Shredder (desktop or other folder) creates 0 byte file
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.
Assignee: nobody → david.ascher
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b4
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.
Assignee: david.ascher → sid.bugzilla
> 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.
Created attachment 379264 [details] [diff] [review]
patch, v1

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.
(Assignee)

Updated

9 years ago
Attachment #379264 - Flags: superreview?(bugzilla)
Attachment #379264 - Flags: review?(bienvenu)
Created attachment 379268 [details] [diff] [review]
patch, v1.01

More 64-bit goodness.
Attachment #379264 - Attachment is obsolete: true
Attachment #379268 - Flags: superreview?(bugzilla)
Attachment #379268 - Flags: review?(bienvenu)
Attachment #379264 - Flags: superreview?(bugzilla)
Attachment #379264 - Flags: review?(bienvenu)

Comment 16

9 years ago
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 !
Flags: wanted-thunderbird3? → in-testsuite?
Created attachment 379355 [details] [diff] [review]
patch, v2

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?
Attachment #379268 - Attachment is obsolete: true
Attachment #379355 - Flags: superreview?(bugzilla)
Attachment #379355 - Flags: review?(bienvenu)
Attachment #379268 - Flags: superreview?(bugzilla)
Attachment #379268 - Flags: review?(bienvenu)

Comment 20

9 years ago
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.

Comment 21

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

Updated

9 years ago
Attachment #379355 - Flags: review?(bienvenu) → review+

Comment 22

9 years ago
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.

Comment 25

9 years ago
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?
Created attachment 379894 [details] [diff] [review]
patch, v3

Carrying forward r+ with comments addressed. I'll file a bug for the widget/ bug shortly.
Attachment #379355 - Attachment is obsolete: true
Attachment #379894 - Flags: superreview?(bugzilla)
Attachment #379894 - Flags: review+
Attachment #379355 - Flags: superreview?(bugzilla)
(Assignee)

Updated

9 years ago
Depends on: 495075
(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).
Attachment #379894 - Flags: superreview?(bugzilla) → superreview+
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: Thunderbird 3.0b4 → Thunderbird 3.0b3
(Assignee)

Updated

9 years ago
Depends on: 497080
(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.
Depends on: 782629
You need to log in before you can comment on or make changes to this bug.