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

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect)

Description

[Matt]

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

[Magnus Melin [:mkmelin]]

Which message is that? I think bug 57115 touched this.
If possible can you attach a sample message?

Comment 2

[:aceman]

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?

Comment 3

[Matt]

Attachment: 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

[Magnus Melin [:mkmelin]]

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

Comment 5

[:aceman]

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?

Comment 6

[Magnus Melin [:mkmelin]]

(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

[:aceman]

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 8

[Matt]

Attachment: EML as requested.

See attachment of EML file.

Comment 9

[Matt]

Attachment: HTML from DOM

HTML from DOM, I hope it helps the process.

Comment 10

[:aceman]

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

[:aceman]

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

[:aceman]

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]

Comment 13

[Matt]

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.

Comment 14

[Matt]

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

[Magnus Melin [:mkmelin]]

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

Comment 16

[Matt]

(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

[:aceman]

(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

[:aceman]

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

[Boris Zbarsky [:bzbarsky]]

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

[:aceman]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

[:aceman]

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

[Boris Zbarsky [:bzbarsky]]

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

[:aceman]

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

[Boris Zbarsky [:bzbarsky]]

Ok, good.  ;)

Comment 26

[:aceman]

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?

Comment 27

[David :Bienvenu]

(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

[:aceman]

And what can we do about it?

Comment 29

[:aceman]

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

[:aceman]

I mean a build of Thunderbird.

Comment 31

[:aceman]

Attachment: patch, revert check in NewURI

Can anybody make us a Windows Thunderbird try build with this patch?

Comment 32

[Serge Gautherie (:sgautherie)]

(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

[Serge Gautherie (:sgautherie)]

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.

Comment 34

[:aceman]

Attachment: patch, revert check in NewURI + debug warnings

Thanks, good idea.

Comment 35

[Serge Gautherie (:sgautherie)]

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

[:aceman]

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 37

[David :Bienvenu]

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.

Comment 38

[:aceman]

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.

Comment 39

[David :Bienvenu]

then I think you wanted feedback, not review?

Comment 40

[:aceman]

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

Comment 41

[David :Bienvenu]

(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

[:aceman]

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

[Serge Gautherie (:sgautherie)]

(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

[:aceman]

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.

Comment 45

[David :Bienvenu]

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.

Comment 46

[David :Bienvenu]

Attachment: fix with better comment and early return [Wrong patch]

I don't think warning is useful here since it just pollutes the console.

Comment 47

[Mark Banner (:standard8)]

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.

Comment 49

[David :Bienvenu]

Attachment: right patch this time

Comment 50

[Wayne Mery (:wsmwk)]

can a test be devised to cover this?

Comment 51

[David :Bienvenu]

fixed on trunk - http://hg.mozilla.org/comm-central/rev/2fe9dc371bfd

Comment 52

[Mark Banner (:standard8)]

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.

Comment 53

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/releases/comm-aurora/rev/bd2c19647e4d

Comment 54

[:aceman]

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

[Wayne Mery (:wsmwk)]

what bug caused the regression?

flagging v12 in case there is a respin of v12

Comment 56

[Serge Gautherie (:sgautherie)]

(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 :-(

Comment 57

[Matt]

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

[:aceman]

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.