1746841 - moz-extension:// CSS urls cached across addon reloads and updates

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[erosman [:erosman]]

ref: Nightly 97.0a1 (2021-12-19) (64-bit)

Recently, Firefox Nightly caches browserAction Pop-up CSS. While it improves the performance for the normally installed extensions, it hinders extension development.

Extension developers, working on unpacked add-ons, often need to make regular changes to the add-on pop-up during devotement. Currently, the changes are not reflected and hence the dilemma.

In fact, a display issue that occurred due to CSS rule in a pop-up, could not be easily identified since the changes where not applied. At the time, I was not aware of the caching situation therefore, could not pinpoint the problem (and filed a bug for it 🤦‍♂️).

STR

• Unpack install any basic add-on with browserAction Pop-up e.g. beastify
• Make changes to the pop-up CSS i.e. choose_beast.css

Suggestion
Disable CSS caching for the unpacked extension only

Note
mozregression doesn't run on my set-up

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

STR

• Unpack install any basic add-on with browserAction Pop-up

Was it installed temporarily?
If it was not installed temporarily, then what you describe is the expected behaviorl.

Comment 2

[erosman [:erosman]]

Was it installed temporarily?

No... permanently but unpacked.

If it was not installed temporarily, then what you describe is the expected behavior.

Until recently, changes to the pop-up CSS would show in the next pop-up launch.

Now, the changes do not show which prevents updates to the design of the pop-up during the development. There is no force-refresh in browserAction Pop-up.

What would be the option?
It is not practical to reload the extension on every CSS change during development?

Comment 3

[Tomislav Jovanovic :zombie]

It is not practical to reload the extension on every CSS change during development?

We already provide this through the web-ext tool, closing as wontfix.

Comment 4

[erosman [:erosman]]

We already provide this through the web-ext tool, closing as wontfix.

This is not the case.
While about:debugging -> extension -> Inspect -> Disable Pop-up Auto-hide -> popup.html -> Style Editor can be used to edit & view changes live, it only applies to that launched pop-up.

In other words...

• Open Toolbox -Extension
• Make CSS changes
• View with correct content
• Deselect Disable Pop-up Auto-hide
• Launch pop-up to view result with newly created dynamic content
• Pop-up is shown with old CSS

Unless, extension is reloaded/reinstalled, the CSS doesn't update and the changes made via web-ext tool are not applied to new pop-ups.
That was the purpose if this bug filing.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

I'm reopening this with the following motivations:

• I verified that in Firefox >= 96 the stylesheet being used is the cached one even after reloading the addon (either manually from about:debugging, or done automatically by web-ext on extension file changes)
• but also way more concerning than that it looks that the cached css is also being cached across addon updates

Bisecting the change in behavior using mozregression gives the following result:

• https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d08c33b14cc304240a02efacaa19ea832b01d5e0&tochange=62d3854e2849832119ea9611c16b7fbc781f5497

from that group of patches, I'm pretty sure that the patch that introduced this change in behavior is the following one part of Bug 1743377:
https://hg.mozilla.org/integration/autoland/rev/62d3854e2849832119ea9611c16b7fbc781f5497#l1.14

Another details that I did notice is that the issue does not happen if the extension doesn't have a background page (I suspect that it may be because without a background page the popup becomes the only extension document and it is transient, because we destroy it when the popup is being closed, and that may be clearing the cached css entry as a side effect).

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-cached-css-updates-0.1.zip

test extension v1 (browserAction popup expected to have a red background).

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-cached-css-updates_v2-0.2.zip

test extension v2 (update to the v1 test extension, browserAction popup expected to have a green background).

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

STR:

• create a new empty Firefox profile
• run Firefox >= 96 non-release build (either Nightly, Beta or a non branded Firefox 96 build) on that profile
• set the about:config pref "xpinstall.signatures.required" to false
• open an about:addons tab and install test-cached-css-updates-0.1.zip non temporarily
• open the browserAction popup and confirm that its background is red
• install test-cached-css-updates_v2-0.2.zip non temporarily
• expected:
  • open the browserAction popup and confirmed that its background is green and the popup document content of the updated extension
