Closed Bug 1328814 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: Networking: IMAP, defect)

Unspecified
All
defect
Not set
normal

Tracking

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

RESOLVED FIXED
Thunderbird 53.0
Tracking Status
thunderbird_esr45 + affected
thunderbird51 --- fixed
thunderbird52 + fixed
thunderbird53 --- fixed

People

(Reporter: wsmwk, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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).
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: → 1083994
Attached patch 1328814-imap-move.patch (v1) (obsolete) — Splinter Review
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)
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
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
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+
Attachment #8825214 - Flags: feedback?(jorgk)
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+
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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Attached patch landed patchSplinter Review
[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+
Attachment #8823972 - Attachment is obsolete: true
Attachment #8825214 - Attachment is obsolete: true
See Also: 1083994
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+
Blocks: TB52found
Depends on: 1332606
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
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.
I think we should leave it backed out until we have a fix with the information gained in bug 1335638 comment #10.
Depends on: 1335345
Depends on: 1335638
Setting the status-thunderbird52 field back to "affected" to reflect comment 20.
Duplicate of this bug: 1251831
Duplicate of this bug: 1352858
You need to log in before you can comment on or make changes to this bug.