We haven't got any proper facilities for sharing JavaScript code - neither between JavaScript-implemented XPCOM modules nor between XUL windows or XBL bindings. I don't mean an include-facility (as implemented by mozIJSSubScriptLoader) where the included scripts get evaluated in the master script's context everytime they are included somewhere, but a library-facility where shared scripts only get evaluated once and some of their symbols can be dynamically imported into other scripts. The idea is something like this: --------------------------------------------------------- // myUtils.js MOZ_EXPORTED_SYMBOLS = ['XXXUtils', 'FOOFunc', 'ABCConst']; var XXXUtils = { ... }; function FOOFunc(...) {...}; const ABCConst = "@mozilla.org/foobar;1"; ... --------------------------------------------------------- // myComponent.js importModule('resource:/jscodelib/myUtils.js'); function bar() { XXXUtils.xxx(); ... FOOFunc(); .... } ---------------------------------------------------------- Patch coming up.

This patch adds the jscodelib facility. Basic Operation: Components.classes["@mozilla.org/jscodelib;1"].getService(C.I.mozIJSCodeLib) .importModule('resource:/jscodelib/utils.js'); installs all symbols in the array MOZ_EXPORTED_SYMBOLS in utils.js as properties on the caller's global object. If utils.js gets imported into more than 1 file, its exported objects will be shared between all files; utils.js only gets evaluated once. For convenience, the JS component loader defines the JS function importModule(), which is functionally equivalent to the XPConnect version above. importModule() (both the xpconnect & js version) takes an object as an optional 2. argument. If given, the exported symbols will be installed on this object instead of the global object. Instead of using the jscodelib as a service where imported modules will be shared by all clients, it can also (simultaneously) be used as a component, enabling private code collections: var collection = Compts.classes["@mozilla.org/jscodelib;1"].createInstance(C.I.mozIJSCodeLib); The patch also adds a file JSComponentUtils.js which can be used by JS-implemented XPCOM modules to automatically generate the boilerplate nsIModule and factory implementations, e.g.: importModule('resource:/jscodelib/JSComponentUtils.js'); ... var NSGetModule = ComponentUtils.generateNSGetModule( [{ className : "FilePicker JS Component", cid : FILEPICKER_CID, contractID : FILEPICKER_CONTRACTID, factory : ComponentUtils.generateFactory(nsFilePicker) }]);

Brendan, Robert: Can I get you guys to give me some feedback on this?

Adding updated patch. I accidentally put some unrelated stuff into the first patch.

Updated patch. I forgot to add js/src/xpconnect/loader/JSComponentUtils.js in the previous patch.

Comment on attachment 144554 [details] [diff] [review] codelib - 3rd attempt Shaver should definitely review. Cool stuff -- a couple of nits below. >+ JSContext *ctx = nsnull; >+ cc->GetJSContext (&ctx); Canonical name is cx, not ctx. >+ if (!JS_ConvertArguments(ctx, argc, argv, "*o", &targetObject)) { Seems like overkill, compared to just calling JS_ValueToObject(cx, argv[1], &targetObject). >+ // Our targetObject is the caller's global object. Find it by >+ // walking the calling object's parent chain. >+ >+ nsCOMPtr<nsIXPConnectWrappedNative> wn; >+ cc->GetCalleeWrapper (getter_AddRefs(wn)); >+ if (!wn) { >+ NS_ERROR("null callee wrapper"); >+ return NS_ERROR_FAILURE; >+ } >+ wn->GetJSObject (&targetObject); >+ if (!targetObject) { >+ NS_ERROR("null calling object"); >+ return NS_ERROR_FAILURE; >+ } >+ >+ JSObject *parent = nsnull; >+ while ((parent = JS_GetParent(ctx, targetObject))) >+ targetObject = parent; Nice to see correct static scoping being observed! >+ JSObject *moduleObj=nsnull; Discordant style of = operator with no space on both sides. >+ JSContext*ctx=nsnull; Ditto, and cx is canonical name... >+ cxstack->GetSafeJSContext(&ctx); ...especially in light of cxstack name here! >+ instream->Read(*buf, content_length, &bytesRead); >+ if (bytesRead!=content_length) { Ouch, spaces are nicer around operators. Etc. below. I'll give an sr when shaver has stamped r=. /be

Thanks for the comments, Brendan. Here's an updated patch.

Updated patch for the nsIScriptObjectPrincipal deCOMtamination changes (bug 240745).

Alex, do you want shaver and me to review the updated patch? If so, go ahead and request review. Thanks, /be

Comment on attachment 146412 [details] [diff] [review] updated patch Yes please! :-)

Just as an aside... I'm experiencing some xpconnect NewResolve() weirdness with the BackstagePass global object. The symptom is that in debug builds, if I do 'dump(this)', I get "[BackstagePass]". If, on the other hand, I do an implicit cast, as in 'dump(" "+this)', I get "[BackstagePass @ XXXXX]". In the first case mozBackstagePass::NewResolve() is called to locate "toString" (which isn't found). In the second case mozBackstagePass::NewResolve() is *not* called and we somehow rightly end up in XPCWrappedNative::ToString(). What is the right way of getting the first case to resolve to XPCWrappedNative::ToString() as well? It seems to work for all the DOM classes. I stepped through this for an hour or so, but I don't fully understand what is going on.

Maybe the thing to do is grab shaver or me on IRC when you've got things in the debugger.... /be

Apologies if you have started reviewing... I've updated the patch with 2 additions: - mozIJSCodeLib now has a new method 'probeModule(url)' which loads the given module if it isn't loaded yet and returns its global object. - jsloader implements a new interface 'mozIJSComponentLib' with a method 'probeComponent(location)', which similarly loads a js xpcom component and returns its global object. The rationale for having these methods is so that we can dynamically modify js code modules & components while Mozilla is running. E.g. JSSh's xemacs script now honours the variable 'moz-jssh-buffer-globalobj'. If this is given, e.g. as a file variable, as in -*- moz-jssh-buffer-globalobj: "Components.classes['@mozilla.org/jscodelib;1'] .getService(Components.interfaces.mozIJSCodeLib) .probeModule('resource:/jscodelib/JSComponentUtils.js')" -*-, a call to moz-jssh-eval-buffer (C-c m e) will execute the buffer's code in the given global object.