• actual:
  • open the browserAction popup and got a popup background still red (but the popup document content of the updated extension)

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Emilio,
Given that 96 beta is affected (and "addon updates" seems to also be affected scenario), at a first glance removing just the || aURI->SchemeIs("moz-extension") part from https://hg.mozilla.org/integration/autoland/rev/62d3854e2849832119ea9611c16b7fbc781f5497#l1.14 seems potentially a smaller and less risky upliftable fix, but I may be wrong and I wanted to double-check your perspective about the issue (and if there are other side-effects that we should take into account, e.g. I haven't explicitly checked what would happen with cached images).

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

For a longer term approach we may want to evaluate if we would like to bring back that cached entries, I was wondering if nsIClearDataService could be used to explicitly clear cache entries as part of the "temporary extension reloading" and "addon updates" paths, and so I gave that a quick try and the following snippet does clear the cached css (along with some other other potential cache entries that may also be reasonable to clear across addon reloads, e.g. image cache):

Services.clearData.deleteDataFromPrincipal(
  WebExtensionPolicy.getByID(ADDON_ID).extension.principal,
  false,
  Ci.nsIClearDataService.CLEAR_ALL_CACHES,
  () => {}
);

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Images already behaved like stylesheets here, right? Removing the moz-extension bit from there would make images from extensions always get reloaded, which is unfortunate (and probably causes flickering on toolbar icons when they get restyled because they are set via CSS variables IIRC).

Backing out the regressing patch from 96 seems better, potentially, while we try to find an alternative fix. Comment 10 seems reasonable to me fwiw.

Comment 12

[Shane Caraveo (:mixedpuppy)]

IIUC, it is ok to request the patches to be backed out from 96, I also don't see the time given holidays to address this in 97. Should it be backed out entirely, and re-land in 98 with a fix along the lines that Luca suggested?

Comment 13

[Emilio Cobos Álvarez [:emilio]]

I'd prefer to not back out from nightly because I have some other image loader clean-up and so that I want to get to. Actually, I'll land a workaround that should be upliftable in a dependent bug.

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-cached-css-updates-0.1.zip

test extension v1 (browserAction popup expected to have a red background and an image with a red puzzle over a white background).

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-cached-css-updates_v2-0.2.zip

test extension v2 (update to the v1 test extension, browserAction popup expected to have a green background and an image with a green puzzle over a white background).

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

In comment 14 and comment 15 I added an image to the test extension and its upgrade (a red puzzle piece in the first version and a green puzzle piece in the "extension update" version) and it looks that the same also happens for the images, but that behavior did change a longer time ago.

