1652279 - Starting TB 78 on a profile with many folders gives unresponsive script error - take 2

Status: VERIFIED FIXED

(Thunderbird :: Folder and Message Lists, defect, P1)

Description

[Jorg K (CEST = GMT+2)]

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

Comment 1

[Jorg K (CEST = GMT+2)]

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

[Wayne Mery (:wsmwk)]

SSD or spinning disk? (Not relevant to the bug, just curious)

Comment 3

[Alessandro Castellani [:aleca]]

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?

Comment 4

[Jorg K (CEST = GMT+2)]

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.

Comment 5

[Alessandro Castellani [:aleca]]

(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

[Wayne Mery (:wsmwk)]

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

Comment 7

[Jorg K (CEST = GMT+2)]

Actually, the most hangs are here: Script: chrome://messenger/content/folderPane.js:1284

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1652279-folder-color.diff

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.

Comment 9

[Alessandro Castellani [:aleca]]

Mh, maybe I should use the _modeNames instead of a custom kColorMode 🤔️

Comment 10

[Alessandro Castellani [:aleca]]

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.

Comment 11

[Alessandro Castellani [:aleca]]

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.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9164138 [details] [diff] [review]
1652279-folder-color.diff

Sorry, not my field of expertise.

Comment 13

[Magnus Melin [:mkmelin]]

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)

Comment 14

[Alessandro Castellani [:aleca]]

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

[Magnus Melin [:mkmelin]]

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

[alta88]

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.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9164138 [details] [diff] [review]
1652279-folder-color.diff

That f+ got set inadvertently :-(

Comment 18

[Alessandro Castellani [:aleca]]

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.

Comment 19

[Alessandro Castellani [:aleca]]

Attachment: The other version is better yes

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,
});

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: 1652279-folder-color-V2.diff

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

[alta88]

(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

[Magnus Melin [:mkmelin]]

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

[Magnus Melin [:mkmelin]]

(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

[Magnus Melin [:mkmelin]]

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

Comment 26

[Alessandro Castellani [:aleca]]

Attachment: 1652279-folder-color.diff

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

[:aceman]

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

Comment 28

[Alessandro Castellani [:aleca]]

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?

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

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.

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.

Comment 29

[Alessandro Castellani [:aleca]]

Attachment: 1652279-folder-color.diff

Comment 30

[Magnus Melin [:mkmelin]]

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

Comment 31

[Alessandro Castellani [:aleca]]

Attachment: 1652279-folder-color.diff

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

[alta88]

(In reply to :aceman from comment #27)

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.

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.

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: 1652279-folder-color.diff

Found a small typo.

Comment 35

[Magnus Melin [:mkmelin]]

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.

Comment 36

[Jorg K (CEST = GMT+2)]

Can you do a Windows 64bit opt ESR build for me to try, please.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1271811d565a
Fix startup performance issues with folder colour customization. r=mkmelin

Comment 38

[Magnus Melin [:mkmelin]]

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

[Wayne Mery (:wsmwk)]

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.

Comment 40

[Alessandro Castellani [:aleca]]

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

[Joshua Cranmer [:jcranmer]]

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

[Jorg K (CEST = GMT+2)]

See comment #36.

Comment 43

[Wayne Mery (:wsmwk)]

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/

Comment 44

[Alessandro Castellani [:aleca]]

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

Comment 45

[Jorg K (CEST = GMT+2)]

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.

Comment 46

[Jorg K (CEST = GMT+2)]

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

[Wayne Mery (:wsmwk)]

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.

Comment 48

[Jorg K (CEST = GMT+2)]

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.

Comment 49

[Jorg K (CEST = GMT+2)]

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

[Magnus Melin [:mkmelin]]

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)

Comment 51

[Jorg K (CEST = GMT+2)]

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

[Rob Lemley [:rjl]]

Thunderbird 79.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/5bff797de075

Comment 53

[Rob Lemley [:rjl]]

Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/a2f92932c546

Comment 54

[:aceman]

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

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

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

[Wayne Mery (:wsmwk)]

anyone who is currently using BETA please try https://archive.mozilla.org/pub/thunderbird/candidates/80.0b3-candidates/build1/ and report your results

Comment 57

[Wayne Mery (:wsmwk)]

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

anyone who is currently using BETA please try https://archive.mozilla.org/pub/thunderbird/candidates/80.0b3-candidates/build1/ and report your results

ignore that