Closed Bug 572065 Opened 14 years ago Closed 14 years ago

Attachment functions broken - open/save/detach don't work (IMAP only)

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: u331436, Assigned: Bienvenu)

References

Details

(Keywords: regression, Whiteboard: [tbtrunkneeds])

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100614 Lightning/1.1a1pre SeaMonkey/2.0pre - not Firefox
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100614 Lightning/1.1a1pre SeaMonkey/2.0pre - not Firefox

unable to open or save attachments to mail.
Message display one or several attachment. Double clicking on it to open or "save as" not working.
workaround: forward message "inline", the new message opens and then it is possible to double-click to open the attachment or "save as"..
Seems a regression since about Friday 11th or Saturday 12th of June.
Will try from a test profile (no add-on)

Reproducible: Always

Steps to Reproduce:
1.double-click and attachment
2 [review].expect the registered application to open (word, excel, adobe...)
3.nothing happens

Alternatively, 
1. "save as" open up the dialog to save the attachment
2 [review]. Type in name and location of file to save
3. Nothing happens



Expected Results:  
Application should open / file should be save in chosen location

Will try ASAP with a test profile
Version: unspecified → Trunk
Test profile shows the same behavior.
- unable to open an attachment using the default applications
- unable to save as a file, in any chosen location

It seems that the download do not start !
(My email account use IMAP with all message downloaded locally (main profile) or staying on the mail server (test profile)).
See also bug 572086.

Attachments open and save for me if they are in Local Folders, but not IMAP. So you can copy the affected message into Local Folders/Trash and then open and/or save the attachment.

Attachments open for me if they are readable by the browser (text, PDF etc.) but I still can't save them directly from IMAP.

Another workaround: right-click, View Source, then use the Save As option.
Confirmed for Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100615 Lightning/1.1a1pre SeaMonkey/2.1a2pre

> Attachments open and save for me if they are in Local Folders, but not IMAP. So
> you can copy the affected message into Local Folders/Trash and then open and/or
> save the attachment.

> Attachments open for me if they are readable by the browser (text, PDF etc.)
> but I still can't save them directly from IMAP.

Same here. IMAP account FAIL. Local folder WFM.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Setting dependency for now since Bug 572086 is in a different product.
Depends on: 572086
(In reply to comment #4)
> Setting dependency for now since Bug 572086 is in a different product.

I think its fairly safe to say these are the same issue, therefore moving product and merging etc.
Component: MailNews: Message Display → Networking: IMAP
No longer depends on: 572086
OS: Windows XP → All
Product: SeaMonkey → MailNews Core
QA Contact: message-display → networking.imap
Hardware: x86 → All
Summary: unable to open or save message attachment → Attachment functions broken - open/save/detach don't work (IMAP only)
Whiteboard: [tb32needs]
Saving of an attachment from an unsynchronized IMAP folder still works for me
on trunk with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100608 Shredder/3.2a1pre; I didn't set any mime_parts_on_demand prefs for this profile, in case that makes a difference.
Dominique in comment 0 was spot on with the range.

Regression range:

Works:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100610 Shredder/3.2a1pre
2010-06-10-04-comm-central-trunk

Broken:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a6pre) Gecko/20100611 Shredder/3.2a1pre
2010-06-11-03-comm-central-trunk

Pushlog - Comm-central:
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2010-06-10+03%3A00%3A00&enddate=2010-06-11+04%3A00%3A00

Pushlog - Mozilla-central:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-06-10+03%3A00%3A00&enddate=2010-06-11+04%3A00%3A00
Wild guess:

neil@parkwaycc.co.uk
Thu Jun 10 03:10:37 2010 -0700	c2c24182c931	Jan Horak — Bug 377319 Convert mailnews/imap to the external API r=Neil sr=bienvenu
	49782a242a72	Jan Horak — Bug 377319 Convert mailnews/base/src to the external API r=Neil sr=bienvenu
	12981f9325ed	Jan Horak — Bug 377319 Convert mailnews/mime to the external API r=Neil sr=bienvenu
Typo fix aside, I also fixed MsgFindCharInSet, the code is loosely based on nsStringObsolete.cpp's version of FindCharInSet.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #451542 - Flags: superreview?(bienvenu)
Attachment #451542 - Flags: review?(bugzilla)
Attachment #451542 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 451542 [details] [diff] [review]
Proposed patch - checked in.

this does fix the problem, and the code is based on the nsStringObsolete code, as asserted...
Comment on attachment 451542 [details] [diff] [review]
Proposed patch - checked in.

This seems to fix my open problem, but not the saving or detaching of attachments.

I also think we should be thinking about a unit test for at least one of these scenarios.
Assignee: neil → nobody
Status: ASSIGNED → NEW
Is there anything blocking resolution? Any chance or getting this bug solved ? (Sorry, I do not intend to be rude or too pressing on the developers.. my apologies for my own impatience).
This is next on my list of things to do.
Assignee: nobody → bienvenu
      if (fetchService)
      {
        PRInt32 sectionPos = urlString.Find("?section");
        fullMessageUri.Append(Substring(urlString, sectionPos));
      }

vs previous.

      if (fetchService)
      {
        nsCString mimePart;
        PRInt32 sectionPos = urlString.Find("?section");
        urlString.Right(mimePart, urlString.Length() - sectionPos);
        fullMessageUri.Append(mimePart);
      }

If sectionPos is -1, the new code does not do the same thing as the old code. Granted, the old code produced a rather strange uri, but it's what nsParseImapMessageURI expects. I'll fix it, and try to write a unit test for saving imap attachments.
Substring(urlString, -1) returns the empty string, I believe.
Along with your previous patch, this patch makes save as and detach work for imap. I'm working on a unit test now. Once we have a unit test, we can maybe improve the code, but w/o one, I'd rather make it work the way it used to...
Attached patch fix with unit test (obsolete) — Splinter Review
I'm really asking rkent for a review of the test, and neil for r/sr for the fix itself.
Attachment #453963 - Flags: superreview?(neil)
Attachment #453963 - Flags: review?(kent)
Comment on attachment 453963 [details] [diff] [review]
fix with unit test