Comment on attachment 147317 [details] [diff] [review] new patch >+mozJSCodeLib::LoadURL(char **buf, const nsACString &url) >+ channel->Open(getter_AddRefs(instream)); >+ if (!instream) { >+ NS_WARNING("could not open stream"); >+ goto failure; >+ } >+ >+ if (NS_FAILED(channel->GetContentLength(&content_length))) { >+ NS_WARNING("could not get content length"); >+ goto failure; >+ } >+ >+ *buf = new char[content_length+1]; >+ if (!*buf) { >+ NS_WARNING("could not alloc buffer"); >+ goto failure; >+ } >+ >+ instream->Read(*buf, content_length, &bytesRead); >+ if (bytesRead != content_length) { >+ NS_WARNING("stream read error"); >+ goto failure; >+ } >+ >+ return content_length; better not depend on the return value of GetContentLength. the API allows the nsIChannel to return -1 !! you should instead use the value returned from instream->Available(). and you will need to write a loop. something like this: nsCString buf; PRUint32 len, n; nsresult rv; for (;;) { rv = instream->Available(&len); if (NS_FAILED(rv) || len == 0) break; buf.SetLength(buf.Length() + len); char *p = buf.EndWriting() - len; rv = instream->Read(p, len, &n); if (NS_FAILED(rv) || n != len) break; } i'd make LoadURL take a |nsCString &buf| out param. also, beware that if you call nsIChannel::open on a http:// URI or some other remote resource, the Open, Available, and Read calls may block the calling thread (and hence the browser UI) for lengthy periods of time. moreover, calls to these methods may lead to PLEvents being dispatched, which could cause your code to be re-entered. that may or may not be a concern. if you are loading a local URI (e.g., file://), then this is not a problem.

Patch updated to trunk & incorporating Darin's feedback.

Comment on attachment 187113 [details] [diff] [review] updated patch I'll take a look at this this weekend. Alex: does this incorporate the single-load/shared-object stuff we talked about at XTech?

(In reply to comment #15) > Alex: does this incorporate the single-load/shared-object stuff we talked about > at XTech? Shaver: It works much like the js component loader. Code modules are loaded and evaluated with a new BackstagePass the first time they are asked for (by 'importModule()') and a reference to the global object is stored in a hash table. All symbols listed in MOZ_EXPORTED_SYMBOLS are then set as properties on the caller's global object. Next time the module is requested the script won't be re-evaluated; the MOZ_EXPORTED_SYMBOLS will be taken straight from the hashed global object.

Patch updated for trunk changes.

Comment on attachment 188027 [details] [diff] [review] patch v8 I understand better where this is headed now, thanks. I'd like to see it hung off of Components.util, where it a) doesn't pollute the global namespace for components, and b) is also accessible to chrome. We've had many painful experiences in calendar/OMX land recently in which we wanted to share utility code between components and chrome. We don't need or want, IMO, the configure switch. Honestly, I'd rather see us lose the jsloader config switch too. There are too many configuration knobs, and it just weakens our platform to not know what's set where. I'd also rather see us not add another shared/ directory. Put the ContextHelper def'n in xpcprivate.h, rename and de-static the sandbox's dump/debug from xpccomponents.cpp to use in all 3 contexts (sandbox, components, code-module), and lose both JSFunctions.h and JSFunctions.cpp. (That way I don't have to tell you to rename them to avoid using the sacred "JS" prefix, too. =) ) In terms of how the importing works, I think I'd like to see us support some way to import into an existing object, such that, for example, your ComponentUtils functions could be loaded right into Components.utils.generateFactory and .generateModule. We don't have to do that right now, but I wanted to write it down here somewhere, and we should make sure that we like what happens in these scenarios: 1) EXPORTED_SYMBOLS = ["Foo.Bar"]; 2) Components.util.importModule("exports-Whatever.js"); Whatever.newPropertyFoo = "blip"; Components.util.importModule("exports-Whatever.js"); I am really not in love with the mozI prefix, I have to say. I know, I started it back in the day, but I'd rather see the remaining interface be xpcICodeLoader or something. In fact, I think I'm going to want some other interface points on the component loader for something else I'm playing with; what do you think of just calling it xpcIJSComponentLoader? "probeComponent" doesn't seem like a great name to me: can we use importModule in both places? Thanks for sticking with this; I think it'll be a real boon for us.

