Closed Bug 1361020 Opened 2 years ago Closed 2 years ago

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

Categories

(MailNews Core :: Backend, defect, major)

defect
Not set
major

Tracking

(thunderbird_esr5253+ fixed, thunderbird53 wontfix, thunderbird54 fixed, thunderbird55 fixed)

VERIFIED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 53+ fixed
thunderbird53 --- wontfix
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: henrifos, Assigned: jorgk)

References

(Depends on 2 open bugs, )

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

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.
Windows 7 Pro 32bit used.
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?
Flags: needinfo?(gds)
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 .
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.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(gds) → needinfo?(alice0775)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Download rest of message not working after fetch headers only → Download rest of message not working after fetch headers only - POP account
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://user%40jorgk%2Ecom@mail.your-server.de/Inbox?number=70046&messageid=09ce35a7-2ae1-573d-f7c3-73473ebaf8d6%40jorgk.com&uidl=UID4029-1354708431
Blocks: 968342
Link in the message shows:
mailbox://jorgk%40jorgk%2Ecom@mail.jorgk.com/Trash?number=21&messageid=1493653621007.535478.c7c04ef1a15e54f9a86b98f7a4a84b62cd37e391%40user.telekom.de&uidl=UID4030-1354708431
Jörg's famous content policy debug patch ;-) This shows:

=== nsMsgContentPolicy::ShouldLoad - nsIMsgMailNewsUrl check
=== content mailbox://user%40jorgk%2Ecom@mail.your-server.de
=== 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.
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.
Flags: needinfo?(alice0775)
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 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.
Attachment #8863509 - Attachment description: 1361020-download-headers.patch → 1361020-download-headers.patch - ESR52 fix
Attachment #8863509 - Flags: review?(rkent)
Severity: normal → major
Component: Untriaged → Backend
Product: Thunderbird → MailNews Core
Version: 52 Branch → 52
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 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.
Attachment #8863509 - Flags: review?(rkent) → review-
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://tbtest%40jorgk%2Ecom@mail.your-server.de/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?
Flags: needinfo?(rkent)
Sorry, edited the server name in one case but not the other. Comparing:
mailbox://tbtest%40jorgk%2Ecom@server.de/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
Attachment #8863509 - Attachment description: 1361020-download-headers.patch - ESR52 fix → 1361020-download-headers.patch (v1)
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 ;-)
Attachment #8865155 - Flags: review?(rkent)
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.
Attachment #8863509 - Attachment is obsolete: true
Attachment #8865152 - Attachment is obsolete: true
Attachment #8865155 - Attachment is obsolete: true
Attachment #8865155 - Flags: review?(rkent)
Flags: needinfo?(rkent)
Attachment #8865235 - Flags: review?(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.
Attachment #8865235 - Flags: review?(rkent) → review+
Nits fixed.
Assignee: nobody → jorgk
Attachment #8865235 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8865244 - Flags: review+
https://hg.mozilla.org/comm-central/rev/92e89bd8daf85910e04d9c44098ba9279735a8a7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Comment on attachment 8865244 [details] [diff] [review]
1361020-download-headers.patch (v4b).

[Approval Request Comment]
Regression caused by (bug #): Bug 968342.
Attachment #8865244 - Flags: approval-comm-esr52?
Attachment #8865244 - Flags: approval-comm-beta+
Duplicate of this bug: 1362963
Duplicate of this bug: 1363274
Summary: Download rest of message not working after fetch headers only - POP account → Download rest of message not working after fetch headers only or partial download (do not download messages larger than ...) - POP account
Duplicate of this bug: 1363296
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.
Attachment #8863509 - Attachment is obsolete: false
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 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.
Attachment #8863509 - Flags: approval-comm-esr52+
Duplicate of this bug: 1364670
Tb 52.1.1 confirmed working.
Status: RESOLVED → VERIFIED
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.
Wrong query. What's the purpose of your comment? To prove that we didn't fix it?
Magnificent effort guys (M&F).
Well worth a donation.
Gave a few $'s about 2 weeks ago.
(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?
(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.
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
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.
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.
Blocks: 1367707
Depends on: 1364723
Depends on: 1368738
Attachment #8865244 - Flags: approval-comm-esr52? → approval-comm-esr52+
Depends on: 1372411
Depends on: 1372414
Depends on: 1378290
This bug is back in Thunderbird 60 beta 1 build 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.
You need to log in before you can comment on or make changes to this bug.