481603 - Add a way to unload JS modules loaded with Components.utils.import

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Brett Zamir]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6
Build Identifier: 

JavaScript code modules (via Components.utils.import ) offer a great way to build up reusable code, but it becomes significantly more difficult to debug and fix them in a live environment (one of the usual advantages in working in JavaScript) when there is no means of forcing a reevaluation of its contents.

Reproducible: Always

Comment 1

[Sylvain Spinelli]

This feature is also needed for restartless add-ons work in firefox 4 (http://www.oxymoronical.com/blog/2010/04/How-do-restartless-add-ons-work#comment-49076).

Comment 2

[Brett Zamir]

It looks like bug 564674 is a dupe.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is a first attempt to make this work. It does the job but I'm not sure if it might accidentally leak.

On the whole this is trivial, we just remove the module from the imported list so the next attempt to load reloads. The one issue is what happens to references to the old module. I think it would make sense for them to continue to work (pointing at the old module of course), however to do this I had to remove the call to JS_ClearScope for the old module's global. I don't know why that call is there though since my naive interpretation of GC is that once we clear the root from the global then it and all its properties should be eligible for collection.

Leaving the JS_ClearScope in place leaves the old module broken when it tries to access any properties of its global, but maybe that wouldn't be terrible since the main aim here is to get rid of the old code anyway.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 517837 [details] [diff] [review]
patch rev 1

Review of attachment 517837 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1594,5 @@
> +    if (!mInitialized) {
> +        return NS_OK;
> +    }
> +
> +    nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);

Can you factor this stuff out into a GetKeyForLocation function? It looks like this shares a bunch of code with mozJSComponentLoader::ImportInto.

::: js/src/xpconnect/loader/mozJSComponentLoader.h
@@ -177,5 @@
>  
>                  JSAutoEnterCompartment ac;
>                  ac.enterAndIgnoreErrors(sSelf->mContext, global);
>  
> -                JS_ClearScope(sSelf->mContext, global);

Yeah. Your naive understanding of GC is correct. The fear is that because of our crazy C++/JavaScript reference counting system, sometimes having an extra JS_ClearScope does actually break cycles.

I would rather see this patch land with the JS_ClearScope still here (which will leave things in a broken state) and file a followup bug on removing it.

Comment 6

[Dave Townsend [:mossop]]

Attachment: js loader patch rev 2

Patch ready for checkin, over IRC I pointed out that factoring out GetKeyForLocation would be more complex as the other caller needs 3 or 4 of the generated results there.

Comment 7

[Dave Townsend [:mossop]]

Attachment: em patch rev 1

A missing piece here is that the startup cache caches the modules and so if the add-on upgrades it'll still get the old version from the cache. This makes the EM flush the startup cache whenever a restartless add-on has been uninstalled. I've also updated the bootstrap tests to make it properly test packed/unpacked scenarios and it verifies that the add-ons can import/unload the modules and they change when necessary.

Michael, can you just confirm my understanding that invalidating the startup cache at runtime isn't a problem?

Comment 8

[Michael Wu [:mwu]]

(In reply to comment #7)
> Michael, can you just confirm my understanding that invalidating the startup
> cache at runtime isn't a problem?

If it's a problem, it wouldn't be your problem, since code shouldn't really rely on startup cache entries being there.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 538587 [details] [diff] [review]
em patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -7154,30 +7181,20 @@ function AddonWrapper(aAddon) {
>                     createInstance(Ci.nsIZipReader);
>     zipReader.open(bundle);
>     let result = zipReader.hasEntry(aPath);
>     zipReader.close();
>     return result;
>   },
> 
>   this.getResourceURI = function(aPath) {
this returns either a jar or a file uri... would be good to add a comment about this. Looks like getURIForResourceInFile does the same?

>-    let bundle = aAddon._sourceBundle.clone();
>-
>-    if (bundle.isDirectory()) {
>-      if (aPath) {
>-        aPath.split("/").forEach(function(aPart) {
>-          bundle.append(aPart);
>-        });
>-      }
>-      return Services.io.newFileURI(bundle);
>-    }
>-
>     if (!aPath)
>-      return Services.io.newFileURI(bundle);
>-    return buildJarURI(bundle, aPath);
>+      return Services.io.newFileURI(aAddon._sourceBundle);
>+
>+    return getURIForResourceInFile(aAddon._sourceBundle, aPath);
>   }
> }

