Accessing IMAP URI pointing to a non-existent folder (due to folder rename, move, etc.) fails leaving message pane broken

RESOLVED FIXED in Thunderbird 53.0

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
8 months ago
5 months ago

People

(Reporter: wsmwk, Assigned: Jorg K (GMT+2))

Tracking

(Blocks: 2 bugs)

unspecified
Thunderbird 53.0
Unspecified
All
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr45+ affected, thunderbird51 fixed, thunderbird52+ fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

followup to bug 1083994 ... Comment below from bug 1083994 comment 147. Relevent patch is Attachment 8822875 [details] [diff] from bug 1083994 comment 149.

I think the patch is self-explanatory.
...
One word about the regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2014-01-13+03%3A00&enddate=2014-01-14+05%3A00

I don't have time to analyse this. Most likely there is a subtle change somewhere in the code that clones URIs. Most likely it was changed to pay more attention to errors returned from underlying functions. The FindOnlineSubFolder() has been in nsImapService::NewURI() for a long time.

Kent, all you need to know are the STR:
View a message in an IMAP folder in the message/preview pane.
At the same time, rename the folder.
Try the same with a local folder (which always worked).
(Reporter)

Comment 1

8 months ago
Before I forget, I did some crude bug queries (which I did not attempt to refine) to see what might be relevant to this imap bug https://mzl.la/2iKOWZ1 https://mzl.la/2iKNkOV
See Also: → bug 1083994
(Assignee)

Comment 2

8 months ago
Created attachment 8823972 [details] [diff] [review]
1328814-imap-move.patch (v1)

Analysis in bug 1083994 comment #145 and bug 1083994 comment #146.

Quoting bug 1083994 comment #147:

It is just a really bad idea to overload a low-level service with intelligent processing. If you ask for "helllow Doly" to be copied, you want a copy and not the advice that is has two spelling mistakes and can't be copied. This is what happened here.

Quoting bug 1083994 comment #151:

The low-level function that creates an nsIURI object checks whether the IMAP folder addressed by this URI exists. That's simply wrong, you should be able to create/copy/clone a reference, even if the referenced object has gone away. The equivalent mailbox function doesn't check folder existence and it all works.

Quoting bug 1083994 comment #155:

The sad fact is that with the message pane having a location that is an IMAP URI pointing to a non-existent folder, absolutely NOTHING will work, since the location cannot be read (messagePane.location.href != "about:blank" just fails, in fact, accessing messagePane.location already fails) and it cannot be set either. I also do not care how this regressed, since the code in nsImapService::NewURI() is wrong and was always wrong (only that apparently no one noticed).
Attachment #8823972 - Flags: review?(rkent)
(Assignee)

Updated

8 months ago
Status: NEW → ASSIGNED
Summary: create/return URI if imap folder not found (due to folder rename, move, etc) → Accessing IMAP URI pointing to a non-existent folder (due to folder rename, move, etc.) fails leaving message pane broken
(Assignee)

Comment 3

