1830136 - addonStartup.json.lz4 grows unbounded due to nested _processedColors property

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Rob Wu [:robwu]]

The patch for bug 1742918 introduced logic that copies themeData to _processedColors, i.e. themeData._processedColors = { ...themeData };, by _setProperties in LightweightThemeConsumer.

The problem with this is that themeData is a reference to a part of the object that is read from and written to addonStartup.json.lz4. As a data point: From an active profile that predates bug 1742918, I can count 328 occurrences of _processedColors. While the JSON version of the specific data amounts to 730 KB, the lz4-compressed version of the whole file (not just the specific entry) is merely 14 kb. While the impact on data read at startup is minimal due to lz4 compression, the uncompressed data still wastes memory in the parent process.

themeData in the above code snippet originates from it caller (LightweightThemeConsumer.prototype._update), and is actually themeData.darkTheme or themeData.theme, which in turn is a direct reference to the data from the "lightweight-theme-styling-update" observer (which in turn is sent from ext-theme.js).
ext-theme.js stores the data in extension.startupData (or re-uses previously stored startupData.lwtData). This data is ultimately saved in addonStartup.json.lz4, e.g. when extension.saveStartupData() is called.

Comment 1

[Rob Wu [:robwu]]

Attachment: addonStartup.json.lz4-uncompressed-pretty.json

Excerpt from addonStartup.json.lz4:

• after lz4-decompression (using tool from https://gist.github.com/Tblue/62ff47bef7f894e92ed5/raw/c12fce199a97ecb214eb913cc5d762eac2e92c57/mozlz4a.py)
• removed all entries except for the default theme
• Prettified with json_pp (i.e. with whitespace)

This file is 6468 KB. The non-prettified version is 716 KB. The lz4-compressed version of the same data would be 8 KB (the whole file is 14 KB).

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1742918

Comment 3

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1742918

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1830136 - Stop adding _processedColors to startupData of theme

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1830136 - Delete obsolete _processedColors entries

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c2ed7e7cecb
Stop adding _processedColors to startupData of theme r=willdurand
https://hg.mozilla.org/integration/autoland/rev/d831221d25c6
Delete obsolete _processedColors entries r=willdurand

Comment 7

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/7c2ed7e7cecb
https://hg.mozilla.org/mozilla-central/rev/d831221d25c6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR128 approval when you're comfortable doing so.

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1830136 - Stop adding _processedColors to startupData of theme

Original Revision: https://phabricator.services.mozilla.com/D214540

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1830136 - Delete obsolete _processedColors entries

Original Revision: https://phabricator.services.mozilla.com/D214831

Comment 11

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Unnecessarily higher disk and memory usage whenever Firefox starts up due to unnecessary data in addonStartup.json.lz4
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: QE not needed due to automatic test coverage, but if you want to test manually: Start Firefox and look up the profile directory, and the addonStartup.json.lz4 file within. It should NOT contain "_processedColors" (as seen in comment 1)
• Risk associated with taking this patch: None
• Explanation of risk level: Very targeted fix with no side effects: Fix in D216988 (LightweightThemeConsumer.sys.mjs) is trivial and covered by unit tests. D216989 introduces cleanup of old data, which may trigger a write to the add-ons database, but that already happens anyway with the current implementation (independent of what had been changed).
• String changes made/needed: None
• Is Android affected?: no

Comment 12

[Donal Meehan [:dmeehan]]

:robwu this failed to land in ESR128

https://lando.services.mozilla.com/D216992/
While applying revision D216988 to esr128, the following files had conflicts:
toolkit/modules/LightweightThemeConsumer.sys.mjs

The conflicts look to be caused by Bug 1905726.
Could you update the patch rebased onto ESR128?

Comment 13

[Rob Wu [:robwu]]

I have updated the patch, by updating the patch to an earlier version of the original patch (https://phabricator.services.mozilla.com/D214540?id=884012).

I did so by copying the diff from https://phabricator.services.mozilla.com/D214540?id=884012&download=true and pasting it in the Phabricator UI ("Update Diff"). This is the first time that I'm updating the diff this way, so please let me know if it works.

Comment 14

[Donal Meehan [:dmeehan]]

:robwu I can't land the patch
Diff does not have proper author information in Phabricator. See the Lando FAQ for help with this error.
https://wiki.mozilla.org/Phabricator/FAQ#Lando

The revision was created via the Phabricator Web UI or via an unsupported client.
Use moz-phab to submit the patch instead; see the Mozilla Phabricator User Guide for help.

Comment 15

[Rob Wu [:robwu]]

I updated it. I first tried fetching the latest revision from the patch, but it seems to not work:

$ moz-phab patch D216988 --skip-dependencies
Patching revision: D216988
A diff without commit information detected in revision D216988.
Use `--no-commit` to patch the working tree.

Then I looked in my local git reflog for the relevant commit (earlier version of the patch) and applied it (on top of ESR128 branch). Finally, I uploaded it:

$ git cherry-pick HEAD@{60}

$ git commit -v --am  # edit commit message, to change the existing "Differential Revision" to "Original Revision" + append Differential Revision 
[esr128-uplift-request 48e392f162e13] Bug 1830136 - Stop adding _processedColors to startupData of theme
 Date: Fri Jun 21 13:47:21 2024 +0200
 4 files changed, 143 insertions(+), 6 deletions(-)
 create mode 100644 toolkit/components/extensions/test/xpcshell/test_ext_theme_startupData.js

$ moz-phab uplift --single HEAD --train esr128 -i --no-wip
Couldn't find a head for esr128 in version control, submitting without rebase.
Submitting 1 commit for review
(D216988) 48e392f162e1 Bug 1830136 - Stop adding _processedColors to startupData of theme
!! Missing reviewers
Submit to https://phabricator.services.mozilla.com (YES/No/Always)? yes

Updating revision D216988:
48e392f162e1 Bug 1830136 - Stop adding _processedColors to startupData of theme

Completed
(D216988) 48e392f162e1 Bug 1830136 - Stop adding _processedColors to startupData of theme
-> https://phabricator.services.mozilla.com/D216988

Please navigate to the tip-most commit and complete the uplift request form.

(documenting it here for future reference, because the scenario of "update uplift revision" is currently not documented at https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift)

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/5669b04c826f
https://hg.mozilla.org/releases/mozilla-esr128/rev/59c79ea1898f