And so, even if the "cached image across updates" part of the issue seems also something we would want to fix as part of this issue, it is not a new regression (and I don't personally recall us triaging any old issue about that, it is possible that none did actually notice) and so it would be fine to don't fix that part in Bug 1747091 (which is meant to only prevent the new regression on the css resource currently cached in Firefox 96).

I did run mozregression, but the change in behavior was quite old and so mozregression wasn't able to bisect further, the range got from mozregression is the following one:

• https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=786f094a30ae94722246f078561928990ab1e0b7&tochange=1ae707852b608ea77dc82c892f25e169cbc316b5

Anyway, the results of mozregression tell us that in Firefox 67 the image from the old version was replaced by the new one after an addon update, while in Firefox 68 that wasn't already the case.

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1746841 - Add a test case to verify that css cached for moz-extension urls are not used after the addon is updated. r?mixedpuppy!

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Setting the leave-open keyword, because we may land the test case earlier (along or right after the Emilio's fix from Bug 1747091) but we still want to followup with a proper longer term fix as part of this bugzilla issue.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff6e1b951fa6
Add a test case to verify that css cached for moz-extension urls are not used after the addon is updated. r=mixedpuppy

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1746841 - Explicitly clear cached resources associated to an extension being installed/upgraded/downgraded or installed temporarily. r?mixedpuppy!

This patch contains a proposed approach for the longer term fix we agreed on (see Bug 1746841 comment 10 and comment 11) and replace the
mochitest-browser added in the other patch attached to this bug with an xpcshell test (which besides covering the same base scenario
that mochitest-browser was convering, it is also lighter and covers also other cases that were not explicitly covered yet).

TODO List:

• [x] double-check (and possibly confirm with an appropriate peer) that nsIClearDataService.CLEAR_ALL_CACHES (defined in nsIClearDataService.idl as
  CLEAR_NETWORK_CACHE | CLEAR_IMAGE_CACHE | CLEAR_CSS_CACHE | CLEAR_PREFLIGHT_CACHE) does clear the cached resource we need and nothing more than that,
  otherwise replace it with just the specific ones we really need moved to Bug 1750053 follow up task.

Comment 21

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/ff6e1b951fa6

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

The recent regression (related to css cache) introduced in Firefox 96 has been fixed in Bug 1747091 (by temporarily reverting caching css resources got from a moz-extension url), and so I'm updating the tracking flags accordingly.

This bug will stay open until we land the long term fix drafted in the patch attached in comment 20 (+ reverting the short term workaround landed as part of Bug 1747091), which will also fix the older regression (introduced in Firefox 68) related to cached images got from moz-extension urls.

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1746841 - Revert Bug 1747091 temporary workaround to prevent moz-extension css from being cached. r?emilio!

Depends on D134508

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba305e5abd92
Explicitly clear cached resources associated to an extension being installed/upgraded/downgraded or installed temporarily. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/a6f552923a44
Revert Bug 1747091 temporary workaround to prevent moz-extension css from being cached. r=emilio

Comment 25

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ba305e5abd92
https://hg.mozilla.org/mozilla-central/rev/a6f552923a44

Comment 26

[erosman [:erosman]]

In line with the original request, as the under-development addon issue is still an issue, I would like to request disabling CSS caching for unsigned addons, or provide an about:config entry for such.

Should a new request/bug be made for it?

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to erosman from comment #26)

In line with the original request, as the under-development addon issue is still an issue, I would like to request disabling CSS caching for unsigned addons, or provide an about:config entry for such.

Should a new request/bug be made for it?

We are clearing the cache when the addon is reloaded, that is what is meant to be used for more easily ensuring the cache is cleared during development.

Comment 28

[erosman [:erosman]]

We are clearing the cache when the addon is reloaded, that is what is meant to be used for more easily ensuring the cache is cleared during development.

Reloading of the addon occurs at Firefox restart or addon temporarily installed/reloaded.

In case of the unsigned unpacked addons, restarting Firefox with every change to CSS is not practical.

Comment 29

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to erosman from comment #28)

We are clearing the cache when the addon is reloaded, that is what is meant to be used for more easily ensuring the cache is cleared during development.

Reloading of the addon occurs at Firefox restart or addon temporarily installed/reloaded.

In case of the unsigned unpacked addons, restarting Firefox with every change to CSS is not practical.

Installing unsigned addon as unpacked is not practical by definition (e.g. requires a non-release channel build, like unbranded, beta or nightly), it may be more reasonable as a testing strategy for some specific corner cases ([1]) than as an "iterative development" strategy.

To iteratively develop and test CSS changes, a temporarily installed extension is much more practical (and, at least in my opinion, the two testing scenarios, testing CSS changes vs. testing behavior on startup, shouldn't be overlapping with each other that often).

Having said that, this bug was tracking a defect which has been fixed quite some time ago now, it is not an ideal place to discuss what is (in the best case) a low priority enhancement request.

If you think that you can motivate the request with common enough use cases (not just tied to personal preferences), file a new bug and link this as a "see also".

[1]: e.g. to specifically test extension behavior on startup which wouldn't be possible if the addon is installed temporarily, and some of the scenarios around updates, like pending updates that are completed on the next browser startup