Closed Bug 1527764 Opened 1 year ago Closed 7 months ago

Remove RDF use from folder-lookup-service

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: benc, Assigned: benc)

References

Details

Attachments

(2 files, 7 obsolete files)

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)
Blocks: mail-killrdf
Assignee: nobody → benc
Blocks: 1527772
Attached patch remove-rdf-from-fls-1.patch (obsolete) — Splinter Review

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

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

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

Attached patch remove-rdf-from-fls-1.patch (obsolete) — Splinter Review

Ignore this one - uploaded the wrong patch!

Attachment #9043770 - Attachment is obsolete: true
Attachment #9045534 - Flags: review?(acelists)

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

Attachment #9043771 - Attachment is obsolete: true
Attachment #9045535 - Flags: review?(acelists)
Attachment #9045534 - Attachment is obsolete: true
Attachment #9045534 - Flags: review?(acelists)
Attached patch remove-rdf-from-fls-2.patch (obsolete) — Splinter Review

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

Attachment #9045537 - Flags: review?(acelists)

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

Status: NEW → ASSIGNED

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.

(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 :-)

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.

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 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?
Attachment #9045537 - Flags: feedback+
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 ?
Attachment #9045535 - Flags: feedback+

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

Depends on: 1534163
Attachment #9045535 - Flags: review?(acelists)
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 :)
Attachment #9045537 - Flags: review?(acelists)

(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 ;)

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

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

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.
Attachment #9045537 - Attachment is obsolete: true
Attachment #9054794 - Flags: review?(acelists)

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.

Attachment #9045535 - Attachment is obsolete: true
Attachment #9054795 - Flags: review?(acelists)

(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 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.
Attachment #9054794 - Flags: review?(acelists) → review+
Comment on attachment 9054795 [details] [diff] [review]
part-two--remove-rdf-inheritance-from-folders-3.patch

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

Thanks.
Attachment #9054795 - Flags: review?(acelists) → review+
Blocks: 1543183

Getting too late to take this in 68.

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

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?

Attachment #9054794 - Attachment is obsolete: true
Flags: needinfo?(jorgk)
Attachment #9095065 - Flags: review+
Attachment #9054795 - Attachment is obsolete: true
Attachment #9095066 - Flags: review+

I'm landing this now ;-)

Flags: needinfo?(jorgk)

Pushed by mozilla@jorgk.com:
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

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
You need to log in before you can comment on or make changes to this bug.