Comment 10

[Dave Townsend [:mossop]]

landed:

http://hg.mozilla.org/mozilla-central/rev/210112f045f7
http://hg.mozilla.org/mozilla-central/rev/274e2d2a93a8

Comment 11

[Dave Garrett]

Thanks for implementing this Mossop! Now as soon as a chrome.manifest can be manually (re-)parsed (bug 564667) then converting more than simple extensions to restartless will start to become practical.

WRT this bug, might I suggest also adding a test for reloading a new version of a JSM file? e.g. load one dummy JSM file with just one variable set, edit the file to change the variable's value, then unloading and loading it again to make sure that the new value is accessible and thus the new file is now loaded. The current test makes sure the scopes are different but doesn't use a different file when reloading.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> WRT this bug, might I suggest also adding a test for reloading a new version
> of a JSM file? e.g. load one dummy JSM file with just one variable set, edit
> the file to change the variable's value, then unloading and loading it again
> to make sure that the new value is accessible and thus the new file is now
> loaded. The current test makes sure the scopes are different but doesn't use
> a different file when reloading.

The tests in the EM patch should cover this case

Comment 13

[Dave Garrett]

(In reply to comment #12)
> (In reply to comment #11)
> The tests in the EM patch should cover this case

Yes, sorry, you're right. I didn't notice the second changeset modified an existing test to cover this case. Thanks; sorry for the bugspam.

Comment 14

[Dimitri Saltanov]

This works for me when installing a new XPI over the old one like in the test above; but not from a live directory even after [manual] tinkering with file creation and modification times under Windows XP and 8.0a1. Is there any way to explicitly force the jsm module to flush its cache?

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> This works for me when installing a new XPI over the old one like in the
> test above; but not from a live directory even after [manual] tinkering with
> file creation and modification times under Windows XP and 8.0a1. Is there
> any way to explicitly force the jsm module to flush its cache?

Restarting the app after changing modification times should do it I think (if not file a bug). If you're trying to do it without restarting the app then you'll have to manually flush teh startup cache like the code in the em patch does.

Comment 16

[Eric Shepherd [:sheppy]]

Documented by trevor:

https://developer.mozilla.org/en/Components.utils.unload

Some links added elsewhere as appropriate to reference this capability.

Also mentioned on Firefox 7 for developers.

Comment 17

[Michael Balazs]

Thanks for the link Eric. 

Question about usage, when working in a development environment with a live directory. If I do an unload before a load which is triggered by page loading in the browser will this load the live updated version or is the file cached upon the browser starting? If it does live load, it might be nice to note that for developers as well on the page linked above. 

Also thanks for this feature as I can I work towards a restartless addon.

Comment 18

[Kris Maglione [:kmag]]

Michael: If you call Cu.load for a script which is already in the startup cache, the version in the startup cache is loaded regardless of whether you've used Cu.unload. Cu.unload only removes the reference to the module's cached global. The Add-on Manager will flush the startup cache when your add-on is updated, though, if that's the circumstance that you're worried about.

Comment 19

[Michael Balazs]

Kris: Thanks for the reply. Do I understand this correctly, my addon (loaded from a directory instead of an xpi) is stored in the startup cache. Cu.unload doesn't remove it from here thus doing a load will reload this version and not a modified version I have in the directory. If this is the case, is there a way to clear the startup cache / force the reload of my addons files from the directory without the need to restart the browser?