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

RESOLVED FIXED in mozilla7

Status

()

Core
XPConnect
--
enhancement
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: Brett Zamir, Assigned: mossop)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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

7 years ago
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).
(Reporter)

Comment 2

6 years ago
It looks like bug 564674 is a dupe.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 564674
(Assignee)

Updated

6 years ago
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

Updated

6 years ago
Blocks: 467520
(Assignee)

Comment 4

6 years ago
Created attachment 517837 [details] [diff] [review]
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.
Attachment #517837 - Flags: review?(mrbkap)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 6

6 years ago
Created attachment 538586 [details] [diff] [review]
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.
Attachment #517837 - Attachment is obsolete: true
Attachment #538586 - Flags: review+
(Assignee)

Comment 7

6 years ago
Created attachment 538587 [details] [diff] [review]
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?
Attachment #538587 - Flags: review?(robert.bugzilla)
Attachment #538587 - Flags: review?(mwu)
(Assignee)

Updated

6 years ago
Whiteboard: [has patch][needs review mrbkap] → [has patch][needs review rs]

Comment 8

6 years ago
(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.

Updated

6 years ago
Attachment #538587 - Flags: review?(mwu) → review+

Updated

6 years ago
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]
(Assignee)

Comment 10

6 years ago
landed:

http://hg.mozilla.org/mozilla-central/rev/210112f045f7
http://hg.mozilla.org/mozilla-central/rev/274e2d2a93a8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla7

Updated

6 years ago
Keywords: dev-doc-needed

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
(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

6 years ago
(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

6 years ago
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?
(Assignee)

Comment 15

6 years ago
(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.
Keywords: dev-doc-needed → dev-doc-complete

Comment 17

6 years ago
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.

Comment 19

6 years ago
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?
Depends on: 1195689
You need to log in before you can comment on or make changes to this bug.