This has serious performance implications on my Add-on QuickFolders.

When building a MRU list if call folder.getStringProperty("MRUTime") - this throws if the property doesn't exist. It now takes about 8 Seconds to build the mru list (it was 200ms in Thunderbird 91)
The error thrown is:

Component returned failure code: 0x80550007 [nsIMsgFolder.getStringProperty]

When trying to look for icon modifications in the folder tree I use folder.getStringProperty("folderIcon") which throws if the attribute is not added, again effecting serious performance penalties. The whole UI of Thundebird gets extremely sluggish, the more folders are shown in the tree.

Would it be possible to patch getStringProperty so that it doesn't throw and just returns NULL when the property doesn't exist? There is no other way to find out about a nsIMsgFolder property

Not my territory, likely something to do with bug 418551. I don't see how throwing would be that much slower than returning a value.

2022-07-11_1317_mass-mark-TB102found

Hmm. I could definitely imagine exceptions causing a slowdown. And even if we didn't, we definitely shouldn't be relying on exceptions in normal operation, so it sounds to me like a better solution is required.

I don't think we can easily return null for undefined properties - the idl spec specifies a string and I don't think there's any way to return null via C++ (it appears as a nsACString& param in the C++ signature).
We could return an empty string, but that wouldn't let us distinguish between "" and an undefined property.

