1660557 - Themes shouldn't be in resource:///modules

Status: RESOLVED FIXED

(Firefox :: Theme, task, P3)

Description

[Mike Hommey [:glandium]]

My guess is they were moved there when moving off LWTs because they wouldn't really fit in chrome or components, and modules was the least worst option in what's left in resource:///. Of course, another option would have been to add a directory, but that requires manual intervention to the packager to put in omni.ja (at the end of python/mozbuild/mozpack/packager/formats.py).

In any case, installing the theme files via EXTRA_JS_MODULES is a misuse of the variable.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a defect, but please change it back in case of error.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1660557 - stop using js_modules to pack builtin and default themes, r?glandium!,zombie!

This is ultimately less build magic and makes for more meaningful URIs.

As a driveby, I also removed the exception for the alpenglow stuff in all_files_referenced.js,
and made it cope with images referenced from manifests the way it already does for background
scripts and so on.

This patch paves the way for the second patch in this stack which fixes bug 1655456.

Comment 4

[Florian Quèze [:florian]]

(In reply to :Gijs (he/him) from comment #3)

Created attachment 9173614 [details]

As a driveby, I also removed the exception for the alpenglow stuff in all_files_referenced.js,
and made it cope with images referenced from manifests the way it already does for background
scripts and so on.

I r+'ed a patch for this in bug 1659871, so you may want to coordinate with dmose to see which of the 2 patches will land this change.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/752b189cd836
stop using js_modules to pack builtin and default themes, r=glandium,zombie

Comment 6

[Richard Marti (:Paenglab)]

When I want to switch to one of the built-in themes I get a NS_ERROR_FILE_NOT_FOUND because it still searches in modules. Is there something I need to delete in the profile? If yes, what needs to be deleted and is this needed for every user?

Comment 7

[:Gijs (he/him)]

(In reply to Richard Marti (:Paenglab) from comment #6)

When I want to switch to one of the built-in themes I get a NS_ERROR_FILE_NOT_FOUND because it still searches in modules. Is there something I need to delete in the profile? If yes, what needs to be deleted and is this needed for every user?

I don't know, but this sounds bad. I'm guessing the add-on cache needs invalidating or something - :zombie ?

Comment 8

[Tomislav Jovanovic :zombie]

(In reply to Richard Marti (:Paenglab) from comment #6)

When I want to switch to one of the built-in themes I get a NS_ERROR_FILE_NOT_FOUND because it still searches in modules. Is there something I need to delete in the profile? If yes, what needs to be deleted and is this needed for every user?

I don't understand what you're asking? Are you building locally with this patch, and restarting Firefox with the same profile, and the theme is broken? What exactly is the file not found? Can you post the whole log from (presumably) the browser console?

Comment 9

[Richard Marti (:Paenglab)]

I get this error:
Error while loading 'file:///Z:/Mozilla/FF/dist/bin/modules/themes/default/manifest.json' (NS_ERROR_FILE_NOT_FOUND) Extension.jsm:600

And yes, a local FX build with the patch applied. When starting with an existing profile I get the errors above for all built-in themes. Additionally installed themes still work.

Comment 10

[Alexandru Michis [:malexandru]]

Backed out changeset 752b189cd836 (Bug 1660557) for causing xpcshell failures in test_prefs_store.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/729f9618019d7fc623ec63e50810fc5de9d4147b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=xpcshell&fromchange=10fc7701db1b35525251495dfa5b5d8c7570e5e8&tochange=729f9618019d7fc623ec63e50810fc5de9d4147b

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315099703&repo=autoland&lineNumber=7707

Comment 11

[:Gijs (he/him)]

(In reply to Richard Marti (:Paenglab) from comment #9)

I get this error:
Error while loading 'file:///Z:/Mozilla/FF/dist/bin/modules/themes/default/manifest.json' (NS_ERROR_FILE_NOT_FOUND) Extension.jsm:600

And yes, a local FX build with the patch applied. When starting with an existing profile I get the errors above for all built-in themes. Additionally installed themes still work.

I'm guessing we should fix this before relanding. Am I right in thinking we should invalidate the cached add-on info in the profile? How do we normally accomplish that (without breaking caching forever going forward)?

Comment 12

[Tomislav Jovanovic :zombie]

So yeah, we cache the manifest of installed addons. I never done this, but I expect the cleanest way to do this would be to bump the versions of themes when we maybeInstall them:
https://searchfox.org/mozilla-central/rev/76a83d0a21/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2897

Comment 13

[:Gijs (he/him)]

(In reply to Tomislav Jovanovic :zombie from comment #12)

So yeah, we cache the manifest of installed addons. I never done this, but I expect the cleanest way to do this would be to bump the versions of themes when we maybeInstall them:
https://searchfox.org/mozilla-central/rev/76a83d0a21/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2897

I tried this, and although it appears to fix things if you're using the default theme when updating (and then try to use one of the builtin themes), if you're using the "old" version of one of these themes before updating then we try to just load the theme from the old location, and that doesn't work. :zombie said he could try to look at this, so I've updated the patch and I'm going to needinfo here. :-)

Comment 14

[Tomislav Jovanovic :zombie]

So we have several levels of fail here:

1. The root problem is that at AddonManager startup, we're trying to load previously installed builtin theme that's not there anymore before the call to maybeInstallBuiltin in BrowserGlue.

2. The second (minor) problem is that, as I mentioned above, we're supposed to cache the manifest, but we still try to read one from the old location. This is the error as reported by Richard above. (We can probably track this down, but shouldn't depend on cache being there).

3. The biggest issue is with the code that's supposed to remedy Problem 1, by enabling the LWT if it was previously selected. Unfortunately, it's reading the wrong pref, which was supposed to be fixed in bug 1540856 Part 2, but this instance was missed.

4. With the issue 3 corrected, the active theme is preserved, though not applied on first start after this patch. This is an improvement obviously (no data loss), but still need to fix this, probably in LWT handling code somewhere.

5. Once we fix problem 4, one last potential issue might be a flash of default or incomplete theme on first startup after this patch. Our current startup setup might not allow us to avoid this without major refactoring or some ugly hard-coding.

Comment 15

[Tomislav Jovanovic :zombie]

Gijs suggested invalidating the addons cache in BrowserGlue might fix issue (1) and avoid all others that follow. I tried that before discovering issue (3), and it obviously didn't work because the preservation of the active LWT was broken.

But a similar invalidation by bumping the SCHEMA_DB (which is checked earlier in startup before we get to BrowserGlue), along with a fix for (3) did work. So, just these two changes are needed:

in XPIInstall.jsm

const PREF_SELECTED_LWT = "extensions.activeThemeID";

and in XPIProvider.jsm

const DB_SCHEMA = 33;

As I anticipated, we do get a brief flash of default theme on first update, especially visible if the Dark theme was active, but I'm not sure that's so bad to warrant some ugly hacking or hard coding.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8846ec72f7c4
stop using js_modules to pack builtin and default themes, r=glandium,zombie

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8846ec72f7c4