Closed Bug 736782 Opened 13 years ago Closed 13 years ago

No (inactive) download link for 'download message headers only' to download the rest of the pop message

Categories

(MailNews Core :: MIME, defect)

x86
Windows XP
defect
Not set
major

Tracking

(thunderbird13+ fixed)

VERIFIED FIXED
Thunderbird 14.0
Tracking Status
thunderbird13 + fixed

People

(Reporter: unicorn.consulting, Assigned: Bienvenu)

References

()

Details

(Keywords: regression, Whiteboard: [gs])

Attachments

(4 files, 4 obsolete files)

Have had a Yahoo pop mail account configured for 'fetch headers only' however I can not fetch the mail body as there is no link or button within the message body to download the body of the message. I get the standard message about header only downloaded.
Which message is that? I think bug 57115 touched this. If possible can you attach a sample message?
Keywords: regression
I have tested this on current TB14 (where bug 57115 is applied) and it seems to work. The link is there and the message body is downloaded. Just for some reason I get always thrown onto some other message. But selecting the original message shows that its body was downloaded. So can you attach a screenshot?
Attached image Screen shot as requested (deleted) —
Attached is a screen shot, fresh as today. Have just done the daily update. Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20120318 Thunderbird/14.0a1 Build Id 20120318030022 It does not matter where I click there does not appear to be a way to download the body of the message.
Can you also do Save as File (.eml) and attach the .eml here?
Ah, OK. So the placeholder is there. The text "Download the rest of the message." should contain the link to download. I see you do not have the link there, is is not underlined. Or do you have some special theming? Can you confirm this same message when viewed in TB older than 13 does have the link properly? Yes, I have updated the design of that placeholder in bug 57115. But I did not intend to change the logic. This is the code that fills in the URL for the link (mailnews/mime/src/mimemsg.cpp): 900 if (msgIdPtr) { 901 partialMsgHtml.AppendLiteral("&messageid="); 902 903 MsgEscapeString(nsDependentCString(msgIdPtr), nsINetUtil::ESCAPE_URL_PATH, 904 item); 905 906 partialMsgHtml.Append(item); 907 } 908 909 if (uidl) { 910 partialMsgHtml.AppendLiteral("&uidl="); 911 912 MsgEscapeString(nsDependentCString(uidl), nsINetUtil::ESCAPE_XALPHAS, 913 item); 914 915 partialMsgHtml.Append(item); 916 } So it is true in some cases the URL could theoretically be empty, but that should not be changed by bug 57115. Squib, any idea?
Component: Message Reader UI → MIME
Product: Thunderbird → MailNews Core
QA Contact: message-reader → mime
(In reply to :aceman from comment #5) > Can you confirm this same message when viewed in TB older than 13 does have > the link properly? That may not be easily possible, as the html code is written when the msg is first downloaded. An .eml could shed some light...
Are you sure about that? I have never seen the html be stored in the message. Also when I worked on the code, the changes were clearly reflected on already downloaded messages, I didn't need to download new ones. I could only see the HTML in the message preview pane via DOM Inspector.
Attached file EML as requested. (deleted) —
See attachment of EML file.
Attached file HTML from DOM (deleted) —
HTML from DOM, I hope it helps the process.
So in the HTML from DOM you do have the link there. How did you export it? Does it not work if you click "Download the rest of the message." inside the message preview?
Anyway, I can see the problem on the attached message. The "Download the rest of the message." is not underlined and an active link. However, using DOM Inspector, the A element containing it has href set, like this: mailbox://user@account/Inbox?number=82973&messageid=RNTT.Av8swwpVDv8S%7EQOjGqOrm8x9qYkqUy7%7EMuj%7E.1331427990.wA3YLyE%21%40webwc07.int.rightnowtech.com&uidl=AJ5YimIAAAFuT1v6lwEFLB18gUI
But clicking on the "Download the rest of the message." text appears to do something, I get an error in the Error console: Something may be wrong with the URL of the link. Is it valid? Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIIOService.newURI]" nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)" location: "JS frame :: chrome://global/content/contentAreaUtils.js :: makeURI :: line 766" data: no]
I exported it using Dom inspector. This does have me baffled if I open the HTML in Firefox there is a link bold as you like. It is a hyperlink as I would have expected Thunderbird to create. It obviously does not work, as the link is mailbox, but it is functional and it does appear to contain all the relevant information to download the mail body. I have tried Thunderbird with add-ons disabled [Help >restart with add-ons disabled]. Without success, but hey I thought it might work. Also tried -safe-mode just in case it was different in some fine detail, again without success. The text "Download the rest of the message." remains stubbornly black and having none of the characteristics of a hyperlink (it is black, the cursor does not change and clicking it does nothing at all. As a test of HTML links in Chrome, I find everything in the add-on manger to be functional (Browse all add-ons opens a new tab), and about:buildconfig in Troubleshooting also works as expected. General HTML mail hyperlinks are functional as well.
Your right, it is trying to do something. From the Error console. Timestamp: 19/03/2012 11:30:33 PM Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIIOService.newURI]" nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)" location: "JS frame :: chrome://global/content/contentAreaUtils.js :: makeURI :: line 766" data: no]
(In reply to :aceman from comment #7) > Are you sure about that? I was wrong, that doesn't happen when it's only the header. I think it happens when you limit download to certain max size though.
(In reply to :aceman from comment #12) > But clicking on the "Download the rest of the message." text appears to do > something, I get an error in the Error console: > Something may be wrong with the URL of the link. Is it valid? Just sent a new mail to that account, without the HTTP in the header as I though it might be what is causing the difficulty. Basic test mail. I receive the exact same error message in the error console as I get with the earlier mails.
(In reply to Magnus Melin from comment #15) > I was wrong, that doesn't happen when it's only the header. I think it > happens when you limit download to certain max size though. It was not the case even in this scenario. I was testing it using this option, not "only download headers".
So what's the issue here? The url in comment 11 uses the "mailbox" scheme. Firefox doesn't support this scheme, so it doesn't treat that <a> as a link (at the moment). Hence the browser behavior in comment 13. But the newURI exception in the console means that some chrome JS is calling newURI, right? And the IOservice version would either throw an exception if it can't find the protocol handler or call into http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMailboxService.cpp#576
The issue is this worked before (for years) and we try to find out what changed.
Worked for years in Thunderbird, right? Did you already find a regression range? I don't see one in this bug... That's usually a good start on answering the "what changed" question.
Yes, worked in Thunderbird of course. We do not expect it in Firefox, it was only an attachment in the bug. (Actually FF works better with it, at least shows the link is existing. TB hides is.) The potential regression was bug 57115 but just in these hours we analyzed it enough to see it could be somewhere else.
OK, maybe I should rephrase.... You cced me on the bug, but you didn't ask me anything obvious and the code in question is mailnews code that I'm not familiar with. So what were you looking for from me? Did comment 19 cover it?
Yes thanks, I think you helped us with pointing to the NewURI in nsMailBoxService.cpp. And I have made changes in that function so I will investigate. http://hg.mozilla.org/comm-central/rev/53fcee1f9ec3
Ok, good. ;)
Well, this is weird. I can't reproduce with the attached EML file with my Linux build. Can't reproduce on a real message received into my account. The link is there and is working fine. We could assume this could be fallout of bug 186724, but it is not sure as reverting it on Linux doesn't help. David Bienvenu, any idea why nsMailboxService::NewURI could fail with the link from comment 11 on Windows XP but not on Linux?
(In reply to :aceman from comment #26) > David Bienvenu, any idea why nsMailboxService::NewURI could fail with the > link from comment 11 on Windows XP but not on Linux? We might turn it into a file uri, which might have different impls on linux and windows.
And what can we do about it?
Severity: normal → major
Serge, can you help us here? I could prepare a patch that reverts the changes in nsMailboxService::NewURI but we need a try build for Windows so that somebody can test it (I could test but can't build it).
I mean a build of Thunderbird.
Attached patch patch, revert check in NewURI (obsolete) (deleted) — Splinter Review
Can anybody make us a Windows Thunderbird try build with this patch?
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #608996 - Flags: feedback?(sgautherie.bz)
(In reply to :aceman from comment #31) > Can anybody make us a Windows Thunderbird try build with this patch? See http://www.mozilla.org/hacking/committer/ for a level 1(+) access ;-)
Comment on attachment 608996 [details] [diff] [review] patch, revert check in NewURI This patch should certainly be enough to confirm whether that is the cause of this bug. Yet, I would suggest to add a NS_WARN_IF_FALSE(_expr,_msg) after each SetSpec() in order to debug which fails and why.
Attachment #608996 - Flags: feedback?(sgautherie.bz)
Attached patch patch, revert check in NewURI + debug warnings (obsolete) (deleted) — Splinter Review
Thanks, good idea.
Attachment #608996 - Attachment is obsolete: true
Attachment #609143 - Flags: review?(dbienvenu)
Comment on attachment 609143 [details] [diff] [review] patch, revert check in NewURI + debug warnings That's better to know which one is failing. But, for debug purpose, you probably need to output rv and aSpec/newSpec...
I haven't seen a usage of the macro where it accepted variables. Do I have to construct the output string beforehand, using something like sprintf?
Comment on attachment 609143 [details] [diff] [review] patch, revert check in NewURI + debug warnings we shouldn't comment out the NS_ENSURE_SUCCESS(rv, rv), just remove it. And I suspect those added lines should be wrapped. Other than that, this seems reasonable. If you could attach a new patch, we can land it.
Attachment #609143 - Flags: review?(dbienvenu) → review+
I mean this to be just a temporary patch to see if it quickly fixes the problem. Then we should find out what the problem is and put the ENSURE back where appropriate. That is why I left it in.
then I think you wanted feedback, not review?
Attached patch patch v3 (obsolete) (deleted) — Splinter Review
No. We must check-in something for 11.0+, so it needs review. But then we need to make a better patch to solve the real problem.
Attachment #609143 - Attachment is obsolete: true
Attachment #609798 - Flags: review+
(In reply to :aceman from comment #40) > Created attachment 609798 [details] [diff] [review] > patch v3 > > No. We must check-in something for 11.0+, so it needs review. > But then we need to make a better patch to solve the real problem. I'd rather have a real fix. I don't think we're doing this for 11.0x
So can you fix it for real? With also satisfying bug 186724, where you mark the cases where SetSpec should not be checked and it is OK for them to fail.
(In reply to :aceman from comment #36) > Do I have to construct the output string beforehand, using something like sprintf? Very likely. (In reply to :aceman from comment #38) > I mean this to be just a temporary patch to see if it quickly fixes the > problem. I thought you wanted a Try build (only) to at least confirm first... (In reply to :aceman from comment #40) > No. We must check-in something for 11.0+, so it needs review. If this bug is caused by bug 186724, then it's in TB13+: I'm not sure why you marked the bug as TB12 too, nor why you would push it to released TB11.
Sorry, I confused it with other bug, thanks for reminding me. Yes, this is only in TB13+. Yes, I wanted a try build but didn't get any yet. But maybe it would be better if somebody can try it on Windows directly.
This patch does fix the issue. The reason it fixes the issue is that the mailbox url for a partial download url is of the form mailbox://<account>/<mailbox name>?number==<msgKey>&messageid=<msgID>&uidl=<uidl> but nsMailboxUrl::ParseUrl wants a url of the form "mailbox://<path to folder>?number=301592075, etc. This is specifically what the aBaseURI->Resolve call does. I'll check if we need the setSpec call at all in this case. It's guaranteed to fail because ParseUrl will always complain that the file doesn't exist, because it doesn't have a full path to the file.
I don't think warning is useful here since it just pollutes the console.
Assignee: acelists → dbienvenu
Attachment #609798 - Attachment is obsolete: true
Attachment #609937 - Flags: review?(mbanner)
Comment on attachment 609937 [details] [diff] [review] fix with better comment and early return [Wrong patch] This looks like a patch for a different bug.
Attachment #609937 - Flags: review?(mbanner)
Summary: No download link for 'download message headers only' → No (inactive) download link for 'download message headers only' to download the rest of the message
Attached patch right patch this time (deleted) — Splinter Review
Attachment #609937 - Attachment is obsolete: true
Attachment #610523 - Flags: review?(mbanner)
Attachment #609937 - Attachment description: fix with better comment and early return. → fix with better comment and early return [Wrong patch]
can a test be devised to cover this?
Flags: in-testsuite?
Summary: No (inactive) download link for 'download message headers only' to download the rest of the message → No (inactive) download link for 'download message headers only' to download the rest of the pop message
Attachment #610523 - Flags: review?(mbanner) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Comment on attachment 610523 [details] [diff] [review] right patch this time [Triage Comment] From the tracking flags, and the regression comments it looks like we should take this on aurora, so a=me as this fixes a function for some people.
Attachment #610523 - Flags: approval-comm-aurora+
I can see the link in blue and underlined again in today's TB14+ (Windows). Matt, can you test it verify the fix?
what bug caused the regression? flagging v12 in case there is a respin of v12
(In reply to Wayne Mery (:wsmwk) from comment #55) > what bug caused the regression? See comment 43. > flagging v12 in case there is a respin of v12 Your url doesn't report version :-(
can confirm working for trunk 14.0a1 A nit that may be relevant of fodder for another bug ( please tell me if I need to file another bug). After the link is clicked and the download completes the message list resets to the top and displays the first message in the list not the one selected to dowload.
I have noticed that problem too. It is already filed in bug 748097 and bug 749147. Thanks for confirmation, please change the status of the bug to VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: