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

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

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

Comment 1

[Wayne Mery (:wsmwk)]

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

Comment 2

[Jorg K (CEST = GMT+2)]

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

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Kent James (:rkent)]

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.

Comment 10

[Kent James (:rkent)]

Attachment: Only set rv if we intend to fail from it.

Comment 11

[Jorg K (CEST = GMT+2)]

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.

Comment 12

[Jorg K (CEST = GMT+2)]

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.

Comment 13

[Jorg K (CEST = GMT+2)]

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

Comment 14

[Jorg K (CEST = GMT+2)]

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

Comment 15

[Kent James (:rkent)]

Comment on attachment 8825317 [details] [diff] [review]
landed patch

https://hg.mozilla.org/releases/comm-esr45/rev/ccb801cd360e

Comment 16

[Wayne Mery (:wsmwk)]

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

[Kent James (:rkent)]

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.

Comment 18

[Jorg K (CEST = GMT+2)]

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

Comment 19

[Kent James (:rkent)]

OK, let's leave it backed out.

Comment 20

[Jorg K (CEST = GMT+2)]

Backed out from TB 52, currently at beta:
https://hg.mozilla.org/releases/comm-beta/rev/e3e9f49f289f4a97db9860bc52dda812a3afe870

Comment 21

[Daniel Veditz [:dveditz]]

Setting the status-thunderbird52 field back to "affected" to reflect comment 20.

Comment 23

[Jorg K (CEST = GMT+2)]

Relanded on TB 52, currently at beta:
https://hg.mozilla.org/releases/comm-beta/rev/0582ed45b68a28fc582175f39ee99d61b03a6401