8 months ago
Kent, please note that the "folder finding" business depends on two conditions:
https://dxr.mozilla.org/comm-central/rev/6f666ad0456ce4e56c4ab24de882320fa28059ba/mailnews/imap/src/nsImapService.cpp#2579

  if (rootFolder && !folderName.IsEmpty())
  {
    nsCOMPtr<nsIMsgFolder> folder;
    nsCOMPtr<nsIMsgImapMailFolder> imapRoot = do_QueryInterface(rootFolder, &rv);
    nsCOMPtr<nsIMsgImapMailFolder> subFolder;
    if (imapRoot)
    {
      imapRoot->FindOnlineSubFolder(folderName, getter_AddRefs(subFolder));
      folder = do_QueryInterface(subFolder, &rv);

so I don't see while "folder not found" should be returned here since the "folder found" isn't enforced anyway when these two conditions aren't met.
Duplicate of this bug: 1120361
Duplicate of this bug: 1292170
Duplicate of this bug: 1248660
Duplicate of this bug: 1062408
Duplicate of this bug: 1193885
Blocks: 1083994

Comment 9

8 months ago
Comment on attachment 8823972 [details] [diff] [review]
1328814-imap-move.patch (v1)

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

OK I made sure that I understood all of the setup in NewURI, and how the program would respond to incomplete information. The short version is that the setup in SetImapUrlSink gets missed in NewURI if the folder is missing, but that method is really called prior to use in most if not all places that the uri would be used anyway. So I see little risk in avoiding that in NewURI, so that someone could fix that folder and still use the URI later. I also believe that this call:

rv = aImapUrl->SetImapMessageSink(msgSink);

is not needed, as SetImapUrlSink does that anyway. But I don't intend to change that, only the structure of the error calls to eliminate unused rv returns.

Also the patch seems to fix the symptoms. So I also don't see the value in trying to analyze it any deeper.

But I would suggest some cosmetic changes to the patch.  The code should not set rv if it does not intend to fail on that. We could either use a secondary rv, or use alternate ways to check for failure that don't involve rv. I'll r+ this patch, but make some suggested reordering of checks in an alternate patch. If you are happy with the second patch, still the status is r=me with Assigned=jorgk as it is just reordering jorg's patch.
Attachment #8823972 - Flags: review?(rkent) → review+

Comment 10

8 months ago
Created attachment 8825214 [details] [diff] [review]
Only set rv if we intend to fail from it.
Attachment #8825214 - Flags: feedback?(jorgk)
(Assignee)

Comment 11

8 months ago
Comment on attachment 8825214 [details] [diff] [review]
Only set rv if we intend to fail from it.

Thanks, fine by me. I'll get this landed.
Attachment #8825214 - Flags: feedback?(jorgk) → feedback+
(Assignee)

Comment 12

8 months ago
https://hg.mozilla.org/comm-central/rev/f3c3430dddfc3e15ede7d7d98abc1943a0ed1aef

Landed with double-authorship: "Jorg K and R Kent James", "JJ" ;-)
I took the liberty to move the comment up and add |(void)| in front of the calls whose statuses we ignore.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-thunderbird51: --- → affected
status-thunderbird52: --- → affected
status-thunderbird53: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 13

8 months ago
Created attachment 8825317 [details] [diff] [review]
landed patch

[Approval Request Comment]
Regression caused by (bug #): This seem so be a regression by an unrelated change but we never worked out which.
User impact if declined: Very annoying, damages preview pane when IMAP folder is renamed/moved.
Testing completed (on c-c, etc.): Manual.

I'll take it to Aurora and Beta.
Attachment #8825317 - Flags: review+
Attachment #8825317 - Flags: approval-comm-esr45?
Attachment #8825317 - Flags: approval-comm-beta+
Attachment #8825317 - Flags: approval-comm-aurora+
(Assignee)

Updated

8 months ago
Attachment #8823972 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8825214 - Attachment is obsolete: true
(Assignee)

Comment 14

8 months ago
C-A (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/3a87bcb0b61cfdddf34a95ec9e421085ab2b4f24

C-B (TB 51):
https://hg.mozilla.org/releases/comm-beta/rev/a2376bf830a39f7e07e418512e752594e72726ca
status-thunderbird51: affected → fixed
status-thunderbird52: affected → fixed
See Also: bug 1083994

Comment 15

7 months ago
Comment on attachment 8825317 [details] [diff] [review]
landed patch

https://hg.mozilla.org/releases/comm-esr45/rev/ccb801cd360e
Attachment #8825317 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

7 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 51+
(Reporter)

Updated

7 months ago
Blocks: 1313774
(Reporter)

Updated

7 months ago
Depends on: 1332606
(Reporter)

Comment 16

7 months ago
ref bug 1332606 comment 6.

If it is concluded that this caused bug 1332606 and friends, we should back out the esr patch and issue a point release for 45.7

Comment 17

7 months ago
Note this was backed out of comm-esr45 as a test (https://hg.mozilla.org/releases/comm-esr45/rev/6811cb04c86f) Not clear yet if we will leave it backed out.
(Assignee)

Comment 18

7 months ago
I think we should leave it backed out until we have a fix with the information gained in bug 1335638 comment #10.

Comment 19

7 months ago
OK, let's leave it backed out.
status-thunderbird_esr45: fixed → affected
tracking-thunderbird_esr45: 51+ → +
(Reporter)

Updated

7 months ago
Depends on: 1335345
(Reporter)

Updated

7 months ago
Depends on: 1335638
(Assignee)

Comment 20

7 months ago
Backed out from TB 52, currently at beta:
https://hg.mozilla.org/releases/comm-beta/rev/e3e9f49f289f4a97db9860bc52dda812a3afe870
Setting the status-thunderbird52 field back to "affected" to reflect comment 20.
status-thunderbird52: fixed → affected
tracking-thunderbird52: --- → ?
(Reporter)

Updated

6 months ago
Duplicate of this bug: 1251831
(Assignee)

Comment 23

6 months ago
Relanded on TB 52, currently at beta:
https://hg.mozilla.org/releases/comm-beta/rev/0582ed45b68a28fc582175f39ee99d61b03a6401
status-thunderbird52: affected → fixed
tracking-thunderbird52: ? → +
(Reporter)

Updated

5 months ago
Duplicate of this bug: 1352858
You need to log in before you can comment on or make changes to this bug.