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)
Tracking
(thunderbird13+ fixed)
VERIFIED
FIXED
Thunderbird 14.0
People
(Reporter: unicorn.consulting, Assigned: Bienvenu)
References
()
Details
(Keywords: regression, Whiteboard: [gs])
Attachments
(4 files, 4 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
message/rfc822
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
standard8
:
review+
standard8
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
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.
Comment 1•13 years ago
|
||
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 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.
Comment 4•13 years ago
|
||
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
Comment 6•13 years ago
|
||
(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.
Comment 10•13 years ago
|
||
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?
Comment 11•13 years ago
|
||
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
Comment 12•13 years ago
|
||
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]
Reporter | ||
Comment 13•13 years ago
|
||
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.
Reporter | ||
Comment 14•13 years ago
|
||
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]
Comment 15•13 years ago
|
||
(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.
Reporter | ||
Comment 16•13 years ago
|
||
(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.
Comment 17•13 years ago
|
||
(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".
Comment 18•13 years ago
|
||
I think this may be the code for NewURI:
http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsIOService.cpp#559
Comment 19•13 years ago
|
||
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
Comment 20•13 years ago
|
||
The issue is this worked before (for years) and we try to find out what changed.
Comment 21•13 years ago
|
||
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.
Comment 22•13 years ago
|
||
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.
Comment 23•13 years ago
|
||
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?
Comment 24•13 years ago
|
||
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
Comment 25•13 years ago
|
||
Ok, good. ;)
Comment 26•13 years ago
|
||
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?
Assignee | ||
Comment 27•13 years ago
|
||
(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.
Comment 28•13 years ago
|
||
And what can we do about it?
Updated•13 years ago
|
Blocks: 186724
tracking-thunderbird13:
--- → ?
Updated•13 years ago
|
Severity: normal → major
Comment 29•13 years ago
|
||
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).
Comment 30•13 years ago
|
||
I mean a build of Thunderbird.
Comment 31•13 years ago
|
||
Can anybody make us a Windows Thunderbird try build with this patch?
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #608996 -
Flags: feedback?(sgautherie.bz)
Comment 32•13 years ago
|
||
(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 33•13 years ago
|
||
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)
Comment 34•13 years ago
|
||
Thanks, good idea.
Attachment #608996 -
Attachment is obsolete: true
Attachment #609143 -
Flags: review?(dbienvenu)
Comment 35•13 years ago
|
||
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...
Comment 36•13 years ago
|
||
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?
tracking-thunderbird12:
--- → ?
Assignee | ||
Comment 37•13 years ago
|
||
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+
Comment 38•13 years ago
|
||
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.
Assignee | ||
Comment 39•13 years ago
|
||
then I think you wanted feedback, not review?
Comment 40•13 years ago
|
||
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+
Assignee | ||
Comment 41•13 years ago
|
||
(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
Comment 42•13 years ago
|
||
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.
Comment 43•13 years ago
|
||
(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.
Comment 44•13 years ago
|
||
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.
tracking-thunderbird12:
? → ---
Assignee | ||
Comment 45•13 years ago
|
||
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.
Assignee | ||
Comment 46•13 years ago
|
||
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 47•13 years ago
|
||
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
Assignee | ||
Comment 49•13 years ago
|
||
Attachment #609937 -
Attachment is obsolete: true
Attachment #610523 -
Flags: review?(mbanner)
Updated•13 years ago
|
Attachment #609937 -
Attachment description: fix with better comment and early return. → fix with better comment and early return
[Wrong patch]
Comment 50•13 years ago
|
||
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
Updated•13 years ago
|
Updated•13 years ago
|
Attachment #610523 -
Flags: review?(mbanner) → review+
Assignee | ||
Comment 51•13 years ago
|
||
fixed on trunk - http://hg.mozilla.org/comm-central/rev/2fe9dc371bfd
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Comment 52•13 years ago
|
||
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+
Comment 53•13 years ago
|
||
status-thunderbird13:
--- → fixed
Comment 54•13 years ago
|
||
I can see the link in blue and underlined again in today's TB14+ (Windows).
Matt, can you test it verify the fix?
Comment 55•13 years ago
|
||
what bug caused the regression?
flagging v12 in case there is a respin of v12
Comment 56•13 years ago
|
||
(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 :-(
tracking-thunderbird12:
? → ---
Reporter | ||
Comment 57•13 years ago
|
||
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.
Comment 58•13 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•