Starting TB 78 on a profile with many folders gives unresponsive script error - take 2
Categories
(Thunderbird :: Folder and Message Lists, defect, P1)
Tracking
(thunderbird_esr78+ fixed, thunderbird79 fixed)
People
(Reporter: jorgk-bmo, Assigned: aleca)
References
(Regression)
Details
(Keywords: perf, regression)
Attachments
(1 file, 6 obsolete files)
11.77 KB,
patch
|
mkmelin
:
review+
wsmwk
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1647251 +++
A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.
Script: chrome://messenger/content/folderPane.js:2622
A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.
Script: chrome://messenger/content/folderPane.js:1739
A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.
Script: chrome://messenger/content/folderPane.js:1284
This could affect may users. All related to custom folder colours.
Reporter | ||
Comment 1•3 years ago
|
||
I have about 1700 folders in that profile. Looks like updating to TB 78 also destroys MSF files, I have 54 zero-byte MSF files which were created exactly at the point in time of the update.
Comment 2•3 years ago
|
||
SSD or spinning disk? (Not relevant to the bug, just curious)
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
All right, let's change approach completely.
I'll see how to implement what was suggested by alta88 on bug 1637668 comment 30, which was:
Hint: the better architecture would be to store properties in the folderTree.json file already supported. I should think a priority would be to get folder properties out of mork/msf and into a json store. If you need to figure out how to persist json async, see how it's done in session store code.
Moving the custom colour property there instead of relying on the folder.msgDeatabase
will allow us to not loop through the folders and not open and close the database for each of those folders.
Looks like updating to TB 78 also destroys MSF files
Is this issue related to the folder colour customization as well?
Reporter | ||
Comment 4•3 years ago
|
||
SSD or spinning disk?
SSD.
Is this issue related to the folder colour customization as well?
Jolly good question. All I can say is that I converted to TB 78 twice (and went back to TB 68 in between) and each time I got zero-byte MSF files. Not many, but some. Maybe caused by a hanging script? It's not so much fun when you have filters with those folders as a target.
I quite like the new icons and the fact that one can colour them. But 25 sec startup followed by an "empty script" warning aren't a good trade-off.
Assignee | ||
Comment 5•3 years ago
|
||
(In reply to Jorg K (CEST = GMT+2) from comment #4)
I quite like the new icons and the fact that one can colour them. But 25 sec startup followed by an "empty script" warning aren't a good trade-off.
That's a trade off that I'm definitely not willing to make.
I'll get to it this week and fix it, apologies for the annoying issues.
Comment 6•3 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #3)
Looks like updating to TB 78 also destroys MSF files
Is this issue related to the folder colour customization as well?
FWIW the msf file space can be fragile. https://mzl.la/303ZRR0 lists some reported bugs, and I'm sure the are more. There is also impact from antivirus and programs like ccleaner.
Reporter | ||
Comment 7•3 years ago
|
||
Actually, the most hangs are here: Script: chrome://messenger/content/folderPane.js:1284
Assignee | ||
Comment 8•3 years ago
|
||
This patch removes the usage of the folder.msgDatabase
and stores all the custom colors in the folderTree.json
file by leveraging the _persistOpenMap
object which already stores the open states of folders.
Pinging alta88 for feedback since this solution was originally suggested by him.
Assignee | ||
Comment 9•3 years ago
|
||
Mh, maybe I should use the _modeNames
instead of a custom kColorMode
🤔️
Assignee | ||
Comment 10•3 years ago
|
||
Or maybe not, since those modes are also listed in the folder pane menulist and we don't really want to allow users to sort folders by color.
Assignee | ||
Comment 11•3 years ago
|
||
Comment on attachment 9164138 [details] [diff] [review] 1652279-folder-color.diff I'm pinging additional people for review to see who can do it first since this is a pretty severe bug. Apologies for the annoying pings, but I'm hoping to get this reviewed as soon as possible.
Reporter | ||
Comment 12•3 years ago
|
||
Comment on attachment 9164138 [details] [diff] [review] 1652279-folder-color.diff Sorry, not my field of expertise.
Comment 13•3 years ago
|
||
Comment on attachment 9164138 [details] [diff] [review] 1652279-folder-color.diff Review of attachment 9164138 [details] [diff] [review]: ----------------------------------------------------------------- Looks like it's in line with what we do for open states, so should be ok. ::: mail/base/content/folderPane.js @@ +1532,5 @@ > ], > _modeDisplayNames: {}, > > /** > + * This is a javascript map of which folders we had open, or ahve a custom JavaScript have @@ +1539,2 @@ > */ > _persistOpenMap: {}, I think you shouldn't overload the meaning of the _persistOpenMap which is for the different folder modes we have. It would be better to have a separate persistence map for it, like _persistColorMap @@ +1599,5 @@ > + return; > + } > + > + // Loop through all the saved folders and restore their colors. > + for (let item of map) { could use for (let {color, id} of map)
Assignee | ||
Comment 14•3 years ago
|
||
Thank you so much for the review. I fixed pretty much everything.
I think you shouldn't overload the meaning of the _persistOpenMap which is for the different folder modes we have.
It would be better to have a separate persistence map for it, like _persistColorMap
That was my first approach but I discarded it since I was forced to duplicate the methods in the load
and unload
to read and write the JSON file.
https://searchfox.org/comm-central/rev/5edba8a2dd51172df411fbac749d944a9c65dae2/mail/base/content/folderPane.js#184
Maybe we can rename that object and call it _persistFolderMap
to be more generic and handle both colors and open states.
What do you think?
Comment 15•3 years ago
|
||
It doesn't look like there would be too much duplication. You wouldn't need to duplicate anything but the reading and writing of the file, so all in all around 20 lines.
Comment 16•3 years ago
|
||
It is completely bizarre to want a second file to be read when the color or other properties can be added to an existing file. Only 2 of the several possible display modes are persisted.
Reporter | ||
Comment 17•3 years ago
|
||
Comment on attachment 9164138 [details] [diff] [review] 1652279-folder-color.diff That f+ got set inadvertently :-(
Assignee | ||
Comment 18•3 years ago
|
||
It is completely bizarre to want a second file to be read when the color or other properties can be added to an existing file. Only 2 of the several possible display modes are persisted.
That's not the direction we're going.
The question was regarding the usage of the _persistOpenMap object for all properties, or having a dedicated object only for the colors.
The file would be the same.
Assignee | ||
Comment 19•3 years ago
•
|
||
I implemented the _persistColorMap
object to collect and interact with the colours.
I'm using the same foldeTree.json file as I too think we shouldn't use another file only for colours.
I'm separating the two sets of data after the parse simply by ID, 0 is for the open state, and 1 is for colours.
I'm worried that we might have some small issues with this if in the future we decide to extend the foldeTree.json file to persist other data sets.
Do you think I should stringify the data with something like this?
let data = JSON.stringify({
open: this._persistOpenMap,
colors: this._persistColorMap,
});
Assignee | ||
Comment 20•3 years ago
|
||
To speed things up due to the time zone difference, here's the same patch with the stringified object variation.
I personally prefer this approach, but up to you.
Comment 21•3 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #18)
It is completely bizarre to want a second file to be read when the color or other properties can be added to an existing file. Only 2 of the several possible display modes are persisted.
That's not the direction we're going.
That is exactly where comment 15 wanted you to go.
The question was regarding the usage of the _persistOpenMap object for all properties, or having a dedicated object only for the colors.
The file would be the same.
The in memory object should be renamed as proposed and contain open, color, and any future properties.
There is a serious misunderstanding of how a cache should be used. For expensive fetches like favicons off a network or opening mork dbs to get a trivial thing, a cache for nsITree is mandatory. Now that a map is finally being implemented, there is no need for the extraneous code to cache folder properties; the map is the cache. Please clean that up.
It is still suboptimal that you are not using JSONFile and that you are not processing an nsITree row properly.
Comment 22•3 years ago
|
||
(In reply to alta88 from comment #16)
It is completely bizarre to want a second file to be read when the color or other properties can be added to an existing file. Only 2 of the several possible display modes are persisted.
It's separating concerns. But I guess it's ok to keep in the same file too since it's only a cache.
Comment 23•3 years ago
•
|
||
Comment 24•3 years ago
|
||
(In reply to alta88 from comment #21)
It is still suboptimal that you are not using JSONFile and that you are not processing an nsITree row properly.
We could do JSONFile in another bug.
I don't know what you think is wrong with the nsITree row processing. If you have concrete suggestions, why don't just spell them out.
Comment 25•3 years ago
|
||
Comment on attachment 9165238 [details] [diff] [review] 1652279-folder-color-V2.diff Review of attachment 9165238 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/folderPane.js @@ +1553,5 @@ > + * This is a JavaScript map of which folders have a custom color so that we > + * can persist the customization state over-time. It is designed to be used > + * as a JSON object. > + */ > + _persistColorMap: [], This shouldn't be an array. The openMap used a plain object. (It's a shame js Map can't be serialized easily) @@ +1612,5 @@ > + // Loop through all the saved folders and restore their colors. > + for (let { color, id } of map) { > + // Store the color in the cache property so we can use this for > + // properties changes and updates. > + gFolderTreeView.setFolderCacheProperty( I guess all of this is what alta88 says is now not necessary
Assignee | ||
Comment 26•3 years ago
|
||
Patch updated.
Since we're changing the structure of the JSON file, I implemented a nullish coalescing operator to prevent losing the current open state of the user.
![]() |
||
Comment 27•3 years ago
|
||
Comment on attachment 9165444 [details] [diff] [review] 1652279-folder-color.diff Review of attachment 9165444 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks OK. (Maybe observe the improvements alta88 is suggesting). Also, it is sure this solves the reported startup slowness? (It does not fix slowness I observe, but I do not have the "unresponsive script dialog".) There are 2 considerations though: 1. There is no migration yet. If I had colors set in the current folder databases, they are lost (not used) after the patch. 2. The newly used json file is NOT a cache (as written in the other comments), it is the single source of this information (folder colors and open state). It could get reset easily. Also, colors stored in this file are not migrated if a folder (the mbox file and its msf) is moved in the profile to a different position (power-users and devs may do that). The folder URI changes and the color is not found (stale entries may stay in the json file, but that probably happens with the open states too). When the color attribute is in the .msf if moves with the folder is not lost. So this behaviour needs UX decision if the color information is less important (disposable) than the other info in the .msf file. ::: mail/base/content/folderPane.js @@ +1603,5 @@ > + * Iterate over the custom color list and apply the CSS style stored in it. > + */ > + _restoreCustomColors() { > + let tree = this; > + let map = tree._persistColorMap; Why this assignment? You only use the tree._persistColorMap once. @@ +1611,5 @@ > + return; > + } > + > + // Loop through all the saved folders and restore their colors. > + for (let { color, id } of map) { Are the spaces after { needed? @@ +1648,5 @@ > + /** > + * Add the item to the persisted list of custom colored folder. > + * > + * @param {string} folderId - The URI of the folder item. > + * @param {string} color - The selected custom color. So if no color is given, the current color is cleared intentionally? If so, this could be in the description here. @@ +1659,5 @@ > + if (!color) { > + return; > + } > + > + // Create the map if is undefined. ...if it is...
Assignee | ||
Comment 28•3 years ago
|
||
Thanks, this looks OK. (Maybe observe the improvements alta88 is suggesting).
Thanks.
Stripping the folderCache now would require way more work since we're using that to handle all our folder properties.
I'll open a follow up bug to investigate this approach and improve it.
Also, it is sure this solves the reported startup slowness? (It does not fix slowness I observe, but I do not have the "unresponsive script dialog".)
Based on comment 0, all the unresponsive script warnings are related to the way the custom colour was getting fetched by the Folder.msgDatabase, forcing us to loop through all the available folders to restore the colours.
With this method, it doesn't matter how many folders the user has, we only apply CSS colours and a cache property during the getProperties()
method of the tree.
What slowness are you observing?
- There is no migration yet. If I had colors set in the current folder databases, they are lost (not used) after the patch.
Unfortunately, that's something we can't do.
If users are experiencing slow downs due to the amount of folders, looping through them all to transfer the colours from the old method will cause the same performance issue during migration.
I think this is a trade off we're forced to make.
- The newly used json file is NOT a cache (as written in the other comments), it is the single source of this information (folder colors and open state). It could get reset easily. Also, colors stored in this file are not migrated if a folder (the mbox file and its msf) is moved in the profile to a different position (power-users and devs may do that). The folder URI changes and the color is not found (stale entries may stay in the json file, but that probably happens with the open states too). When the color attribute is in the .msf if moves with the folder is not lost. So this behaviour needs UX decision if the color information is less important (disposable) than the other info in the .msf file.
Thanks for this overview. What about comment 6 regarding using MSF files?
Are the spaces after { needed?
Required by our linter.
So if no color is given, the current color is cleared intentionally? If so, this could be in the description here.
No, the previous colour is always cleared when interacting with a folder, which is written in the inline comment underneath.
If no colour is defined, the method doesn't continue, which is also written in an inline comment.
Thanks for the feedback, I'm addressing the other issues.
Assignee | ||
Comment 29•3 years ago
|
||
Comment 30•3 years ago
|
||
Comment on attachment 9165581 [details] [diff] [review] 1652279-folder-color.diff Review of attachment 9165581 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/folderPane.js @@ +183,5 @@ > try { > + let parsedData = JSON.parse(data); > + // Migrate all the data from the old stored object if the "open" > + // object doesn't exist. > + this._persistOpenMap = parsedData.open ?? parsedData; = parsedData.open || parsedData; ... would be the more traditional and easy to read way @@ +1544,2 @@ > */ > _persistOpenMap: {}, I think there is some confusion around this, and the code in jsTreeView.js looks wrong (it's accessing _persistOpenMap but should access _persistOpenMap[mode]). folderPane.js looks correct _persistOpenMap is an object, with the mode as key, it's like {"all" -> [ "imap://...", "mailbox://nobody@Feeds"] } @@ +1658,5 @@ > + } > + > + // Create the map if it is undefined. > + if (!this._persistColorMap) { > + this._persistColorMap = []; This is still changed to an array now, not a {} @@ +1664,5 @@ > + > + this._persistColorMap.push({ > + id: folderId, > + color, > + }); You're now pushing objects into that array. I think the idea is that you would use it as an object. I.e. this._persistColorMap[folderId] = color; + other changes needed due to this of course
Assignee | ||
Comment 31•3 years ago
|
||
Sorry about the confusion between object and array, good that you caught that mistake.
Please be sure to manually delete the "colors" array from the folderTree.json file to prevent errors. This won't be necessary for the users since that array is there only because of my previous wrong patch.
I think there is some confusion around this, and the code in jsTreeView.js looks wrong (it's accessing _persistOpenMap but should access _persistOpenMap[mode]). folderPane.js looks correct
Do you think we should open a dedicated bug to tackle the error in the jsTreeView.js file?
Comment 32•3 years ago
|
||
(In reply to :aceman from comment #27)
Comment on attachment 9165444 [details] [diff] [review]
1652279-folder-color.diffReview of attachment 9165444 [details] [diff] [review]:
Thanks, this looks OK. (Maybe observe the improvements alta88 is suggesting).
Also, it is sure this solves the reported startup slowness? (It does not fix
slowness I observe, but I do not have the "unresponsive script dialog".)There are 2 considerations though:
- There is no migration yet. If I had colors set in the current folder
databases, they are lost (not used) after the patch.- The newly used json file is NOT a cache (as written in the other
comments), it is the single source of this information (folder colors and
open state). It could get reset easily.
Not sure why you say this. It is an in memory cache. The primary, reference data is and must be the persisted json stringified object stored on disk. This patch doesn't have a concept of updating the cache and flushing it (nor did the original implementation), thus out of sync data loss may happen. These are really very basic concepts. That is why JSONFile and its apis.
The folder URI changes a
Yes, this wasn't considered at all. There's at least one example of how to handle folder rename/move/etc in js, in the feeds code, which db is entirely keyed off folder uris.
Assignee | ||
Comment 33•3 years ago
|
||
Found a small typo.
Comment 35•3 years ago
|
||
Comment on attachment 9165753 [details] [diff] [review] 1652279-folder-color.diff Review of attachment 9165753 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I filed bug 1654990 for the wrong usage.
Updated•3 years ago
|
Reporter | ||
Comment 36•3 years ago
|
||
Can you do a Windows 64bit opt ESR build for me to try, please.
Comment 37•3 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1271811d565a
Fix startup performance issues with folder colour customization. r=mkmelin
Comment 38•3 years ago
|
||
Comment on attachment 9165753 [details] [diff] [review] 1652279-folder-color.diff [Approval Request Comment] Regression caused by (bug #): bug 1637668 User impact if declined: UI slowness when opening with many folders Testing completed (on c-c, etc.): soon ;) Risk to taking this patch (and alternatives if risky): All things considering, not super risky.
Comment 39•3 years ago
|
||
Comment on attachment 9165753 [details] [diff] [review] 1652279-folder-color.diff Approved for beta Approved for esr78 We have a day or two where this will be on daily, before we actually ship beta. So we'll want to check bugzilla before actually shipping. ALso, I can post links to the candidate build so some people like sguarin who have seen the problem can give it a test.
Assignee | ||
Comment 40•3 years ago
|
||
If this lands in 78.1, we should mention in the release notes that previously configured custom colours won't be migrated, unfortunately.
Comment 41•3 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #28)
Stripping the folderCache now would require way more work since we're using that to handle all our folder properties.
I'll open a follow up bug to investigate this approach and improve it.
Totally agree that it should not be fixed in this bug, but moving the entire folder cache infrastructure to the same JSON file is probably the right way to go.
Comment 43•3 years ago
|
||
sguarin,
Please check this candidate build this weekend and tell us if the situation improves.
https://archive.mozilla.org/pub/thunderbird/candidates/79.0b3-candidates/build1/
Assignee | ||
Comment 44•3 years ago
|
||
(In reply to Jorg K (CEST = GMT+2) from comment #36)
Can you do a Windows 64bit opt ESR build for me to try, please.
I don't know how to do an ESR build.
Is it the candidate that Wayne posted above?
Reporter | ||
Comment 45•3 years ago
|
||
Well, I'm offering to test this in order to avoid a "take 3" here. It would be nice if the TB team were able to provide an ESR build. I'm sure someone in the team knows.
Reporter | ||
Comment 46•3 years ago
|
||
What happened to posting changesets for landing into the bug?
Kai, if you land on beta or ESR, please don't forget to post the changesets accordingly. From some spot checks, that was omitted for this push:
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=e5db4835eac70659001df518050e793a450ba007
Rob, how is this organised these days, it's not working reliably?
For the record:
https://hg.mozilla.org/releases/comm-beta/rev/5bff797de075928fbb660cf9197890d0e3701663
Comment 47•3 years ago
•
|
||
Rob is finishing off moving and Geoff is PTO, so I asked Kai to fill in late Friday night to do the uplifts so we can get beta patches out in time for doing 78.1 - he graciously agreed and I suggested he could defer the cset posting and flag changes for Rob. They will be taken care of in due course.
Reporter | ||
Comment 48•3 years ago
|
||
Sigh, this already got pushed to ESR78, again, without noting the changeset in the bug :-(
https://hg.mozilla.org/releases/comm-esr78/rev/a2f92932c5462d55dcfa0a20750e7ac059aecb7a
Alex, I can just grab a build off the c-esr78 tree.
Reporter | ||
Comment 49•3 years ago
|
||
Seriously, this is not proper process. Besides, it was done in this push
https://treeherder.mozilla.org/#/jobs?repo=comm-esr78&revision=96737f97b69731aa8b805ee07bb408fc504bc53f - Sat, Jul 25, 03:40:40
but not this one:
https://treeherder.mozilla.org/#/jobs?repo=comm-esr78&revision=d8d6256308094b6684d141895ef6e8f988ac8138 - Fri, Jul 24, 18:45:44
(and apparently the respective beta ones).
Comment 50•3 years ago
|
||
Win64 try build of esr78 tip coming here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d604e0fb3f456d5daaa69ac6120c360d1933e9f0
(This and some other bugs are indeed landed on esr78 already)
Reporter | ||
Comment 51•3 years ago
|
||
Thanks, but as I said in comment #48, I just grabbed a build from the ESR 78 tree. Care to cancel your try?
Result is: It now starts in about 3.5 seconds instead of 25, and there is no unresponsive script warning.
(Would have been nice if you had fixed bug 1653545, so I could actually use TB 78 :-()
Comment 52•3 years ago
|
||
bugherderuplift |
Thunderbird 79.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/5bff797de075
Updated•3 years ago
|
Comment 53•3 years ago
•
|
||
bugherderuplift |
Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/a2f92932c546
![]() |
||
Comment 54•3 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #28)
What slowness are you observing?
I don't know, there is a multiminute hang of TB after startup, but it is after the folder pane is shown. So it is some other bug.
- There is no migration yet. If I had colors set in the current folder databases, they are lost (not used) after the patch.
Unfortunately, that's something we can't do.
If users are experiencing slow downs due to the amount of folders, looping through them all to transfer the colours from the old method will cause the same performance issue during migration.
I think this is a trade off we're forced to make.
It would be a one-time slowness to do the migration. But I'm fine with it, if this can land in TB78 soon enough before users start to use the colors in .msf files.
- The newly used json file is NOT a cache (as written in the other comments), it is the single source of this information (folder colors and open state). It could get reset easily. Also, colors stored in this file are not migrated if a folder (the mbox file and its msf) is moved in the profile to a different position (power-users and devs may do that). The folder URI changes and the color is not found (stale entries may stay in the json file, but that probably happens with the open states too). When the color attribute is in the .msf if moves with the folder is not lost. So this behaviour needs UX decision if the color information is less important (disposable) than the other info in the .msf file.
Thanks for this overview. What about comment 6 regarding using MSF files?
I probably never experienced .msf file loss, but I know some users get them lost sometimes. But it is a matter of principle. We try to fix losing data from .msf files. But losing the color or open state from the .json file is accepted as 'by design'.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Description
•