1679333 - Remove support for dangling (unparented) folders

Status: REOPENED

(Thunderbird :: General, task)

Description

[Ben Campbell]

For various reasons, we support creating nsIMsgFolder objects which don't (yet) have parents.
This is pretty dodgy - an unparented folder is in a very undefined state (e.g. there's no way to map it to the filesystem!).
We don't really need to support unparented folders, it's just that there are a bunch of places in the code which assume they are possible. We should phase them out.
In most of these places the code doesn't expect to have to deal with dangling folders, but for historical reasons calls a nsIFolderLookupService.getOrCreateFolderForURL, which may return a dangling folder.

To fix this:

Find every call to nsIFolderLookupService.getOrCreateFolderForURL, which includes:

• Calls to GetOrCreateFolder(), defined in mailnews/base/src/nsMsgUtils.h.
• Calls to MailUtils.getOrCreateFolder()

For each call, decide if the code actually expects to deal with dangling folders, and if not, replace it with something to fetch an existing folder OR to create a new folder. Not both.

For the places that genuinely might expect dangling folders, figure out how to reorganise things so dangling folders are not required.

Lastly, in nsIFolderLookupService:

• getOrCreateFolderForUrl() should be removed entirely (along with the corresponding fns in MailUtils.jsm and nsMsgUtils.h).
• there should no longer be an internal map of URLs of known folders. getFolderForURL() should be reimplemented to just get the nsIMsgIncomingServer for the given URL and iterate down the folder hierarchy to find the folder.

Comment 1

[Ben Campbell]

Extra notes to self:

• getOrCreateFolderForUrl() is the core place where folders are created... but it's only part of the story. getOrCreateFolderForUrl() leaves new folders unparented, and uninitialised in all kinds of other ways (eg flags). Ultimately all folder creation should be performed by the type-specific nsIMsgFolder/nsIMsgIncomingServer implementations.

• The new, massively cut-down FLS should be in C++. Rationale: currently it's an utter arse to debug! Folder creation is initiated in C++ (eg via nsIMsgFolder.createSubfolder()), which then calls out to the FLS getOrCreateFolderForUrl() in javascript, which then calls nsIMsgFolder.Init() in C++, then returns back to JS, then returns to C++ to finish up (set up parent, flags and whatever protocol-specific stuff is required).

Comment 2

[Ben Campbell]

Attachment: 1679333-remove-getOrCreateFolder-in-mailWindowOverlay-1.patch

A first step into a bright new future without getOrCreateFolderFromUrl()!

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9198039 [details] [diff] [review]
1679333-remove-getOrCreateFolder-in-mailWindowOverlay-1.patch

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

::: mail/base/content/mailWindowOverlay.js
@@ +910,3 @@
>      "mail.last_msg_movecopy_target_uri"
>    );
> +  let isMove = Services.prefs.getBoolPref("mail.last_msg_movecopy_was_move");

please move this down to where it's used

Comment 4

[Ben Campbell]

Attachment: 1679333-remove-getOrCreateFolder-in-mailWindowOverlay-2.patch

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6fafd089d308
Remove getOrCreateFolder() use in mailWindowOverlay. r=mkmelin

Comment 6

[Eleanor Dicharry]

Attachment: WIP: Bug 1679333 - Remove use of GetOrCreateFolder in nsMsgDBFolder::FindSubFolder.

Comment 7

[Eleanor Dicharry]

Attachment: Bug 1679333 - Remove use of `FindSubFolder` in `nsLocalMailFolder`. r=BenC,darktrojan,#thunderbird-back-end-reviewers

Comment 8

[Eleanor Dicharry]

Attachment: Bug 1679333 - Remove use of `FindOrCreateFolder` in nsMsgDBFolder::AddSubfolder. r=BenC,darktrojan,#thunderbird-back-end-reviewers

Comment 9

[Eleanor Dicharry]

Attachment: WIP: Bug 1679333 - Move FolderLookupService to C++.

Comment 10

[Phabricator Automation]

Comment on attachment 9488420 [details]
Bug 1679333 - Remove use of FindSubFolder in nsLocalMailFolder. r=BenC,darktrojan,#thunderbird-back-end-reviewers

Revision D249847 was moved to bug 1968768. Setting attachment 9488420 [details] to obsolete.

Comment 11

[Phabricator Automation]

Comment on attachment 9488436 [details]
Bug 1679333 - Remove use of FindOrCreateFolder in nsMsgDBFolder::AddSubfolder. r=BenC,darktrojan,#thunderbird-back-end-reviewers

Revision D249862 was moved to bug 1968769. Setting attachment 9488436 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9490543 [details]
WIP: Bug 1679333 - Move FolderLookupService to C++.

Revision D251086 was moved to bug 1968770. Setting attachment 9490543 [details] to obsolete.

Comment 13

[Eleanor Dicharry]

Attachment: Bug 1679333 - Always create a new root folder. r=#thunderbird-back-end-reviewers

Comment 14

[Phabricator Automation]

Comment on attachment 9504091 [details]
Bug 1679333 - Always create a new root folder. r=#thunderbird-back-end-reviewers

Revision D259296 was moved to bug 1980401. Setting attachment 9504091 [details] to obsolete.