1361020 - Download rest of message not working after fetch headers only or partial download (do not download messages larger than ...) - POP account

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[henrifos]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

1. Updated to v52.1.0 (32 bit),
Normal procedure to check for emails.
2. Subsequently. Changed another account to Fetch headers only.



Actual results:

1. Header downloaded,
When I click on "Download the rest of the message" nothing happens.
2. Subsequently. Same as 1.


Expected results:

Rest of message should have downloaded in both cases.

Comment 1

[henrifos]

Windows 7 Pro 32bit used.

Comment 2

[Jorg K (CEST = GMT+2)]

Gene, you've looked at a lot of IMAP issues lately, could you please take a look at this one here.

Reporter: You mention an update to TB 52.1. Did this work in TB 45?

Comment 3

[henrifos]

FYI - this is via POP3 accounts.

I believe that it did work in TB 45 though that version sounds like it's a fair way in the past.
It also worked in the previous version which I think was 52.0.1 .

Comment 4

[Jorg K (CEST = GMT+2)]

Yes, that works in TB 45.8 and not Daily (TB 55), I haven't tried TB 52.

Alice, can you please find the regression for me.

STR:
In the account setup under "Server settings", select "Fetch headers only". Send a message to that account and download via POP. In the body it will say "Not Downloaded. Only the headers for this message were downloaded from the mail server. Download the rest of the message."

When you click it should fetch the entire message and display it.

I'm not aware of having broken this, but most likely I have :-(

(Sorry, Gene, false alarm, no IMAP here.)

Comment 5

[Jorg K (CEST = GMT+2)]

I have the feeling that bug 968342 broke this:
https://hg.mozilla.org/comm-central/rev/c322afd9bf99c3b9c168e1319ccd4b419c209564
Landed 2017-03-31 12:58 and uplifted to all branches.

Some security thing that suddenly doesn't let you click on to the link in the message any more, for example:
mailbox://[e-mail]/Inbox?number=70046&messageid=09ce35a7-2ae1-573d-f7c3-73473ebaf8d6%40jorgk.com&uidl=UID4029-1354708431

Comment 6

[Alice0775 White]

Regression window on ESR52 branch :(
https://hg.mozilla.org/releases/mozilla-esr52/pushloghtml?fromchange=1970761a6677e76a55dfed1a45bc0f1d77dc4851&tochange=1970761a6677e76a55dfed1a45bc0f1d77dc4851
https://hg.mozilla.org/releases/comm-esr52/pushloghtml?fromchange=ce2a38b113a1b08feede1829a39fc591c3bd2eb1&tochange=183d25ea0e84b53f707458c1091f8057951e6bdc

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: test-not-downloaded.eml - Test message with only headers downloaded

Link in the message shows:
mailbox://[e-mail]/Trash?number=21&messageid=1493653621007.535478.c7c04ef1a15e54f9a86b98f7a4a84b62cd37e391%40user.telekom.de&uidl=UID4030-1354708431

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: content-policy-debug.patch

Jörg's famous content policy debug patch ;-) This shows:

=== nsMsgContentPolicy::ShouldLoad - nsIMsgMailNewsUrl check
=== content mailbox://[e-mail]
=== request mailbox://
=== returning (5) -1

So the special nsIMsgMailNewsUrl test fails since one mailbox URL: has a user@server, the other one hasn't.

That's easily fixed, patch coming.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1361020-download-headers.patch (v1)

This fixes the damage done by bug 968342. However, it still doesn't work. Now you can click on the download link in the message and something happens, but the message is still not downloaded and displayed.

Alice, thanks for confirming the regression on ESR52, but can you bisect this a little more on C-C, please.

I know that bug 968342 damaged clicking the link in the message, but even if I repair that, it doesn't work.

So while it's clearly broken in TB 52 by uplifting bug 968342, I believe that there's something else is wrong on trunk. And that I need to find.

Did it still work completely before 2017-03-31 when bug 968342 landed? Or was it already broken then? I also backed out bug 968342 and it still doesn't work.

Comment 10

[Alice0775 White]

Regression window(c-c):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a288fe35e494cec3620717eab66eaff6f68a6369&tochange=a288fe35e494cec3620717eab66eaff6f68a6369
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=3a925dc53d92ab7c4fbf04c19cc9c7d918adaa72&tochange=a81dbcf91a42c909817cf47aa9693c20fcfe02a6

Comment 11

[Jorg K (CEST = GMT+2)]

Alice, thank you so much, as always, I was afraid that the "proxy bug" (bug 791645) is behind this. That makes it hard to fix.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8863509 [details] [diff] [review]
1361020-download-headers.patch (v1)

We both reviewed the new code in bug 968342 and missed the issue. Here's a simple fix for the issue on ESR52. Or do you have some grander solution in mind?
QI to nsIMailboxUrl instead of looking at the scheme? Or use the new GetPrincipalSpec() which in it's current form doesn't strip the user@domain@server part from mailbox: URLs.

Comment 13

[Wayne Mery (:wsmwk)]

https://support.mozilla.org/en-US/questions/1158683
https://support.mozilla.org/en-US/questions/1158546

Comment 14

[henrifos]

Thank you Wayne for the link to https://support.mozilla.org/en-US/questions/1158683
Also thanks to eve2 for her suggestion: "File -> Offline -> Get Selected Messages".
It works for me too so I'm presently using that as a work around.

Comment 15

[Kent James (:rkent)]

Comment on attachment 8863509 [details] [diff] [review]
1361020-download-headers.patch (v1)

Review of attachment 8863509 [details] [diff] [review]:
-----------------------------------------------------------------

After discussions on IRC, Jörg and I agreed that the right approach will be to apply the same GetPrincipalSpec to mailbox: as we do to other protocols, but eliminate any username or password from the spec.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1361020-download-headers.patch (v2) - not working.

This is the alternate solution which doesn't work as the debug shows.

GetPrincipalSpec() doesn't strip the user@domain@server so far, as noted in comment #12, but even if it did, it wouldn't match.

Here's the debug, we're comparing:
mailbox://[e-mail]/Inbox?number=1 (shown for the message where only headers where downloaded)
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/5gfhfw7f.POP%20tbtest/Mail/server.de/Inbox?number=1

Sadly, the mailbox URLs seem to have two different types or spec, a "funny" one with the folder name only and one with the full folder path.

And no, we can't make the principal spec use the folder only, since
a) is doesn't work, I've tried using nsParseLocalMessageURI() in
   bug 1347598 comment #56. I've also tried to use nsMailboxUrl::GetUri(),
   but that also doesn't work on the "funny" URL.
