nsMessageManagerScriptExecutor holds a script cache which is only purged on child process shutdown. If an addon uses loadFrameScript and gets reloaded, it will never be able to execute the updated frame script. AddonManager fires various observer notifications when reinstalling an addon. We may listen for existing "chrome-flush-caches" notification and purge this cache accordingly. The addon manager would have to fire this in all processes, but at least it will be possible to purge this cache somehow.

Hmm, I guess in theory we could purge all the cached scripts and then just next new tab would be a tad slower to create. But would it use also more memory, not sure. Can we somehow detect which scripts are from addons and purge only those?

(In reply to Olli Pettay [:smaug] from comment #1) > Hmm, I guess in theory we could purge all the cached scripts and then just > next new tab would be a tad slower to create. But would it use also more > memory, not sure. Oh I didn't thought about that. But I think chrome-flush-caches is already something that makes firefox slow. I wouldn't expect it to be fired on any regular addon install but only when re-installing one. But that's something to be aware and carefully reviewed. > Can we somehow detect which scripts are from addons and purge only those? It's not that easy as addons may have arbitrary URLs chrome://myaddon or resource://myaddon. Would that work for you if we fire this cache cleanup only when working on an add-on? (and what about also doing that when updating addons?)

So there is removeDelayedFrameScript. Could we use that and propagate that information also to child side to remove cache scripts?

Yes, we could do that. It would be up to each addon to call such cleanup method for each of its frame script. So, are you suggesting that removeDelayedFrameScript would also purge the cache for non-delayed frame script? Because in my case, the script being cached is a non delayed one.

ah, we'd need some other method then I guess, or use the notification. FWIW, the caching was added in bug Bug 584866. Alexandre, want to test whether everything works still just fine if all the scripts are purged when chrome-flush-caches is fired? Adding or updating addons shouldn't happen too often, so it might not be too bad. And we already do have the issue with XBL protos to show up in CC graph after addon updates, IIRC. Currently nsScriptCacheCleaner observes only xpcom-shutdown, but it could observe also other notifications. (nsMessageManagerScriptExecutor::Shutdown should be renamed if it starts to observe more stuff)

But I sent the message manually from an addon and it works. And the AddonManager patch to send this event to child processes. I haven't really played with this patch. I would need some careful review of this to ensure it's not sending the event on regular install/startup. Waiting for try results first.

Actually, previous patch was wrong and dispatching the event only in some cases during startup. Here is a more agressive patch which sends this event whenever we are purging startup cache, which, I think it to be sent for the same reasons. So we may purge cache more frequently, but overall it is being dispatch when addons are uninstalled. It seems to be also fired during startup on some conditions. Mossop, any feedback about this? I'm wondering if we should or should not purge message manager automatically? It may bite addons during updates... If an addon uses a frame script and updates it. We could also start with just flushing caches from about:debugging, via the reload button :kumar is working on (bug 1246030).

Comment on attachment 8733317 [details] [diff] [review] Fire chrome-flush-caches in child processes to purge message manager caches. Review of attachment 8733317 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm @@ +1503,5 @@ > Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageBroadcaster) > .broadcastAsyncMessage(MSG_JAR_FLUSH, aJarFile.path); > } > > +function flushCaches() { This isn't a great name since we have another cache to flush above it that isn't included. Maybe flushChromeCaches? @@ +1512,5 @@ > + Services.ppmm.loadProcessScript("data:,new " + function () { > + let obs = Components.classes["@mozilla.org/observer-service;1"] > + .getService(Components.interfaces.nsIObserverService); > + obs.notifyObservers(null, "chrome-flush-caches", null); > + }, false); We already have a script running in content processes (toolkit/mozapps/extensions/internal/Content.js). Just broadcast a message to that like above. @@ +2633,5 @@ > Services.obs.addObserver(this, NOTIFICATION_TOOLBOXPROCESS_LOADED, false); > } > } > > + let doFlushCaches = this.checkForChanges(aAppChanged, aOldAppVersion, This change looks unnecessary so let's leave it alone.

I have some concerns about this patch: * fires chrome-flush-caches on every uninstall and on startup when firefox is upgraded. As :smaug highlighted, it will reduce the performances of the next opened tab... We should only clean caches when required. Addon update sounds fine to me, but other scenarios shouldn't slip in!! * fires chrome-flush-caches also in parent process: My test highlighted it was necessary to update the frame script. (browser mochitest runs without e10s) But it will also flush various other things. May be that was some resources we didn't correctly updated after update? Or may be all other cleanup associated with chrome-flush-caches are useless? In that case, we should be using a specific event to cleanup message manager cache!

Also, I had to unsign the test addons. Help is welcomed to resign them! That's really not handy to have to sign test addons :/

We do we want to flush caches during startup? The scripts are just cached when first time loaded. And yes, parent needs the same notification since there are in-process message managers.

(In reply to Olli Pettay [:smaug] from comment #15) > We do we want to flush caches during startup? The scripts are just cached > when first time loaded. I'm adding a flush right here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2648 It's during startup but in middle of addon manager startup. I imagine we already loaded various addons and may update some new? I don't know for sure.

well, could we flush only in case we actually update addons during startup. Hmm, perhaps I misunderstood "fires chrome-flush-caches on every uninstall and on startup when firefox is upgraded." I thought that was only about FF update, but less so about addons being updated during startup.

(In reply to Olli Pettay [:smaug] from comment #17) > well, could we flush only in case we actually update addons during startup. > Hmm, perhaps I misunderstood **"fires chrome-flush-caches on every uninstall** I don't think it will be easy to have a callsite within the addon manager for when we only update addons. But I would be happy to be mentored by mossop about that! So I just flush the cache on every addon uninstall. As we do for startup cache flush. The important scenario here is when we update an addon, not really when we startup firefox. This cache isn't saved to disk, right? So we mostly need to purge cache when we upgrade an addon so that its frame script eventually get update when its new version gets run. > and **on startup when firefox is upgraded.**" This is here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2648 When `flushCache` boolean ends up being true. This is a complex scenario but happens on startup when firefox is upgraded, but I can't describe it more precisely. It seems to involve the dialog some sometimes happens to update your addons when upgrading firefox. It may only happen when we actually do update at least one addon... We may just remove this precise callsite.

(In reply to Alexandre Poirot [:ochameau] from comment #18) > (In reply to Olli Pettay [:smaug] from comment #17) > > well, could we flush only in case we actually update addons during startup. > > Hmm, perhaps I misunderstood **"fires chrome-flush-caches on every uninstall** > > I don't think it will be easy to have a callsite within the addon manager > for when we only update addons. But I would be happy to be mentored by > mossop about that! > So I just flush the cache on every addon uninstall. As we do for startup > cache flush. The call-site you link to is only called when the add-ons manager detects a change in the set of installed add-ons during startup or if the compatibility UI has been shown (which is during some Firefox upgrades). The latter is necessary because that UI is shown with the default theme but then we want any installed theme or locale to take effect for the rest of the run. We might be able to reduce the flushes there by checking if there are non-default theme or locales active. But then I'm pretty sure we already flush the startup cache for a Firefox upgrade anyway since we expect for previously cached files to have changed. > The important scenario here is when we update an addon, not really when we > startup firefox. > This cache isn't saved to disk, right? So we mostly need to purge cache when > we upgrade an addon so that its frame script eventually get update when its > new version gets run. Yes, the cache is saved to disk and used everytime you run Firefox.

(In reply to Dave Townsend [:mossop] from comment #19) > The call-site you link to is only called when the add-ons manager detects a > change in the set of installed add-ons during startup or if the > compatibility UI has been shown (which is during some Firefox upgrades). The > latter is necessary because that UI is shown with the default theme but then > we want any installed theme or locale to take effect for the rest of the > run. We might be able to reduce the flushes there by checking if there are > non-default theme or locales active. But then I'm pretty sure we already > flush the startup cache for a Firefox upgrade anyway since we expect for > previously cached files to have changed. If it's all about themes and locales. And no addons are being involved, then there is no point in flushing the message managers. Also, I imagine if an addon is upgraded, the other call to flushChromeCaches on various uninstall call-sites are going to flush the caches. > > The important scenario here is when we update an addon, not really when we > > startup firefox. > > This cache isn't saved to disk, right? So we mostly need to purge cache when > > we upgrade an addon so that its frame script eventually get update when its > > new version gets run. > > Yes, the cache is saved to disk and used everytime you run Firefox. I imagine you talk about the xul startupcache. The one I'm trying to flush, frame scripts in the message manager seems to be transcient: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp#1627

(In reply to Alexandre Poirot [:ochameau] from comment #20) > (In reply to Dave Townsend [:mossop] from comment #19) > > The call-site you link to is only called when the add-ons manager detects a > > change in the set of installed add-ons during startup or if the > > compatibility UI has been shown (which is during some Firefox upgrades). The > > latter is necessary because that UI is shown with the default theme but then > > we want any installed theme or locale to take effect for the rest of the > > run. We might be able to reduce the flushes there by checking if there are > > non-default theme or locales active. But then I'm pretty sure we already > > flush the startup cache for a Firefox upgrade anyway since we expect for > > previously cached files to have changed. > > If it's all about themes and locales. And no addons are being involved, then > there is no point in flushing the message managers. > Also, I imagine if an addon is upgraded, the other call to flushChromeCaches > on various uninstall call-sites are going to flush the caches. Maybe, I'd need to check that. > > > The important scenario here is when we update an addon, not really when we > > > startup firefox. > > > This cache isn't saved to disk, right? So we mostly need to purge cache when > > > we upgrade an addon so that its frame script eventually get update when its > > > new version gets run. > > > > Yes, the cache is saved to disk and used everytime you run Firefox. > > I imagine you talk about the xul startupcache. > The one I'm trying to flush, frame scripts in the message manager seems to > be transcient: > > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameMessageManager. > cpp#1627 Right, but you're doing that from the same method right now. Sounds like you should separate out the cache you want to flush and call it separately at the appropriate times if you don't want to flush whenever we flush the startup cache.

Comment on attachment 8733988 [details] [diff] [review] Fire chrome-flush-caches to update message manager cached scripts - v2 Dropping review based on my understanding of the previous comments, re-ask if I'm misunderstanding.

Ok. let's use a specific notification just for that, so that the addon manager can more precisely flush just this cache.

Ok. I no longer attempt to flush message manager cache on the startup codepath (XPIProvider.startup). Instead I only fire startupcache invalidation in this particular call site. Then, all other call sites where we uninstall an addon, I now clean both startup cache and message manager. To be clear, it is not only when we update an addon. It would be significantly harder to fire this only when we do that as there is many places and combinations in XPIProvider*.js where that can happen. There is not test in the first patch, but we cover it at a very high level with an addon update test.

https://hg.mozilla.org/integration/fx-team/rev/4653c5c78f68b6a437444815de7ed04048f23745 Bug 1258496 - Purge message manager cached scripts on 'message-manager-flush-caches' notification. r=smaug https://hg.mozilla.org/integration/fx-team/rev/6f51002d4589638e0120681f8de5512c48bb5155 Bug 1258496 - Fire message-manager-flush-caches to update message manager cached scripts. r=mossop

https://hg.mozilla.org/mozilla-central/rev/4653c5c78f68 https://hg.mozilla.org/mozilla-central/rev/6f51002d4589