Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in Thunderbird 14.0

Status

MailNews Core
MIME
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Matt, Assigned: Bienvenu)

Tracking

({regression})

Trunk
Thunderbird 14.0
x86
Windows XP
regression
Bug Flags:
in-testsuite ?

Thunderbird Tracking Flags

(thunderbird13+ fixed)

Details

(Whiteboard: [gs], URL)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

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

5 years ago
Which message is that? I think bug 57115 touched this.
If possible can you attach a sample message?
Keywords: regression

Comment 2

5 years ago
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?
(Reporter)

Comment 3

5 years ago
Created attachment 607112 [details]
Screen shot as requested

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

5 years ago
Can you also do Save as File (.eml) and attach the .eml here?

Comment 5

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

5 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...

Comment 7

5 years ago
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.
(Reporter)

Comment 8

5 years ago
Created attachment 607122 [details]
EML as requested.

See attachment of EML file.
(Reporter)

Comment 9

5 years ago
Created attachment 607129 [details]
HTML from DOM

HTML from DOM, I hope it helps the process.

Comment 10

5 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

5 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

5 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

5 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

5 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

5 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

5 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

5 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

5 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

5 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

5 years ago
The issue is this worked before (for years) and we try to find out what changed.

Comment 21

5 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

5 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

5 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

5 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

5 years ago
Ok, good.  ;)

Comment 26

5 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

5 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

5 years ago
And what can we do about it?
Blocks: 186724
tracking-thunderbird13: --- → ?
Severity: normal → major

Comment 29

5 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

5 years ago
I mean a build of Thunderbird.

Comment 31

5 years ago
Created attachment 608996 [details] [diff] [review]
patch, revert check in NewURI

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)

Comment 34

5 years ago
Created attachment 609143 [details] [diff] [review]
patch, revert check in NewURI + debug warnings

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...

Comment 36

5 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?

Updated

5 years ago
tracking-thunderbird12: --- → ?
(Assignee)

Comment 37

5 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

5 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

5 years ago
then I think you wanted feedback, not review?

Comment 40

5 years ago
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.
Attachment #609143 - Attachment is obsolete: true
Attachment #609798 - Flags: review+
(Assignee)

Comment 41

5 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

5 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.
(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

5 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

5 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

5 years ago
Created attachment 609937 [details] [diff] [review]
fix with better comment and early return
[Wrong patch]

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)

Updated

5 years ago
Duplicate of this bug: 740152

Updated

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

5 years ago
Created attachment 610523 [details] [diff] [review]
right patch this time
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
tracking-thunderbird13: ? → +
Attachment #610523 - Flags: review?(mbanner) → review+
(Assignee)

Comment 51

5 years ago
fixed on trunk - http://hg.mozilla.org/comm-central/rev/2fe9dc371bfd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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+
Checked in: http://hg.mozilla.org/releases/comm-aurora/rev/bd2c19647e4d
status-thunderbird13: --- → fixed

Comment 54

5 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?
what bug caused the regression?

flagging v12 in case there is a respin of v12
tracking-thunderbird12: --- → ?
Whiteboard: [gs]
(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

5 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

5 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.
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.