1527764 - Remove RDF use from folder-lookup-service

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Ben Campbell]

Currently the folder-lookup-service is just a pass-through to the rdf service.
We want the folder-lookup-service to implement folder lookup and creation all by itself.

Two aspects to this:

1. remove RDF code from the FLS. It needs to maintain it's own map of existent folders, and gain the ability to construct new folders.
2. The inheritance from nsIRDFResource needs to be removed from existing folder implementations (mainly involves adding the Init() function from nsIRDFResource to nsIMsgFolder)

Comment 1

[Ben Campbell]

Attachment: remove-rdf-from-fls-1.patch

This patch makes the folder-lookup-service create and track folders itself, rather than just acting as a pass-through to the rdf service.

Comment 2

[Ben Campbell]

Attachment: remove-rdf-inheritance-from-folders-1.patch

And this one removes the nsIRDFResource inheritance from the folders.
(the remove-rdf-from-fls patch works on its own, and this patch should be applied on top).

Comment 3

[Ben Campbell]

I've had clean try runs on both these patches, but there's some test borkage in the tree right now, so I'll hold off going for any review of these until it clears up.
(I made a few cosmetic changes to the patches, so I'd like to run them through another try build first, just to be sure of them).

Comment 4

[Ben Campbell]

Attachment: remove-rdf-from-fls-1.patch

Ignore this one - uploaded the wrong patch!

Comment 5

[Ben Campbell]

Attachment: remove-rdf-inheritance-from-folders-2.patch

And here's a rebase of the second patch. This one removes RDF inheritance from the folder classes.

Comment 6

[Ben Campbell]

Attachment: remove-rdf-from-fls-2.patch

Here's the proper version of the first patch. Updates folder-lookup-service to not rely on RDF service.

Comment 7

[Ben Campbell]

Here's a try run with these patches (actually from before I rebased them - these ones are against yesterdays code):

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5bc93198cb35d23abf33f544556328a413e151ea

Looks pretty clean... although I don't know what test_sss_sanitizeOnShutdown.js is... will investigate (update: looks unrelated, see Bug 1529202).

Comment 8

[alta88]

benc, have you considered how the new fls handles folders that have been deleted on disk, under a running Tb? Since rdf was a cache, it returned a folder even though it didn't exist on disk, and I had to use exists() since a smart app handles these sorts of edge cases. Just a thought.

Comment 9

[Ben Campbell]

(In reply to alta88 from comment #8)

benc, have you considered how the new fls handles folders that have been deleted on disk, under a running Tb? Since rdf was a cache, it returned a folder even though it didn't exist on disk, and I had to use exists() since a smart app handles these sorts of edge cases. Just a thought.

It's a good point - I'd not really considered it. Can you point me at any specific exists() examples in the current code?
I'd imagine they'd still be called - the fls stuff is more-or-less a drop-in replacement for the RDF service, so everything else around it should be preserved...

However, the actual filesystem checking would now be the domain of the pluggable mailstore (ie mbox or maildir or whatever), so maybe there's some updating required (I've definitely found a few places that assume they only need to deal with mbox, so I know there are more lurking out there :-)

Comment 10

[alta88]

Here's a place:
https://searchfox.org/comm-central/rev/fac8703e87a0c058c70263bb1c5a6fff8815c6e6/mailnews/extensions/newsblog/content/FeedUtils.jsm#667
The comment notes that nonexistent folders can be returned, from a stale cache.

Comment 11

[Ben Campbell]

After pondering this for a while, I don't think the deal-with-suddenly-disappearing-folders cases will change under my new patches.
In any case, if we really wanted to properly cope with folders being deleted on disk while the app is running in a holistic kind of way, there's a lot more serious work involved...
And I'm inclined to think that it falls in the "don't do that!" section of the user manual :-)

Comment 12

[:aceman]

Comment on attachment 9045537 [details] [diff] [review]
remove-rdf-from-fls-2.patch

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

I have played with both patches applied. It appears to work OK, folders are displayed.

When visiting an RSS account, I got a ton of errors like this:
2019-03-03 01:50:28	Feeds	ERROR	getFeedUrlsInFolder: feeds.rdf db error - [Exception... "Could not convert JavaScript argument arg 1 [nsIRDFDataSource.GetSources]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: resource:///modules/FeedUtils.jsm :: getFeedUrlsInFolder :: line 683"  data: no]
log4moz.js:680

::: mailnews/base/src/folderLookupService.js
@@ +24,4 @@
>    QueryInterface: ChromeUtils.generateQI([Ci.nsIFolderLookupService]),
>    classID: Components.ID("{a30be08c-afc8-4fed-9af7-79778a23db23}"),
>  
> +  getFolderForURL(uri) {

Please add documentation blocks for the functions now that we are cleaning them up.

@@ +43,5 @@
> +
> +    // Check that uri has an active scheme, in case this folder is from
> +    // an extension that is currently disabled or hasn't started up yet.
> +    let scheme = uri.match(/\w*/)[0];
> +    let contractID = "@mozilla.org/rdf/resource-factory;1?name=" + scheme;

A 'rdf resource factory'? Is this the final version or will this also be converted to something else soon?

@@ +53,1 @@
>        return null;

Do we really want to be silent when all this fails? Maybe an error to the Error console would be in order. Folders not reappearing after restart could be confusing.

@@ +85,5 @@
> +    return folder;
> +  },
> +
> +  _isValidFolder(folder) {
> +    // RDF is liable to return folders that don't exist, and we may be working

Is this comment still supposed to mention RDF?

Comment 13

[:aceman]

Comment on attachment 9045535 [details] [diff] [review]
remove-rdf-inheritance-from-folders-2.patch

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

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +2967,5 @@
>  {
> +  NS_ASSERTION(aURI != nullptr, "null ptr");
> +  if (!aURI)
> +    return NS_ERROR_NULL_POINTER;
> +  mURI = aURI;

It is interesting there were no checks on the validity of the uri. Any random string could be passed here that would not be a folder path. So maybe it thne failed later on when searching/creating the backing file on disk for this folder.

::: mailnews/base/util/nsMsgDBFolder.h
@@ -100,5 @@
>    NS_IMETHOD WriteToFolderCacheElem(nsIMsgFolderCacheElement *element);
>    NS_IMETHOD ReadFromFolderCacheElem(nsIMsgFolderCacheElement *element);
>  
> -  // nsRDFResource overrides
> -  NS_IMETHOD Init(const char* aURI) override;

Don't you now also need to remove subclass versions of Init, e.g. at https://searchfox.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.h#109 ?

::: mailnews/jsaccount/test/unit/test_jaMsgFolder.js
@@ -39,5 @@
>  
>    // Make sure the DB actually works.
>    let dbFolder = db.folder;
>    Assert.ok(dbFolder instanceof Ci.nsIMsgFolder);
> -  Assert.equal(dbFolder.QueryInterface(Ci.nsIRDFResource).Value, "testja://foouser@foohost/somefolder");

Why removing this? Can you test dbFolder.URI ?

Comment 14

[:aceman]

(In reply to :aceman from comment #12)

• let contractID = "@mozilla.org/rdf/resource-factory;1?name=" + scheme;

A 'rdf resource factory'? Is this the final version or will this also be
converted to something else soon?

Ok, I see you have bug 1527772 for this.

Comment 15

[:aceman]

Comment on attachment 9045537 [details] [diff] [review]
remove-rdf-from-fls-2.patch

I think I wrote comments here, so your turn now with replies or new patches :)

Comment 16

[alta88]

(In reply to Ben Campbell from comment #11)

After pondering this for a while, I don't think the deal-with-suddenly-disappearing-folders cases will change under my new patches.
In any case, if we really wanted to properly cope with folders being deleted on disk while the app is running in a holistic kind of way, there's a lot more serious work involved...

I don't think an exists() test and then a stock folder delete call, which clears the cache and propagates pretty well in my experience with the folder change listener, is that serious, but will assume you've gotten really deep into folders and have cases which don't fit with the above. The trickiest thing in the old days (before aceman fixed folderpickers) would have been handling account manager folder destinations, and believe almost all accesses of required folders and handled cleanly already.

And I'm inclined to think that it falls in the "don't do that!" section of the user manual :-)

I can't actually agree with that as a good principle. A good principle is code that prevents users from footgunning themselves and recovers when they do. But it's an edge case and your call ;)

Comment 17

[Magnus Melin [:mkmelin]]

Ben, let's get these through the final reviews now so it can land.

Comment 18

[Ben Campbell]

I need to get Bug 1534163 sorted out first (de-RDFing the feeds code). Without that, these changes break stuff.

Comment 19

[Ben Campbell]

Attachment: part-one--remove-rdf-from-fls-3.patch

Fixups as per review in Comment #12:

• added jsdoc blocks
• added an error log if folder creation fails. I used Cu.reportError(), so I guess it'll go out the the javascript console. Open to better suggestions!
• reworded the comment block that still referred to RDF.

Comment 20

[Ben Campbell]

Attachment: part-two--remove-rdf-inheritance-from-folders-3.patch

And this one addressing comment #13:

• removed the redundant Init() implementation in nsLocalMailFolder.
• restored the removed subtest in test_jaMsgFolder.js, using the dbFolder.URI

Thanks for picking those ones up!

Also if the patches on Bug 1534163 are also applied, this stuff should work fine on newsfeed folders.

Comment 21

[Ben Campbell]

(In reply to alta88 from comment #16)

And I'm inclined to think that it falls in the "don't do that!" section of the user manual :-)

I can't actually agree with that as a good principle. A good principle is code that prevents users from footgunning themselves and recovers when they do. But it's an edge case and your call ;)

Sorry, I didn't mean to sound so flippant about this! But I think the folder-lookup-service doesn't have enough information to deal with these kind of cases, and that they need to be handled at a higher level. For example, IMAP folders can have some pretty complicated (and asynchronous) life cycles.
Also, the exists() test is something that'd ideally be delegated to the plugablemailstore used by the folder. There's a lot of code that called exists() to directly check for a mbox, and this still works for maildir (exists() doesn't care if it's a directory or file). This is all fine for now, but in theory we could have other stores (eg database-backed ones) where this'll break.
Maybe the whole issue is worth moving into a bug of it's own?

Comment 22

[:aceman]

Comment on attachment 9054794 [details] [diff] [review]
part-one--remove-rdf-from-fls-3.patch

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

Thanks, this looks good now.

Comment 23

[:aceman]

Comment on attachment 9054795 [details] [diff] [review]
part-two--remove-rdf-inheritance-from-folders-3.patch

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

Thanks.

Comment 24

[Magnus Melin [:mkmelin]]

Getting too late to take this in 68.

Comment 25

[Jorg K (CEST = GMT+2)]

I'm sure this stuff had some bitrot since April. I guess the checkin was waiting for bug 1534163 which is ready now.

Comment 26

[Ben Campbell]

Attachment: part-one--remove-rdf-from-fls-4.patch

Rebased rdf-removal patches. Already reviewed by aceman, and no actual changes. But they did span the big eslint/clang-format restylings, so there were a lot of changes to pick through... the try build looks solid, but not sure if it's worth another pair of eyes to sanity check it?

Comment 27

[Ben Campbell]

Attachment: part-two--remove-rdf-inheritance-from-folders-4.patch

Comment 28

[Jorg K (CEST = GMT+2)]

I'm landing this now ;-)

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1fec6625cb31
Remove rdf use from folder-lookup-service. r=aceman
https://hg.mozilla.org/comm-central/rev/729209b67d45
Remove RDFResource inheritance from nsMsgDBFolder. r=aceman