While investigating bug 1830136, I found that there is an unconditional assignment extension.startupData = { that is saved to addonStartup.json.lz4 in ext-theme.js).

The logic is intended to only cover static themes: startupData is only used in onManifestEntry. This only runs for the theme manifest key, because this is the only manifest key registered for the module in toolkit.json (I am mentioning this explicitly, because the theme_experiment manifest key is also specified for extension manifests).

However, dynamic themes also trigger the same logic. startupData is explicitly not passed to the Theme constructor, and consequently the startupData of the extension always gets overwritten (link to ext-theme.js with all relevant links highlighted).

Overwriting startupData for non-themes is a problem, because extension.startupData is also used for other extension features (notably persistent listeners).

We should:

• Fix this bug by not using/overwriting startupData for dynamic themes (only static themes, i.e. extension.type === "theme").
• Add migration logic to drop unsupported startupData entries (i.e. lwtData, lwtStyles, lwtDarkStyles, experiment) from extensions. Alternatively, add logic to only keep supported properties.
  • This could be considered along with bug 1830136, because that bug also needs migration logic to fix up bad entries.
• Add unit test. At least for the migration logic, and optionally also for this bug (i.e. that calling theme.update does not change startupData).

Fix along with bug 1830136, so that we only need migration logic once.

Stop using startupData.lwtData for extensions that are not static
themes, because this feature was designed for use by static themes
only.

Also added test coverage that explicitly confirms when
startupData.lwtData is used and when not.

Add comments explaining the role of startupData, and explicitly assign
properties from startupData instead of Object.assign to make it obvious
which properties are intended to be read (and to also make it impossible
to overwrite properties that should not be overwritten).

Note: although I previously mentioned the possibility of data migration to delete obsolete startupData entries, I decided not to, as explained at https://phabricator.services.mozilla.com/D214833#7377465. Copy-pasted below:

Note: unlike the other bug I did not implement "data migration" in the form of deleting obsolete startupData entries. The reason for that is simplicity, and that the number of affected extensions is small, and that the data will automatically disappear when an extension updates.

The following extensions have 10k+ users, the "theme" permission and call theme.update. Unless stated otherwise, they are regularly updated, so the issue will go away on its own.

• Dark Reader
• Firefox Color
• Turn Off the Lights
• ColorfulTabs (Apr 12, 2021)
• automaticDark (Feb 23, 2023)
• Midnight Lizard (Jan 20, 2021)

There are 9 other extensions with 1k - 10k users that I didn't check, but based on the numbers and the minimal impact I decided to not add migration code for this case.

https://hg.mozilla.org/mozilla-central/rev/c939006a7aed
https://hg.mozilla.org/mozilla-central/rev/31a0a029f6a5
https://hg.mozilla.org/mozilla-central/rev/ac092434fee4

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

Stop using startupData.lwtData for extensions that are not static
themes, because this feature was designed for use by static themes
only.

Also added test coverage that explicitly confirms when
startupData.lwtData is used and when not.

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

Add comments explaining the role of startupData, and explicitly assign
properties from startupData instead of Object.assign to make it obvious
which properties are intended to be read (and to also make it impossible
to overwrite properties that should not be overwritten).

And add test coverage verifying that after a restart, the stored theme
is actually applied, for the light and dark theme, because such test
coverage was missing before.

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

esr128 Uplift Approval Request

• User impact if declined: (D216990 D216991 D216992) Extension state may be lost if theme.update() is called. This can result in extensions being unable to intercept network requests at early startup, and cause extensions that don't need to start up early to start up early at startup.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Covered by automated tests.
• Risk associated with taking this patch: None
• Explanation of risk level: Fixes unintended behavior by patching the unintended behavior in the specific place in the code, with no unwanted side effects. Plus fully covered by automated tests.
• String changes made/needed: None
• Is Android affected?: no