649029 - Disabling a restartless add-on doesn't automatically unload JS modules it loaded

Status: NEW

(Core :: XPCOM, defect)

Description

[Mike Hommey [:glandium]]

I've modified the about:startup extension to use a jsm module and register a resource:// url so that some things can be shared between the bootstrap code and the about:startup page itself. It turns out when upgrading the extension between revisions having a jsm module, the old version is always used, because it is kept in startup cache. The only way to make it get the new version is to stop Firefox, remove the startup cache and start Firefox again.

Comment 1

[Benjamin Smedberg]

Since we don't implement per-resource validation (and don't want to), it sounds like we should remove the startup cache when an extension is installed or removed. Mossop, does this sound reasonable?

Comment 2

[Dave Townsend [:mossop]]

We should already be doing this in the extension manager (by dispatching the observer notification "startupcache-invalidate"), so if that isn't happening then it's a bug.

Comment 3

[Mike Hommey [:glandium]]

Note that currently, the path used in startup cache for that extension is jsloader/resource/aboutstartup/startupdata.jsm.bin.
With bug 620931, it would use the resolved path to construct the startup cache path, e.g. jsloader/home/mh/.mozilla/firefox/g7i9o0v9.4.0/extensions/[e-mail]/startupdata.jsm.bin, which makes it easier to filter if we want to remove only them.

Comment 4

[Mike Hommey [:glandium]]

(In reply to comment #2)
> We should already be doing this in the extension manager (by dispatching the
> observer notification "startupcache-invalidate"), so if that isn't happening
> then it's a bug.

FWIW, it is a restart-less extension, maybe that's why it doesn't happen?

Comment 5

[Dave Townsend [:mossop]]

Oh, I don't think this is anything to do with the startup cache them, the scopes loaded from JS modules are cached in mozJSComponentLoader and at the moment can only be unloaded on shutdown. Any attempt to retrieve a module with the same URL will give you back the same scope. A potential solution is in bug 481603

Comment 6

[Dave Townsend [:mossop]]

Though I guess it looks like we don't invalidate the startup cache for restartless extensions either, so looks like we have two problems!

Comment 7

[Ben Bucksch (:BenB)]

mossop, was your comment 5 resolved by the fix in bug 481603?
If so, can you change the summary of this bug to "...never invalidates restart-less extensions", please?

Comment 8

[Dave Townsend [:mossop]]

(In reply to Ben Bucksch (:BenB) from comment #7)
> mossop, was your comment 5 resolved by the fix in bug 481603?
> If so, can you change the summary of this bug to "...never invalidates
> restart-less extensions", please?

Currently the restartless extension still has to manually call Components.utils.unload for all their JS modules, nothing automatic has changed here.

Comment 9

[Dave Townsend [:mossop]]

Really we are talking about JS modules, not about the startup cache here. We could in theory know which resource URIs an add-on registered and so automatically unload any JS modules that way, I'm not sure this is something worth spending time on though.

Comment 10

[Ben Bucksch (:BenB)]

(In reply to Dave Townsend (:Mossop) from comment #6)
> Though I guess it looks like we don't invalidate the startup cache for
> restartless extensions either, so looks like we have two problems!

I guess this should be a different bug, then?

> I'm not sure this is something worth spending time on though.

I think it's better to solve this once in the platform then to let each ext dev run into this (many of them the hard way), and each of them having to work around it with custom code.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Ben Bucksch (:BenB) from comment #10)
> (In reply to Dave Townsend (:Mossop) from comment #6)
> > Though I guess it looks like we don't invalidate the startup cache for
> > restartless extensions either, so looks like we have two problems!
> 
> I guess this should be a different bug, then?

This bug was always about JS modules not getting unloaded, it was just mis-diagnosed as being a startupcache problem.

> > I'm not sure this is something worth spending time on though.
> 
> I think it's better to solve this once in the platform then to let each ext
> dev run into this (many of them the hard way), and each of them having to
> work around it with custom code.

If it was straightforward to do then I agree, but it isn't.