(In reply to comment #18) [...] > In terms of how the importing works, I think I'd like to see us support some > way to import into an existing object, such that, for example, your > ComponentUtils functions could be loaded right into > Components.utils.generateFactory and .generateModule. We don't have to do that > right now, but I wanted to write it down here somewhere, Note that importModule() takes an optional 'targetObject' arg, so you can do: var a = {}; importModule("foo.js", a); Here, all exported symbols from foo.js would be installed as properties on 'a' instead of the global object. > and we should make > sure that we like what happens in these scenarios: > > 1) > EXPORTED_SYMBOLS = ["Foo.Bar"]; Hmm, do you think this would be of much use in practice? At the moment only top-level properties can be exported. > 2) > Components.util.importModule("exports-Whatever.js"); > Whatever.newPropertyFoo = "blip"; > Components.util.importModule("exports-Whatever.js"); This should work fine. The second importModule() will have no effect. Whatever will have newPropertyFoo=='blip' everywhere it is being imported to (or has been imported to).

(In reply to comment #19) > Note that importModule() takes an optional 'targetObject' arg, so you can do: > > var a = {}; > importModule("foo.js", a); Yeah, or the code could just mutate Components.util in its top-level script (or from an EXPORTED_SYMBOLS getter, woo) and omit those from the list it returns. > > 1) > > EXPORTED_SYMBOLS = ["Foo.Bar"]; > Hmm, do you think this would be of much use in practice? At the moment only > top-level properties can be exported. I'm not sure, but I bet that someone writing that code expects to be setting global.Foo.Bar and not global["Foo.Bar"] Want to toss up a message via reportError if there's a . in the symbol name, and omit it from the list? > > 2) > > Components.util.importModule("exports-Whatever.js"); > > Whatever.newPropertyFoo = "blip"; > > Components.util.importModule("exports-Whatever.js"); > This should work fine. The second importModule() will have no effect. Whatever > will have newPropertyFoo=='blip' everywhere it is being imported to (or has been > imported to). Quite right (and righteous). I'm not sure how I feel about MOZ_EXPORTED_SYMBOLS vs. EXPORTED_SYMBOLS. Probably fine, and I'm just knee-jerking against MOZ_ification. Also: do you want to set __LOCATION__ here like I do for JS components? Might help libraries find their sub-libraries/dependencies. (And in the nearish future, I'd like to finish off bug 291374, which would want to find this, I _think_.) It'd also let people parameterize their library imports, through query strings, though that's getting a little arcane. I thought about the global/this stuff a bit, and I think it's OK: though a library implementor has to write a bit of code to find the global for the current call, the common case (calling functions in the same library, including its own importModule calls) is what people expect. I like that those imports are "private" too, by default. I have hopes that JS2 or JS1.9 or JS3000 will give us additional language features to help package, version, and import code in pieces like this, but until then -- and perhaps beyond! -- this looks great.

(In reply to comment #18) > "probeComponent" doesn't seem like a great name to me: can we use importModule > in both places? Hmm, how about getComponent() instead of probeComponent() and getModule() instead of probeModule()? These functions are different to Component.utils.importModule in that they return the global object of the component or module instead of looking at EXPORTED_SYMBOLS.

(In reply to comment #21) > Hmm, how about getComponent() instead of probeComponent() and getModule() > instead of probeModule()? > These functions are different to Component.utils.importModule in that they > return the global object of the component or module instead of looking at > EXPORTED_SYMBOLS. What about making importModule return the global object, and losing probeModule altogether? If importModule is explicitly passed null, it doesn't load into the caller's context, but just returns the global object. importModuleToJSObject would behave similarly. We'd lose the force parameter, but I don't especially like it anyway. Also, I just noticed that you're using the safe context to compile the scripts. On the trunk, we now use mContext for all our compilation needs, to avoid problems with context reuse.

Ok, how about this? This patch should address all review comments apart from checking the exported symbols for ".". I've also added some code to set the __LOCATION__ property if the module url points to a local file.

Comment on attachment 188292 [details] [diff] [review] patch v9 >+++ js/src/xpconnect/Makefile.in 4 Jul 2005 20:52:10 -0000 1.26.16.2 >@@ -46,6 +46,8 @@ include $(DEPTH)/config/autoconf.mk > MODULE = xpconnect > DIRS = public idl > >+DIRS += codelib >+ Just stick that into the previous line. >+ * The implementation maintains a hash of moduleURL->global >+ * obj. Subsequent invocations of importModule with the same >+ * 'moduleURL' will not cause the file at 'moduleURL' to be >+ * re-evaluated. ..., but the symbols in EXPORTED_SYMBOLS will be exported into the specified targetObj as above. >Index: js/src/xpconnect/src/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/js/src/xpconnect/src/Makefile.in,v >retrieving revision 1.81 >retrieving revision 1.80.2.3 >diff -u -p -r1.81 -r1.80.2.3 >--- js/src/xpconnect/src/Makefile.in 29 Jun 2005 14:22:52 -0000 1.81 >+++ js/src/xpconnect/src/Makefile.in 4 Jul 2005 20:52:12 -0000 1.80.2.3 >@@ -62,6 +62,7 @@ PACKAGE_FILE = xpconnect.pkg > REQUIRES = xpcom \ > string \ > js \ >+ jsloader \ If we put the IDL in idl/, instead of the loader/ dir, then it seems like we don't have to do nearly as much Makefile whacking here. That loader is a separate directory is a historical artifact, from when it used to be its own component. (See also below about just merging the component loader and the code-lib loader.) >Index: js/src/xpconnect/src/xpccomponents.cpp >+JSBool JS_DLL_CALLBACK >+JSDump(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) >+JSBool JS_DLL_CALLBACK >+JSDebug(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) We shouldn't use the "JS" prefix here; that belongs to the JS engine. XPCScriptDebug and XPCScriptDump? > #ifdef XPC_USE_SECURITY_CHECKED_COMPONENT > /* string canCreateWrapper (in nsIIDPtr iid); */ > NS_IMETHODIMP >+NS_IMPL_THREADSAFE_ISUPPORTS2(mozJSComponentLoader, >+ nsIComponentLoader, >+ xpcIJSComponentLoader) How do you feel about just making the JS component loader implement xpcIJSCodeLoader? It looks like it could share a decent amount of bootstrapping code, as well as the compilation context, principal, etc. (Maybe even use the same hash table for components and modules?) (That would also let us leave the contexthelper and errorreporterhelper in mozJSComponentLoader.cpp, though that's not really an interesting motivation either way.) >+ jsval *retval = nsnull; >+ cc->GetRetValPtr(&retval); >+ if (*retval) >+ *retval = global ? OBJECT_TO_JSVAL(global) : JSVAL_NULL; This doesn't look right to me; shouldn't you be testing |retval|, rather than |*retval|? Also, it's legal to call OBJECT_TO_JSVAL on null, so you don't need the alternation. >Index: js/src/xpconnect/loader/JSComponentUtils.js Let's name this "ComponentUtils.js" to match its exported symbol? >+#ifdef DEBUG >+ printf("] onto %s\n", PromiseFlatCString(moduleName).get()); >+#endif "from" here rather than "onto"? (Though I should really convert the JS component loader and this to NSPR logging as well someday, that day is not today.) I think we're almost there!

(In reply to comment #24) > > How do you feel about just making the JS component loader implement > xpcIJSCodeLoader? It looks like it could share a decent amount of > bootstrapping code, as well as the compilation context, principal, etc. (Maybe > even use the same hash table for components and modules?) I like the idea. Code modules could just go into the mGlobals hash. I'll try to make some time for this in the next few weeks.

This new, improved, slimmer patch implements the module loading code on the component loader as shaver suggested, and thus takes advantage of bryner's optimizations from bug#279839 :-) This is what is does: - The component loader implements a new interface xpcIJSModuleLoader.idl which implements two methods: importModule() & importModuleToJSObject(). The former is to be called from JS only, the latter is the [noscript] version. - xpccomponents implements a shortcut to importModule: Components.utils.importModule(). - importModule() can be used in 3 ways: 1. Components.utils.importModule('rel:foo.js', null); returns the files global object. 2. Components.utils.importModule('rel:foo.js'); installs all properties found in the array EXPORTED_SYMBOLS (if any) in foo.js onto the caller's global object (and returns foo's global object). 3. var bar = {}; Components.utils.importModule('rel:foo.js', bar); installs all properties from EXPORTED_SYMBOLS onto 'bar' (and returns foo's global object). I've found that all of these three uses are actually useful. - The component loader maintains a cache of the global objects of files imported in this way, so a file will only be evaluated once per Mozilla session (which is of course the whole point of this patch). - Whether or not a JS implements nsIModule is irrelevant to the operation of importModule. (*) see also below - In contrast to the earlier patches, importModule expects a registry location rather than a URI. I've found that often you want a js file to both implement an xpcom module (i.e. implement nsIModule) AND, when using it from script, be accessible directly via EXPORTED_SYMBOLS or by getting the global object, so locating all js modules (be they xpcom, non-xpcom or both) in bin/components seemed to make sense. One slight problem with this is that the loader will try to autoregister js files that don't implement nsIModule. While this is benign it could be a performance problem, so I have started giving non-xpcom js files the prefix *.jsm instead of *.js. - The patch comes with ComponentUtils.jsm, which can be imported by js modules to implement the nsIModule boilerplate. (*) The one thing I am not entirly happy about is the ambiguous naming introduced by this patch. 'JS Module' can now mean xpcom module or js module or both. Any thoughts welcome.

Comment on attachment 201132 [details] [diff] [review] patch v10 This is now all broken because of 316416. *sigh*

Since #316416 is backed out, v10 of the patch is working again. I also have an updated version that works with the changes on #316416 (reverting back from a single hashtable to separate globals/modules hashtables).

Changing to more appropriate product/component.

Comment on attachment 201132 [details] [diff] [review] patch v10 On scan, this looks great. We should not let MOZ_JSLOADER continue to live, so don't bother with it here. The DEBUG printfs should be to stderr, or more likely DEBUG_xpchacker or use PR_LOG. Pinging brendan for input here, since he's designing package and import for JS1.9 now. I'll get back to this tomorrow to stamp and nit-pick.

Comment on attachment 201132 [details] [diff] [review] patch v10 >+#ifdef MOZ_JSLOADER >+ nsCOMPtr<xpcIJSModuleLoader> moduleloader = do_GetService(MOZJSCOMPONENTLOADER_CONTRACTID); >+ if (!moduleloader) >+ return NS_ERROR_FAILURE; >+ return moduleloader->ImportModule(registryLocation); >+#else >+ return NS_ERROR_NOT_AVAILABLE; >+#endif I boldly propose that we eliminate MOZ_JSLOADER ifdefs, but that's not a job for this patch. >+ } >+ else { We cuddle our elses in the JS loader! >+ // Our targetObject is the caller's global object. Find it by >+ // walking the calling object's parent chain. >+ >+ nsCOMPtr<nsIXPConnectWrappedNative> wn; >+ rv = cc->GetCalleeWrapper(getter_AddRefs(wn)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ wn->GetJSObject (&targetObject); >+ if (!targetObject) { >+ NS_ERROR("null calling object"); >+ return NS_ERROR_FAILURE; >+ } >+ >+ JSObject *parent = nsnull; >+ while ((parent = JS_GetParent(cx, targetObject))) >+ targetObject = parent; >+ } Is this going to run afoul of inner/outer stuff? (I really have no idea, and will hopefully remember to cc: mrbkap after I finish this review.) >+ JSObject *globalObj = nsnull; >+ rv = ImportModuleToJSObject(registryLocation, targetObject, &globalObj); >+ >+ jsval *retval = nsnull; >+ cc->GetRetValPtr(&retval); >+ if (*retval) >+ *retval = globalObj ? OBJECT_TO_JSVAL(globalObj) : JSVAL_NULL; It's legal to use OBJECT_TO_JSVAL on null, so just if (*retval) *retval = OBJECT_TO_JSVAL(globalObj); >+ if (globalObj) >+ *_retval = globalObj; >+ else { >+ *_retval = nsnull; >+ return NS_ERROR_FAILURE; >+ } If else-block has braces, then-block must also. >+ for (jsuint i=0; i<symbolCount; ++i) { Spaces around = and <, please. >+ if (!JS_GetProperty(mContext, globalObj, JS_GetStringBytes(symbolName), &val)) { >+ NS_ERROR("Could not get symbol"); >+ return NS_ERROR_FAILURE; >+ } >+ >+ if (!JS_SetProperty(mContext, targetObj, JS_GetStringBytes(symbolName), &val)) { >+ NS_ERROR("Could not set property on target"); >+ return NS_ERROR_FAILURE; Should those set ExceptionWasThrown to propagate errors more usefully? (With some source info, etc.) >Index: js/src/xpconnect/loader/ComponentUtils.jsm I'm leaving the utils thing unreviewed for now, though the patch would be very welcome in 61789, given the code sharing system. (My 2004 reassignment looks positively prescient now, no?) r+, please fix above stuffs, and accept my insufficient apologies for the delay.

Where is this interface available? Is it part of SpiderMonkey and therefor available everywhee including XULRunner 1.9 or is it part of SeaMonkey?

This hasn't been checked in, so nowhere, unless you patch and build yourself.

Alex, do you have a plan for this patch? Is this something that you plan to resurrect?

Sayrer may want to help get this landed. I'll promptly review an updated patch that addresses shaver's comments. /be

working on it right now, figuring out what bug 316416 did to mozJSComponentLoader::GlobalForLocation

doesn't compile due to API changes in GlobalForLocation, just saving work

This works, but it needs refactoring, since it copies a bunch of code from LoadModule. Also haven't gotten to shaver's comments. Given this file -------------------------------------- EXPORTED_SYMBOLS = ["foo"] function foo(a, b) { return "arg1: " + a + " arg2: " + b + "." + " Called with " + arguments.length + " arguments."; } -------------------------------------- this is how you use it from the shell: -------------------------------------- js> Components.utils.importModule("/home/sayrer/test.js") Installing symbols [ foo ] from /home/sayrer/test.js [object BackstagePass @ 0x61e3f0 (native @ 0x71a308)] js> foo("hmm", "yeah", "something else") arg1: hmm arg2: yeah. Called with 3 arguments. js> --------------------------------------

(In reply to comment #38) > > js> Components.utils.importModule("/home/sayrer/test.js") Oh, this is wrong too, it needs to take chrome URLs.

This patch loads URLs in the same manner as the patch reviewed so long ago. It uses a method on nsIComponentManagerObsolete to get this done. Basically, the "rel:" scheme looks for files in dist/bin/components. This isn't as bad as it sounds, since the current ComponentManager still uses this, just doesn't expose it. This crashes on shutdown in JS_ClearScope, but otherwise seems to work.

Comment on attachment 264440 [details] [diff] [review] (WIP) load as before >+ nsAutoPtr<ModuleEntry> entry(new ModuleEntry); It doesn't look like entry is ever forgotten, so won't this stick a dangling pointer in the hashtable? Maybe that's the source of your crash?

(In reply to comment #41) > > It doesn't look like entry is ever forgotten, so won't this stick a dangling > pointer in the hashtable? Maybe that's the source of your crash? Thanks, that's it. Good catch.

(In reply to comment #31) > > Is this going to run afoul of inner/outer stuff? (I really have no > idea, and will hopefully remember to cc: mrbkap after I finish this > review.) I don't know the answer to this either. > >Index: js/src/xpconnect/loader/ComponentUtils.jsm > > I'm leaving the utils thing unreviewed for now Should we keep this file? If we do, I propose we rename it XPCOMUtils.js, because it is confusing to write Components.utils.import("rel:ComponentUtils.js,") Once we have a file we can keep, I will write an XPCShell test for it.

... now including the added files

> EXPORTED_SYMBOLS = ["foo"] > Components.utils.import("rel:ComponentUtils.js,") > Components.utils.import("rel:test.js"); Am I the only one who thinks that this is nice as a project internal framework, but not a proper syntax for all Mozilla users? At the minimum, "foo" should not be a string, but the (function) object foo, and the "rel:" should not be necessary. IMHO, it should be more like: definition: export function foo() { ... } usage: import "test.js"; foo();

(And, preferably, "all Mozilla users" should include webpages. That would relieve AJAX and co libs from having everything in one file.)

I forgot to mention two important details. Shaver and I changed the name to "Components.utils.import" because the name module makes it hard to read the source file, since we actually don't want our entry objects to have a |module| member. The second is that we need two hashtables to guard against a file being imported as both a component and a js library. If it's loaded using Components.utils.import first, it will trigger the "bad hashtable entry" error check in LoadModule. (updated patch drops a mistakenly included diff, sorry for the bugspam)

Comment on attachment 264476 [details] [diff] [review] Components.utils.import Peanut gallery comments, coming to this fairly late: I realize the original patch didn't have a xpcshell testcase. Could we have a supplementary patch to do so? >+ generateFactory: function(ctor, interfaces) { Venkman users will thank you if you name your functions: generateFactory: function generateFactory(ctor, interfaces) {}

(In reply to comment #44) > Should we keep this file? If we do, I propose we rename it XPCOMUtils.js, > because it is confusing to write I would vote for keeping it... That's boilerplate code that is annoying to write over and over again in JS components. From comment 26, though, I think you mean XPCOMUtils.jsm, right? That also helps to make sure you're importing files that are designed to be imported.

Comment on attachment 264476 [details] [diff] [review] Components.utils.import >+ /** >+ * Imports the JS module at 'registryLocation' to the JS object >+ * 'targetObj' (if != null) as described for importModule() and >+ * returns the module's global object. >+ */ >+ [noscript] JSObjectPtr importToJSObject(in AUTF8String registryLocation, >+ in JSObjectPtr targetObj); Nit: importToObject? importInto? The "JS" seems unnecessary, and perhaps the ending word could be more specific, or left out with the right preposition? >+/** >+ * Utilities for JavaScript components loaded by the JS component >+ * loader. >+ * >+ * Import into a JS component using >+ * 'importModule("res:/jscodelib/ComponentUtils.js");' This is out of date now, right? [righteous use of closures deleted ;-)] >+ if (argc > 1) { >+ // The caller passed in the optional second argument. Get it. >+ jsval *argv = nsnull; >+ rv = cc->GetArgvPtr(&argv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ if (!JS_ValueToObject(cx, argv[1], &targetObject)) { >+ cc->SetExceptionWasThrown(JS_TRUE); This is right, IIRC. >+ return NS_OK; But shouldn't this be NS_ERROR_FAILURE or the like? >+ } >+ } >+ else { >+ // Our targetObject is the caller's global object. Find it by >+ // walking the calling object's parent chain. >+ >+ nsCOMPtr<nsIXPConnectWrappedNative> wn; >+ rv = cc->GetCalleeWrapper(getter_AddRefs(wn)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ wn->GetJSObject (&targetObject); >+ if (!targetObject) { >+ NS_ERROR("null calling object"); >+ return NS_ERROR_FAILURE; >+ } >+ >+ JSObject *parent = nsnull; No reason to initialize parent when the very next line sets it: >+ while ((parent = JS_GetParent(cx, targetObject))) >+ targetObject = parent; >+ } >+ nsAutoPtr<ModuleEntry> entry(new ModuleEntry); >+ if (!entry) >+ return NS_ERROR_OUT_OF_MEMORY; >+ >+ Nit: doubled empty line. >+ rv = GlobalForLocation(componentFile, &entry->global, &entry->location); >+ if (NS_FAILED(rv)) { >+ *_retval = nsnull; >+ return NS_ERROR_FAILURE; >+ } >+ >+ *_retval = entry->global; >+ >+ jsval symbols; >+ nsCAutoString err; >+ if (targetObj && >+ JS_GetProperty(mContext, entry->global, >+ "EXPORTED_SYMBOLS", &symbols)) { If JS_GetProperty returns false, you want to note the pending exception and fail here. >+ >+ JSObject *symbolsObj = nsnull; >+ if (!JSVAL_IS_OBJECT(symbols) || Use JSVAL_IS_PRIMITIVE instead of !JSVAL_IS_OBJECT here to handle null as well. >+ if (!JS_GetArrayLength(mContext, symbolsObj, &symbolCount)) { >+ err += aLocation; >+ err.AppendLiteral(" - Error getting array length of "); >+ err.AppendLiteral("EXPORTED_SYMBOLS."); >+ NS_ERROR(err.get()); >+ return NS_ERROR_FAILURE; Need to note pending exception here? Or is this different from the case above (in a different function) where you have to tell xpconnect about that? >+ } >+#ifdef DEBUG >+ printf("Installing symbols [ "); >+#endif >+ >+ for (jsuint i = 0; i < symbolCount; ++i) { >+ jsval val; >+ JSString *symbolName; >+ if (!JS_GetElement(mContext, symbolsObj, i, &val) || !JS_GetElement => pending exception on mContext, tell xpconnect about it. >+ !JSVAL_IS_STRING(val) || No pending exception in this case. >+ !(symbolName = JSVAL_TO_STRING(val))) { Or this one. >+ err += aLocation; >+ err.AppendLiteral(" - EXPORTED_SYMBOLS array element "); >+ err.AppendLiteral("is not a string."); >+ NS_ERROR(err.get()); >+ return NS_ERROR_FAILURE; >+ } >+#ifdef DEBUG >+ printf("%s ", JS_GetStringBytes(symbolName)); >+#endif >+ >+ if (!JS_GetProperty(mContext, entry->global, >+ JS_GetStringBytes(symbolName), &val)) { Ditto. >+ err += aLocation; >+ err.AppendLiteral(" - Could not get symbol '"); >+ err.Append(nsDependentCString(JS_GetStringBytes(symbolName))); >+ err.AppendLiteral("'"); >+ NS_ERROR(err.get()); >+ return NS_ERROR_FAILURE; >+ } >+ >+ if (!JS_SetProperty(mContext, targetObj, >+ JS_GetStringBytes(symbolName), &val)) { Ditto. >+ err += aLocation; >+ err.AppendLiteral(" - Could not set property '"); >+ err.Append(nsDependentCString(JS_GetStringBytes(symbolName))); >+ NS_ERROR("' on target object."); >+ return NS_ERROR_FAILURE; >+ } >+ } >+#ifdef DEBUG >+ printf("] from %s\n", PromiseFlatCString(aLocation).get()); >+#endif >+ } The comment at the top of Import about "must be called only from JS" -- you could #ifdef DEBUG some code to test and essentially assert that fact. Looks good otherwise, minusing pro-forma to get comments addressed. /be

still need to add unit tests

Comment on attachment 264694 [details] [diff] [review] updated with brendan's comments Incomplete? $ lsdiff ~/tmp/sayrer-A js/src/xpconnect/idl/Makefile.in js/src/xpconnect/idl/xpcIJSModuleLoader.idl js/src/xpconnect/idl/xpccomponents.idl js/src/xpconnect/loader/ComponentUtils.jsm js/src/xpconnect/loader/Makefile.in js/src/xpconnect/loader/mozJSComponentLoader.cpp js/src/xpconnect/loader/mozJSComponentLoader.h js/src/xpconnect/src/xpccomponents.cpp $ lsdiff ~/tmp/sayrer-B js/src/xpconnect/idl/Makefile.in js/src/xpconnect/idl/xpccomponents.idl js/src/xpconnect/loader/Makefile.in js/src/xpconnect/loader/mozJSComponentLoader.cpp js/src/xpconnect/loader/mozJSComponentLoader.h js/src/xpconnect/src/xpccomponents.cpp Ultra-nit: canonical name for JSBool API success/fail result var is "ok", not "success". /be

now with unit tests and added files. The tests caught a pretty bad bug. Turns out that you still need to install the symbols, even if you find the module in the cache :)

Comment on attachment 264755 [details] [diff] [review] Components.utils.import v2.2 (fix ultra-nits) >+ jsval symbols; >+ nsCAutoString err; >+ if (targetObj) { >+ if (!JS_GetProperty(mContext, mod->global, >+ "EXPORTED_SYMBOLS", &symbols)) { >+ if (cc) >+ cc->SetExceptionWasThrown(JS_TRUE); >+ err += aLocation; >+ err.AppendLiteral(" - EXPORTED_SYMBOLS is not present."); >+ NS_ERROR(err.get()); As noted, NS_ERROR compiles away in release builds, so the err mutations should be #ifdef NS_DEBUG I guess -- also err's declaration. >+ return NS_ERROR_FAILURE; >+ } >+ >+ JSObject *symbolsObj = nsnull; >+ if (JSVAL_IS_PRIMITIVE(symbols) || >+ !(symbolsObj = JSVAL_TO_OBJECT(symbols)) || Sorry, I misread things here -- if you test !JSVAL_IS_OBJECT(symbols) and keep the explicit !(symbolsObj = ...) test, you're better off. This way duplicates the null test. >+ !JS_IsArrayObject(mContext, symbolsObj)) { >+ err += aLocation; >+ err.AppendLiteral(" - EXPORTED_SYMBOLS is not an array."); >+ NS_ERROR(err.get()); Ditto the NS_DEBUG comment. >+ return NS_ERROR_FAILURE; >+ } >+ >+ // Iterate over symbols array, installing symbols on targetObj: >+ >+ jsuint symbolCount = 0; >+ if (!JS_GetArrayLength(mContext, symbolsObj, &symbolCount)) { >+ if (cc) >+ cc->SetExceptionWasThrown(JS_TRUE); >+ err += aLocation; >+ err.AppendLiteral(" - Error getting array length of "); >+ err.AppendLiteral("EXPORTED_SYMBOLS."); >+ NS_ERROR(err.get()); Ditto. >+ return NS_ERROR_FAILURE; >+ } >+#ifdef DEBUG >+ printf("Installing symbols [ "); >+#endif >+ >+ for (jsuint i = 0; i < symbolCount; ++i) { >+ jsval val; >+ JSString *symbolName; >+ JSBool ok; >+ if (!(ok = JS_GetElement(mContext, symbolsObj, i, &val))) { Uber-ultra-nit: I find it clearer to initialize the ok decl and not test if (!(...)). >+ if (cc) >+ cc->SetExceptionWasThrown(JS_TRUE); >+ } >+ >+ if (!ok || !JSVAL_IS_STRING(val) || >+ !(symbolName = JSVAL_TO_STRING(val))) { There's no need to null-test JSVAL_TO_STRING(val) if JSVAL_IS_STRING(val). >+ err += aLocation; >+ err.AppendLiteral(" - EXPORTED_SYMBOLS array element "); >+ err.AppendLiteral("is not a string."); >+ NS_ERROR(err.get()); Ditto NS_DEBUG (or is DEBUG the thing to test? See below, and sigh). >+ return NS_ERROR_FAILURE; >+ } >+#ifdef DEBUG >+ printf("%s ", JS_GetStringBytes(symbolName)); >+#endif >+ >+ if (!JS_GetProperty(mContext, mod->global, >+ JS_GetStringBytes(symbolName), &val)) { >+ if (cc) >+ cc->SetExceptionWasThrown(JS_TRUE); >+ err += aLocation; >+ err.AppendLiteral(" - Could not get symbol '"); >+ err.Append(nsDependentCString(JS_GetStringBytes(symbolName))); >+ err.AppendLiteral("'"); >+ NS_ERROR(err.get()); Ditto. >+ return NS_ERROR_FAILURE; >+ } >+ >+ if (!JS_SetProperty(mContext, targetObj, >+ JS_GetStringBytes(symbolName), &val)) { >+ if (cc) >+ cc->SetExceptionWasThrown(JS_TRUE); >+ err += aLocation; >+ err.AppendLiteral(" - Could not set property '"); >+ err.Append(nsDependentCString(JS_GetStringBytes(symbolName))); >+ NS_ERROR("' on target object."); Ditto. Maybe the right thing to do is build up the error detail string and attach it to the exception object that should be pending in mContext. Sounds like work, though. So question: is this debug chatter likely to be very helpful to developers using release builds? /be

(In reply to comment #57) > > Maybe the right thing to do is build up the error detail string and attach it > to the exception object that should be pending in mContext. Sounds like work, > though. So question: is this debug chatter likely to be very helpful to > developers using release builds? > The stuff that is #ifdef DEBUG right now should stay that way, because it prints for successful imports. I think developers using release builds could use the error cases, otherwise the failures can be pretty hard to diagnose unless you understand exactly how this function works. Will fix.

> >+ if (!JS_ValueToObject(cx, argv[1], &targetObject)) { > >+ cc->SetExceptionWasThrown(JS_TRUE); > > This is right, IIRC. > > >+ return NS_OK; > > But shouldn't this be NS_ERROR_FAILURE or the like? I think NS_OK is what we want, because otherwise XPConnect seems to overwrite our error messages with COM stuff. If returning a failure code is required for some reason, we should audit the tree, because I found other callers doing this. This also means we don't need separate handling for situations that set an exception on mContext, I think.

Comment on attachment 264916 [details] [diff] [review] Components.utils.import v3 Nice -- right you are about NS_OK with JS exception set (confusing; also we no doubt do have bugs where we overwrite a JS exception; we greatly fear hidden JS exceptions, because they'll bite subsequent attempts to interpret on the tainted cx). Imputing r+jst, he should re-review if the code changes warrant but I think this is ready. Thanks for doing it. /be

The testcases which were checked in didn't actually createInstance() any components. This patch adds support for that test case too, and hopefully demonstrates the Right Way. I do wonder why the generated factory's createInstance method calls on "return ctor().QueryInterface(iid);" instead of "return new ctor().QueryInterface(iid)". Support for services, maybe?

Comment on attachment 264944 [details] [diff] [review] additional testcase (loading a component) nice, thanks.

Checking in idl/Makefile.in; /cvsroot/mozilla/js/src/xpconnect/idl/Makefile.in,v <-- Makefile.in new revision: 1.24; previous revision: 1.23 done Checking in idl/xpcIJSModuleLoader.idl; /cvsroot/mozilla/js/src/xpconnect/idl/xpcIJSModuleLoader.idl,v <-- xpcIJSModuleLoader.idl new revision: 1.2; previous revision: 1.1 done Checking in idl/xpccomponents.idl; /cvsroot/mozilla/js/src/xpconnect/idl/xpccomponents.idl,v <-- xpccomponents.idl new revision: 1.30; previous revision: 1.29 done Checking in loader/Makefile.in; /cvsroot/mozilla/js/src/xpconnect/loader/Makefile.in,v <-- Makefile.in new revision: 1.20; previous revision: 1.19 done RCS file: /cvsroot/mozilla/js/src/xpconnect/loader/XPCOMUtils.jsm,v done Checking in loader/XPCOMUtils.jsm; /cvsroot/mozilla/js/src/xpconnect/loader/XPCOMUtils.jsm,v <-- XPCOMUtils.jsm initial revision: 1.1 done Checking in loader/mozJSComponentLoader.cpp; /cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v <-- mozJSComponentLoader.cpp new revision: 1.133; previous revision: 1.132 done Checking in loader/mozJSComponentLoader.h; /cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.h,v <-- mozJSComponentLoader.h new revision: 1.28; previous revision: 1.27 done Checking in src/xpccomponents.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpccomponents.cpp,v <-- xpccomponents.cpp new revision: 1.109; previous revision: 1.108 done Checking in tests/Makefile.in; /cvsroot/mozilla/js/src/xpconnect/tests/Makefile.in,v <-- Makefile.in new revision: 1.32; previous revision: 1.31 done RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/unit/bogus_element_type.jsm,v done Checking in tests/unit/bogus_element_type.jsm; /cvsroot/mozilla/js/src/xpconnect/tests/unit/bogus_element_type.jsm,v <-- bogus_element_type.jsm initial revision: 1.1 done RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/unit/bogus_exports_type.jsm,v done Checking in tests/unit/bogus_exports_type.jsm; /cvsroot/mozilla/js/src/xpconnect/tests/unit/bogus_exports_type.jsm,v <-- bogus_exports_type.jsm initial revision: 1.1 done RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/unit/test_bogus_files.js,v done Checking in tests/unit/test_bogus_files.js; /cvsroot/mozilla/js/src/xpconnect/tests/unit/test_bogus_files.js,v <-- test_bogus_files.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/unit/test_import.js,v done Checking in tests/unit/test_import.js; /cvsroot/mozilla/js/src/xpconnect/tests/unit/test_import.js,v <-- test_import.js initial revision: 1.1 done

Comment on attachment 264944 [details] [diff] [review] additional testcase (loading a component) I'm actually withdrawing this patch, to switch to the "new ctor()" code I suggested in comment 61. This will require a trivial sr for the one- or two-line change to XPCOMUtils.

Thanks, sayrer!

shaver: aside from the change from "ctor()" to "new ctor()", no in-process code gets changed. This is the only reason I'm asking for sr: everything else is either comments or test code, and would normally not require sr. sayrer: this patch is identical to what you saw earlier, except for what we discussed via IRC. So the review should be pretty easy.

Comment on attachment 264965 [details] [diff] [review] additional testcase (loading a component) >Index: js/src/xpconnect/loader/XPCOMUtils.jsm >+ * 'Components.utils.import("rel:XPCOMUtils.js");' Oops. I still didn't get it right. That should be "rel:XPCOMUtils.jsm".

I just noticed something else. XPCOMUtils does not alleviate the need for QueryInterface if your component implements nsIClassInfo and you want to show all your interfaces at once. We might need a follow-up bug for that.

Comment on attachment 264965 [details] [diff] [review] additional testcase (loading a component) I checked this in. Please create new bugs for additional follow-up patches.

Robert (and everyone else who got this landed): A big thank you!

(In reply to comment #61) > I do wonder why the generated factory's createInstance method calls on "return > ctor().QueryInterface(iid);" instead of "return new > ctor().QueryInterface(iid)". Support for services, maybe? > Alex: There are a couple of reasons why I think the version without 'new' is preferable: - Support for singleton objects - 'Complex' constructors; such as a function that returns an object out of some pool, or an object that is constructed differently than by calling 'new'. E.g. in zap (and in joost), we use a JS class framework where objects are constructed using 'myClass.instantiate()'.

(In reply to comment #71) So let me see if I understand this. Under the ctor() model (as opposed to new ctor()), services would be returned like this: Foo = { // nsISupports QueryInterface: function QI(aIID) { // ... } } FooFactory = XPCOMUtils.generateFactory(function getFoo() { return Foo; }); That's how I'd prefer to implement services too, but I think it's not very correct. That's one advantage of nsIClassInfo's flags; it allows a component t tell xpconnect that it is a service, and thus only one instance should be created. Complex constructors would look like: FooClass = { instantiate: function instantiate() { var rv = new Object(); rv.QueryInterface = function QueryInterface(aIID) { /* ... */}; rv._count = this._count++; return rv; } } FooFactory = XPCOMUtils.generateFactory(function getFoo() { return FooClass.instantiate(); }); My main beef with this is that I've never seen a JS-based factory written to call a method to return a component. The above code could be done just as easily with |return new Foo()|. If instead you wanted to pass in arguments to the instantiate() method, I have two arguments against that. One, factories generally don't get additional arguments (although according to xpconnect, I think they could - shaver will probably correct me on that). So you'd be stuck with what you already have as properties on the factory and in the factory's scope. Two, even if additional arguments were permissible or even desired, why wouldn't an initialize() method on the component be appropriate for the component's interface? It's quite common to have one, and we do have errors for NOT_INITIALIZED and ALREADY_INITIALIZED. Further, Components.Constructor allows you to hide the need for initialize(), just like it would hide the need for the instantiate() method. I am perfectly willing to accept that I'm wrong here, but I think you might win me (and more importantly, the reviewers!) over with test components demonstrating what you have in mind, loaded via xpcshell-simple's do_load_module(). My primary argument here is that your proposal bends the convention of XPCOM factories we'd been writing for years, and may not be worth the extra couple of lines for the vast majority of JS-based components we already have.

(In reply to comment #72) > [...] > My primary argument here is that your proposal bends the > convention of XPCOM factories we'd been writing for years, and may not be worth > the extra couple of lines for the vast majority of JS-based components we > already have. Right, I agree. The zap/joost usage of complex constructors is not very common, so best to stick with 'new ctor()' in XPCOMUtils.jsm. I'll stick a more generic version of generateFactory in a zap/joost-specific zapXPCOMUtils.jsm.

(In reply to comment #71) > (In reply to comment #61) > > I do wonder why the generated factory's createInstance method calls on "return > > ctor().QueryInterface(iid);" instead of "return new > > ctor().QueryInterface(iid)". Support for services, maybe? > > > > Alex: > There are a couple of reasons why I think the version without 'new' is > preferable: > - Support for singleton objects > - 'Complex' constructors; such as a function that returns an object out of some > pool, or an object that is constructed differently than by calling 'new'. E.g. > in zap (and in joost), we use a JS class framework where objects are > constructed using 'myClass.instantiate()'. You can still call |new| and get those behaviours, because an object returned by a constructor function is taken as the result of the expression: Edited lightly: js> a = [1,2,3]; js> function ctor() { return a; } js> a1 = new ctor(); js> a2 = new ctor(); js> a1 === a2 true

(In reply to comment #74) > [...] an object returned by a constructor function is taken as > the result of the expression Ah, nice, didn't know that. That simplifies things.

We have documentation for this now.

Please see bug 447728

Isn't import a reserved word? In strict mode now, but in not-strict in JS 1.9 FF 5 according to https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words.

See ES5.1 clause 7.6 -- import and other reserved identifiers are fine to use as property names (after . in member expressions, before : in object initialisers). /be

Ah, interesting. Looking at http://es5.github.com/#A.1, the difference seems to be IdentifierName which can be used as you said and Identifier (which is an IdentifierName without the Reserved Words) which what is used for FunctionDeclaration and FunctionExpression. So it seems that https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words needs to be updated? Still, its an edge case, and import would be illegal in a FunctionDeclaration, so some might suggest that using some other name might have been a better idea. Water under the bridge I guess, but some JS lint tools are not happy and will have to be updated.