Axel: off the top of your head, do you know what the old behaviour was? (I had a quick peek at the bug 418551 changes, but it's not obvious)

I think a proper isPropertyDefined() kind of method would be the way forward here...

I wouldn't expect throwing to cause that much of an overhead. Perhaps the problem is a symptom of something else, like problem with the .msf files (not existing perhaps)?

Like Ben I'd also like to know what the old behavior was. Did it really change?

The behaviour has changed, null values in the folder cache are no longer returned and cause an error. This caused bug 1726319, bug 1773822, bug 1773401 and bug 1776280. Please refer to bug 1776280 comment #7 and cmt. #9.

We've set all MRUTime entries to null in folderCache.json and then ran "Quick Folder Move". Same speed as without the patch and all MRUTimes populated after bug 1776280. So this is the better solution.

(In reply to b1 from comment #6)

Created attachment 9285187 [details] [diff] [review]
1778888-folder-cache-null-value.patch

We've set all MRUTime entries to null in folderCache.json and then ran "Quick Folder Move". Same speed as without the patch and all MRUTimes populated after bug 1776280. So this is the better solution.

not bad - I was thinking along the same line for my custom property "folderIcon" - instead of setting it for only the customized folders I could set it to an empty string to all folders in the tree at startup, this would also avoid the throwing. Will do some testing along these lines this week. I call the getStringProperty function from the gFolderTreeView.getCellProperties() in order to generate a CSS stylable attribute on the element in folderNameCol, and there are a lot of calls for that one.

(In reply to Ben Campbell from comment #3)

Axel: off the top of your head, do you know what the old behaviour was? (I had a quick peek at the bug 418551 changes, but it's not obvious)

I think a proper isPropertyDefined() kind of method would be the way forward here...

certainly an isPropertyDefined() or hasStringProperty() function would be a godsend. Would it be hard to implement?

Thanks for the review. This was the easiest way to return to TB 91 behaviour after all the regressions the behaviour change has caused, see comment #5. We'll ship this in our fork until a better solution arrives. Sadly there are more regressions and people haven't made the connection yet: If MSFs are deleted (bug 1773822) of course virtual/unified folders stop working (maybe related to bug 1777606) and filters also stop working (bug 1777731).

Note that no uplift was requested in bug 1776280, so users of "recent anything" are still suffering slowness, see bug 1773401.

We believe there is a difference between a property missing the the folder cache and a property being present with a value of null. In all the data we've seen, MRUTime and LastPurgeTime was null, not missing, so being null should not return an error, whereas missing should go off and fetch the value from the database. Side note: We don't quite understand the inner workings of Mork, looking at some panacea.dat files we got the impression that those two timestamp properties were "mentioned" in the "header" without specific individual values. Maybe that caused a general "default" null value to be returned which is now visible in folderCache.json after migration.

Your point three: Were not sure that remove and unset have the same semantic, remove leads to missing from the cache (causing a DB access) and unset causes a null value.

Just repeating: The behaviour change caused exisiting null values to be rejected and then databases were opened (and not closed again, bug 1773822) to fetch the value. That caused hitting the "max open files" and subsequent MSF deletion since any error in opening the MSF deemed it invalid and caused its deletion. And even after successful retrieval the value wasn't added to the cache (bug 1776280). It took quite some time to a) recognise the behaviour change of the folder cache and b) understand all the series of events and consequences.

Or are you trying to distinguish between null and empty string? They are the same, in all the conversions from char* to nsCString in the past, a null char pointer or a char pointer pointing to a single null byte were replaced with EmptyCString(). What we think needs to be distinguished is: value present in the cache, be it null, empty string on non-empty string, and value missing from the cache (needs to trigger retrieval from the DB).

That's why believe that replacing if (!o.isNull() && o.isConvertibleTo(Json::stringValue)) with if (o.isConvertibleTo(Json::stringValue)) is the correct change. The quoted M-C code will return an empty string for a null value which is desirable. If you're at that point in the code, there was an entry in the cache, so we don't understand what you mean by "undefined/missing entries". Undefined is not a good term in this context, the value is defined, it's null, and it's also not missing.

Well, looks like the JSON lookup returns null even for missing entries:
https://searchfox.org/mozilla-central/rev/0d11f3660945ce35c49501bb44bc4f82bb2b503c/toolkit/components/jsoncpp/src/lib_json/json_value.cpp#1104,1110

:-(

Looks like the panacea.dat to JSON migration was wrong then and should have migrated default/empty/null values to an empty string instead of null since null can't be distinguished from missing :-( - Nothing in folderCache.json should be null since that's considered "not present".

(In reply to b1 from comment #12)

Well, looks like the JSON lookup returns null even for missing entries:
https://searchfox.org/mozilla-central/rev/0d11f3660945ce35c49501bb44bc4f82bb2b503c/toolkit/components/jsoncpp/src/lib_json/json_value.cpp#1104,1110

Yes, sorry, I wasn't explicit enough about that - jsoncpp seems to use null to represent things that JS would use undefined for...
I've been picking my way through the jsoncpp library, and there is an isMember() method, which would let us check if a entry exists before fetching it. So we can make the distinction.
The question is how to represent that in our .getStringProperty()?

What you wrote in comment #10 seems to point the way:

(In reply to b1 from comment #10)

We believe there is a difference between a property missing the the folder cache and a property being present with a value of null. In all the data we've seen, MRUTime and LastPurgeTime was null, not missing, so being null should not return an error, whereas missing should go off and fetch the value from the database.

SO... how about:

1. in the cache code,

• null (actually null in json!) cache value returns empty string.
• entirely missing throws exception (rv=NS_ERR_NOT_FOUND or whatever it is)
• apply this to the numeric methods too, to be consistent (so actual null reads as 0, which seems reasonable).

2. in nsIMsgFolder:

• add a .hasStringProperty() and maybe a .removeStringProperty() to nsiMsgFolder.

I think that behaviour would sort this all out nicely, right?

Thanks for the tip. This should be the right solution then. Feel free to refactor it and apply the same principle to numeric retrieval. Some tests to document the behaviour would of course also be very useful. Please attribute the research here to our project.

Thanks, we can't set "checkin needed" since we didn't file the bug.

And maybe it would be best to do more work in a new bug to avoid the situation where only a part is uplifted.

Oop - no, hang off on landing this - comm/mailnews/base/test/unit/test_nsIMsgFolderCache.js fails (getCachedString() doesn't throw an exception for non-exsitent key). Investigating now.

Easy fix. Turns out the JsonCpp Value& Value::operator[char*] method creates a null entry in the object if it doesn't already exist. So we just need to do the isMember() check first.
The numeric methods have to do that too, because I used the same non-existent-key name in test_nsIMsgFolderCache.js :-)

try run in progress:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a59cfc6873258a49e9f0a3535248e14fc7c9aae6

Could someone try this patch to see if it fixes the QuickFolders issue?
If so, and the try run looks OK, then we land it and flag it for uplift...

(In reply to Ben Campbell from comment #20)

Easy fix. Turns out the JsonCpp Value& Value::operator[char*] method creates a null entry in the object if it doesn't already exist.
So we just need to do the isMember() check first.

Thanks for the adjustment and sorry for not running the test. We wanted to avoid two lookups of the same key, hence the isMember() was only called if a null value was returned.

This can land now.

I've marked it to get landed now.

(In reply to Ben Campbell from comment #20)

Turns out the JsonCpp Value& Value::operator[char*] method creates a null entry in the object if it doesn't already exist.

For the record: Since the Obj() isn't const, not these functions are triggered
https://searchfox.org/mozilla-central/rev/99c3ccc2f02c5fff7c9850f7a0770ee92cc693f6/toolkit/components/jsoncpp/src/lib_json/json_value.cpp#1101-1112
but these:
https://searchfox.org/mozilla-central/rev/99c3ccc2f02c5fff7c9850f7a0770ee92cc693f6/toolkit/components/jsoncpp/src/lib_json/json_value.cpp#1114-1124
And resolveReference() inserts a null value here:
https://searchfox.org/mozilla-central/rev/99c3ccc2f02c5fff7c9850f7a0770ee92cc693f6/toolkit/components/jsoncpp/src/lib_json/json_value.cpp#1050-1051

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/2f49d0b8575f
Revert bug 1726319 and bug 1776280 and allow folder cache to return empty strings for null values. r=benc

uplift to 102 with bug 1776280?

(In reply to Wayne Mery (:wsmwk) from comment #27)

uplift to 102 with bug 1776280?

Yes, I think so.
The patch in bug 1776280 isn't needed - this patch rolls it back out. But maybe the one in bug 1776280 needs to be taken first so that this one applies cleanly?

Comment on attachment 9285581 [details] [diff] [review]
1778888-folder-cache-null-value-2.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):

Comment on attachment 9285581 [details] [diff] [review]
1778888-folder-cache-null-value-2.patch

[Triage Comment]
Approved for esr102

Take this after bug 1776280

Thunderbird 102.2.0:
https://hg.mozilla.org/releases/comm-esr102/rev/e6161c676599

Thanks for fixing, I can confirm displaying my recent folders is orders of magnitude faster. I would supply quantitative data on this but logging seems to be borked:

See bug 1785222