Closed Bug 481603 Opened 11 years ago Closed 9 years ago

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

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: brettz9, Assigned: mossop)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

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
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).
It looks like bug 564674 is a dupe.
Duplicate of this bug: 564674
Assignee: nobody → dtownsend
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows Vista → All
Hardware: x86 → All
Summary: Add a means to refresh code modules → Add a way to unload JS modules loaded with Components.utils.import
Blocks: abp
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #517837 - Flags: review?(mrbkap)
Whiteboard: [has patch][needs review mrbkap]
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.
Attachment #517837 - Flags: review?(mrbkap) → review+
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.
Attachment #517837 - Attachment is obsolete: true
Attachment #538586 - Flags: review+
Attached patch em patch rev 1Splinter Review
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?
Attachment #538587 - Flags: review?(robert.bugzilla)
Attachment #538587 - Flags: review?(mwu)
Whiteboard: [has patch][needs review mrbkap] → [has patch][needs review rs]
(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.
Attachment #538587 - Flags: review?(mwu) → review+
Blocks: 648125
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);
>   }
> }
Attachment #538587 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][needs review rs] → [has patch]
landed:

http://hg.mozilla.org/mozilla-central/rev/210112f045f7
http://hg.mozilla.org/mozilla-central/rev/274e2d2a93a8
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla7
Keywords: dev-doc-needed
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.
(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
(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.
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?
(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.
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.
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.
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.
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?
You need to log in before you can comment on or make changes to this bug.