and
b) There could be two folders with the same name.

So, Kent, what is plan B?

Comment 17

[Jorg K (CEST = GMT+2)]

Sorry, edited the server name in one case but not the other. Comparing:
mailbox://[e-mail]/Inbox?number=1 (shown for the message where only headers where downloaded)
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/5gfhfw7f.POP%20tbtest/Mail/server.de/Inbox?number=1

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1361020-download-headers.patch (v3).

OK, this works completely although I thought that there was also a proxy problem to fix here. I also tried (v1) again and it also works now. Strange.

Anyway, (v3) is a variation of (v3), just using QI instead of retrieving the scheme.

If you have a good idea of saving (v2) by improving GetPrincipalSpec() for mailbox: URLs, I'm happy to see your idea or even patch ;-)

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1361020-download-headers.patch (v4).

This is (v2) that uses GetPrincipalSpec() made to work. mailbox: URLs needed a little push to get them going ;-)

This is Kent's favourite solution, so I hope he likes it.

Comment 20

[Kent James (:rkent)]

Comment on attachment 8865235 [details] [diff] [review]
1361020-download-headers.patch (v4).

Review of attachment 8865235 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM (with of course the printf and nits fixed)

::: mailnews/base/src/nsMsgContentPolicy.cpp
@@ +247,5 @@
> +  nsCOMPtr<nsIMsgMessageUrl> contentURL(do_QueryInterface(aContentLocation));
> +  if (contentURL) {
> +    nsCOMPtr<nsIMsgMessageUrl> requestURL(do_QueryInterface(aRequestingLocation));
> +    // If the request URL is not also a message URL, then we don't accept.
> +    *aDecision = nsIContentPolicy::REJECT_REQUEST;

Nit: this is a duplicate of an earlier line.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1361020-download-headers.patch (v4b).

Nits fixed.

Comment 22

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/92e89bd8daf85910e04d9c44098ba9279735a8a7

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8865244 [details] [diff] [review]
1361020-download-headers.patch (v4b).

[Approval Request Comment]
Regression caused by (bug #): Bug 968342.

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8863509 [details] [diff] [review]
1361020-download-headers.patch (v1)

I'll use this simple tweak as a temporary band-aid fix for ESR52 since we start getting complaints. This will be backed out when the more sophisticated solutions was undergone the beta cycle.

Comment 28

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/4c7df36ffbd6a5b4cf25bfeacbbba11eee9b9942
Temporary band-aid fix, will be backed out when full solution lands.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8863509 [details] [diff] [review]
1361020-download-headers.patch (v1)

We need this temporary band-aid regression fix.

We simple accept mailbox: loads from mailbox: requesters like we've done since the beginning of time until bug 968342 introduced some incorrect restrictions.

Zero risk.

Comment 30

[Jorg K (CEST = GMT+2)]

Beta (TB 54):
https://hg.mozilla.org/releases/comm-beta/rev/3f12c85ba9e79fb5c8e1df69e69a8a0037e7072e

Comment 32

[Wayne Mery (:wsmwk)]

52.1.1 *candidate for release* is available for testing at https://archive.mozilla.org/pub/thunderbird/candidates/52.1.1-candidates/build1/win32/en-US/Thunderbird%20Setup%2052.1.1.exe

Comment 33

[Aris]

Tb 52.1.1 confirmed working.

Comment 34

[David E. Ross]

Comment #32 and comment #33 both indicate this bug is resolved in Thunderbird 52.1.1.  However, the list of resolved bugs for 52.1.1 at <https://bugzilla.mozilla.org/buglist.cgi?list_id=13560552&o1=equals&v1=53%2B&f1=cf_tracking_thunderbird_esr52&query_format=advanced> does not list this bug.

Comment 35

[Jorg K (CEST = GMT+2)]

Wrong query. What's the purpose of your comment? To prove that we didn't fix it?

Comment 36

[henrifos]

Magnificent effort guys (M&F).
Well worth a donation.
Gave a few $'s about 2 weeks ago.

Comment 37

[David E. Ross]

(In reply to Jorg K (GMT+2) from comment #35)
> Wrong query. What's the purpose of your comment? To prove that we didn't fix
> it?

I made NO assertion whether this bug is or is not fixed.  

At <https://www.mozilla.org/en-US/thunderbird/52.1.1/releasenotes/>, I see:
> Thunderbird Notes
> Version 52.1.1, first offered to Release channel users on May 15, 2017

There, "the complete list of changes in this release" has the URI <https://bugzilla.mozilla.org/buglist.cgi?o1=equals&v1=53%2B&f1=cf_tracking_thunderbird_esr52&query_format=advanced&list_id=13591599>.  That query lists 19 bug reports closed (Reso/Fixed).  This bug #1361020 is NOT in that list.  

I previously reverted back from Thunderbird 52.1.0 to 52.0.1 because of this problem.  Before I now update to Thunderbird 52.1.1, I want to know if this is indeed fixed in 52.1.1.  If it is fixed, why does it not appear in my cited bugzilla.mozilla.org query?  Is the wrong query linked from the release notes?  Has someone failed to update this bug report properly to have it appear in the query's list?  Why is this bug report not included in the list?

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to David E. Ross from comment #37)
> I want to know if this is indeed fixed in 52.1.1.
It is fixed in TB 52.1.1.

Wayne, can you please take care of the other questions. tracking-thunderbird_esr52: 53+ is not set here since the fix is not in TB 52.1.0 which has the fixes which went through the beta 53 cycle.

Comment 39

[alexrayne]

It is not fixed in TB52.1.1 32bit on WinXP sp3. work in russian locale cp1251.
maybe it helps - after try to download rest of a message, error console reports this:

Применение Mutation Events является устаревшим. Вместо них используйте MutationObserver.  calendar-widgets.xml:506:18

uncaught exception: 2147746065  autosync.js:210:13

DEPRECATION WARNING: Encoding to non-UTF-8 values is obsolete
You may find more details about this deprecation at: http://bugzilla.mozilla.org/show_bug.cgi?id=790855
resource://gre/components/mimeJSComponents.js 443 MimeConverter.prototype.encodeMimePartIIStr_UTF8
  Deprecated.jsm:79

uncaught exception: 2147746065  autosync.js:210:13

Comment 40

[Jorg K (CEST = GMT+2)]

I can't try the Russian version, but en-US 52.1.1 has this issue fixed.
The errors from the console log are harmless and unrelated.

Comment 41

[alexrayne]

what can i do more? i can resent my mails to you, and you try to open ones.
as for locale - cp1251 used as resevre text encoding.

Comment 42

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/11d062cea2ca537584f8eea4001e6e810de14bd8 (Backout of temporary fix)
https://hg.mozilla.org/releases/comm-esr52/rev/3aa1ccacadbe1fad58b56545b00c6059a9f81634 (correct solution)

Comment 43

[Aris]

This bug is back in Thunderbird 60 beta 1 build 2!

Comment 44

[Jorg K (CEST = GMT+2)]

Yes, the bug is back, thanks for letting us know. The link in the message "Download the rest of the message" is subject to access control and it looks like something has changed so it can't be clicked any more. I've filed bug 1446679 for it.