>         PRInt32 sectionPos = urlString.Find("?section");
>-        fullMessageUri.Append(Substring(urlString, sectionPos));
>+        if (sectionPos == kNotFound)
>+          fullMessageUri.Append(urlString);
>+        else
>+          fullMessageUri.Append(Substring(urlString, sectionPos));
Could it be that the IMAP code just needs the query part of the string, whether or not it contains section, i.e. call FindChar('?') instead?
Comment on attachment 451542 [details] [diff] [review]
Proposed patch - checked in.

Shouldn't the review request here be cancelled?
No, we need that patch too, I believe, right, Neil?

I think we look for section later on, but perhaps not. I'll look.
(In reply to comment #22)
> No, we need that patch too, I believe, right, Neil?
Yes, we need that for opening attachments.

> I think we look for section later on, but perhaps not. I'll look.
Obviously if it is ?section then it doesn't matter whether we look for ? or ?section but in the non-?section case then just looking for ? should work.
Attached patch fix addressing Neil's comment (obsolete) — Splinter Review
per Neil's suggestion of looking for '?'
Attachment #453963 - Attachment is obsolete: true
Attachment #454149 - Flags: superreview?(neil)
Attachment #454149 - Flags: review?(kent)
Attachment #453963 - Flags: superreview?(neil)
Attachment #453963 - Flags: review?(kent)
Comment on attachment 454149 [details] [diff] [review]
fix addressing Neil's comment

Doesn't hurt to check that the ? exists, I guess, just in case someone passes in a non-attachment ;-)
Attachment #454149 - Flags: superreview?(neil) → superreview+
Comment on attachment 451542 [details] [diff] [review]
Proposed patch - checked in.

given that we (will soon) have a test for this, I'm going to steal the review from standard8 and r/sr+ it.
Attachment #451542 - Flags: review?(bugzilla) → review+
The unit test is hanging for me (Windows 7), with the last log message "Starting test: startDetach"

I tried a few other tests to make sure that my setup is working OK, and they worked fine.

I've mentioned this before, but if you just wait the test eventually times out, and then claims that it "passed". There is something rotten somewhere in the testing infrastructure that a timeout is a pass.
rkent, does this hang for you? 

I've seen the same thing with tests passing on timeout - I wonder if it's an issue with the async_driver framework stuff, or something more basic.
Attachment #454149 - Attachment is obsolete: true
Attachment #454280 - Flags: review?(kent)
Attachment #454149 - Flags: review?(kent)
The test still times out. Your change was in the wrong place anyway, it never gets past the yield at the end of startDetach.

I don't have time now to confirm this, but what I think I see in the debugger is that this code in nsParseImapMessageURI:

    if (part && keyEndSeparator != -1)
    {
      PRInt32 partPos = MsgFind(uriStr, "part=", PR_FALSE, keyEndSeparator);
      if (partPos != -1)
      {
        *part = ToNewCString(Substring(uriStr, keyEndSeparator));
      }
    }

is not finding the part when uriStr is:

+		uriStr	{mStorage=0x0014c5c0 "imap-message://user@localhost/INBOX#1?part=1.2&filename=bob.txt" }	nsCAutoString

which I don't understand. Maybe I'm completely offtrack though. I'll try to get to this again soon.
rkent, did you apply Neil's first patch? It's also required.
With Neil's patch applied, the test is now running normally. Sorry about that. I'll now review the code itself.
Comment on attachment 454280 [details] [diff] [review]
fix timeout function - checked in.

Nit: I could not find any global definition of gMessage, so I think it should be just "let message =" and then use message instead of gMessage.

I don't really understand the requirement for dummyWindow, though I did confirm the test fails without it. Is this an issue with detachAttachmentsWOPrompts that should be fixed?
Attachment #454280 - Flags: review?(kent) → review+
(In reply to comment #33)
> I don't really understand the requirement for dummyWindow, though I did confirm
> the test fails without it. Is this an issue with detachAttachmentsWOPrompts
> that should be fixed?

Yes, the detach code in general requires that the folder be open in a message window, so that after doing the append of the new copy of the message, we update the folder, and download the header for the newly appended message, which triggers the detach code to continue. I've expanded the comment in the test code to try to explain this a little better, and I'll file a bug on detachAttachmentsWOPrompts.
Flags: in-testsuite+
Attachment #451542 - Attachment description: Proposed patch → Proposed patch - checked in.
Attachment #454280 - Attachment description: fix timeout function → fix timeout function - checked in.
both patches checked in (sorry, Neil, forgot to put bug # when I pushed your patch :-( )
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Verified fixed with IMAP account and today's nightly:  20100629010301 (Win)
Status: RESOLVED → VERIFIED
Pushed on the SeaMonkey 2.1a2 relbranch as http://hg.mozilla.org/comm-central/rev/c2d2f31db138 and http://hg.mozilla.org/comm-central/rev/238198843d19 so this is fixed there (in build2) as well.
This problem occurs with imap and pop3 account settings.
Whiteboard: [tb32needs] → [tbtrunkneeds]
With Thunderbird 3.1.4 and CompactHeader 1.3.0beta3 installed, it is not possible to open some attachment (not completely reproducible, seems to be correlated to the attachment type but also to the specific computer/OS). After the update to Thunderbird 3.1.5 that problem disappeared. Is it related to this bug or is there another bug description at bugzilla which fixed it?
You need to log in before you can comment on or make changes to this bug.